New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: handle files with unspecified path in getRulesMetaForResults
#16437
Changes from 3 commits
cf2c5a8
b756fda
31d8e86
90b7df2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3707,7 +3707,7 @@ describe("FlatESLint", () => { | |
|
||
describe("getRulesMetaForResults()", () => { | ||
|
||
it("should throw an error when results were not created from this instance", async () => { | ||
it("should throw an error when this instance did not lint any files", async () => { | ||
const engine = new FlatESLint({ | ||
overrideConfigFile: true | ||
}); | ||
|
@@ -3744,7 +3744,93 @@ describe("FlatESLint", () => { | |
"var err = doStuff();\nif (err) console.log('failed tests: ' + err);\nprocess.exit(1);\n" | ||
} | ||
]); | ||
}, /Results object was not created from this ESLint instance/u); | ||
}, { | ||
constructor: TypeError, | ||
message: "Results object was not created from this ESLint instance." | ||
}); | ||
}); | ||
|
||
it("should throw an error when results were created from a different instance", async () => { | ||
const engine1 = new FlatESLint({ | ||
overrideConfigFile: true, | ||
cwd: path.join(fixtureDir, "foo"), | ||
overrideConfig: { | ||
rules: { | ||
semi: 2 | ||
} | ||
} | ||
}); | ||
const engine2 = new FlatESLint({ | ||
overrideConfigFile: true, | ||
cwd: path.join(fixtureDir, "bar"), | ||
overrideConfig: { | ||
rules: { | ||
semi: 2 | ||
} | ||
} | ||
}); | ||
|
||
const results1 = await engine1.lintText("1", { filePath: "file.js" }); | ||
const results2 = await engine2.lintText("2", { filePath: "file.js" }); | ||
|
||
engine1.getRulesMetaForResults(results1); // should not throw an error | ||
assert.throws(() => { | ||
engine1.getRulesMetaForResults(results2); | ||
}, { | ||
constructor: TypeError, | ||
message: "Results object was not created from this ESLint instance." | ||
}); | ||
}); | ||
|
||
it("should treat a result without `filePath` as if the file was located in `cwd`", async () => { | ||
const engine = new FlatESLint({ | ||
overrideConfigFile: true, | ||
cwd: path.join(fixtureDir, "foo", "bar"), | ||
ignorePatterns: "*/**", // ignore all subdirectories of `cwd` | ||
overrideConfig: { | ||
rules: { | ||
eqeqeq: "warn" | ||
} | ||
} | ||
}); | ||
|
||
const results = await engine.lintText("a==b"); | ||
const rulesMeta = engine.getRulesMetaForResults(results); | ||
|
||
assert.deepStrictEqual(rulesMeta.eqeqeq, coreRules.get("eqeqeq").meta); | ||
}); | ||
|
||
it("should not throw an error if a result without `filePath` contains an ignored file warning", async () => { | ||
const engine = new FlatESLint({ | ||
overrideConfigFile: true, | ||
cwd: path.join(fixtureDir, "foo", "bar"), | ||
ignorePatterns: "**" | ||
}); | ||
|
||
const results = await engine.lintText("", { warnIgnored: true }); | ||
const rulesMeta = engine.getRulesMetaForResults(results); | ||
|
||
assert.deepStrictEqual(rulesMeta, {}); | ||
}); | ||
|
||
it("should not throw an error if results contain linted files and one ignored file", async () => { | ||
const engine = new FlatESLint({ | ||
overrideConfigFile: true, | ||
cwd: getFixturePath(), | ||
ignorePatterns: "passing*", | ||
overrideConfig: { | ||
rules: { | ||
"no-undef": 2, | ||
semi: 1 | ||
} | ||
} | ||
}); | ||
|
||
const results = await engine.lintFiles(["missing-semicolon.js", "passing.js", "undef.js"]); | ||
const rulesMeta = engine.getRulesMetaForResults(results); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we verify that a file is actually being ignored here before doing the other assertions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I've added an assertion to make sure that one of the messages returned by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This works. 👍 |
||
|
||
assert.deepStrictEqual(rulesMeta["no-undef"], coreRules.get("no-undef").meta); | ||
assert.deepStrictEqual(rulesMeta.semi, coreRules.get("semi").meta); | ||
}); | ||
|
||
it("should return empty object when there are no linting errors", async () => { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There may be an edge case here. getConfig returns undefined if a file is ignore by the configuration, so it doesn’t necessarily mean that the file wasn’t run in this instance. I think this will result in an error if the results contain a warning for a file that was ignored.
You can test this by passing in a file that is specifically ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I also thought about that. Now that #16402 has been fixed, the specific edge case you mention should be handled correctly. I will rebase the branch once again and add a new unit test to check the behavior in case an explicitly ignored file appears in the results when
warnIgnored
is set. Thanks for the feedback!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a new unit test for when an anonymous file is ignored and linted with
warnIgnored
. A similar case, where the file has a specified path, is already covered in another test:eslint/tests/lib/eslint/flat-eslint.js
Lines 3864 to 3867 in 740b208
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add a test with
lintFiles()
as well. We need to make sure that if there are multiple files linted and one ignored that this won't throw an error.My concern:
isFileIgnored()
just doesgetConfig() === undefined
, so what you are really testing here is if a file is ignored. Because of that, it still seems like there's an edge case here that will bite us.I think it would be safe to remove this check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignored file results, as they don't have any messages with a
ruleId
, should be skipped by thecontinue
statement a few lines above.The point of checking if a config is found here is to avoid results linted by a different engine. If a result was not linted by the current engine, it may have no config without being ignored. This specific case is covered in this test. If this check is removed,
getRulesMetaForResults
will still fail, but with a non-specific error message (becauseconfig.plugins
cannot be accessed at some point).Adding a test with linted files and an ignored file at the same time is a good idea. There are no tests with multiple results so far. Thanks for the suggestion!