feat!: detect duplicate test cases (#17955)

* feat!: detect duplicate test cases Part of: https://github.com/eslint/rfcs/tree/main/designs/2021-stricter-rule-test-validation#disallow-identical-test-cases * remove some duplicate test cases * add more tests * better detection of duplicate test cases * fix more rule tests * Update tests/lib/rules/no-misleading-character-class.js * add function to check if an object is serializable * tweak name * add to migration guide * tweak wording * add test cases * remove lodash.isplainobject * rename obj to val * add obj check
eslint · Jan 20, 2024 · 2e1d549 · 2e1d549
1 parent 1fedfd2
commit 2e1d549
Show file tree

Hide file tree

Showing 12 changed files with 398 additions and 64 deletions.
diff --git a/docs/src/use/migrate-to-9.0.0.md b/docs/src/use/migrate-to-9.0.0.md
@@ -463,10 +463,11 @@ In order to aid in the development of high-quality custom rules that are free fr
 
 1. **Suggestion messages must be unique.** Because suggestions are typically displayed in an editor as a dropdown list, it's important that no two suggestions for the same lint problem have the same message. Otherwise, it's impossible to know what any given suggestion will do. This additional check runs automatically.
 1. **Suggestions must generate valid syntax.** In order for rule suggestions to be helpful, they need to be valid syntax. `RuleTester` now parses the output of suggestions using the same language options as the `code` value and throws an error if parsing fails.
+1. **Test cases must be unique.** Identical test cases can cause confusion and be hard to detect manually in a long test file. Duplicates are now automatically detected and can be safely removed.
 
 **To address:** Run your rule tests using `RuleTester` and fix any errors that occur. The changes you'll need to make to satisfy `RuleTester` are compatible with ESLint v8.x.
 
-**Related Issues(s):** [#15735](https://github.com/eslint/eslint/issues/15735), [#16908](https://github.com/eslint/eslint/issues/16908)
+**Related Issues(s):** [#15104](https://github.com/eslint/eslint/issues/15104), [#15735](https://github.com/eslint/eslint/issues/15735), [#16908](https://github.com/eslint/eslint/issues/16908)
 
 ## <a name="flat-eslint"></a> `FlatESLint` is now `ESLint`
 

diff --git a/lib/rule-tester/rule-tester.js b/lib/rule-tester/rule-tester.js
@@ -17,7 +17,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");
@@ -27,6 +28,7 @@ const ajv = require("../shared/ajv")({ strictDefaults: true });
 const parserSymbol = Symbol.for("eslint.RuleTester.parser");
 const { SourceCode } = require("../source-code");
 const { ConfigArraySymbol } = require("@humanwhocodes/config-array");
+const { isSerializable } = require("../shared/serialization");
 
 //------------------------------------------------------------------------------
 // Typedefs
@@ -500,6 +502,9 @@ class RuleTester {
             linter = this.linter,
             ruleId = `rule-to-test/${ruleName}`;
 
+        const seenValidTestCases = new Set();
+        const seenInvalidTestCases = new Set();
+
         if (!rule || typeof rule !== "object" || typeof rule.create !== "function") {
             throw new TypeError("Rule must be an object with a `create` method");
         }
@@ -803,6 +808,32 @@ 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 (!isSerializable(item)) {
+
+                /*
+                 * If we can't serialize a test case (because it contains a function, RegExp, etc), skip the check.
+                 * This might happen with properties like: options, plugins, settings, languageOptions.parser, languageOptions.parserOptions.
+                 */
+                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
@@ -818,6 +849,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;
 
@@ -869,6 +902,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;
 

diff --git a/lib/shared/serialization.js b/lib/shared/serialization.js
@@ -0,0 +1,55 @@
+/**
+ * @fileoverview Serialization utils.
+ * @author Bryan Mishkin
+ */
+
+"use strict";
+
+/**
+ * Check if a value is a primitive or plain object created by the Object constructor.
+ * @param {any} val the value to check
+ * @returns {boolean} true if so
+ * @private
+ */
+function isSerializablePrimitiveOrPlainObject(val) {
+    return (
+        val === null ||
+        typeof val === "string" ||
+         typeof val === "boolean" ||
+         typeof val === "number" ||
+         (typeof val === "object" && val.constructor === Object) ||
+         Array.isArray(val)
+    );
+}
+
+/**
+ * Check if a value is serializable.
+ * Functions or objects like RegExp cannot be serialized by JSON.stringify().
+ * Inspired by: https://stackoverflow.com/questions/30579940/reliable-way-to-check-if-objects-is-serializable-in-javascript
+ * @param {any} val the value
+ * @returns {boolean} true if the value is serializable
+ */
+function isSerializable(val) {
+    if (!isSerializablePrimitiveOrPlainObject(val)) {
+        return false;
+    }
+    if (typeof val === "object") {
+        for (const property in val) {
+            if (Object.hasOwn(val, property)) {
+                if (!isSerializablePrimitiveOrPlainObject(val[property])) {
+                    return false;
+                }
+                if (typeof val[property] === "object") {
+                    if (!isSerializable(val[property])) {
+                        return false;
+                    }
+                }
+            }
+        }
+    }
+    return true;
+}
+
+module.exports = {
+    isSerializable
+};
diff --git a/tests/lib/rule-tester/rule-tester.js b/tests/lib/rule-tester/rule-tester.js
@@ -2846,6 +2846,200 @@ 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 when options is a nested serializable object", () => {
+                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: [{ foo: [{ a: true, b: [1, 2, 3] }] }] },
+                            { code: "const x = 123;", errors: [{ message: "foo bar" }], options: [{ foo: [{ a: true, b: [1, 2, 3] }] }] }
+                        ]
+                    });
+                }, "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 non-serializable property present (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 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 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 non-serializable property present (options)", () => {
+                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", () => {
 
         [

diff --git a/tests/lib/rules/indent.js b/tests/lib/rules/indent.js
@@ -4624,14 +4624,6 @@ ruleTester.run("indent", rule, {
             `,
             options: [0]
         },
-        {
-            code: unIndent`
-              <App
-              \tfoo
-              />
-            `,
-            options: ["tab"]
-        },
         unIndent`
                 <App
                     foo

diff --git a/tests/lib/rules/multiline-comment-style.js b/tests/lib/rules/multiline-comment-style.js
@@ -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

diff --git a/tests/lib/rules/newline-after-var.js b/tests/lib/rules/newline-after-var.js
@@ -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;",