-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
process: correctly parse Unicode in NODE_OPTIONS #34476
Conversation
const NODE_OPTIONS = `--redirect-warnings=${expected_redirect_value}`; | ||
const result = cp.spawnSync(process.argv0, | ||
['--expose-internals', __filename, 'test'], | ||
{ | ||
env: { | ||
NODE_OPTIONS | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
const NODE_OPTIONS = `--redirect-warnings=${expected_redirect_value}`; | |
const result = cp.spawnSync(process.argv0, | |
['--expose-internals', __filename, 'test'], | |
{ | |
env: { | |
NODE_OPTIONS | |
} | |
}); | |
const env = { NODE_OPTIONS: `--redirect-warnings=${expected_redirect_value}` }; | |
const result = cp.spawnSync(process.argv0, | |
['--expose-internals', __filename, 'test'], | |
{ env }); |
} | ||
}); | ||
assert.strictEqual(result.status, 0); | ||
process.exit(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary?
|
||
const expected_redirect_value = 'foó'; | ||
|
||
if (process.argv.length === 2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'd prefer to have explicit check for 'child process' case (i.e. if (process.argv[2] === 'test') {
) instead to handle possibly different test start args.
|
||
if (process.argv.length === 2) { | ||
const NODE_OPTIONS = `--redirect-warnings=${expected_redirect_value}`; | ||
const result = cp.spawnSync(process.argv0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const result = cp.spawnSync(process.argv0, | |
const result = cp.spawnSync(process.execPath, |
f5f8a79
to
1a31d81
Compare
Fixed some nits and rebased. |
Ok, this is weird. This fails only on
It looks like other tests can spawn new Node instances just fine. I'm stuck here 🤔 |
const result = cp.spawnSync(process.argv0, | ||
['--expose-internals', __filename, 'test'], | ||
{ | ||
env: { NODE_OPTIONS }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this help with the current CI failure?
env: { NODE_OPTIONS }, | |
env: { ...process.env, NODE_OPTIONS }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it helped. Thanks!
Fixes an issue on Windows, where Unicode in NODE_OPTIONS was not parsed correctly. Fixes: nodejs#34399
398d7d5
to
15ac48a
Compare
Fixes an issue on Windows, where Unicode in NODE_OPTIONS was not parsed correctly. Fixes: nodejs#34399 PR-URL: nodejs#34476 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Green CI, landed in de565ad |
Fixes an issue on Windows, where Unicode in NODE_OPTIONS was not parsed
correctly.
Fixes: #34399
Reused the code from
node_env_var.cc
for getting the enviroment variable value.Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes