Skip to content

Commit 9163646

Browse files
DMartensmdjermanovic
DMartens
and
mdjermanovic
authored
feat!: Rule Tester checks for missing placeholder data in the message (#18073)
* feat!: rule tester checks for missing placeholder data for the reported message * chore: incorporate bmishs suggestions * chore: differentiate message between a single and multiple missing data properties * fix: check for missing placeholders with data specified * feat: share regular expression for message placeholders * feat: ignore introduced message placeholders * refactor: simplified logic for getting unsubstituted message placeholders * docs: also use term unsubstituted for migration guide Co-authored-by: Milos Djermanovic <[email protected]> * docs: clarify placeholder versus data Co-authored-by: Milos Djermanovic <[email protected]> * chore: message grammar fixes Co-authored-by: Milos Djermanovic <[email protected]> * chore: update error messages for the grammar fixes * fix: remove unnecessary check for added placeholders * chore: split up object to avoid referencing the placeholder matcher via module.exports * chore: add mention of the issue for the migration guide * chore: stylize rule tester in migration guide * chore: clarify message for unsubstituted placeholders and introduce fixture for non-string data values --------- Co-authored-by: Milos Djermanovic <[email protected]>
1 parent 53f0f47 commit 9163646

9 files changed

Lines changed: 393 additions & 7 deletions

File tree

‎docs/src/use/migrate-to-9.0.0.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -553,10 +553,11 @@ In order to aid in the development of high-quality custom rules that are free fr
553553
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.
554554
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.
555555
1. **`filename` and `only` must be of the expected type.** `RuleTester` now checks the type of `filename` and `only` properties of test objects. If specified, `filename` must be a string value. If specified, `only` must be a boolean value.
556+
1. **Messages cannot have unsubstituted placeholders.** The `RuleTester` now also checks if there are {% raw %}`{{ placeholder }}` {% endraw %} still in the message as their values were not passed via `data` in the respective `context.report()` call.
556557
557558
**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.
558559

559-
**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)
560+
**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), [#18016](https://github.com/eslint/eslint/issues/18016)
560561

561562
## <a name="flat-eslint"></a> `FlatESLint` is now `ESLint`
562563

‎lib/linter/index.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
"use strict";
22

33
const { Linter } = require("./linter");
4-
const interpolate = require("./interpolate");
4+
const { interpolate } = require("./interpolate");
55
const SourceCodeFixer = require("./source-code-fixer");
66

77
module.exports = {

‎lib/linter/interpolate.js‎

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,30 @@
99
// Public Interface
1010
//------------------------------------------------------------------------------
1111

12-
module.exports = (text, data) => {
12+
/**
13+
* Returns a global expression matching placeholders in messages.
14+
* @returns {RegExp} Global regular expression matching placeholders
15+
*/
16+
function getPlaceholderMatcher() {
17+
return /\{\{([^{}]+?)\}\}/gu;
18+
}
19+
20+
/**
21+
* Replaces {{ placeholders }} in the message with the provided data.
22+
* Does not replace placeholders not available in the data.
23+
* @param {string} text Original message with potential placeholders
24+
* @param {Record<string, string>} data Map of placeholder name to its value
25+
* @returns {string} Message with replaced placeholders
26+
*/
27+
function interpolate(text, data) {
1328
if (!data) {
1429
return text;
1530
}
1631

32+
const matcher = getPlaceholderMatcher();
33+
1734
// Substitution content for any {{ }} markers.
18-
return text.replace(/\{\{([^{}]+?)\}\}/gu, (fullMatch, termWithWhitespace) => {
35+
return text.replace(matcher, (fullMatch, termWithWhitespace) => {
1936
const term = termWithWhitespace.trim();
2037

2138
if (term in data) {
@@ -25,4 +42,9 @@ module.exports = (text, data) => {
2542
// Preserve old behavior: If parameter name not provided, don't replace it.
2643
return fullMatch;
2744
});
45+
}
46+
47+
module.exports = {
48+
getPlaceholderMatcher,
49+
interpolate
2850
};

‎lib/linter/report-translator.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111

1212
const assert = require("assert");
1313
const ruleFixer = require("./rule-fixer");
14-
const interpolate = require("./interpolate");
14+
const { interpolate } = require("./interpolate");
1515

1616
//------------------------------------------------------------------------------
1717
// Typedefs

‎lib/rule-tester/rule-tester.js‎

Lines changed: 59 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,8 @@ const
1717
equal = require("fast-deep-equal"),
1818
Traverser = require("../shared/traverser"),
1919
{ getRuleOptionsSchema } = require("../config/flat-config-helpers"),
20-
{ Linter, SourceCodeFixer, interpolate } = require("../linter"),
20+
{ Linter, SourceCodeFixer } = require("../linter"),
21+
{ interpolate, getPlaceholderMatcher } = require("../linter/interpolate"),
2122
stringify = require("json-stable-stringify-without-jsonify");
2223

2324
const { FlatConfigArray } = require("../config/flat-config-array");
@@ -304,6 +305,39 @@ function throwForbiddenMethodError(methodName, prototype) {
304305
};
305306
}
306307

308+
/**
309+
* Extracts names of {{ placeholders }} from the reported message.
310+
* @param {string} message Reported message
311+
* @returns {string[]} Array of placeholder names
312+
*/
313+
function getMessagePlaceholders(message) {
314+
const matcher = getPlaceholderMatcher();
315+
316+
return Array.from(message.matchAll(matcher), ([, name]) => name.trim());
317+
}
318+
319+
/**
320+
* Returns the placeholders in the reported messages but
321+
* only includes the placeholders available in the raw message and not in the provided data.
322+
* @param {string} message The reported message
323+
* @param {string} raw The raw message specified in the rule meta.messages
324+
* @param {undefined|Record<unknown, unknown>} data The passed
325+
* @returns {string[]} Missing placeholder names
326+
*/
327+
function getUnsubstitutedMessagePlaceholders(message, raw, data = {}) {
328+
const unsubstituted = getMessagePlaceholders(message);
329+
330+
if (unsubstituted.length === 0) {
331+
return [];
332+
}
333+
334+
// 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
335+
const known = getMessagePlaceholders(raw);
336+
const provided = Object.keys(data);
337+
338+
return unsubstituted.filter(name => known.includes(name) && !provided.includes(name));
339+
}
340+
307341
const metaSchemaDescription = `
308342
\t- If the rule has options, set \`meta.schema\` to an array or non-empty object to enable options validation.
309343
\t- If the rule doesn't have options, omit \`meta.schema\` to enforce that no options can be passed to the rule.
@@ -997,6 +1031,18 @@ class RuleTester {
9971031
error.messageId,
9981032
`messageId '${message.messageId}' does not match expected messageId '${error.messageId}'.`
9991033
);
1034+
1035+
const unsubstitutedPlaceholders = getUnsubstitutedMessagePlaceholders(
1036+
message.message,
1037+
rule.meta.messages[message.messageId],
1038+
error.data
1039+
);
1040+
1041+
assert.ok(
1042+
unsubstitutedPlaceholders.length === 0,
1043+
`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.`
1044+
);
1045+
10001046
if (hasOwnProperty(error, "data")) {
10011047

10021048
/*
@@ -1096,6 +1142,18 @@ class RuleTester {
10961142
expectedSuggestion.messageId,
10971143
`${suggestionPrefix} messageId should be '${expectedSuggestion.messageId}' but got '${actualSuggestion.messageId}' instead.`
10981144
);
1145+
1146+
const unsubstitutedPlaceholders = getUnsubstitutedMessagePlaceholders(
1147+
actualSuggestion.desc,
1148+
rule.meta.messages[expectedSuggestion.messageId],
1149+
expectedSuggestion.data
1150+
);
1151+
1152+
assert.ok(
1153+
unsubstitutedPlaceholders.length === 0,
1154+
`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.`
1155+
);
1156+
10991157
if (hasOwnProperty(expectedSuggestion, "data")) {
11001158
const unformattedMetaMessage = rule.meta.messages[expectedSuggestion.messageId];
11011159
const rehydratedDesc = interpolate(unformattedMetaMessage, expectedSuggestion.data);

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

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,3 +34,110 @@ module.exports.withMessageOnly = {
3434
};
3535
}
3636
};
37+
38+
module.exports.withMissingData = {
39+
meta: {
40+
messages: {
41+
avoidFoo: "Avoid using variables named '{{ name }}'.",
42+
unused: "An unused key"
43+
}
44+
},
45+
create(context) {
46+
return {
47+
Identifier(node) {
48+
if (node.name === "foo") {
49+
context.report({
50+
node,
51+
messageId: "avoidFoo",
52+
});
53+
}
54+
}
55+
};
56+
}
57+
};
58+
59+
module.exports.withMultipleMissingDataProperties = {
60+
meta: {
61+
messages: {
62+
avoidFoo: "Avoid using {{ type }} named '{{ name }}'.",
63+
unused: "An unused key"
64+
}
65+
},
66+
create(context) {
67+
return {
68+
Identifier(node) {
69+
if (node.name === "foo") {
70+
context.report({
71+
node,
72+
messageId: "avoidFoo",
73+
});
74+
}
75+
}
76+
};
77+
}
78+
};
79+
80+
module.exports.withPlaceholdersInData = {
81+
meta: {
82+
messages: {
83+
avoidFoo: "Avoid using variables named '{{ name }}'.",
84+
unused: "An unused key"
85+
}
86+
},
87+
create(context) {
88+
return {
89+
Identifier(node) {
90+
if (node.name === "foo") {
91+
context.report({
92+
node,
93+
messageId: "avoidFoo",
94+
data: { name: '{{ placeholder }}' },
95+
});
96+
}
97+
}
98+
};
99+
}
100+
};
101+
102+
module.exports.withSamePlaceholdersInData = {
103+
meta: {
104+
messages: {
105+
avoidFoo: "Avoid using variables named '{{ name }}'.",
106+
unused: "An unused key"
107+
}
108+
},
109+
create(context) {
110+
return {
111+
Identifier(node) {
112+
if (node.name === "foo") {
113+
context.report({
114+
node,
115+
messageId: "avoidFoo",
116+
data: { name: '{{ name }}' },
117+
});
118+
}
119+
}
120+
};
121+
}
122+
};
123+
124+
module.exports.withNonStringData = {
125+
meta: {
126+
messages: {
127+
avoid: "Avoid using the value '{{ value }}'.",
128+
}
129+
},
130+
create(context) {
131+
return {
132+
Literal(node) {
133+
if (node.value === 0) {
134+
context.report({
135+
node,
136+
messageId: "avoid",
137+
data: { value: 0 },
138+
});
139+
}
140+
}
141+
};
142+
}
143+
};

‎tests/fixtures/testers/rule-tester/suggestions.js‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -198,3 +198,61 @@ module.exports.withFailingFixer = {
198198
};
199199
}
200200
};
201+
202+
module.exports.withMissingPlaceholderData = {
203+
meta: {
204+
messages: {
205+
avoidFoo: "Avoid using identifiers named '{{ name }}'.",
206+
renameFoo: "Rename identifier 'foo' to '{{ newName }}'"
207+
},
208+
hasSuggestions: true
209+
},
210+
create(context) {
211+
return {
212+
Identifier(node) {
213+
if (node.name === "foo") {
214+
context.report({
215+
node,
216+
messageId: "avoidFoo",
217+
data: {
218+
name: "foo"
219+
},
220+
suggest: [{
221+
messageId: "renameFoo",
222+
fix: fixer => fixer.replaceText(node, "bar")
223+
}]
224+
});
225+
}
226+
}
227+
};
228+
}
229+
};
230+
231+
module.exports.withMultipleMissingPlaceholderDataProperties = {
232+
meta: {
233+
messages: {
234+
avoidFoo: "Avoid using identifiers named '{{ name }}'.",
235+
rename: "Rename identifier '{{ currentName }}' to '{{ newName }}'"
236+
},
237+
hasSuggestions: true
238+
},
239+
create(context) {
240+
return {
241+
Identifier(node) {
242+
if (node.name === "foo") {
243+
context.report({
244+
node,
245+
messageId: "avoidFoo",
246+
data: {
247+
name: "foo"
248+
},
249+
suggest: [{
250+
messageId: "rename",
251+
fix: fixer => fixer.replaceText(node, "bar")
252+
}]
253+
});
254+
}
255+
}
256+
};
257+
}
258+
};

‎tests/lib/linter/interpolate.js‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,40 @@
55
//------------------------------------------------------------------------------
66

77
const assert = require("chai").assert;
8-
const interpolate = require("../../../lib/linter/interpolate");
8+
const { getPlaceholderMatcher, interpolate } = require("../../../lib/linter/interpolate");
99

1010
//------------------------------------------------------------------------------
1111
// Tests
1212
//------------------------------------------------------------------------------
1313

14+
describe("getPlaceholderMatcher", () => {
15+
it("returns a global regular expression", () => {
16+
const matcher = getPlaceholderMatcher();
17+
18+
assert.strictEqual(matcher.global, true);
19+
});
20+
21+
it("matches text with placeholders", () => {
22+
const matcher = getPlaceholderMatcher();
23+
24+
assert.match("{{ placeholder }}", matcher);
25+
});
26+
27+
it("does not match text without placeholders", () => {
28+
const matcher = getPlaceholderMatcher();
29+
30+
assert.notMatch("no placeholders in sight", matcher);
31+
});
32+
33+
it("captures the text inside the placeholder", () => {
34+
const matcher = getPlaceholderMatcher();
35+
const text = "{{ placeholder }}";
36+
const matches = Array.from(text.matchAll(matcher));
37+
38+
assert.deepStrictEqual(matches, [[text, " placeholder "]]);
39+
});
40+
});
41+
1442
describe("interpolate()", () => {
1543
it("passes through text without {{ }}", () => {
1644
const message = "This is a very important message!";

0 commit comments

Comments
 (0)