Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: ignore messages without a ruleId in getRulesMetaForResults #16409

Merged
merged 3 commits into from Oct 19, 2022
Merged

fix: ignore messages without a ruleId in getRulesMetaForResults #16409

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
72cab02
fix: ignore messages without a `ruleId` in `getRulesMetaForResults`
fasttime Oct 11, 2022
61768c8
Update unit tests as suggested
fasttime Oct 14, 2022
1d6060c
Explicitly ignore a file path in a unit test for `getRulesMetaForResu…
fasttime Oct 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/eslint/flat-eslint.js
View file
Open in desktop
Expand Up @@ -700,6 +700,10 @@ class FlatESLint {
const allMessages = result.messages.concat(result.suppressedMessages);

for (const { ruleId } of allMessages) {
if (!ruleId) {
continue;
}

const rule = getRuleFromConfig(ruleId, config);

// ensure the rule exists
Expand Down
42 changes: 42 additions & 0 deletions tests/lib/eslint/eslint.js
View file
Open in desktop
Expand Up @@ -4987,6 +4987,7 @@ describe("ESLint", () => {
const results = await engine.lintText("a");
const rulesMeta = engine.getRulesMetaForResults(results);

assert.strictEqual(Object.keys(rulesMeta).length, 1);
assert.strictEqual(rulesMeta.semi, coreRules.get("semi").meta);
});

Expand All @@ -5003,6 +5004,7 @@ describe("ESLint", () => {
const results = await engine.lintText("a // eslint-disable-line semi");
const rulesMeta = engine.getRulesMetaForResults(results);

assert.strictEqual(Object.keys(rulesMeta).length, 1);
assert.strictEqual(rulesMeta.semi, coreRules.get("semi").meta);
});

Expand Down Expand Up @@ -5051,6 +5053,46 @@ describe("ESLint", () => {
nodePlugin.rules["no-new-require"].meta
);
});

it("should ignore messages not related to a rule", async () => {
const engine = new ESLint({
useEslintrc: false,
overrideConfig: { rules: { "no-var": "warn" } },
reportUnusedDisableDirectives: "warn"
});

{
const results = await engine.lintText("syntax error");
const rulesMeta = engine.getRulesMetaForResults(results);

assert.deepStrictEqual(rulesMeta, {});
}
{
const results = await engine.lintText("// eslint-disable-line no-var");
const rulesMeta = engine.getRulesMetaForResults(results);

assert.deepStrictEqual(rulesMeta, {});
}
{
const results = await engine.lintText("", { filePath: "/.ignored.js", warnIgnored: true });
const rulesMeta = engine.getRulesMetaForResults(results);

assert.deepStrictEqual(rulesMeta, {});
}
});

it("should return a non-empty value if some of the messages are related to a rule", async () => {
const engine = new ESLint({
useEslintrc: false,
overrideConfig: { rules: { "no-var": "warn" } },
reportUnusedDisableDirectives: "warn"
});

const results = await engine.lintText("// eslint-disable-line no-var\nvar foo;");
const rulesMeta = engine.getRulesMetaForResults(results);

assert.deepStrictEqual(rulesMeta, { "no-var": coreRules.get("no-var").meta });
});
});

describe("outputFixes()", () => {
Expand Down
58 changes: 58 additions & 0 deletions tests/lib/eslint/flat-eslint.js
View file
Open in desktop
Expand Up @@ -3660,6 +3660,24 @@ describe("FlatESLint", () => {
const results = await engine.lintText("a", { filePath: "foo.js" });
const rulesMeta = engine.getRulesMetaForResults(results);

assert.strictEqual(Object.keys(rulesMeta).length, 1);
assert.strictEqual(rulesMeta.semi, coreRules.get("semi").meta);
});

it("should return one rule meta when there is a suppressed linting error", async () => {
const engine = new FlatESLint({
overrideConfigFile: true,
overrideConfig: {
rules: {
semi: 2
}
}
});

const results = await engine.lintText("a // eslint-disable-line semi");
const rulesMeta = engine.getRulesMetaForResults(results);

assert.strictEqual(Object.keys(rulesMeta).length, 1);
assert.strictEqual(rulesMeta.semi, coreRules.get("semi").meta);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it also make sense to ensure that there is only one key in rulesMeta? (Based on the name of this test.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added an assertion to make sure that there is only one key to the two unit tests that mention "one rule meta" in the title. Thanks @nzakas!

});

Expand Down Expand Up @@ -3707,6 +3725,46 @@ describe("FlatESLint", () => {
nodePlugin.rules["no-new-require"].meta
);
});

it("should ignore messages not related to a rule", async () => {
const engine = new FlatESLint({
overrideConfigFile: true,
overrideConfig: { rules: { "no-var": "warn" } },
reportUnusedDisableDirectives: "warn"
});

{
const results = await engine.lintText("syntax error");
const rulesMeta = engine.getRulesMetaForResults(results);

assert.deepStrictEqual(rulesMeta, {});
}
{
const results = await engine.lintText("// eslint-disable-line no-var");
const rulesMeta = engine.getRulesMetaForResults(results);

assert.deepStrictEqual(rulesMeta, {});
}
{
const results = await engine.lintText("", { filePath: "/.ignored.js", warnIgnored: true });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use some explicitly ignored file path instead of /.ignored.js? /.ignored.js will indeed most likely be treated as an ignored file due to being outside of cwd, but that still depends on how the environment where the tests are run is set up. It may also be ignored because it's a dotfile, but it is not yet certain how dotfiles will be treated. For the purposes of this test, I think it would be better to avoid these assumptions. We can, for example, add ignorePatterns: ["ignored.js"] to new FlatESLint({ ... }) and use filePath: "ignored.js".

(the same for the eslint.js test)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated as suggested, thanks!

const rulesMeta = engine.getRulesMetaForResults(results);

assert.deepStrictEqual(rulesMeta, {});
}
});

it("should return a non-empty value if some of the messages are related to a rule", async () => {
const engine = new FlatESLint({
overrideConfigFile: true,
overrideConfig: { rules: { "no-var": "warn" } },
reportUnusedDisableDirectives: "warn"
});

const results = await engine.lintText("// eslint-disable-line no-var\nvar foo;");
const rulesMeta = engine.getRulesMetaForResults(results);

assert.deepStrictEqual(rulesMeta, { "no-var": coreRules.get("no-var").meta });
});
});

describe("outputFixes()", () => {
Expand Down