Breaking: Make rule-tester strictly check messageId and data. (fixes e…

…slint#9890)

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

@@ -190,6 +190,8 @@ Instead of typing out messages in both the `context.report()` call and your test
 
 This allows you to avoid retyping error messages. It also prevents errors reported in different sections of your rule from having out-of-date messages.
 
+If the `data` property is included, it is checked for strict deep equality with the resulting runtime error's `data` field. However, if the `data` property is omitted, only the `messageId` will be checked.
+
 ```js
 {% raw %}
 // in your rule
@@ -226,14 +228,34 @@ var rule = require("../../../lib/rules/my-rule");
 var RuleTester = require("eslint").RuleTester;
 
 var ruleTester = new RuleTester();
+
 ruleTester.run("my-rule", rule, {
     valid: ["bar", "baz"],
+    invalid: [
+        {
+            code: "foo",
+            errors: [
+                {
+                    // Here we are asserting that the avoideName message
+                    // was used, but not checking against the data object
+                    // used to format it. This form is also used when the
+                    // message has no formatting options and is just a string.
+                    messageId: "avoidName"
+                }
+            ]
+        }
+    ]
+});
 
+ruleTester.run("my-rule", rule, {
+    valid: ["bar", "baz"],
     invalid: [
         {
             code: "foo",
             errors: [
                 {
+                    // Here we're asserting that both the messageId "avoidName"
+                    // was used as well as the exact data used to format it.
                     messageId: "avoidName",
                     data: {
                         name: "foo"

lib/report-translator.js

@@ -198,6 +198,8 @@ 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 {Object} [options.data] The error message data.
  * @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
@@ -222,6 +224,10 @@ function createProblem(options) {
         problem.messageId = options.messageId;
     }
 
+    if (options.data) {
+        problem.data = options.data;
+    }
+
     if (options.loc.end) {
         problem.endLine = options.loc.end.line;
         problem.endColumn = options.loc.end.column + 1;
@@ -271,13 +277,13 @@ module.exports = function createReportTranslator(metadata) {
             descriptor.message = messages[id];
         }
 
-
         return createProblem({
             ruleId: metadata.ruleId,
             severity: metadata.severity,
             node: descriptor.node,
             message: normalizeMessagePlaceholders(descriptor),
             messageId: descriptor.messageId,
+            data: descriptor.data,
             loc: normalizeReportLoc(descriptor),
             fix: normalizeFixes(descriptor, metadata.sourceCode),
             sourceLines: metadata.sourceCode.lines

lib/rules/no-undef.js

@@ -63,7 +63,7 @@ module.exports = {
                     context.report({
                         node: identifier,
                         message: "'{{name}}' is not defined.",
-                        data: identifier
+                        data: { name: identifier.name }
                     });
                 });
             }

lib/rules/no-unused-vars.js

@@ -632,7 +632,9 @@ module.exports = {
                             message: unusedVar.references.some(ref => ref.isWrite())
                                 ? getAssignedMessage()
                                 : getDefinedMessage(unusedVar),
-                            data: unusedVar
+                            data: {
+                                name: unusedVar.name
+                            }
                         });
                     }
                 }

lib/testers/rule-tester.js

@@ -47,8 +47,7 @@ const lodash = require("lodash"),
     ajv = require("../util/ajv"),
     Linter = require("../linter"),
     Environments = require("../config/environments"),
-    SourceCodeFixer = require("../util/source-code-fixer"),
-    interpolate = require("../util/interpolate");
+    SourceCodeFixer = require("../util/source-code-fixer");
 
 //------------------------------------------------------------------------------
 // Private Members
@@ -487,34 +486,36 @@ 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.strictEqual(hasOwnProperty(error, "messageId"), false, "Error should not specify both 'message' and a 'messageId'.");
+                            assert.strictEqual(hasOwnProperty(error, "data"), false, "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}.`);
                             }
