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

feat!: detect duplicate test cases #17955

Merged
merged 16 commits into from
Jan 20, 2024
Merged

feat!: detect duplicate test cases #17955

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
fbce335
feat!: detect duplicate test cases
bmish Jan 4, 2024
85b5070
Merge branch 'main' into duplicate-test-cases
bmish Jan 12, 2024
a208201
Merge branch 'main' into duplicate-test-cases
bmish Jan 16, 2024
1f49b10
remove some duplicate test cases
bmish Jan 16, 2024
821ab1d
add more tests
bmish Jan 16, 2024
cde2d1b
better detection of duplicate test cases
bmish Jan 16, 2024
bc1b483
fix more rule tests
bmish Jan 16, 2024
a784f4d
Update tests/lib/rules/no-misleading-character-class.js
bmish Jan 17, 2024
b400d1e
add function to check if an object is serializable
bmish Jan 17, 2024
3a7d588
tweak name
bmish Jan 17, 2024
6cf0fad
add to migration guide
bmish Jan 17, 2024
bd687c8
tweak wording
bmish Jan 17, 2024
b345788
add test cases
bmish Jan 18, 2024
891d761
remove lodash.isplainobject
bmish Jan 19, 2024
6467cda
rename obj to val
bmish Jan 20, 2024
7a34ae7
add obj check
bmish Jan 20, 2024
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
33 changes: 32 additions & 1 deletion lib/rule-tester/rule-tester.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ const
equal = require("fast-deep-equal"),
Traverser = require("../shared/traverser"),
{ getRuleOptionsSchema } = require("../config/flat-config-helpers"),
{ Linter, SourceCodeFixer, interpolate } = require("../linter");
{ Linter, SourceCodeFixer, interpolate } = require("../linter"),
stringify = require("json-stable-stringify-without-jsonify");

const { FlatConfigArray } = require("../config/flat-config-array");
const { defaultConfig } = require("../config/default-config");
Expand Down Expand Up @@ -499,6 +500,9 @@ class RuleTester {
linter = this.linter,
ruleId = `rule-to-test/${ruleName}`;

const seenValidTestCases = new Set();
const seenInvalidTestCases = new Set();
bmish marked this conversation as resolved.
Show resolved Hide resolved

if (!rule || typeof rule !== "object" || typeof rule.create !== "function") {
throw new TypeError("Rule must be an object with a `create` method");
}
Expand Down Expand Up @@ -794,6 +798,29 @@ class RuleTester {
}
}

/**
* Check if this test case is a duplicate of one we have seen before.
* @param {Object} item test case object
* @param {Set<string>} seenTestCases set of serialized test cases we have seen so far (managed by this function)
* @returns {void}
* @private
*/
function checkDuplicateTestCase(item, seenTestCases) {
if ((Array.isArray(item.options) && item.options.some(i => typeof i === "object")) || item.settings || item.languageOptions?.parser || item.languageOptions?.parserOptions || item.plugins) {
Copy link
Member

Choose a reason for hiding this comment

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

there is a room to improve it, but I think it's fine to just check these properties 👍 - as it's described in the rfc.

Copy link
Sponsor Member Author

@bmish bmish Jan 17, 2024 •
edited

Choose a reason for hiding this comment

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

It's a start.

However, I would love it if I could replace this bail-out logic with a simple function call to see if a given object is serializable. Surprisingly, I haven't found a third-party library to do this. Another option is to try to piece together such a function based on StackOverflow snippets (https://stackoverflow.com/questions/30579940/reliable-way-to-check-if-objects-is-serializable-in-) etc and add unit tests for it. Thoughts?

It might be best to solve this the right way now (in a way that avoids a lot of false negatives) since changing this later could be a breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

IMHO, it's not worth adding a new dependency: rule tester is shipped with eslint, but most eslint users don't really use it. A simple implementation to cover 90%+ cases is good enough for me. 😄

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

I went ahead and added a isSerializable() function which uses only the tiny lodash.isplainobject as a dependency. While the previous approach could have been sufficient as you mentioned, this new approach is a lot more robust (no false negatives) and the code is cleaner.


// Don't check for duplicates if the test case has any properties that could be non-serializable.
return;
}

const serializedTestCase = stringify(item);

assert(
!seenTestCases.has(serializedTestCase),
"detected duplicate test case"
);
seenTestCases.add(serializedTestCase);
}

/**
* Check if the template is valid or not
* all valid cases go through this
Expand All @@ -809,6 +836,8 @@ class RuleTester {
assert.ok(typeof item.name === "string", "Optional test case property 'name' must be a string");
}

checkDuplicateTestCase(item, seenValidTestCases);

const result = runRuleForItem(item);
const messages = result.messages;

Expand Down Expand Up @@ -860,6 +889,8 @@ class RuleTester {
assert.fail("Invalid cases must have at least one error");
}

checkDuplicateTestCase(item, seenInvalidTestCases);

const ruleHasMetaMessages = hasOwnProperty(rule, "meta") && hasOwnProperty(rule.meta, "messages");
const friendlyIDList = ruleHasMetaMessages ? `[${Object.keys(rule.meta.messages).map(key => `'${key}'`).join(", ")}]` : null;

Expand Down
192 changes: 192 additions & 0 deletions tests/lib/rule-tester/rule-tester.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2830,6 +2830,198 @@ describe("RuleTester", () => {

});

describe("duplicate test cases", () => {
describe("valid test cases", () => {
it("throws with duplicate string test cases", () => {
assert.throws(() => {
ruleTester.run("foo", {
meta: {},
create() {
return {};
}
}, {
valid: ["foo", "foo"],
invalid: [{ code: "foo", errors: [{ message: "foo bar" }] }]
});
}, "detected duplicate test case");
});

it("throws with duplicate object test cases", () => {
assert.throws(() => {
ruleTester.run("foo", {
meta: {},
create() {
return {};
}
}, {
valid: [{ code: "foo" }, { code: "foo" }],
invalid: [{ code: "foo", errors: [{ message: "foo bar" }] }]
});
}, "detected duplicate test case");
});
});

describe("invalid test cases", () => {
it("throws with duplicate object test cases", () => {
assert.throws(() => {
ruleTester.run("foo", {
meta: {},
create(context) {
return {
VariableDeclaration(node) {
context.report(node, "foo bar");
}
};
}
}, {
valid: ["foo"],
invalid: [
{ code: "const x = 123;", errors: [{ message: "foo bar" }] },
{ code: "const x = 123;", errors: [{ message: "foo bar" }] }
]
});
}, "detected duplicate test case");
});

it("throws with duplicate object test cases when options is a primitive", () => {
assert.throws(() => {
ruleTester.run("foo", {
meta: { schema: false },
create(context) {
return {
VariableDeclaration(node) {
context.report(node, "foo bar");
}
};
}
}, {
valid: ["foo"],
invalid: [
{ code: "const x = 123;", errors: [{ message: "foo bar" }], options: ["abc"] },
{ code: "const x = 123;", errors: [{ message: "foo bar" }], options: ["abc"] }
]
});
}, "detected duplicate test case");
});

it("throws with duplicate object test cases even when property order differs", () => {
assert.throws(() => {
ruleTester.run("foo", {
meta: {},
create(context) {
return {
VariableDeclaration(node) {
context.report(node, "foo bar");
}
};
}
}, {
valid: ["foo"],
invalid: [
{ code: "const x = 123;", errors: [{ message: "foo bar" }] },
{ errors: [{ message: "foo bar" }], code: "const x = 123;" }
]
});
}, "detected duplicate test case");
});

it("ignores duplicate test case when potentially non-serializable property (e.g. settings) present", () => {
ruleTester.run("foo", {
meta: {},
create(context) {
return {
VariableDeclaration(node) {
context.report(node, "foo bar");
}
};
}
}, {
valid: ["foo"],
invalid: [
{ code: "const x = 123;", errors: [{ message: "foo bar" }], settings: {} },
{ code: "const x = 123;", errors: [{ message: "foo bar" }], settings: {} }
]
});
});

it("ignores duplicate test case when potentially non-serializable property present (e.g. settings)", () => {
ruleTester.run("foo", {
meta: {},
create(context) {
return {
VariableDeclaration(node) {
context.report(node, "foo bar");
}
};
}
}, {
valid: ["foo"],
invalid: [
{ code: "const x = 123;", errors: [{ message: "foo bar" }], settings: { foo: /abc/u } },
{ code: "const x = 123;", errors: [{ message: "foo bar" }], settings: { foo: /abc/u } }
]
});
});

it("ignores duplicate test case when potentially non-serializable property present (languageOptions.parserOptions)", () => {
ruleTester.run("foo", {
meta: {},
create(context) {
return {
VariableDeclaration(node) {
context.report(node, "foo bar");
}
};
}
}, {
valid: ["foo"],
invalid: [
{ code: "const x = 123;", errors: [{ message: "foo bar" }], languageOptions: { parserOptions: { foo: /abc/u } } },
{ code: "const x = 123;", errors: [{ message: "foo bar" }], languageOptions: { parserOptions: { foo: /abc/u } } }
]
});
});

it("ignores duplicate test case when potentially non-serializable property present (plugins)", () => {
ruleTester.run("foo", {
meta: {},
create(context) {
return {
VariableDeclaration(node) {
context.report(node, "foo bar");
}
};
}
}, {
valid: ["foo"],
invalid: [
{ code: "const x = 123;", errors: [{ message: "foo bar" }], plugins: { foo: /abc/u } },
{ code: "const x = 123;", errors: [{ message: "foo bar" }], plugins: { foo: /abc/u } }
]
});
});

it("ignores duplicate test case when potentially non-serializable property present (options as an object)", () => {
ruleTester.run("foo", {
meta: { schema: false },
create(context) {
return {
VariableDeclaration(node) {
context.report(node, "foo bar");
}
};
}
}, {
valid: ["foo"],
invalid: [
{ code: "const x = 123;", errors: [{ message: "foo bar" }], options: [{ foo: /abc/u }] },
{ code: "const x = 123;", errors: [{ message: "foo bar" }], options: [{ foo: /abc/u }] }
]
});
});
});
});

describe("SourceCode forbidden methods", () => {

[
Expand Down
8 changes: 0 additions & 8 deletions tests/lib/rules/indent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4624,14 +4624,6 @@ ruleTester.run("indent", rule, {
`,
options: [0]
},
{
code: unIndent`
<App
\tfoo
/>
`,
options: ["tab"]
},
unIndent`
<App
foo
Expand Down
14 changes: 0 additions & 14 deletions tests/lib/rules/multiline-comment-style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1262,20 +1262,6 @@ ${" "}
options: ["starred-block"],
errors: [{ messageId: "expectedBlock", line: 2 }]
},
{
code: `
// foo
//
// bar
`,
output: `
/* foo
${" "}
bar */
`,
options: ["bare-block"],
errors: [{ messageId: "expectedBlock", line: 2 }]
},
{
code: `
// foo
Expand Down
4 changes: 2 additions & 2 deletions tests/lib/rules/newline-after-var.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ const NO_VAR = "console.log(greet);",
FOR_LOOP_WITH_VAR = "for(var a = 1; a < 1; a++){\n break;\n}",
FOR_IN_LOOP_WITH_LET = "for(let a in obj){\n break;\n}",
FOR_IN_LOOP_WITH_VAR = "for(var a in obj){\n break;\n}",
FOR_OF_LOOP_WITH_LET = "for(let a in obj){\n break;\n}",
FOR_OF_LOOP_WITH_VAR = "for(var a in obj){\n break;\n}",
FOR_OF_LOOP_WITH_LET = "for(let a of obj){\n break;\n}",
FOR_OF_LOOP_WITH_VAR = "for(var a of obj){\n break;\n}",
EXPORT_WITH_LET = "export let a = 1;\nexport let b = 2;",
EXPORT_WITH_VAR = "export var a = 1;\nexport var b = 2;",
EXPORT_WITH_CONST = "export const a = 1;\nexport const b = 2;",
Expand Down
14 changes: 4 additions & 10 deletions tests/lib/rules/no-invalid-this.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,10 @@ const patterns = [
invalid: [USE_STRICT, IMPLIED_STRICT, MODULES]
},

