Fix: Make rule-tester strictly check messageId. (ref eslint#9890)

This change makes rule-tester check messageId directly by value provided instead
of the current behavior, which is to check the messageId's message value against
the message returned from the rule at runtime.

docs/developer-guide/working-with-rules.md

@@ -228,16 +228,12 @@ var RuleTester = require("eslint").RuleTester;
 var ruleTester = new RuleTester();
 ruleTester.run("my-rule", rule, {
     valid: ["bar", "baz"],
-
     invalid: [
         {
             code: "foo",
             errors: [
                 {
-                    messageId: "avoidName",
-                    data: {
-                        name: "foo"
-                    }
+                    messageId: "avoidName"
                 }
             ]
         }

lib/report-translator.js

@@ -198,6 +198,7 @@ function normalizeFixes(descriptor, sourceCode) {
  * @param {(0|1|2)} options.severity Rule severity
  * @param {(ASTNode|null)} options.node Node
  * @param {string} options.message Error message
+ * @param {string} [options.messageId] The error message ID.
  * @param {{start: SourceLocation, end: (SourceLocation|null)}} options.loc Start and end location
  * @param {{text: string, range: (number[]|null)}} options.fix The fix object
  * @param {string[]} options.sourceLines Source lines

lib/testers/rule-tester.js

@@ -487,36 +487,51 @@ class RuleTester {
 
                         /*
                          * Error object.
-                         * This may have a message, node type, line, and/or
+                         * This may have a message, messageId, data, node type, line, and/or
                          * column.
                          */
-                        if (error.message) {
+                        if (hasOwnProperty(error, "message")) {
+                            assert.ok(!hasOwnProperty(error, "messageId"), "Error should not specify both 'message' and a 'messageId'.");
+                            assert.ok(!hasOwnProperty(error, "data"), "Error should not specify both 'data' and 'message'.");
                             assertMessageMatches(message.message, error.message);
-                        }
-
-                        if (error.messageId) {
-                            const hOP = Object.hasOwnProperty.call.bind(Object.hasOwnProperty);
-
-                            // verify that `error.message` is `undefined`
-                            assert.strictEqual(error.message, void 0, "Error should not specify both a message and a messageId.");
-                            if (!hOP(rule, "meta") || !hOP(rule.meta, "messages")) {
-                                assert.fail("Rule must specify a messages hash in `meta`");
-                            }
-                            if (!hOP(rule.meta.messages, error.messageId)) {
+                        } else if (hasOwnProperty(error, "messageId")) {
+                            assert.ok(
+                                hasOwnProperty(rule, "meta") && hasOwnProperty(rule.meta, "messages"),
+                                "Error can not use 'messageId' if rule under test doesn't define 'meta.messages'."
+                            );
+                            if (!hasOwnProperty(rule.meta.messages, error.messageId)) {
                                 const friendlyIDList = `[${Object.keys(rule.meta.messages).map(key => `'${key}'`).join(", ")}]`;
 
-                                assert.fail(`Invalid messageId '${error.messageId}'. Expected one of ${friendlyIDList}.`);
+                                assert(false, `Invalid messageId '${error.messageId}'. Expected one of ${friendlyIDList}.`);
                             }
-
-                            let expectedMessage = rule.meta.messages[error.messageId];
-
-                            if (error.data) {
-                                expectedMessage = interpolate(expectedMessage, error.data);
+                            assert.strictEqual(
+                                error.messageId,
+                                message.messageId,
+                                `messageId '${message.messageId}' does not match expected messageId '${error.messageId}'.`
+                            );
+                            if (hasOwnProperty(error, "data")) {
+
+                                /*
+                                 *  if data was provided, then directly compare the returned message to a synthetic
+                                 *  interpolated message using the same message ID and data provided in the test.
+                                 *  See https://github.com/eslint/eslint/issues/9890 for context.
+                                 */
+                                const unformattedOriginalMessage = rule.meta.messages[error.messageId];
+                                const rehydratedMessage = interpolate(unformattedOriginalMessage, error.data);
+
+                                assert.strictEqual(
+                                    message.message,
+                                    rehydratedMessage,
+                                    `Hydrated message "${rehydratedMessage}" does not match "${message.message}"`
+                                );
                             }
-
-                            assertMessageMatches(message.message, expectedMessage);
                         }
 
+                        assert.ok(
+                            hasOwnProperty(error, "data") ? hasOwnProperty(error, "messageId") : true,
+                            "Error must specify 'messageId' if 'data' is used."
+                        );
+
                         if (error.type) {
                             assert.strictEqual(message.nodeType, error.type, `Error type should be ${error.type}, found ${message.nodeType}`);
                         }

tests/fixtures/testers/rule-tester/messageId.js

@@ -0,0 +1,36 @@
+"use strict";
+module.exports.withMetaWithData = {
+    meta: {
+        messages: {
+            avoidFoo: "Avoid using variables named '{{ name }}'.",
+            unused: "An unused key"
+        }
+    },
+    create(context) {
+        return {
+            Identifier(node) {
+                if (node.name === "foo") {
+                    context.report({
+                        node,
+                        messageId: "avoidFoo",
+                        data: {
+                            name: "foo"
+                        }
+                    });
+                }
+            }
+        };
+    }
+};
+
+module.exports.withMessageOnly = {
+    create(context) {
+        return {
+            Identifier(node) {
+                if (node.name === "foo") {
+                    context.report({ node, message: "Avoid using variables named 'foo'."});
+                }
+            }
+        };
+    }
+};

tests/lib/rules/no-fallthrough.js

@@ -162,8 +162,8 @@ ruleTester.run("no-fallthrough", rule, {
             }],
             errors: [
                 {
-                    message: errorsDefault.message,
-                    type: errorsDefault.type,
+                    message: errorsDefault[0].message,
+                    type: errorsDefault[0].type,
                     line: 4,
                     column: 1
                 }

tests/lib/testers/rule-tester.js

@@ -830,6 +830,81 @@ describe("RuleTester", () => {
         }, "Test Scenarios for rule foo is invalid:\nCould not find any valid test scenarios");
     });
 
+    // Nominal message/messageId use cases
+    it("should assert match if message provided in both test and result.", () => {
+        assert.throws(() => {
+            ruleTester.run("foo", require("../../fixtures/testers/rule-tester/messageId").withMessageOnly, {
+                valid: [],
+                invalid: [{ code: "foo", errors: [{ message: "something" }] }]
+            });
+        }, /Avoid using variables named/);
+
+        assert.doesNotThrow(() => {
+            ruleTester.run("foo", require("../../fixtures/testers/rule-tester/messageId").withMessageOnly, {
+                valid: [],
+                invalid: [{ code: "foo", errors: [{ message: "Avoid using variables named 'foo'." }] }]
+            });
+        });
+    });
+
+    it("should assert match between messageId if provided in both test and result.", () => {
+        assert.throws(() => {
+            ruleTester.run("foo", require("../../fixtures/testers/rule-tester/messageId").withMetaWithData, {
+                valid: [],
+                invalid: [{ code: "foo", errors: [{ messageId: "unused" }] }]
+            });
+        }, "messageId 'avoidFoo' does not match expected messageId 'unused'.");
+
+        assert.doesNotThrow(() => {
+            ruleTester.run("foo", require("../../fixtures/testers/rule-tester/messageId").withMetaWithData, {
+                valid: [],
+                invalid: [{ code: "foo", errors: [{ messageId: "avoidFoo" }] }]
+            });
+        });
+    });
+    it("should assert match between resulting message output if messageId and data provided in both test and result", () => {
+        assert.throws(() => {
+            ruleTester.run("foo", require("../../fixtures/testers/rule-tester/messageId").withMetaWithData, {
+                valid: [],
+                invalid: [{ code: "foo", errors: [{ messageId: "avoidFoo", data: { name: "notFoo" } }] }]
+            });
+        }, "Hydrated message \"Avoid using variables named 'notFoo'.\" does not match \"Avoid using variables named 'foo'.\"");
+    });
+
+    // messageId/message misconfiguration cases
+    it("should throw if user tests for both message and messageId", () => {
+        assert.throws(() => {
+            ruleTester.run("foo", require("../../fixtures/testers/rule-tester/messageId").withMetaWithData, {
+                valid: [],
+                invalid: [{ code: "foo", errors: [{ message: "something", messageId: "avoidFoo" }] }]
+            });
+        }, "Error should not specify both 'message' and a 'messageId'.");
+    });
+    it("should throw if user tests for messageId but the rule doesn't use the messageId meta syntax.", () => {
+        assert.throws(() => {
+            ruleTester.run("foo", require("../../fixtures/testers/rule-tester/messageId").withMessageOnly, {
+                valid: [],
+                invalid: [{ code: "foo", errors: [{ messageId: "avoidFoo" }] }]
+            });
+        }, "Error can not use 'messageId' if rule under test doesn't define 'meta.messages'");
+    });
+    it("should throw if user tests for messageId not listed in the rule's meta syntax.", () => {
+        assert.throws(() => {
+            ruleTester.run("foo", require("../../fixtures/testers/rule-tester/messageId").withMetaWithData, {
+                valid: [],
+                invalid: [{ code: "foo", errors: [{ messageId: "useFoo" }] }]
+            });
+        }, /Invalid messageId 'useFoo'/);
+    });
+    it("should throw if data provided without messageId.", () => {
+        assert.throws(() => {
+            ruleTester.run("foo", require("../../fixtures/testers/rule-tester/messageId").withMetaWithData, {
+                valid: [],
+                invalid: [{ code: "foo", errors: [{ data: "something" }] }]
+            });
+        }, "Error must specify 'messageId' if 'data' is used.");
+    });
+
     describe("naming test cases", () => {
 
         /**