Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: Stricter rule test validations #17654

Merged
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
1e3529a
feat!: rule tester require suggestion matchers
DMartens Oct 16, 2023
f9edbb6
feat!: rule tester require message or messageId
DMartens Oct 26, 2023
e85f3ae
feat!: rule tester require output for suggestions
DMartens Nov 2, 2023
9b3fa60
feat!: rule tester only allow desc or messageId in suggestion matchers
DMartens Nov 2, 2023
0f90cbe
feat!: rule tester require suggestion output differs from original so…
DMartens Nov 2, 2023
a618f0c
feat!: rule tester typecheck only and filename test case properties
DMartens Nov 13, 2023
603bcca
feat!: rule tester check whether code and output is equal
DMartens Dec 4, 2023
8db902f
docs: update rule tester documentation to reflect additional assertions
DMartens Dec 4, 2023
285a575
fix: fix invalid test cases of builtin rules
DMartens Dec 4, 2023
17df6d0
fix: remove unnecessary space for the suggestion prefix
DMartens Dec 4, 2023
dde1be3
chore: fixing merge diversions
DMartens Jan 12, 2024
cc23f0f
fix: remove incorrect claim that data is required if messageId with p…
DMartens Jan 20, 2024
ebafff5
chore: remove potentially confusing differentation between missing an…
DMartens Jan 20, 2024
73c4363
fix: better explanation for missing suggestions
DMartens Jan 20, 2024
e4592d4
feat: support specifying an suggestion amount
DMartens Jan 20, 2024
1da9f3c
fix: ecmaVersion should be a languageOptions property (not parserOpti…
DMartens Jan 20, 2024
afb1a9e
chore: tweak message for omitting output if there is no autofix
DMartens Jan 20, 2024
820405e
chore: fixup using old error message for the output property and upda…
DMartens Jan 20, 2024
3fb10c5
fix: fixup reported error messages
DMartens Jan 26, 2024
d57d3c1
fix: refine more assertion messages
DMartens Jan 30, 2024
4453f41
docs: document suggestion can also be a number
DMartens Jan 30, 2024
cb2f783
chore: simplify suggestion existence checks
DMartens Jan 30, 2024
d4cca7f
feat: check string and regexp error matchers for missing suggestion m…
DMartens Jan 31, 2024
80d190f
chore: cleanup message for missing suggestions when testing only the …
DMartens Feb 1, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 10 additions & 10 deletions docs/src/integrate/nodejs-api.md
Expand Up @@ -735,19 +735,19 @@ A test case is an object with the following properties:

In addition to the properties above, invalid test cases can also have the following properties:

* `errors` (number or array, required): Asserts some properties of the errors that the rule is expected to produce when run on this code. If this is a number, asserts the number of errors produced. Otherwise, this should be a list of objects, each containing information about a single reported error. The following properties can be used for an error (all are optional):
* `message` (string/regexp): The message for the error
* `messageId` (string): The Id for the error. See [testing errors with messageId](#testing-errors-with-messageid) for details
* `errors` (number or array, required): Asserts some properties of the errors that the rule is expected to produce when run on this code. If this is a number, asserts the number of errors produced. Otherwise, this should be a list of objects, each containing information about a single reported error. The following properties can be used for an error (all are optional unless otherwise noted):
* `message` (string/regexp): The message for the error. Must provide this or `messageId`
* `messageId` (string): The Id for the error. Must provide this or `message`. See [testing errors with messageId](#testing-errors-with-messageid) for details
* `data` (object): Placeholder data which can be used in combination with `messageId`
* `type` (string): The type of the reported AST node
* `line` (number): The 1-based line number of the reported location
* `column` (number): The 1-based column number of the reported location
* `endLine` (number): The 1-based line number of the end of the reported location
* `endColumn` (number): The 1-based column number of the end of the reported location
* `suggestions` (array): An array of objects with suggestion details to check. See [Testing Suggestions](#testing-suggestions) for details
* `suggestions` (array): An array of objects with suggestion details to check. Required if the rule produces suggestions. See [Testing Suggestions](#testing-suggestions) for details

If a string is provided as an error instead of an object, the string is used to assert the `message` of the error.
* `output` (string, required if the rule fixes code): Asserts the output that will be produced when using this rule for a single pass of autofixing (e.g. with the `--fix` command line flag). If this is `null`, asserts that none of the reported problems suggest autofixes.
* `output` (string, required if the rule fixes code): Asserts the output that will be produced when using this rule for a single pass of autofixing (e.g. with the `--fix` command line flag). If this is `null` or omitted, asserts that none of the reported problems suggest autofixes.

Any additional properties of a test case will be passed directly to the linter as config options. For example, a test case can have a `languageOptions` property to configure parser behavior:

Expand Down Expand Up @@ -784,12 +784,12 @@ Please note that `data` in a test case does not assert `data` passed to `context

### Testing Suggestions

Suggestions can be tested by defining a `suggestions` key on an errors object. The options to check for the suggestions are the following (all are optional):
Suggestions can be tested by defining a `suggestions` key on an errors object. The options to check for the suggestions are the following:
DMartens marked this conversation as resolved.
Show resolved Hide resolved

* `desc` (string): The suggestion `desc` value
* `messageId` (string): The suggestion `messageId` value for suggestions that use `messageId`s
* `data` (object): Placeholder data which can be used in combination with `messageId`
* `output` (string): A code string representing the result of applying the suggestion fix to the input code
* `desc` (string): The suggestion `desc` value. Must provide this or `messageId`
* `messageId` (string): The suggestion `messageId` value for suggestions that use `messageId`s. Must provide this or `desc`
* `data` (object): Placeholder data which can be used in combination with `messageId`.
* `output` (string, required): A code string representing the result of applying the suggestion fix to the input code

Example:

Expand Down
143 changes: 78 additions & 65 deletions lib/rule-tester/rule-tester.js
Expand Up @@ -631,7 +631,11 @@ class RuleTester {
configs.push(itemConfig);
}

if (item.filename) {
if (hasOwnProperty(item, "only")) {
assert.ok(typeof item.only === "boolean", "Optional test case property 'only' must be a boolean");
}
if (hasOwnProperty(item, "filename")) {
assert.ok(typeof item.filename === "string", "Optional test case property 'filename' must be a string");
filename = item.filename;
}

Expand Down Expand Up @@ -964,13 +968,10 @@ class RuleTester {
`Hydrated message "${rehydratedMessage}" does not match "${message.message}"`
);
}
} else {
assert.fail("Error must have either a 'messageId' or 'message'.");
DMartens marked this conversation as resolved.
Show resolved Hide resolved
}

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}`);
}
Expand All @@ -991,81 +992,89 @@ class RuleTester {
assert.strictEqual(message.endColumn, error.endColumn, `Error endColumn should be ${error.endColumn}`);
}

assert.ok(!message.suggestions || hasOwnProperty(error, "suggestions"), `Error at index ${i} has suggestions. Please specify 'suggestions' property on the test error object.`);
if (hasOwnProperty(error, "suggestions")) {
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved

// Support asserting there are no suggestions
if (!error.suggestions || (Array.isArray(error.suggestions) && error.suggestions.length === 0)) {
if (Array.isArray(message.suggestions) && message.suggestions.length > 0) {
assert.fail(`Error should have no suggestions on error with message: "${message.message}"`);
}
if (!message.suggestions) {
assert.ok(!error.suggestions || (Array.isArray(error.suggestions) && error.suggestions.length === 0), `Error should have suggestions on error with message: "${message.message}"`);
} else if (!error.suggestions || Array.isArray(error.suggestions) && error.suggestions.length === 0) {
assert.ok(message.suggestions.length === 0, `Error should have no suggestions on error with message: "${message.message}"`);
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!message.suggestions) {
assert.ok(!error.suggestions || (Array.isArray(error.suggestions) && error.suggestions.length === 0), `Error should have suggestions on error with message: "${message.message}"`);
} else if (!error.suggestions || Array.isArray(error.suggestions) && error.suggestions.length === 0) {
assert.ok(message.suggestions.length === 0, `Error should have no suggestions on error with message: "${message.message}"`);
} else {
if (!error.suggestions || (Array.isArray(error.suggestions) && error.suggestions.length === 0)) {
if (message.suggestions) {
assert.fail(`Error should have no suggestions on error with message: "${message.message}"`);
}
} else {
if (!message.suggestions) {
assert.fail(`Error should have suggestions on error with message: "${message.message}"`);
}

I think we can simplify this to avoid duplicating the !error.suggestions || (Array.isArray(error.suggestions) && error.suggestions.length === 0 condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought some more about it and I think assigning a hasSuggestions + expectSuggestions variables and collapsing the second branch is easier to understand (see the new commits).

assert.strictEqual(Array.isArray(message.suggestions), true, `Error should have an array of suggestions. Instead received "${message.suggestions}" on error with message: "${message.message}"`);
assert.strictEqual(message.suggestions.length, error.suggestions.length, `Error should have ${error.suggestions.length} suggestions. Instead found ${message.suggestions.length} suggestions`);

error.suggestions.forEach((expectedSuggestion, index) => {
assert.ok(
typeof expectedSuggestion === "object" && expectedSuggestion !== null,
"Test suggestion in 'suggestions' array must be an object."
);
Object.keys(expectedSuggestion).forEach(propertyName => {
assert.ok(
suggestionObjectParameters.has(propertyName),
`Invalid suggestion property name '${propertyName}'. Expected one of ${friendlySuggestionObjectParameterList}.`
);
});
if (typeof error.suggestions === "number") {
assert.strictEqual(message.suggestions.length, error.suggestions, `Error should have ${error.suggestions} suggestions. Instead found ${message.suggestions.length} suggestions`);
} else if (Array.isArray(error.suggestions)) {
assert.strictEqual(message.suggestions.length, error.suggestions.length, `Error should have ${error.suggestions.length} suggestions. Instead found ${message.suggestions.length} suggestions`);

const actualSuggestion = message.suggestions[index];
const suggestionPrefix = `Error Suggestion at index ${index} :`;

if (hasOwnProperty(expectedSuggestion, "desc")) {
error.suggestions.forEach((expectedSuggestion, index) => {
assert.ok(
!hasOwnProperty(expectedSuggestion, "data"),
`${suggestionPrefix} Test should not specify both 'desc' and 'data'.`
);
assert.strictEqual(
actualSuggestion.desc,
expectedSuggestion.desc,
`${suggestionPrefix} desc should be "${expectedSuggestion.desc}" but got "${actualSuggestion.desc}" instead.`
typeof expectedSuggestion === "object" && expectedSuggestion !== null,
"Test suggestion in 'suggestions' array must be an object."
);
}
Object.keys(expectedSuggestion).forEach(propertyName => {
assert.ok(
suggestionObjectParameters.has(propertyName),
`Invalid suggestion property name '${propertyName}'. Expected one of ${friendlySuggestionObjectParameterList}.`
);
});

if (hasOwnProperty(expectedSuggestion, "messageId")) {
assert.ok(
ruleHasMetaMessages,
`${suggestionPrefix} Test can not use 'messageId' if rule under test doesn't define 'meta.messages'.`
);
assert.ok(
hasOwnProperty(rule.meta.messages, expectedSuggestion.messageId),
`${suggestionPrefix} Test has invalid messageId '${expectedSuggestion.messageId}', the rule under test allows only one of ${friendlyIDList}.`
);
assert.strictEqual(
actualSuggestion.messageId,
expectedSuggestion.messageId,
`${suggestionPrefix} messageId should be '${expectedSuggestion.messageId}' but got '${actualSuggestion.messageId}' instead.`
);
if (hasOwnProperty(expectedSuggestion, "data")) {
const unformattedMetaMessage = rule.meta.messages[expectedSuggestion.messageId];
const rehydratedDesc = interpolate(unformattedMetaMessage, expectedSuggestion.data);
const actualSuggestion = message.suggestions[index];
const suggestionPrefix = `Error Suggestion at index ${index}:`;

if (hasOwnProperty(expectedSuggestion, "desc")) {
assert.ok(
!hasOwnProperty(expectedSuggestion, "data"),
`${suggestionPrefix} Test should not specify both 'desc' and 'data'.`
);
assert.ok(
!hasOwnProperty(expectedSuggestion, "messageId"),
`${suggestionPrefix} Test should not specify both 'desc' and 'messageId'.`
);
assert.strictEqual(
actualSuggestion.desc,
rehydratedDesc,
`${suggestionPrefix} Hydrated test desc "${rehydratedDesc}" does not match received desc "${actualSuggestion.desc}".`
expectedSuggestion.desc,
`${suggestionPrefix} desc should be "${expectedSuggestion.desc}" but got "${actualSuggestion.desc}" instead.`
);
} else if (hasOwnProperty(expectedSuggestion, "messageId")) {
assert.ok(
ruleHasMetaMessages,
`${suggestionPrefix} Test can not use 'messageId' if rule under test doesn't define 'meta.messages'.`
);
assert.ok(
hasOwnProperty(rule.meta.messages, expectedSuggestion.messageId),
`${suggestionPrefix} Test has invalid messageId '${expectedSuggestion.messageId}', the rule under test allows only one of ${friendlyIDList}.`
);
assert.strictEqual(
actualSuggestion.messageId,
expectedSuggestion.messageId,
`${suggestionPrefix} messageId should be '${expectedSuggestion.messageId}' but got '${actualSuggestion.messageId}' instead.`
);
if (hasOwnProperty(expectedSuggestion, "data")) {
const unformattedMetaMessage = rule.meta.messages[expectedSuggestion.messageId];
const rehydratedDesc = interpolate(unformattedMetaMessage, expectedSuggestion.data);

assert.strictEqual(
actualSuggestion.desc,
rehydratedDesc,
`${suggestionPrefix} Hydrated test desc "${rehydratedDesc}" does not match received desc "${actualSuggestion.desc}".`
);
}
} else if (hasOwnProperty(expectedSuggestion, "data")) {
assert.fail(
`${suggestionPrefix} Test must specify 'messageId' if 'data' is used.`
);
} else {
assert.fail(
`${suggestionPrefix} Test must specify either 'messageId' or 'desc'.`
);
}
} else {
assert.ok(
!hasOwnProperty(expectedSuggestion, "data"),
`${suggestionPrefix} Test must specify 'messageId' if 'data' is used.`
);
}

if (hasOwnProperty(expectedSuggestion, "output")) {
assert.ok(hasOwnProperty(expectedSuggestion, "output"), `${suggestionPrefix} The "output" property is required.`);
const codeWithAppliedSuggestion = SourceCodeFixer.applyFixes(item.code, [actualSuggestion]).output;

// Verify if suggestion fix makes a syntax error or not.
const errorMessageInSuggestion =
linter.verify(codeWithAppliedSuggestion, result.configs, result.filename).find(m => m.fatal);
linter.verify(codeWithAppliedSuggestion, result.configs, result.filename).find(m => m.fatal);

assert(!errorMessageInSuggestion, [
"A fatal parsing error occurred in suggestion fix.",
Expand All @@ -1075,8 +1084,11 @@ class RuleTester {
].join("\n"));

assert.strictEqual(codeWithAppliedSuggestion, expectedSuggestion.output, `Expected the applied suggestion fix to match the test suggestion output for suggestion at index: ${index} on error with message: "${message.message}"`);
}
});
assert.notStrictEqual(expectedSuggestion.output, item.code, `The output of a suggestion should differ from the original source code for suggestion at index: ${index} on error with message: "${message.message}"`);
});
} else {
assert.fail("Test error object property 'suggestions' should be an array or a number");
}
}
}
} else {
Expand All @@ -1096,6 +1108,7 @@ class RuleTester {
);
} else {
assert.strictEqual(result.output, item.output, "Output is incorrect.");
assert.notStrictEqual(item.code, item.output, "Test error object 'output' matches 'code'. If no autofix is expected, then omit the 'output' property or set it to null.");
DMartens marked this conversation as resolved.
Show resolved Hide resolved
}
} else {
assert.strictEqual(
Expand Down