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

test-module-loading-error fails on Windows with MUI set to a non-English language #13376

Closed
tniessen opened this issue Jun 1, 2017 · 9 comments · Fixed by #13393
Closed

test-module-loading-error fails on Windows with MUI set to a non-English language #13376

tniessen opened this issue Jun 1, 2017 · 9 comments · Fixed by #13393
Labels
module Issues and PRs related to the module subsystem. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.

Comments

@tniessen
Copy link
Member

tniessen commented Jun 1, 2017

  • Version: master
  • Platform: Windows 10 x64
  • Subsystem: test

On Windows, error messages are translated to the user's primary language as determined by the Windows MUI, so

win32: ['%1 is not a valid Win32 application'],

does not match the error message. This results in the test case failing like this:

E:\node> Release\node test\parallel\test-module-loading-error.js
assert.js:532
    throw actual;
    ^

Error: %1 ist keine zulässige Win32-Anwendung.
\\?\E:\node\test\fixtures\module-loading-error.node
    at Object.Module._extensions..node (module.js:598:18)
    at Module.load (module.js:503:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)
    at Module.require (module.js:513:17)
    at require (internal/module.js:11:18)
    at assert.throws (E:\node\test\parallel\test-module-loading-error.js:35:11)
    at _tryBlock (assert.js:489:5)
    at _throws (assert.js:510:12)
    at Function.throws (assert.js:540:3)

I didn't find a way to change the language settings for a single process / process tree. I added my localized error message to test/parallel/test-module-loading-error.js to suppress the error, but I got tired of doing that every time I cloned the repo. I switched to ignoring the error, knowing that the cause was localization, but I would prefer a clean solution (e.g. the early failure prevents the other tests in the file to be executed). Finally, I changed the primary system language to en-US to get rid of the test failure.

@nodejs/testing Any ideas on how to tackle this problem, except ignoring it or changing the system language?

@tniessen tniessen added test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform. labels Jun 1, 2017
@Trott
Copy link
Member

Trott commented Jun 1, 2017

IMO, we shouldn't be matching on operating system error messages in our code. The problem you're facing here is an excellent reason why.

In this particular case, it's not like all the variants we're accepting are consistent anyway. "Not a valid application" vs. "file too short" vs. "exec file format error" vs. "unknown file type"... At this point, why not just accept any error message as long as there's an error?

I think it would be acceptable to remove the message string checking entirely in this test.

@mscdex mscdex added the module Issues and PRs related to the module subsystem. label Jun 1, 2017
@refack
Copy link
Contributor

refack commented Jun 2, 2017

There is a way to change the locale of the process or even just a thread.

  1. We can just ignore the content, and accept any Error
  2. Change the locale just for running this test
  3. skip if !execSync('wmic os get locale').toString('utf8').includes('0809') - that is not en_US

Looking at this I'm pro (3).

@Trott
Copy link
Member

Trott commented Jun 2, 2017

I'm fine with any of the 3 options proposed.

@tniessen
Copy link
Member Author

tniessen commented Jun 2, 2017

(1) sounds fine to me, even though there are a few paranoid reasons not to do it (e.g. if someone deletes the fixture file, the test would still pass).

(2) sounds non-trivial. How would you change the locale for the particular thread? Wouldn't we need either an additional native binding or to integrate that logic into node itself? I know that a process can change its own locale, but I had hoped for a solution to integrate into the test runner instead of the node process.

(3) sounds good to me as long as no other complaints about the handling of error messages on other platforms come up. Note that my system reports 0409 though, which, according to this table, is en-US. 0809 appears to be en-GB. We'd probably need to include all of the English variants:

