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

Tests that call more fail in Windows with Unicode shell codepage #11469

Closed
vsemozhetbyt opened this issue Feb 20, 2017 · 10 comments
Closed

Tests that call more fail in Windows with Unicode shell codepage #11469

vsemozhetbyt opened this issue Feb 20, 2017 · 10 comments
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 20, 2017

  • Version: possibly all
  • Platform: Windows 7 x64
  • Subsystem: test, child_process

test\parallel\test-child-process-stdin.js uses more command and does not expect any error messages in the stderr (see common.mustNotCall()).

However, some core Windows shell commands work wrong with Unicode shell codepage. See this answer.

To check more command:

chcp 65001 && more
Active code page: 65001
Not enough memory.

chcp 1252 && more
Active code page: 1252
[reading from stdio]

To check test\parallel\test-child-process-stdin.js pass:

chcp 65001 && node test\parallel\test-child-process-stdin.js
Active code page: 65001
... AssertionError: function should not have been called ...

chcp 1252 && node test\parallel\test-child-process-stdin.js
Active code page: 1252
stdout: hello world
@addaleax addaleax added test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform. labels Feb 20, 2017
@vsemozhetbyt vsemozhetbyt changed the title test\parallel\test-child-process-stdin.js fails in Windows with Unicode shell codepage Tests that call more fail in Windows with Unicode shell codepage Feb 20, 2017
@vsemozhetbyt
Copy link
Contributor Author

It seems the same with these two tests:

test\parallel\test-child-process-spawn-shell.js calls more here.

test\parallel\test-child-process-spawnsync-shell.js calls more here.

The same checks:

chcp 65001 && node test\parallel\test-child-process-spawnsync-shell.js
... AssertionError: '' === 'bar' ...

chcp 1252 && node test\parallel\test-child-process-spawnsync-shell.js
[pass]

chcp 65001 && node test\parallel\test-child-process-spawn-shell.js
... AssertionError: '' === 'bar' ...

chcp 1252 && node test\parallel\test-child-process-spawn-shell.js
[pass]

@seishun
Copy link
Contributor

seishun commented Feb 21, 2017

So I guess it should be documented that tests should be run in a non-Unicode codepage.

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Feb 21, 2017

Or maybe the test runner (or a concerned test / common lib) should set an appropriate codepage before a run.

@joaocgreis
Copy link
Member

I can reproduce this on Windows 7 and 2008R2 but cannot on Windows Server 2012R2. So, this looks like an issue with more (that ships with Windows: C:\Windows\System32\more.com) that has been fixed on Windows 8/2012.

I suspect more is used just because it was never changed to cat, we already require cat (installed by git) to run the tests. I'd be happy to discuss a PR to s/more/cat/, if it works.

@TimothyGu
Copy link
Member

I can't reproduce this on Windows 8.1 (6.3.9600). chcp 65001 && more works just fine. Changing to cat SGTM.

@vsemozhetbyt
Copy link
Contributor Author

See also: #11470

@TimothyGu
Copy link
Member

@vsemozhetbyt do you want to submit a PR for this?

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Mar 20, 2017

@TimothyGu If @joaocgreis has no plans to do this, I can try. However, I am not good at *nix shell tools so I can overlook something.

@joaocgreis
Copy link
Member

I've added this to my queue, but I'm not likely to get there soon. Feel free to go ahead, I can help review.

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Mar 21, 2017

@joaocgreis The PR, PTAL.

MylesBorins pushed a commit that referenced this issue Mar 28, 2017
PR-URL: #11953
Fixes: #11469
Reviewed-By: João Reis <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
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