Fix missing original env variable issue on multiple test cases. #3190
Conversation
|
copied my comments from #3191 lgtm I imagine the proper way to do this is with |
|
I think @cjihrig was suggesting using this approach env: util._extend(process.env, {NODE_DEBUG: process.argv[2]}) |
|
I forgot we have |
john-yan
force-pushed
the
master
branch
2 times, most recently
from
8a16df9
to
7e8844d
Compare
| @@ -16,7 +16,7 @@ var callbacks = 0; | |||
| function test(env, cb) { | |||
| var filename = path.join(common.fixturesDir, 'test-fs-readfile-error.js'); | |||
| var execPath = '"' + process.execPath + '" "' + filename + '"'; | |||
| var options = { env: env || {} }; | |||
| var options = { env: Object.assign(process.env, env) || {} }; | |||
There was a problem hiding this comment.
I think you can drop the || {} now.
| // env variable is passed to the child to make the test pass. | ||
| // this is fixed in the next version of lttng (2.7+), so we can | ||
| // remove it at sometime in the future. | ||
| HOME: process.env.HOME |
There was a problem hiding this comment.
Is HOME still required? Seems like it should get copied over now.
There was a problem hiding this comment.
Yeah, thanks, you are right. Home should be copied over. Fixed.
|
LGTM |
|
lgtm will land tomorrow |
|
Seeing a CI failure on freebsd (https://ci.nodejs.org/job/node-test-commit-other/854/nodes=freebsd101-64/console) that may be related to this commit. @john-yan @mhdawson ... can you please investigate before landing in master. If landed in master in time, I'll pick it back to v4.x but not sure it'll make it for v4.2.0 |
|
I've been seeing that test fail more often |
|
Pretty sure that test isn't related: #3193 |
|
Maybe do this instead: - Object.assign(process.env, { foo: expected })
+ Object.assign({}, process.env, { foo: expected }) |
|
Hello @LinusU , looks like they wanted to extend process.env instead. |
|
Hello @jasnell , Given that the changes in this commit has no global effect and no change has been made to the failing test cases in the CI, I don't think it's related. |
|
Ok. Thank you for clarifying. Running one more local test then will land on master. |
Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> PR-URL: #3190
|
Landed in a9d42e0 |
Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> PR-URL: #3190
When the parent spawn the child processes, the environment variables passing into the child processes are missing the original env variables passing into the parent. Some missing variables like LD_LIBRARY_PATH cause the child processes unable to run.
PR-URL: #3183