+----------+-----------------------------+-------------+--------+-------------------+
| Language |          Location           | Language ID |  Tag   | Supported Version |
+----------+-----------------------------+-------------+--------+-------------------+
| English  | Australia                   | 0x0c09      | en-AU  | Release A         |
| English  | Belize                      | 0x2809      | en-BZ  | Release B         |
| English  | Canada                      | 0x1009      | en-CA  | Release A         |
| English  | Caribbean                   | 0x2409      | en-029 | Release B         |
| English  | Hong Kong                   | 0x3c09      | en-HK  | Release 8.1       |
| English  | India                       | 0x4009      | en-IN  | Release V         |
| English  | Ireland                     | 0x1809      | en-IE  | Release A         |
| English  | Jamaica                     | 0x2009      | en-JM  | Release B         |
| English  | Malaysia                    | 0x4409      | en-MY  | Release V         |
| English  | New Zealand                 | 0x1409      | en-NZ  | Release A         |
| English  | Republic of the Philippines | 0x3409      | en-PH  | Release C         |
| English  | Singapore                   | 0x4809      | en-SG  | Release V         |
| English  | South Africa                | 0x1c09      | en-ZA  | Release B         |
| English  | Trinidad and Tobago         | 0x2c09      | en-TT  | Release B         |
| English  | United Kingdom              | 0x0809      | en-GB  | Release A         |
| English  | United States               | 0x0409      | en-US  | Release A         |
| English  | Zimbabwe                    | 0x3009      | en-ZW  | Release C         |
+----------+-----------------------------+-------------+--------+-------------------+

@gibfahn
Copy link
Member

gibfahn commented Jun 2, 2017

  1. skip if !execSync('wmic os get locale').toString('utf8').includes('0809') - that is not en_US

By skip you mean "accept any error" right? If so that seems reasonable.

@refack
Copy link
Contributor

refack commented Jun 2, 2017

  1. -1
  2. Use a tool like http://pooi.moe/Locale-Emulator/ to execSync this particular test case
  3. (0409 / 0809 / \d\d09) Yeah I forgot I have en_US language MUI, but en_UK locale (for "normal" looking dates), so maybe a better tests would be:
>powershell -NoProfile -ExecutionPolicy Unrestricted -c "(Get-UICulture).TwoLetterISOLanguageName"
en

Reference:

> powershell -c "get-uiculture | Format-Wide"


Parent                         : en
LCID                           : 1033
KeyboardLayoutId               : 1033
Name                           : en-US
IetfLanguageTag                : en-US
DisplayName                    : English (United States)
NativeName                     : English (United States)
EnglishName                    : English (United States)
TwoLetterISOLanguageName       : en
ThreeLetterISOLanguageName     : eng
ThreeLetterWindowsLanguageName : ENU
CompareInfo                    : CompareInfo - en-US
TextInfo                       : TextInfo - en-US
IsNeutralCulture               : False
CultureTypes                   : SpecificCultures, InstalledWin32Cultures, FrameworkCultures
NumberFormat                   : System.Globalization.NumberFormatInfo
DateTimeFormat                 : System.Globalization.DateTimeFormatInfo
Calendar                       : System.Globalization.GregorianCalendar
OptionalCalendars              : {System.Globalization.GregorianCalendar, System.Globalization.GregorianCalendar}
UseUserOverride                : True
IsReadOnly                     : False

@tniessen can you confirm powershell snippet?

@refack
Copy link
Contributor

refack commented Jun 2, 2017

Cross-ref to fix: #13393

@tniessen
Copy link
Member Author

tniessen commented Jun 2, 2017

Use a tool like http://pooi.moe/Locale-Emulator/ to execSync this particular test case

Thanks, did not find anything similar :)

Yes, powershell -NoProfile -ExecutionPolicy Unrestricted -c "(Get-UICulture).TwoLetterISOLanguageName" correctly determines the language.

@refack
Copy link
Contributor

refack commented Jun 2, 2017

Thanks, did not find anything similar :)

Yes, powershell -NoProfile -ExecutionPolicy Unrestricted -c "(Get-UICulture).TwoLetterISOLanguageName" correctly determines the language.

👍
Please review #13393

refack added a commit to refack/node that referenced this issue Jun 5, 2017
PR-URL: nodejs#13393
Fixes: nodejs#13376
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
jasnell pushed a commit that referenced this issue Jun 7, 2017
PR-URL: #13393
Fixes: #13376
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module Issues and PRs related to the module subsystem. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants