test: add env name into test, and replace comons.fixtureDir with comm.fixtures#15816
test: add env name into test, and replace comons.fixtureDir with comm.fixtures#15816shaohui-liu2000 wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
This could be using method fixtures.path instead of path.join.
4bb4807 to
26f3492
Compare
There was a problem hiding this comment.
Please have a look at fixtures.path definition you will see that you don't have to pass the fixturesDir anymore.
There was a problem hiding this comment.
This should be unchanged as it's not related to fixtures.
26f3492 to
0c38a4e
Compare
|
@pawelgolda PTAL |
|
@shaohui-liu2000 looks good. |
| process.on('exit', function() { | ||
| assert.ok(response.includes('HELLO=WORLD'), | ||
| 'spawn did not use process.env as default'); | ||
| `spawn did not use process.env as default(process.env.HELLO = ${process.env.HELLO})`); |
There was a problem hiding this comment.
The linter doesn't like this long line :-(
It can be fixed by shortening the line a bit or by adding an eslint directive to the top of the file (near 'use strict';) with something like /* eslint-disable max-len */ (you can see examples of this being done in other tests, such as test/parallel/test-readline-keys.js and test/parallel/test-require-extensions-same-filename-as-dir-trailing-slash.js.
There was a problem hiding this comment.
You could disable the rule, but you shouldn't ;-). The line needs wrapping. This can be seen locally by calling make eslint.
There was a problem hiding this comment.
Can you change this line to something like:
'spawn did not use process.env as default' +
`(process.env.HELLO = ${process.env.HELLO})`);
so we don't have to disable the eslint rule?
| 'use strict'; | ||
|
|
||
| const common = require('../common'); | ||
| const fixtures = require('../common/fixtures'); |
There was a problem hiding this comment.
Nit: can you please move this after the common.hasCrypto check?
There was a problem hiding this comment.
Uh..now after the update there are two fixtures. I don't think it can run with two const fixtures defined? Can you remove this line?
|
This should be landed as two commits. |
|
@sam-github @rmg @lpinca - PTAL. |
PR-URL: #15816 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #15816 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs/node#15816 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs/node#15816 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #15816 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #15816 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes(https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#commit-message-guidelines)
Affected core subsystem(s)
test