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: rule tester ignore irrelevant test case properties #18235

Merged
merged 3 commits into from Apr 2, 2024
Merged

fix: rule tester ignore irrelevant test case properties #18235

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e62558d
fix: ignore irrelevant test case properties for the duplication check
DMartens Mar 27, 2024
56d7198
fix: only ignore top-level test case properties
DMartens Mar 28, 2024
9a92dd1
chore: test case merge fixes
DMartens Apr 2, 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
14 changes: 12 additions & 2 deletions lib/rule-tester/rule-tester.js
View file
Open in desktop
Expand Up @@ -136,6 +136,15 @@ const suggestionObjectParameters = new Set([
]);
const friendlySuggestionObjectParameterList = `[${[...suggestionObjectParameters].map(key => `'${key}'`).join(", ")}]`;

/*
* Ignored test case properties when checking for test case duplicates.
*/
const duplicationIgnoredParameters = new Set([
"name",
"errors",
"output"
]);

const forbiddenMethods = [
"applyInlineConfig",
"applyLanguageOptions",
Expand Down Expand Up @@ -848,7 +857,7 @@ class RuleTester {

/**
* Check if this test case is a duplicate of one we have seen before.
* @param {Object} item test case object
* @param {string|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
Expand All @@ -863,7 +872,8 @@ class RuleTester {
return;
}

const serializedTestCase = stringify(item);
const normalizedItem = typeof item === "string" ? { code: item } : item;
const serializedTestCase = stringify(normalizedItem, { replacer: (key, value) => (!duplicationIgnoredParameters.has(key) ? value : void 0) });
Copy link
Member

Choose a reason for hiding this comment

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

We should only exclude top-level test case properties. Otherwise, if the rule has options with these names, test cases like the following would be mistakenly detected as duplicates:

{
    code: "var foo;",
    options: [{ name: "bar" }]
},
{
    code: "var foo;",
    options: [{ name: "baz" }]
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I resolved this via checking whether the current context object this is the root test case object.


assert(
!seenTestCases.has(serializedTestCase),
Expand Down
93 changes: 91 additions & 2 deletions tests/lib/rule-tester/rule-tester.js
View file
Open in desktop
Expand Up @@ -3148,7 +3148,7 @@ describe("RuleTester", () => {
}
}, {
valid: ["foo", "foo"],
invalid: [{ code: "foo", errors: [{ message: "foo bar" }] }]
invalid: []
});
}, "detected duplicate test case");
});
Expand All @@ -3162,10 +3162,57 @@ describe("RuleTester", () => {
}
}, {
valid: [{ code: "foo" }, { code: "foo" }],
invalid: [{ code: "foo", errors: [{ message: "foo bar" }] }]
invalid: []
});
}, "detected duplicate test case");
});

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

it("ignores the name property", () => {
assert.throws(() => {
ruleTester.run("foo", {
meta: {},
create() {
return {};
}
}, {
valid: [{ code: "foo" }, { name: "bar", code: "foo" }],
invalid: []
});
}, "detected duplicate test case");
});

it("does not throw an error for defining the same test case in different run calls", () => {
const rule = {
meta: {},
create() {
return {};
}
};

ruleTester.run("foo", rule, {
valid: ["foo"],
invalid: []
});

ruleTester.run("foo", rule, {
valid: ["foo"],
invalid: []
});
});
});

describe("invalid test cases", () => {
Expand Down Expand Up @@ -3328,6 +3375,48 @@ describe("RuleTester", () => {
]
});
});

it("detects duplicate test cases even if the error matchers differ", () => {
assert.throws(() => {
ruleTester.run("foo", {
meta: { schema: false },
create(context) {
return {
VariableDeclaration(node) {
context.report(node, "foo bar");
}
};
}
}, {
valid: [],
invalid: [
{ code: "const x = 123;", errors: [{ message: "foo bar" }] },
{ code: "const x = 123;", errors: 1 }
]
});
}, "detected duplicate test case");
});

it("detects duplicate test cases even if the presence of the output property differs", () => {
assert.throws(() => {
ruleTester.run("foo", {
meta: { schema: false },
create(context) {
return {
VariableDeclaration(node) {
context.report(node, "foo bar");
}
};
}
}, {
valid: [],
invalid: [
{ code: "const x = 123;", errors: 1 },
{ code: "const x = 123;", errors: 1, output: null }
]
});
}, "detected duplicate test case");
});
});
});

Expand Down
8 changes: 4 additions & 4 deletions tests/lib/rules/accessor-pairs.js
View file
Open in desktop
Expand Up @@ -1047,10 +1047,10 @@ ruleTester.run("accessor-pairs", rule, {

// Full location tests
{
code: "var o = { get a() {} };",
code: "var o = { get b() {} };",
aladdin-add marked this conversation as resolved.
Show resolved Hide resolved
options: [{ setWithoutGet: true, getWithoutSet: true }],
errors: [{
message: "Setter is not present for getter 'a'.",
message: "Setter is not present for getter 'b'.",
type: "Property",
line: 1,
column: 11,
Expand Down Expand Up @@ -1831,11 +1831,11 @@ ruleTester.run("accessor-pairs", rule, {
}]
},
{
code: "class A { static get a() {} };",
code: "class A { static get b() {} };",
aladdin-add marked this conversation as resolved.
Show resolved Hide resolved
options: [{ setWithoutGet: true, getWithoutSet: true, enforceForClassMembers: true }],
languageOptions: { ecmaVersion: 6 },
errors: [{
message: "Setter is not present for class static getter 'a'.",
message: "Setter is not present for class static getter 'b'.",
type: "MethodDefinition",
line: 1,
column: 11,
Expand Down
19 changes: 0 additions & 19 deletions tests/lib/rules/array-bracket-newline.js
View file
Open in desktop
Expand Up @@ -761,25 +761,6 @@ ruleTester.run("array-bracket-newline", rule, {
}
]
},
{
code: "var foo = [\n1\n];",
output: "var foo = [1];",
options: ["never"],
errors: [
{
messageId: "unexpectedOpeningLinebreak",
type: "ArrayExpression",
line: 1,
column: 11
},
{
messageId: "unexpectedClosingLinebreak",
type: "ArrayExpression",
line: 3,
column: 1
}
]
},
{
code: "var foo = [ /* any comment */\n1, 2\n];",
output: "var foo = [ /* any comment */\n1, 2];",
Expand Down
1 change: 0 additions & 1 deletion tests/lib/rules/array-callback-return.js
View file
Open in desktop
Expand Up @@ -273,7 +273,6 @@ ruleTester.run("array-callback-return", rule, {
{ code: "foo.forEach(function(x) { if (a == b) {return x;}})", options: checkForEachOptions, errors: [{ messageId: "expectedNoReturnValue", data: { name: "function", arrayMethodName: "Array.prototype.forEach" } }] },
{ code: "foo.forEach(function(x) { if (a == b) {return undefined;}})", options: checkForEachOptions, errors: [{ messageId: "expectedNoReturnValue", data: { name: "function", arrayMethodName: "Array.prototype.forEach" } }] },
{ code: "foo.forEach(function bar(x) { return x;})", options: checkForEachOptions, errors: [{ messageId: "expectedNoReturnValue", data: { name: "function 'bar'", arrayMethodName: "Array.prototype.forEach" } }] },
{ code: "foo.forEach(function bar(x) { return x;})", options: checkForEachOptions, errors: ["Array.prototype.forEach() expects no useless return value from function 'bar'."] },
{ code: "foo.bar().forEach(function bar(x) { return x;})", options: checkForEachOptions, errors: [{ messageId: "expectedNoReturnValue", data: { name: "function 'bar'", arrayMethodName: "Array.prototype.forEach" } }] },
{ code: "[\"foo\",\"bar\"].forEach(function bar(x) { return x;})", options: checkForEachOptions, errors: [{ messageId: "expectedNoReturnValue", data: { name: "function 'bar'", arrayMethodName: "Array.prototype.forEach" } }] },
{ code: "foo.forEach((x) => { return x;})", options: checkForEachOptions, languageOptions: { ecmaVersion: 6 }, errors: [{ messageId: "expectedNoReturnValue", data: { name: "arrow function", arrayMethodName: "Array.prototype.forEach" } }] },
Expand Down
4 changes: 0 additions & 4 deletions tests/lib/rules/constructor-super.js
View file
Open in desktop
Expand Up @@ -170,10 +170,6 @@ ruleTester.run("constructor-super", rule, {
},

// nested execution scope.
{
code: "class A extends B { constructor() { class C extends D { constructor() { super(); } } } }",
errors: [{ messageId: "missingAll", type: "MethodDefinition" }]
},
{
code: "class A extends B { constructor() { var c = class extends D { constructor() { super(); } } } }",
errors: [{ messageId: "missingAll", type: "MethodDefinition" }]
Expand Down
3 changes: 0 additions & 3 deletions tests/lib/rules/no-object-constructor.js
View file
Open in desktop
Expand Up @@ -191,9 +191,6 @@ ruleTester.run("no-object-constructor", rule, {
})),

...[

// No semicolon required before `({})` because ASI does not occur
{ code: "Object()" },
{
DMartens marked this conversation as resolved.
Show resolved Hide resolved
code: `
{}
Expand Down
24 changes: 3 additions & 21 deletions tests/lib/rules/no-unused-vars.js
View file
Open in desktop
Expand Up @@ -1461,21 +1461,16 @@ ruleTester.run("no-unused-vars", rule, {
// https://github.com/eslint/eslint/issues/10982
{
code: "var a = function() { a(); };",
errors: [assignedError("a")]
errors: [{ ...assignedError("a"), line: 1, column: 5 }]
},
{
code: "var a = function(){ return function() { a(); } };",
errors: [assignedError("a")]
},
{
code: "const a = () => { a(); };",
languageOptions: { ecmaVersion: 2015 },
errors: [assignedError("a")]
errors: [{ ...assignedError("a"), line: 1, column: 5 }]
},
{
code: "const a = () => () => { a(); };",
languageOptions: { ecmaVersion: 2015 },
errors: [assignedError("a")]
errors: [{ ...assignedError("a") }]
DMartens marked this conversation as resolved.
Show resolved Hide resolved
},
{
code: `let myArray = [1,2,3,4].filter((x) => x == 0);
Expand All @@ -1488,24 +1483,11 @@ ruleTester.run("no-unused-vars", rule, {
languageOptions: { ecmaVersion: 2015 },
errors: [{ ...assignedError("a"), line: 1, column: 14 }]
},
{
code: "var a = function() { a(); };",
errors: [{ ...assignedError("a"), line: 1, column: 5 }]
},
{
code: "var a = function(){ return function() { a(); } };",
errors: [{ ...assignedError("a"), line: 1, column: 5 }]
},
{
code: "const a = () => { a(); };",
languageOptions: { ecmaVersion: 2015 },
errors: [{ ...assignedError("a"), line: 1, column: 7 }]
},
{
code: "const a = () => () => { a(); };",
languageOptions: { ecmaVersion: 2015 },
errors: [{ ...assignedError("a"), line: 1, column: 7 }]
},

// https://github.com/eslint/eslint/issues/14324
{
Expand Down
4 changes: 2 additions & 2 deletions tests/lib/rules/no-useless-backreference.js
View file
Open in desktop
Expand Up @@ -162,9 +162,9 @@ ruleTester.run("no-useless-backreference", rule, {
}]
},
{
code: String.raw`/\k<foo>(?<foo>a)/`,
code: String.raw`/\k<foo>(?<foo>bar)/`,
aladdin-add marked this conversation as resolved.
Show resolved Hide resolved
errors: [{
message: String.raw`Backreference '\k<foo>' will be ignored. It references group '(?<foo>a)' which appears later in the pattern.`,
message: String.raw`Backreference '\k<foo>' will be ignored. It references group '(?<foo>bar)' which appears later in the pattern.`,
type: "Literal"
}]
},
Expand Down