From daaf6a8562a833afcf67bae7601ee22efe830e86 Mon Sep 17 00:00:00 2001 From: Vincent Girard Date: Thu, 2 May 2024 14:55:28 +0000 Subject: [PATCH] Enhancement(rule-tester): check for missing placeholder data in the message --- packages/rule-tester/src/RuleTester.ts | 67 ++++++++++- packages/rule-tester/src/utils/interpolate.ts | 28 +++-- .../tests/eslint-base/eslint-base.test.js | 112 ++++++++++++++++++ .../tests/eslint-base/fixtures/messageId.js | 107 +++++++++++++++++ .../tests/eslint-base/fixtures/suggestions.js | 58 +++++++++ 5 files changed, 360 insertions(+), 12 deletions(-) diff --git a/packages/rule-tester/src/RuleTester.ts b/packages/rule-tester/src/RuleTester.ts index 324700fa264a..4b8e54e57856 100644 --- a/packages/rule-tester/src/RuleTester.ts +++ b/packages/rule-tester/src/RuleTester.ts @@ -38,7 +38,7 @@ import { satisfiesAllDependencyConstraints } from './utils/dependencyConstraints import { freezeDeeply } from './utils/freezeDeeply'; import { getRuleOptionsSchema } from './utils/getRuleOptionsSchema'; import { hasOwnProperty } from './utils/hasOwnProperty'; -import { interpolate } from './utils/interpolate'; +import { getPlaceholderMatcher, interpolate } from './utils/interpolate'; import { isReadonlyArray } from './utils/isReadonlyArray'; import * as SourceCodeFixer from './utils/SourceCodeFixer'; import { @@ -73,6 +73,45 @@ let defaultConfig = deepMerge( testerDefaultConfig, ) as TesterConfigWithDefaults; +/** + * Extracts names of {{ placeholders }} from the reported message. + * @param message Reported message + * @returns Array of placeholder names + */ +function getMessagePlaceholders(message: string): string[] { + const matcher = getPlaceholderMatcher(); + + return Array.from(message.matchAll(matcher), ([, name]) => name.trim()); +} + +/** + * Returns the placeholders in the reported messages but + * only includes the placeholders available in the raw message and not in the provided data. + * @param message The reported message + * @param raw The raw message specified in the rule meta.messages + * @param data The passed + * @returns Missing placeholder names + */ +function getUnsubstitutedMessagePlaceholders( + message: string, + raw: string, + data: Record = {}, +): string[] { + const unsubstituted = getMessagePlaceholders(message); + + if (unsubstituted.length === 0) { + return []; + } + + // Remove false positives by only counting placeholders in the raw message, which were not provided in the data matcher or added with a data property + const known = getMessagePlaceholders(raw); + const provided = Object.keys(data); + + return unsubstituted.filter( + name => known.includes(name) && !provided.includes(name), + ); +} + export class RuleTester extends TestFramework { readonly #testerConfig: TesterConfigWithDefaults; readonly #rules: Record = {}; @@ -809,6 +848,19 @@ export class RuleTester extends TestFramework { error.messageId, `messageId '${message.messageId}' does not match expected messageId '${error.messageId}'.`, ); + + const unsubstitutedPlaceholders = + getUnsubstitutedMessagePlaceholders( + message.message, + rule.meta.messages[message.messageId], + error.data, + ); + + assert.ok( + unsubstitutedPlaceholders.length === 0, + `The reported message has ${unsubstitutedPlaceholders.length > 1 ? `unsubstituted placeholders: ${unsubstitutedPlaceholders.map(name => `'${name}'`).join(', ')}` : `an unsubstituted placeholder '${unsubstitutedPlaceholders[0]}'`}. Please provide the missing ${unsubstitutedPlaceholders.length > 1 ? 'values' : 'value'} via the 'data' property in the context.report() call.`, + ); + if (hasOwnProperty(error, 'data')) { /* * if data was provided, then directly compare the returned message to a synthetic @@ -954,6 +1006,19 @@ export class RuleTester extends TestFramework { expectedSuggestion.messageId, `${suggestionPrefix} messageId should be '${expectedSuggestion.messageId}' but got '${actualSuggestion.messageId}' instead.`, ); + + const unsubstitutedPlaceholders = + getUnsubstitutedMessagePlaceholders( + actualSuggestion.desc, + rule.meta.messages[expectedSuggestion.messageId], + expectedSuggestion.data, + ); + + assert.ok( + unsubstitutedPlaceholders.length === 0, + `The message of the suggestion has ${unsubstitutedPlaceholders.length > 1 ? `unsubstituted placeholders: ${unsubstitutedPlaceholders.map(name => `'${name}'`).join(', ')}` : `an unsubstituted placeholder '${unsubstitutedPlaceholders[0]}'`}. Please provide the missing ${unsubstitutedPlaceholders.length > 1 ? 'values' : 'value'} via the 'data' property for the suggestion in the context.report() call.`, + ); + if (hasOwnProperty(expectedSuggestion, 'data')) { const unformattedMetaMessage = rule.meta.messages[expectedSuggestion.messageId]; diff --git a/packages/rule-tester/src/utils/interpolate.ts b/packages/rule-tester/src/utils/interpolate.ts index 53e5aa351938..6dbe785b70b0 100644 --- a/packages/rule-tester/src/utils/interpolate.ts +++ b/packages/rule-tester/src/utils/interpolate.ts @@ -2,6 +2,13 @@ import type { ReportDescriptorMessageData } from '@typescript-eslint/utils/ts-eslint'; +/** + * Returns a global expression matching placeholders in messages. + */ +export function getPlaceholderMatcher(): RegExp { + return /\{\{([^{}]+?)\}\}/gu; +} + export function interpolate( text: string, data: ReportDescriptorMessageData | undefined, @@ -10,18 +17,17 @@ export function interpolate( return text; } + const matcher = getPlaceholderMatcher(); + // Substitution content for any {{ }} markers. - return text.replace( - /\{\{([^{}]+?)\}\}/gu, - (fullMatch, termWithWhitespace: string) => { - const term = termWithWhitespace.trim(); + return text.replace(matcher, (fullMatch, termWithWhitespace: string) => { + const term = termWithWhitespace.trim(); - if (term in data) { - return String(data[term]); - } + if (term in data) { + return String(data[term]); + } - // Preserve old behavior: If parameter name not provided, don't replace it. - return fullMatch; - }, - ); + // Preserve old behavior: If parameter name not provided, don't replace it. + return fullMatch; + }); } diff --git a/packages/rule-tester/tests/eslint-base/eslint-base.test.js b/packages/rule-tester/tests/eslint-base/eslint-base.test.js index 8304dee78f95..56429508e4e3 100644 --- a/packages/rule-tester/tests/eslint-base/eslint-base.test.js +++ b/packages/rule-tester/tests/eslint-base/eslint-base.test.js @@ -1686,6 +1686,63 @@ describe("RuleTester", () => { }, "Hydrated message \"Avoid using variables named 'notFoo'.\" does not match \"Avoid using variables named 'foo'.\""); }); + it("should throw if the message has a single unsubstituted placeholder when data is not specified", () => { + assert.throws(() => { + ruleTester.run("foo", require("./fixtures/messageId").withMissingData, { + valid: [], + invalid: [{ code: "foo", errors: [{ messageId: "avoidFoo" }] }] + }); + }, "The reported message has an unsubstituted placeholder 'name'. Please provide the missing value via the 'data' property in the context.report() call."); + }); + + it("should throw if the message has a single unsubstituted placeholders when data is specified", () => { + assert.throws(() => { + ruleTester.run("foo", require("./fixtures/messageId").withMissingData, { + valid: [], + invalid: [{ code: "foo", errors: [{ messageId: "avoidFoo", data: { name: "name" } }] }] + }); + }, "Hydrated message \"Avoid using variables named 'name'.\" does not match \"Avoid using variables named '{{ name }}'."); + }); + + it("should throw if the message has multiple unsubstituted placeholders when data is not specified", () => { + assert.throws(() => { + ruleTester.run("foo", require("./fixtures/messageId").withMultipleMissingDataProperties, { + valid: [], + invalid: [{ code: "foo", errors: [{ messageId: "avoidFoo" }] }] + }); + }, "The reported message has unsubstituted placeholders: 'type', 'name'. Please provide the missing values via the 'data' property in the context.report() call."); + }); + + it("should not throw if the data in the message contains placeholders not present in the raw message", () => { + ruleTester.run("foo", require("./fixtures/messageId").withPlaceholdersInData, { + valid: [], + invalid: [{ code: "foo", errors: [{ messageId: "avoidFoo" }] }] + }); + }); + + it("should throw if the data in the message contains the same placeholder and data is not specified", () => { + assert.throws(() => { + ruleTester.run("foo", require("./fixtures/messageId").withSamePlaceholdersInData, { + valid: [], + invalid: [{ code: "foo", errors: [{ messageId: "avoidFoo" }] }] + }); + }, "The reported message has an unsubstituted placeholder 'name'. Please provide the missing value via the 'data' property in the context.report() call."); + }); + + it("should not throw if the data in the message contains the same placeholder and data is specified", () => { + ruleTester.run("foo", require("./fixtures/messageId").withSamePlaceholdersInData, { + valid: [], + invalid: [{ code: "foo", errors: [{ messageId: "avoidFoo", data: { name: "{{ name }}" } }] }] + }); + }); + + it("should not throw an error for specifying non-string data values", () => { + ruleTester.run("foo", require("./fixtures/messageId").withNonStringData, { + valid: [], + invalid: [{ code: "0", errors: [{ messageId: "avoid", data: { value: 0 } }] }] + }); + }); + // messageId/message misconfiguration cases it("should throw if user tests for both message and messageId", () => { assert.throws(() => { @@ -1854,6 +1911,61 @@ describe("RuleTester", () => { }); }); + it("should fail with a single missing data placeholder when data is not specified", () => { + assert.throws(() => { + ruleTester.run("suggestions-messageIds", require("../../fixtures/testers/rule-tester/suggestions").withMissingPlaceholderData, { + valid: [], + invalid: [{ + code: "var foo;", + errors: [{ + messageId: "avoidFoo", + suggestions: [{ + messageId: "renameFoo", + output: "var bar;" + }] + }] + }] + }); + }, "The message of the suggestion has an unsubstituted placeholder 'newName'. Please provide the missing value via the 'data' property for the suggestion in the context.report() call."); + }); + + it("should fail with a single missing data placeholder when data is specified", () => { + assert.throws(() => { + ruleTester.run("suggestions-messageIds", require("../../fixtures/testers/rule-tester/suggestions").withMissingPlaceholderData, { + valid: [], + invalid: [{ + code: "var foo;", + errors: [{ + messageId: "avoidFoo", + suggestions: [{ + messageId: "renameFoo", + data: { other: "name" }, + output: "var bar;" + }] + }] + }] + }); + }, "The message of the suggestion has an unsubstituted placeholder 'newName'. Please provide the missing value via the 'data' property for the suggestion in the context.report() call."); + }); + + it("should fail with multiple missing data placeholders when data is not specified", () => { + assert.throws(() => { + ruleTester.run("suggestions-messageIds", require("../../fixtures/testers/rule-tester/suggestions").withMultipleMissingPlaceholderDataProperties, { + valid: [], + invalid: [{ + code: "var foo;", + errors: [{ + messageId: "avoidFoo", + suggestions: [{ + messageId: "rename", + output: "var bar;" + }] + }] + }] + }); + }, "The message of the suggestion has unsubstituted placeholders: 'currentName', 'newName'. Please provide the missing values via the 'data' property for the suggestion in the context.report() call."); + }); + it("should pass when tested using empty suggestion test objects if the array length is correct", () => { ruleTester.run("suggestions-messageIds", require("./fixtures/suggestions").withMessageIds, { diff --git a/packages/rule-tester/tests/eslint-base/fixtures/messageId.js b/packages/rule-tester/tests/eslint-base/fixtures/messageId.js index 8f2bb2a246f3..3813b878edc6 100644 --- a/packages/rule-tester/tests/eslint-base/fixtures/messageId.js +++ b/packages/rule-tester/tests/eslint-base/fixtures/messageId.js @@ -37,3 +37,110 @@ module.exports.withMessageOnly = { }; } }; + +module.exports.withMissingData = { + 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", + }); + } + } + }; + } +}; + +module.exports.withMultipleMissingDataProperties = { + meta: { + messages: { + avoidFoo: "Avoid using {{ type }} named '{{ name }}'.", + unused: "An unused key" + } + }, + create(context) { + return { + Identifier(node) { + if (node.name === "foo") { + context.report({ + node, + messageId: "avoidFoo", + }); + } + } + }; + } +}; + +module.exports.withPlaceholdersInData = { + 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: '{{ placeholder }}' }, + }); + } + } + }; + } +}; + +module.exports.withSamePlaceholdersInData = { + 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: '{{ name }}' }, + }); + } + } + }; + } +}; + +module.exports.withNonStringData = { + meta: { + messages: { + avoid: "Avoid using the value '{{ value }}'.", + } + }, + create(context) { + return { + Literal(node) { + if (node.value === 0) { + context.report({ + node, + messageId: "avoid", + data: { value: 0 }, + }); + } + } + }; + } +}; diff --git a/packages/rule-tester/tests/eslint-base/fixtures/suggestions.js b/packages/rule-tester/tests/eslint-base/fixtures/suggestions.js index d883167889e0..71781086f4b8 100644 --- a/packages/rule-tester/tests/eslint-base/fixtures/suggestions.js +++ b/packages/rule-tester/tests/eslint-base/fixtures/suggestions.js @@ -166,3 +166,61 @@ module.exports.withoutHasSuggestionsProperty = { }; } }; + +module.exports.withMissingPlaceholderData = { + meta: { + messages: { + avoidFoo: "Avoid using identifiers named '{{ name }}'.", + renameFoo: "Rename identifier 'foo' to '{{ newName }}'" + }, + hasSuggestions: true + }, + create(context) { + return { + Identifier(node) { + if (node.name === "foo") { + context.report({ + node, + messageId: "avoidFoo", + data: { + name: "foo" + }, + suggest: [{ + messageId: "renameFoo", + fix: fixer => fixer.replaceText(node, "bar") + }] + }); + } + } + }; + } +}; + +module.exports.withMultipleMissingPlaceholderDataProperties = { + meta: { + messages: { + avoidFoo: "Avoid using identifiers named '{{ name }}'.", + rename: "Rename identifier '{{ currentName }}' to '{{ newName }}'" + }, + hasSuggestions: true + }, + create(context) { + return { + Identifier(node) { + if (node.name === "foo") { + context.report({ + node, + messageId: "avoidFoo", + data: { + name: "foo" + }, + suggest: [{ + messageId: "rename", + fix: fixer => fixer.replaceText(node, "bar") + }] + }); + } + } + }; + } +};