Upgrade ESLint to 6.x #3181
Upgrade ESLint to 6.x #3181
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3181 +/- ##
=======================================
Coverage 99.89% 99.89%
=======================================
Files 58 57 -1
Lines 1963 1981 +18
Branches 413 414 +1
=======================================
+ Hits 1961 1979 +18
Misses 2 2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
@willdurand, thanks a lot for getting this started, I agree with you that it looks very likely that we will have to rethink part of the addons-linter (at least the part more closely related to the eslint internals) and choose the strategy that has the higher chances to not be breaking again too easily in the near future.
I'll come back to this asap, I have to look a bit more closely into these changes to be able to provide a more detailed feedback, think about the potential side-effects they may have and what other strategies we may have, if any (you analysis seems pretty convincing, but I wasn't involved in this project when we did the initial research and took the current design choices, and so I want to dig a bit to have some more detailed opinions and be able to provide a better feedback).
willdurand
force-pushed
the
eslint-6-test
branch
8 times, most recently
from
3c50691
to
ea40fee
Compare
willdurand
force-pushed
the
eslint-6-test
branch
from
55473b0
to
1108dfd
Compare
There was a problem hiding this comment.
@willdurand I spent some time looking a bit more into possible other cons of the current approach drafted in this PR, and I think that the only one that we may want to find a solution for is the fact that this version isn't yet able to collect coverage for the js rules files.
The test for the custom rules defined in this repository are actually being tested, but eslint is loading the webpack bundled modules and jest isn't instrumenting and collecting coverage for them.
Let me know what to you think about the proposed strategy.
Collect coverage data from the custom rules modules
(NOTE: I pushed the changes described below in my fork, in case you want to give it a try quickly by just importing the patch: rpl@3864f80)
I looked into some options to collect that additional coverage data, but the only one that I could think of and that also works as expected without too much work is the following one:
- make the rule modules loadable correctly when "eslint is requiring them during the test case" (which is internally using babel-jest) and when "eslint is requiring them in production" (which will load them from the dist/rules/javascript dir as in this version of the PR), the underlying issue seems to be mainly that babel-jest is returning to eslint require calls a module object that looks like
{ default: { create: Function } }but eslint expects the modules to be objects that looks like{ create: Function }, the simplest way to make a valid es6 module to work in both cases as expected seems to be doing something like the following:
const rule = {
create(context) {
...
},
};
export default rule;
export const { create } = rule;- in
"tests/unit/helpers.js"we could then export a test helper to help us with making sure that thejsScanneris going to use the rule sources (instead of the bundling ones) in the unit tests:
export function runJsScanner(
jsScanner,
{ fixtureRules = [], scanOptions } = {}
) {
// global.appRoot is being set in tests/setup.js.
const ruleSource = path.join(global.appRoot, 'src/rules/javascript');
const fixturePaths = fixtureRules.map(getJsRulePathForRule);
return jsScanner.scan({
...scanOptions,
_rulePaths: [ruleSource].concat(fixturePaths),
});
}- Then use the
runJsScannerhelper in the tests where we want to collect coverage data from the custom rules and where we need to load mock eslint rules.
I did try this locally and looking to the coverage summary it seems that this would be enough to get back a very good chunk of our custom rules test coverage data.
The changes to the rule modules are actually not that different from what you were originally doing (converting them into commonjs modules), but I think that it is actually good that we do not load in production anything that isn't actually coming from the dist directory, and transpiling the eslint rule into their own commonjs modules in dist/rules/javascript sounds still the right approach to me.
Adding a new integration test
I think that it would also good to add one integration test, at least one smoke that that would break if the rules in dist/rules would be missing and eslint fail to load them, e.g. something like the following test case defined in a new test file added in "tests/integration/addons-linter":
it('linter should pass if ran on a simple valid extension', async () => {
const fixture = path.resolve(
__dirname,
'..',
'..',
'fixtures',
'webextension_es6_module'
);
const { exitCode, stderr, stdout } = await executeScript('addons-linter', [
'-o',
'json',
fixture,
]);
expect(stdout).toContain('"summary":{"errors":0,"notices":0,"warnings":0}');
expect(stderr).toBe('');
expect(exitCode).toBe(0);
});
willdurand
force-pushed
the
eslint-6-test
branch
from
1108dfd
to
74b25e6
Compare
There was a problem hiding this comment.
Thanks @willdurand, this looks good to me. I'm wondering about that dotfiles: true config that was used in the previous version and if we do have coverage for that behavior, would you mind to double-check that?
Besides that, follows some small nits related to some tests and the webpack config.
As a side note, it may be good to release this change as part of a major version bump (to be fair I think that we did already discussed briefly about this and we both agreed about that, but I thought to mention it in case I was wrong and we didn't actually talk about it yet).
| // Also, don't ignore dotfiles in scans. | ||
| dotfiles: true, |
There was a problem hiding this comment.
@willdurand did you checked if we do have coverage to be sure that dotfiles are not ignored in the scans?
There was a problem hiding this comment.
Good call, let me double check.
…tom rules sources
Yes, that's what we decided last week :D |
willdurand
force-pushed
the
eslint-6-test
branch
from
7017a4b
to
3cd311b
Compare
| const code = oneLine`var a;`; | ||
|
|
||
| const jsScanner = new JavaScriptScanner( | ||
| code, | ||
| 'node_modules/module/code.js' | ||
| ); | ||
|
|
||
| const { linterMessages } = await jsScanner.scan(); | ||
| expect(linterMessages.length).toEqual(0); |
There was a problem hiding this comment.
@willdurand Thanks for double-checking this and adding new tests to cover it.
While looking to these new test cases I was wondering:
Are these test going to fail without the related fix applied to src/scanners/javascript.js?
Which kind of error they trigger in that case?
maybe we could put into code something that would trigger a validation error or warning on purpose, and then we could assert the expected linting message to be absolutely sure that the file has been scanned as expected and not been skipped.
How that sounds to you?
There was a problem hiding this comment.
Test cases fail indeed. They raise the error mentioned in the original issue:
- Array []
+ Array [
+ Object {
+ "code": "File ignored by default. Use a negated ignore pattern (like \"--ignore-pattern '!<relative/path/to/filename>'\") to override.",
+ "column": undefined,
+ "description": null,
+ "file": ".code.js",
+ "line": undefined,
+ "message": "File ignored by default. Use a negated ignore pattern (like \"--ignore-pattern '!<relative/path/to/filename>'\") to override.",
+ "sourceCode": undefined,
+ "type": "warning",
+ },
+ ]
| const code = 'el.innerHTML = evilContent'; | ||
|
|
||
| const jsScanner = new JavaScriptScanner( | ||
| code, | ||
| 'node_modules/module/code.js' | ||
| ); | ||
|
|
||
| const { linterMessages } = await jsScanner.scan(); | ||
| expect(linterMessages[0]).toMatchObject({ code: 'UNSAFE_VAR_ASSIGNMENT' }); |
There was a problem hiding this comment.
@willdurand thanks for making this assertion a bit more explicit! ❤️
lgtm, r+ renewed
|
Thanks @willdurand for the patch and @rpl for the reviews! |
This patch replaces #2652.
Update 2020.06.10: I updated the code again with less changes and re-enabled all the tests that were skipped in the previous iteration. This patch works locally with or without bundling, and it should be good enough for a review. Thanks!
I don't know if that's the right approach but that's something at least. In short, it's tricky to update to 6.x (or even 7.x) because of the way we use both ESLint
cliandlinter.As of v6, we cannot access or modify the
linterinstance of aCLIEngineinstance (socli.linter.defineRuledoes not work anymore). We could create our ownLinterinstance but it does not work very well (see: eslint/eslint#12689 (comment) but also https://eslint.org/docs/user-guide/migrating-to-6.0.0#linter-parsers).We need the
linterinstance to define rules (usingdefineRule) but that's not possible using theCLIEngine, which is the recommended Node API for ESLint v6 (we'll have to change that for v7 but that's a different story).So we cannot use
defineRule. No problem, we can tellCLIEngineto load the rules for us. That's what is done in this patch and it seems to work based on the test suite. The problem with this approach is that we cannot inject fake rule definitions anymore and that's why I skipped a few test cases.I don't think we can bring this behavior back easily but I dunno, maybe I am missing something obvious (I spent more time than I wished on this patch already).
I have the feeling that we should stop using the
Linterinstance as well as registering the actual rule definitions. Instead, we should configure rule names and paths, and ESLint would load the rules on its own. Hence this patch.WDYT?