-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
doc: mention case-insensitive env on windows #9166
Conversation
Building Node fails on my system, so I'm currently unable to run the tests :( This is the output I get during the build: https://gist.github.com/oliversalzburg/592885b45dbf0a561147faef519b0b05 |
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.
Building Node fails on my system, so I'm currently unable to run the tests :(
You can just use your system node to run this particular test, like node test/parallel/test-process-env.js
.
assert.strictEqual(process.env.TEST, process.env.teST); | ||
} else { | ||
assert.strictEqual('undefined', typeof process.env.test); | ||
} |
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.
I think our linter doesn’t like files that don’t end in newlines
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.
Done
// environment variables should be case-insensitive on Windows, and | ||
// case-sensitive on other platforms | ||
process.env.TEST = 'test'; | ||
if (common.isWindows) { |
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.
I think you’ll need to change require('../common');
to const common = require('../common');
for this to work
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.
Done
@addaleax My system node says it's fine: $ node test/parallel/test-process-env.js; echo $?
0 …unless I'm not doing it right. |
@@ -67,3 +67,12 @@ assert.equal(3, date.getUTCHours()); | |||
assert.equal(5, date.getHours()); | |||
*/ | |||
/* eslint-enable max-len */ | |||
|
|||
// environment variables should be case-insensitive on Windows, and |
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.
Can you please capitalize and punctuate the comments.
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.
Gladly, I just wanted to conform with other comments in the file. Is there some guideline regarding capitalization here?
// case-sensitive on other platforms | ||
process.env.TEST = 'test'; | ||
if (common.isWindows) { | ||
assert.strictEqual(process.env.TEST, process.env.teST); |
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.
Can you replace this with two separate assertions:
assert.strictEqual(process.env.TEST, 'test');
assert.strictEqual(process.env.teST, 'test');
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.
Done
if (common.isWindows) { | ||
assert.strictEqual(process.env.TEST, process.env.teST); | ||
} else { | ||
assert.strictEqual('undefined', typeof process.env.teST); |
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.
Instead of checking the typeof
, can you just directly compare against undefined
.
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.
Done
// case-sensitive on other platforms | ||
process.env.TEST = 'test'; | ||
if (common.isWindows) { | ||
assert.strictEqual(process.env.TEST, process.env.teST); |
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.
Actually, you can move the test for process.env.TEST
out of the if
.
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.
I updated the assertions. Please let me know if that's in line with what you had in mind.
Yeah that seems like something for @nodejs/platform-windows to look at |
Since it doesn't pertain to the PR, I suggest moving this discussion to #node-dev. |
Tested master, it compiles fine. I would suggest |
assert.strictEqual(process.env.test, 'test'); | ||
assert.strictEqual(process.env.teST, 'test'); | ||
} else { | ||
assert.strictEqual(undefined, process.env.test); |
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: can you please swap the arguments? The first one is actual
and second one expected
.
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.
Good call! Done.
@@ -708,6 +708,16 @@ console.log(process.env.TEST); | |||
// => undefined | |||
``` | |||
|
|||
Note that, on Windows operating systems, environment variables are case-insensitive. |
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.
Minor nit: Remove the Note that,
and just make it On Windows, environment variables are case-insensitive.
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.
Should be fixed now. Thanks
On Windows OS, environment variables are case-insensitive and are treated likewise in NodeJS. This can be confusing and can lead to hard-to-debug problems when moving code from one environment to another. Fixes: #9157
Environment variables should be treated case-insensitive on Windows platforms and case-sensitive on UNIX platforms
https://ci.nodejs.org/job/node-test-pull-request/4658/ If no one objects I'll land this if green. |
|
On Windows OS, environment variables are case-insensitive and are treated likewise in NodeJS. This can be confusing and can lead to hard-to-debug problems when moving code from one environment to another. Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> PR-URL: #9166 Fixes: #9157
Environment variables should be treated case-insensitive on Windows platforms and case-sensitive on UNIX platforms. This commit ensures this behavior persists. Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> PR-URL: #9166 Fixes: #9157
On Windows OS, environment variables are case-insensitive and are treated likewise in NodeJS. This can be confusing and can lead to hard-to-debug problems when moving code from one environment to another. Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> PR-URL: #9166 Fixes: #9157
Environment variables should be treated case-insensitive on Windows platforms and case-sensitive on UNIX platforms. This commit ensures this behavior persists. Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> PR-URL: #9166 Fixes: #9157
Environment variables should be treated case-insensitive on Windows platforms and case-sensitive on UNIX platforms. This commit ensures this behavior persists. Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> PR-URL: #9166 Fixes: #9157
On Windows OS, environment variables are case-insensitive and are treated likewise in NodeJS. This can be confusing and can lead to hard-to-debug problems when moving code from one environment to another. Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> PR-URL: #9166 Fixes: #9157
Environment variables should be treated case-insensitive on Windows platforms and case-sensitive on UNIX platforms. This commit ensures this behavior persists. Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> PR-URL: #9166 Fixes: #9157
On Windows OS, environment variables are case-insensitive and are treated likewise in NodeJS. This can be confusing and can lead to hard-to-debug problems when moving code from one environment to another. Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> PR-URL: #9166 Fixes: #9157
Checklist
make -j8 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
doc
Description of change
On Windows OS, environment variables are case-insensitive and are treated likewise in NodeJS. This can be confusing and can lead to hard-to-debug problems when moving code from one environment to another.
Closes #9157