// Just functions.
/*
* Just functions.
* https://github.com/eslint/eslint/issues/3254
*/
{
code: "function foo() { console.log(this); z(x => console.log(x, this)); }",
languageOptions: { ecmaVersion: 6 },
Expand Down Expand Up @@ -588,15 +591,6 @@ const patterns = [
invalid: []
},

// https://github.com/eslint/eslint/issues/3254
{
code: "function foo() { console.log(this); z(x => console.log(x, this)); }",
languageOptions: { ecmaVersion: 6 },
errors,
valid: [NORMAL],
invalid: [USE_STRICT, IMPLIED_STRICT, MODULES]
},

// https://github.com/eslint/eslint/issues/3287
{
code: "function foo() { /** @this Obj*/ return function bar() { console.log(this); z(x => console.log(x, this)); }; }",
Expand Down
27 changes: 2 additions & 25 deletions tests/lib/rules/no-misleading-character-class.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ ruleTester.run("no-misleading-character-class", rule, {
column: 11,
endColumn: 13,
messageId: "surrogatePairWithoutUFlag",
suggestions: null // pattern would be invalid with the 'u' flag
suggestions: null // pattern would be invalid with the 'u' flag, ecmaVersion doesn't support the 'u' flag
}]
},
{
Expand Down Expand Up @@ -148,24 +148,14 @@ ruleTester.run("no-misleading-character-class", rule, {
suggestions: [{ messageId: "suggestUnicodeFlag", output: "var r = /\\uDC4D[\\uD83D\\uDC4D]/u" }]
}]
},
{
code: "var r = /[👍]/",
languageOptions: { ecmaVersion: 3, sourceType: "script" },
errors: [{
column: 11,
endColumn: 13,
messageId: "surrogatePairWithoutUFlag",
suggestions: null // ecmaVersion doesn't support the 'u' flag
}]
},
{
code: "var r = /[👍]/",
languageOptions: { ecmaVersion: 5, sourceType: "script" },
errors: [{
column: 11,
endColumn: 13,
messageId: "surrogatePairWithoutUFlag",
suggestions: null // ecmaVersion doesn't support the 'u' flag
suggestions: null // ecmaVersion doesn't support the 'u' flag, ecmaVersion doesn't support the 'u' flag
bmish marked this conversation as resolved.
Show resolved Hide resolved
}]
},
{
Expand Down Expand Up @@ -1357,19 +1347,6 @@ ruleTester.run("no-misleading-character-class", rule, {
suggestions: null
}]
},
{
code: "var r = /[👍]/",
languageOptions: {
ecmaVersion: 5,
sourceType: "script"
},
errors: [{
column: 11,
endColumn: 13,
messageId: "surrogatePairWithoutUFlag",
suggestions: null // ecmaVersion doesn't support the 'u' flag
}]
},
{
code: "var r = /[👍]/",
languageOptions: {
Expand Down