test: make import common as the first line #7786
Conversation
|
We will need a custom ESLint rule if we want to enforce this. |
| @@ -1,10 +1,10 @@ | |||
| 'use strict'; | |||
| var common = require('../common'); | |||
There was a problem hiding this comment.
In cases like this, where you're moving the require('../common'), it might make sense to also move to const.
thefourtheye
force-pushed
the
import-common-first
branch
3 times, most recently
from
6e7cad9
to
6219a7a
Compare
The `test/common` module has the capability to identify if any variable is leaked to the global scope and fail the test. So that has to be imported at the beginning.
thefourtheye
force-pushed
the
import-common-first
branch
from
6219a7a
to
659160a
Compare
|
@targos True. I'll give it a shot, in the morning. |
|
@thefourtheye If you want to do it, probably the way to go is to add a configuration option to the current custom rule such that you can insist that the required modules are loaded before any other modules. |
|
LGTM. I think enforcing this via ESLint is a good idea, but it can be done in a different PR. |
|
LGTM |
|
LGTM @targos @thefourtheye I've got a modification to the existing custom lint rule ready to go to enforce this once this PR lands. Which it looks like this can land. (Been open for three days, CI is green, three LGTMs.) |
The `test/common` module has the capability to identify if any variable is leaked to the global scope and fail the test. So that has to be imported at the beginning. PR-URL: nodejs#7786 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
|
Landed in 6123075 |
|
Thanks for landing @Trott :) |
The `test/common` module has the capability to identify if any variable is leaked to the global scope and fail the test. So that has to be imported at the beginning. PR-URL: #7786 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
test
Description of change
The
test/commonmodule has the capability to identify if any variableis leaked to the global scope and fail the test. So that has to be
imported at the beginning.
cc @nodejs/testing