Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

node/test/parallel/test-debugger-pid.js fails in localized Windows #11250

Closed
vsemozhetbyt opened this issue Feb 8, 2017 · 7 comments
Closed
Labels
test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.

Comments

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Feb 8, 2017

  • Version: Node.js 7.5.0, Node.js master (8.0.0pre), Node.js vee-eight-lkgr
  • Platform: Windows 7 x64 with Russian locale
  • Subsystem: test

On this test I get this error:

assert.js:85
  throw new assert.AssertionError({
  ^
AssertionError: 'The parameter is incorrect.' === 'Ïàðàìåòð çàäàí íåâåðíî.'
    at ChildProcess.<anonymous> (e:\DOC\prg\js\node\-git\node\fork\test\parallel\test-debugger-pid.js:49:10)
    at emitOne (events.js:96:13)
    at ChildProcess.emit (events.js:189:7)
    at e:\DOC\prg\js\node\-git\node\fork\test\parallel\test-debugger-pid.js:18:16
    at Array.forEach (native)
    at Socket.onData (e:\DOC\prg\js\node\-git\node\fork\test\parallel\test-debugger-pid.js:17:8)
    at emitOne (events.js:96:13)
    at Socket.emit (events.js:189:7)
    at readableAddChunk (_stream_readable.js:176:18)
    at Socket.Readable.push (_stream_readable.js:134:10)

I use cmd.exe with the Unicode codepage:

> chcp
Active code page: 65001

The test suit after building various Node.js versions fails only for this one test.

If I change the codepage back into the default DOS 866, then the message is:

assert.js:85
  throw new assert.AssertionError({
  ^
AssertionError: 'The parameter is incorrect.' === '├П├а├░├а├м├е├▓├░ ├з├а├д├а├н ├н├е├в├е├░├н├о.'
    at ChildProcess.<anonymous> (j:\temp\node-vee-eight-lkgr\test\parallel\test-debugger-pid.js:49:10)
    at emitOne (events.js:96:13)
    at ChildProcess.emit (events.js:188:7)
    at j:\temp\node-vee-eight-lkgr\test\parallel\test-debugger-pid.js:18:16
    at Array.forEach (native)
    at Socket.onData (j:\temp\node-vee-eight-lkgr\test\parallel\test-debugger-pid.js:17:8)
    at emitOne (events.js:96:13)
    at Socket.emit (events.js:188:7)
    at readableAddChunk (_stream_readable.js:176:18)
    at Socket.Readable.push (_stream_readable.js:134:10)

It seems the message somehow is Russian "Параметр задан неверно".

@Trott
Copy link
Member

Trott commented Feb 8, 2017

If you remove lines 37-39 from the test, does the test then pass?

Unless there's an easy way to ask Window for the localized version of the string, I wonder if we should just not check for it at all.

Not sure we need to spend too much time perfecting this test anyway as the CLI debugger is going away in the foreseeable future anyway.

@Trott Trott added the debugger label Feb 8, 2017
@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Feb 8, 2017

If you remove lines 37-39 from the test, does the test then pass?

Yes, it passes then.

@mscdex mscdex added test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform. labels Feb 8, 2017
@Trott
Copy link
Member

Trott commented Feb 9, 2017

If you remove lines 37-39 from the test, does the test then pass?

Yes, it passes then.

Maybe that's the way to go. /cc @nodejs/testing for confirming or contrary opinions.

@gibfahn
Copy link
Member

gibfahn commented Feb 9, 2017

Not sure we need to spend too much time perfecting this test anyway as the CLI debugger is going away in the foreseeable future anyway.

+1

@santigimeno
Copy link
Member

Maybe that's the way to go. /cc @nodejs/testing for confirming or contrary opinions.

I think it's fine removing those lines

@Trott
Copy link
Member

Trott commented Feb 9, 2017

@vsemozhetbyt Do you want to open a PR to remove those lines from the test? Or would you prefer I or someone else do it? (Either way is totally fine by me.)

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Feb 9, 2017

italoacasas pushed a commit that referenced this issue Feb 13, 2017
PR-URL: #11270
Fixes: #11250
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this issue Feb 14, 2017
PR-URL: nodejs#11270
Fixes: nodejs#11250
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
krydos pushed a commit to krydos/node that referenced this issue Feb 25, 2017
PR-URL: nodejs#11270
Fixes: nodejs#11250
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this issue Mar 7, 2017
PR-URL: #11270
Fixes: #11250
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 9, 2017
PR-URL: #11270
Fixes: #11250
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

No branches or pull requests

5 participants