test,module: make message check MUI dependent #13393

refack · 2017-06-02T11:57:09Z

Fixes: #13376

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added - comments
commit message follows commit guidelines

Affected core subsystem(s)

test,module

refack · 2017-06-02T11:59:11Z

Dry-run CI: https://ci.nodejs.org/job/node-test-commit/10308/

refack

Explaining + nits

refack · 2017-06-02T12:00:09Z

test/parallel/test-module-loading-error.js

+// if we don't know a priori what the error would be, we accept anything
+const dlerror_msg = error_desc[process.platform] || [''];
+
+// On Windows error message are MUI dependent


/s/message/messages

and a comma after Windows

refack · 2017-06-02T12:01:28Z

test/parallel/test-module-loading-error.js


 assert.throws(
-  () => { require('../fixtures/module-loading-error.node'); },
+  () => {
+    require('../fixtures/module-loading-error.node');


putting it in a separate line makes for easy breakpoint setting.

This seems like an unnecessary change. Not sure about other debuggers, but chrome://inspect allows you to set breakpoints on statements within a line:

refack · 2017-06-02T12:02:54Z

test/parallel/test-module-loading-error.js

-    if (dlerror_msg && !dlerror_msg.some((msg) => e.message.includes(msg)))
-      return false;
-    if (e.name !== 'Error')
+    if (localeOk && !dlerror_msg.some((msg) => e.message.includes(msg)))


replace dlerror_msg truthy check for a trivial fallback (Line 34) mostly for keeping line short.

refack · 2017-06-02T12:03:12Z

test/parallel/test-module-loading-error.js

      return false;
-    return true;
+    return e.name === 'Error';


tniessen · 2017-06-02T12:07:30Z

test/parallel/test-module-loading-error.js

+    'powershell -NoProfile -ExecutionPolicy Unrestricted -c' +
+    ' "(Get-UICulture).TwoLetterISOLanguageName"';
+  try {
+    localeOk = execSync(powerShellFindMUI).toString('utf8') === 'en';


I fear this might actually be 'en\r\n'. Maybe .startsWith('en'), or even .includes('en'), or trim the output?

refack · 2017-06-02T12:13:55Z

New CI: https://ci.nodejs.org/job/node-test-commit/10310/

tniessen

Just discovered that this method is not 100% accurate (I changed the locale, logged off and back in without rebooting), but I think it is close enough.

> require('./test/fixtures/module-loading-error.node')
Error: %1 is not a valid Win32 application.
\\?\E:\node\test\fixtures\module-loading-error.node
    at Error (native)
    at Object.Module._extensions..node (module.js:597:18)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.require (module.js:497:17)
    at require (internal/module.js:20:19)
    at repl:1:1
    at sigintHandlersWrap (vm.js:22:35)
    at sigintHandlersWrap (vm.js:96:12)
>
(To exit, press ^C again or type .exit)
>

E:\node>powershell -NoProfile -ExecutionPolicy Unrestricted -c "(Get-UICulture).TwoLetterISOLanguageName"
de

Will approve if CI passes.

tniessen · 2017-06-02T12:26:43Z

test/parallel/test-module-loading-error.js


 const error_desc = {
  win32: ['%1 is not a valid Win32 application'],
  linux: ['file too short', 'Exec format error'],
  sunos: ['unknown file type', 'not an ELF file'],
  darwin: ['file too short']
 };
-const dlerror_msg = error_desc[process.platform];
+// if we don't know a priori what the error would be, we accept anything
+const dlerror_msg = error_desc[process.platform] || [''];


Maybe use camelCase (dlErrorMsg) for consistency while you are at it?

s/if/If and add a period at the end

refack · 2017-06-02T18:45:38Z

Windows CI: https://ci.nodejs.org/job/node-test-commit-windows-fanned/9534/

refack · 2017-06-02T21:32:30Z

Full CI: https://ci.nodejs.org/job/node-test-commit/10325/

tniessen

Thank you for implementing this!

gibfahn · 2017-06-03T13:25:29Z

test/parallel/test-module-loading-error.js


-const error_desc = {
+const errorMsgsByPlafrom = {


errorMsgsByPlafrom -> errorMsgsByPlatform (and below)

in for penny in for a pound: errorMessagesByPlatform

gibfahn · 2017-06-03T14:00:03Z

test/parallel/test-module-loading-error.js


 assert.throws(
-  () => { require('../fixtures/module-loading-error.node'); },
+  () => {
+    require('../fixtures/module-loading-error.node');


This seems like an unnecessary change. Not sure about other debuggers, but chrome://inspect allows you to set breakpoints on statements within a line:

gibfahn · 2017-06-03T17:09:24Z

test/parallel/test-module-loading-error.js

+    '"(Get-UICulture).TwoLetterISOLanguageName"';
+  try {
+    // If MUI != 'en' we'll ignore the content of the message
+    localeOk = execSync(powerShellFindMUI).toString('utf8').trim() === 'en';


If this line fails and we hit the catch, won't localeOk still be true rather than false?

i.e. should you do this?

if (common.isWindows) { + localeOk = false; const powerShellFindMUI =

Intentionally I chose to be strict. the less use of the "escape hatch" the better the coverage 🤔

If the powershell script fails will it log an error to the console? If so then I guess that's fine.

Yea because execSync pipes stderr from the child we get the error (this one is a synthetic syntax error):

C:\bin\dev\node\node.exe D:\code\node\test\parallel\test-module-loading-error.js Get-UICulture2 : The term 'Get-UICulture2' is not recognized as the name of a cmdlet, function, script file, or operable program. Check the spelling of the name, or if a path was included, verify that the path is correct and try again. At line:1 char:2 + (Get-UICulture2).TwoLetterISOLanguageName + ~~~~~~~~~~~~~~ + CategoryInfo : ObjectNotFound: (Get-UICulture2:String) [], CommandNotFoundException + FullyQualifiedErrorId : CommandNotFoundException Process finished with exit code 0

Notice that the exit code is 0

PR-URL: nodejs#13393 Fixes: nodejs#13376 Reviewed-By: Tobias Nießen <[email protected]>

refack · 2017-06-05T21:23:49Z

Landed in 30a20bd

PR-URL: #13393 Fixes: #13376 Reviewed-By: Tobias Nießen <[email protected]>

MylesBorins · 2017-07-17T17:47:19Z

Should this be backported to v6.x?

refack · 2017-07-18T00:07:31Z

Yes. Does not land cleanly, I will backport.

nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 2, 2017

refack mentioned this pull request Jun 2, 2017

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

Closed

refack commented Jun 2, 2017

View reviewed changes

tniessen reviewed Jun 2, 2017

View reviewed changes

tniessen added the windows Issues and PRs related to the Windows platform. label Jun 2, 2017

mscdex added the module Issues and PRs related to the module subsystem. label Jun 2, 2017

tniessen approved these changes Jun 2, 2017

View reviewed changes

tniessen approved these changes Jun 3, 2017

View reviewed changes

gibfahn reviewed Jun 3, 2017

View reviewed changes

refack force-pushed the test-module-mui-check branch from adb46b5 to 6ee5498 Compare June 3, 2017 14:21

gibfahn reviewed Jun 3, 2017

View reviewed changes

test,module: make message check MUI dependent

30a20bd

PR-URL: nodejs#13393 Fixes: nodejs#13376 Reviewed-By: Tobias Nießen <[email protected]>

refack force-pushed the test-module-mui-check branch from 6ee5498 to 30a20bd Compare June 5, 2017 21:21

refack merged commit 30a20bd into nodejs:master Jun 5, 2017

refack deleted the test-module-mui-check branch June 5, 2017 21:25

jasnell pushed a commit that referenced this pull request Jun 7, 2017

test,module: make message check MUI dependent

2e3b758

PR-URL: #13393 Fixes: #13376 Reviewed-By: Tobias Nießen <[email protected]>

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

MylesBorins added the lts-watch-v6.x label Jul 17, 2017

refack added the backport-requested-v6.x label Jul 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test,module: make message check MUI dependent #13393

test,module: make message check MUI dependent #13393

refack commented Jun 2, 2017

refack commented Jun 2, 2017

refack left a comment

refack Jun 2, 2017

refack Jun 2, 2017

refack Jun 2, 2017

refack Jun 2, 2017

gibfahn Jun 3, 2017

refack Jun 2, 2017

refack Jun 2, 2017

tniessen Jun 2, 2017

refack Jun 2, 2017

refack commented Jun 2, 2017

tniessen left a comment

tniessen Jun 2, 2017

jasnell Jun 2, 2017

refack commented Jun 2, 2017

refack commented Jun 2, 2017

tniessen left a comment

gibfahn Jun 3, 2017

refack Jun 3, 2017

gibfahn Jun 3, 2017

gibfahn Jun 3, 2017

refack Jun 3, 2017

gibfahn Jun 3, 2017

refack Jun 4, 2017

refack commented Jun 5, 2017

MylesBorins commented Jul 17, 2017

refack commented Jul 18, 2017

test,module: make message check MUI dependent #13393

test,module: make message check MUI dependent #13393

Conversation

refack commented Jun 2, 2017

Checklist

Affected core subsystem(s)

refack commented Jun 2, 2017

refack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Jun 2, 2017

tniessen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Jun 2, 2017

refack commented Jun 2, 2017

tniessen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Jun 5, 2017

MylesBorins commented Jul 17, 2017

refack commented Jul 18, 2017