From ff35ff806e4dff994cb2ba34b25faee64d619aec Mon Sep 17 00:00:00 2001 From: Jacques Favreau Date: Sat, 27 Jan 2018 21:52:56 -0800 Subject: [PATCH] Fix: Make rule-tester strictly check messageId. (ref #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 | 6 +- lib/report-translator.js | 1 + lib/testers/rule-tester.js | 57 ++++++++------ .../fixtures/testers/rule-tester/messageId.js | 36 +++++++++ tests/lib/rules/no-fallthrough.js | 4 +- tests/lib/testers/rule-tester.js | 75 +++++++++++++++++++ 6 files changed, 151 insertions(+), 28 deletions(-) create mode 100644 tests/fixtures/testers/rule-tester/messageId.js diff --git a/docs/developer-guide/working-with-rules.md b/docs/developer-guide/working-with-rules.md index f23afb16bfb..85cc104913a 100644 --- a/docs/developer-guide/working-with-rules.md +++ b/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" } ] } diff --git a/lib/report-translator.js b/lib/report-translator.js index 7893a1f7ad2..0eb54c45263 100644 --- a/lib/report-translator.js +++ b/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 diff --git a/lib/testers/rule-tester.js b/lib/testers/rule-tester.js index de218a875f5..2dcbef61453 100644 --- a/lib/testers/rule-tester.js +++ b/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}`); } diff --git a/tests/fixtures/testers/rule-tester/messageId.js b/tests/fixtures/testers/rule-tester/messageId.js new file mode 100644 index 00000000000..d7386e395a0 --- /dev/null +++ b/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'."}); + } + } + }; + } +}; diff --git a/tests/lib/rules/no-fallthrough.js b/tests/lib/rules/no-fallthrough.js index c2e48fadf61..4d8e4514ba3 100644 --- a/tests/lib/rules/no-fallthrough.js +++ b/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 } diff --git a/tests/lib/testers/rule-tester.js b/tests/lib/testers/rule-tester.js index d7a4bd51c30..54711f42115 100644 --- a/tests/lib/testers/rule-tester.js +++ b/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", () => { /**