test: eslint rule making common.js mandatory - #3157
Conversation
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
|
lgtm tho I'd suggest 2 commits here, one for eslint and one for the changes in test/ |
Sorry, something went wrong.
|
@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.) |
Sorry, something went wrong.
|
single PR is fine |
Sorry, something went wrong.
|
LGTM with one small comment. |
Sorry, something went wrong.
|
@cjihrig Rebased and pushed with the commented-out code removed. Thanks for catching that. |
Sorry, something went wrong.
|
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. :) |
Sorry, something went wrong.
|
@Fishrock123 It's not in an external repo yet, but I'm trying: eslint/eslint#4024 |
Sorry, something went wrong.
|
@Trott if it's not external can you please make it conform to our styles? |
Sorry, something went wrong.
|
(and I guess lint itself if it doesn't already) |
Sorry, something went wrong.
|
@Fishrock123 Cool. Linting of eslint-rules dir submitted as #3195. Style here changed so it will pass that linting. |
Sorry, something went wrong.
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.
|
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. |
Sorry, something went wrong.
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>
Sorry, something went wrong.
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 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.