test: eslint rule making common.js mandatory #3157
Conversation
|
lgtm tho I'd suggest 2 commits here, one for eslint and one for the changes in test/ |
Trott
force-pushed
the
eslint-require-common
branch
from
6492f8b to
bedba46
Compare
|
@rvagg OK, I split it into two commits. (Let me know if I should split them into two PRs as well. Otherwise, I'll just apply the same commit log metadata to each commit before merging, assuming no one expresses disapproval of this PR in the next 24 hours or so.) |
Trott
force-pushed
the
eslint-require-common
branch
from
bedba46 to
01ddc83
Compare
|
single PR is fine |
|
LGTM with one small comment. |
Trott
force-pushed
the
eslint-require-common
branch
from
01ddc83 to
14fbbd4
Compare
|
@cjihrig Rebased and pushed with the commented-out code removed. Thanks for catching that. |
|
If it's an external module please put in a comment with that that points to the upstream repo. I think that's probably the best way. Still needs the EOF newline fixed though. :) |
Trott
force-pushed
the
eslint-require-common
branch
from
14fbbd4 to
9df2c4c
Compare
|
@Fishrock123 It's not in an external repo yet, but I'm trying: eslint/eslint#4024 |
Trott
force-pushed
the
eslint-require-common
branch
from
9df2c4c to
6d25723
Compare
|
@Trott if it's not external can you please make it conform to our styles? |
|
(and I guess lint itself if it doesn't already) |
|
@Fishrock123 Cool. Linting of |
common.js contains code that detects leaked variables. In preparation for an eslint rule that will enforce loading common.js in test files, load it everywhere it can be loaded and use an `eslint-disable` comment for files that intentionally leak.
test/common.js contains code that detects global variable leaks. This eslint rule checks that a module named `common` is loaded. It is only applicable to files in the test directory. Tests that intentionally leak variables can opt out with an eslint-disable comment.
Trott
force-pushed
the
eslint-require-common
branch
from
db84305 to
34fc281
Compare
|
One final CI run before landing: https://ci.nodejs.org/job/node-test-pull-request/431/ Assuming no red flags in the CI and no objections from anyone here, I'll land in the next few hours. |
common.js contains code that detects leaked variables. In preparation for an eslint rule that will enforce loading common.js in test files, load it everywhere it can be loaded and use an `eslint-disable` comment for files that intentionally leak. PR-URL: #3157 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
test/common.js contains code that detects global variable leaks. This eslint rule checks that a module named `common` is loaded. It is only applicable to files in the test directory. Tests that intentionally leak variables can opt out with an eslint-disable comment. PR-URL: #3157 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
common.js contains code that detects leaked variables. In preparation for an eslint rule that will enforce loading common.js in test files, load it everywhere it can be loaded and use an `eslint-disable` comment for files that intentionally leak. PR-URL: #3157 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
test/common.js contains code that detects global variable leaks. This eslint rule checks that a module named `common` is loaded. It is only applicable to files in the test directory. Tests that intentionally leak variables can opt out with an eslint-disable comment. PR-URL: #3157 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
test/common.js contains code that detects global variable leaks.
This eslint rule checks that a module named
commonis loaded. It isonly applicable to files in the test directory. Tests that intentionally
leak variables can opt out with an eslint-disable comment.