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.

Sign up for GitHub

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

Jump to bottom

test,module: make message check MUI dependent #13393

Merged
merged 1 commit into from
Jun 5, 2017
Merged

test,module: make message check MUI dependent #13393

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 21 additions & 6 deletions test/parallel/test-module-loading-error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,23 +22,38 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const { execSync } = require('child_process');

const error_desc = {
const errorMessagesByPlatform = {
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 errorMessages = errorMessagesByPlatform[process.platform] || [''];

// On Windows, error messages are MUI dependent
// Ref: https://github.com/nodejs/node/issues/13376
let localeOk = true;
if (common.isWindows) {
const powerShellFindMUI =
'powershell -NoProfile -ExecutionPolicy Unrestricted -c ' +
'"(Get-UICulture).TwoLetterISOLanguageName"';
try {
// If MUI != 'en' we'll ignore the content of the message
localeOk = execSync(powerShellFindMUI).toString('utf8').trim() === 'en';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 =

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} catch (_) {
// It's only a best effort try to find the MUI
}
}

assert.throws(
() => { require('../fixtures/module-loading-error.node'); },
(e) => {
if (dlerror_msg && !dlerror_msg.some((msg) => e.message.includes(msg)))
return false;
if (e.name !== 'Error')
if (localeOk && !errorMessages.some((msg) => e.message.includes(msg)))
return false;
return true;
return e.name === 'Error';
}
);

Expand Down