-
-                            let expectedMessage = rule.meta.messages[error.messageId];
-
-                            if (error.data) {
-                                expectedMessage = interpolate(expectedMessage, error.data);
+                            assert.strictEqual(
+                                error.messageId,
+                                message.messageId,
+                                `messageId mismatch saw '${message.messageId}' and expected '${error.messageId}'.`
+                            );
+                            if (hasOwnProperty(error, "data")) {
+                                assert.ok(hasOwnProperty(message, "data"), "Error referrs to messageId data, but none provided by rule.");
+                                assert.deepStrictEqual(
+                                    error.data,
+                                    message.data,
+                                    `Data properties do not match. Saw \`${util.inspect(message.data, false)}\` and expected \`${util.inspect(error.data, false)}\``
+                                );
                             }
-
-                            assertMessageMatches(message.message, expectedMessage);
                         }
 
                         if (error.type) {

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.withoutMetaWithoutData = {
+    create(context) {
+        return {
+            Identifier(node) {
+                if (node.name === "foo") {
+                    context.report({ node, message: "Avoid using variables named 'foo'."});
+                }
+            }
+        };
+    }
+};

tests/lib/cli-engine.js

@@ -351,6 +351,7 @@ describe("CLIEngine", () => {
                                 ruleId: "no-undef",
                                 severity: 2,
                                 message: "'foo' is not defined.",
+                                data: { name: "foo" },
                                 line: 1,
                                 column: 11,
                                 endLine: 1,
@@ -1395,6 +1396,10 @@ describe("CLIEngine", () => {
                                     column: 9,
                                     line: 2,
                                     message: "Expected '===' and instead saw '=='.",
+                                    data: {
+                                        actualOperator: "==",
+                                        expectedOperator: "==="
+                                    },
                                     nodeType: "BinaryExpression",
                                     ruleId: "eqeqeq",
                                     severity: 2,
@@ -1416,6 +1421,7 @@ describe("CLIEngine", () => {
                                     endColumn: 21,
                                     endLine: 1,
                                     message: "'foo' is not defined.",
+                                    data: { name: "foo" },
                                     nodeType: "Identifier",
                                     ruleId: "no-undef",
                                     severity: 2,

tests/lib/report-translator.js

@@ -54,7 +54,7 @@ describe("createReportTranslator", () => {
     describe("old-style call with location", () => {
         it("should extract the location correctly", () => {
             assert.deepStrictEqual(
-                translateReport(node, location, message, {}),
+                translateReport(node, location, message),
                 {
                     ruleId: "foo-rule",
                     severity: 2,
@@ -71,7 +71,7 @@ describe("createReportTranslator", () => {
     describe("old-style call without location", () => {
         it("should use the start location and end location of the node", () => {
             assert.deepStrictEqual(
-                translateReport(node, message, {}),
+                translateReport(node, message),
                 {
                     ruleId: "foo-rule",
                     severity: 2,
@@ -336,6 +336,7 @@ describe("createReportTranslator", () => {
                     ruleId: "foo-rule",
                     severity: 2,
                     message: "my message testing!",
+                    data: ["!", "testing"],
                     line: 42,
                     column: 24,
                     nodeType: "ExpressionStatement",
@@ -358,6 +359,7 @@ describe("createReportTranslator", () => {
                     severity: 2,
                     ruleId: "foo-rule",
                     message: "hello ExpressionStatement",
+                    data: { dynamic: "ExpressionStatement" },
                     nodeType: "ExpressionStatement",
                     line: 1,
                     column: 4,
@@ -373,6 +375,7 @@ describe("createReportTranslator", () => {
                     severity: 2,
                     ruleId: "foo-rule",
                     message: "hello ExpressionStatement",
+                    data: { dynamic: "ExpressionStatement" },
                     nodeType: "ExpressionStatement",
                     line: 1,
                     column: 4,
@@ -558,6 +561,7 @@ describe("createReportTranslator", () => {
                     severity: 2,
                     ruleId: "foo-rule",
                     message: "my message testing!",
+                    data: ["!", "testing"],
                     nodeType: "ExpressionStatement",
                     line: 1,
                     column: 1,

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
                 }