feat!: Stricter rule test validations #17654

DMartens · 2023-10-16T09:30:38Z

Prerequisites checklist

I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

Implements part of RFC 84, refs #15104.
As the responding issue is closed, but still planned for v9, I decided to create a draft PR with only the first check implemented.
If this PR is unwanted, free feel to close it.
My plan, if this PR is accepted, is to create one commit per check only for RuleTester, then add documentation as some details may change and at last port the changes to FlatRuleTester.

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain: Rule Tester

What changes did you make? (Give an overview)

Assertions added to invalid test case error objects:

Must contain suggestions if the test case produces suggestions
Must contain the violation message (message or messageId, but not both)

Assertions added to invalid test case suggestion objects:

Must contain the suggestion code output
Must contain the suggestion message (messageId or desc, but not both)
The suggestion code output must differ from the test case code

Assertions added to all test cases:

Any autofix output must differ from the test case code
Cannot have identical test cases (feat!: detect duplicate test cases #17955)
The optional filename property must be a string
The optional only property must be a boolean

Is there anything you'd like reviewers to focus on?

How should the added suggestion matchers for existing core rule be handled?
For the suggestion existence check there are currently 18 missing suggestions from 2 rules.
Should I put them in the same commit as the check, as a separate commit or fix them all at the end?

netlify · 2023-10-16T09:30:48Z

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	`80d190f`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/65bb9367af776e000892589c

bmish · 2023-10-17T00:28:44Z

Note that I do have more of this implemented in: https://github.com/bmish/eslint/tree/RFC-84

DMartens · 2023-10-26T15:30:19Z

Added the second check for requiring a message or messageId assuming this PR is welcome.
This made the check for data without messageId useless as message checks there is no data, so there has to be a data property as then messageId is required.
Currently I just fixed up the rule tester test cases and not the test cases for the rules.
How do you want me to structure this PR (e.g. one commit for rule tester and one for updating rule tests), so that you can review it more easily?

github-actions · 2023-11-12T22:31:24Z

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

github-actions · 2023-11-24T22:31:15Z

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

github-actions · 2023-12-01T22:31:24Z

This pull request was auto-closed due to inactivity. While we wish we could keep working on every request, we unfortunately don't have the bandwidth to continue here and need to focus on other things. You can resubmit this pull request if you would like to continue working on it.

bmish · 2023-12-03T19:51:46Z

@DMartens good progress on this PR. Are you still working on it? We should try to finish it up soon for ESLint v9.

You can pull some of the documentation needed for this PR from my branch main...bmish:eslint:RFC-84
I would recommend saving the task Cannot have identical test cases for a separate PR since it's a much more complex check to get right and shouldn't block the rest of the trivial checks.
You shouldn't worry about what commit each change is in.

Let me know if you're stuck on anything.

DMartens · 2023-12-03T20:33:14Z

Yes, I am still working on this. I was just waiting for feedback but with the PR reopened I guess this PR is wanted.
If you split off the "identical test case" then only the following tasks are left:

updating documentation (thanks for the link)
checking that output uses "null" if no fix is applied
fixing violations in the core rules
copying changes to the flat rule tester
I should be able to finish this in the coming week.

bmish · 2023-12-03T20:38:01Z

Excellent, yes definitely eager to see this PR completed, thanks!

DMartens · 2023-12-04T14:19:17Z

Did everything except copying the assertions / tests for flat rule tester (and the duplicate test case detection (should this be a separate PR?)).
I would prefer a review before doing this to avoid unnecessary additional work copying the fixes.
Some notes:

documentation: also added a note for data that is required if messageId is used and its message has placeholders and for output: null that the fixer did not produce a fix
fixing found errors for builtin rules: preferred the messageId over desc for suggestion

bmish

Looking pretty good so far.

I would definitely leave "duplicate test case detection" to a separate PR, as it's much more complicated as described in the RFC, and I personally ran into a lot of edge cases with it and trouble getting it perfect when implementing it for a similar linter.

bmish · 2023-12-05T16:16:58Z

lib/rule-tester/rule-tester.js

@@ -1153,6 +1160,7 @@ class RuleTester {
                    );
                } else {
                    assert.strictEqual(result.output, item.output, "Output is incorrect.");
+                    assert.notStrictEqual(item.code, item.output, "Use 'output: null' if the rule does not fix this case instead of copying the code.");


Omit 'output' is my first recommendation to fix. Also could simplify wording:

Suggested change

assert.notStrictEqual(item.code, item.output, "Use 'output: null' if the rule does not fix this case instead of copying the code.");

assert.notStrictEqual(item.code, item.output, "Omit 'output' or use 'output: null' if there is no autofix.");

I think this is a preference (maybe a case for eslint-plugin-eslint-plugin?) as personally I try to be as explicit as possible. As the developer copied the code to output I think they want to keep the output property as there would've been no error forcing them to add the output property.

If people want to be explicit, that's fine. But I do think we should mention that the property can be omitted as one option. Similar to how schema: [] can be omitted from rules starting in ESLint v9.

This doesn't seem to be addressed yet.

tests/lib/rule-tester/rule-tester.js

docs/src/integrate/nodejs-api.md

github-actions · 2023-12-15T22:31:18Z

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

mdjermanovic · 2023-12-19T09:31:32Z

My plan, if this PR is accepted, is to create one commit per check only for RuleTester, then add documentation as some details may change and at last port the changes to FlatRuleTester.

We're going to remove RuleTester soon, so all changes should be done in FlatRuleTester only.

bmish · 2023-12-31T15:15:03Z

Now that the alpha is out, it should be a good time to address these merge conflicts. @DMartens are you able to fix this up in the next week? Want to make sure this is ready for the beta.

DMartens · 2024-01-02T22:17:26Z

I should be able to fix those merge issues this week.
Would it be better to start off a new PR as the prior flat-rule-tester.js is now named rule-tester.js and that is the main part of the PR?

bmish · 2024-01-02T22:32:06Z

Great. No need for a new PR. Can just fix the merge conflicts so that the correct file is updated.

bmish · 2024-01-04T23:18:15Z

I've started on the duplicate test case detection here:

feat!: detect duplicate test cases #17955

So we'll need to have both of these PRs ready for the beta in a week.

DMartens · 2024-01-20T20:15:22Z

Thanks for the code reviews. I should have addressed all raised points in their own commit.
Also I split up the test for the explicit testing of missing suggestions.
Further I have an ideas which I think should be part of this PR (but which are not part of the RFC):
If data is not specified for an error or suggestion, then the rule tester should test whether the placeholders are available in the message. For example this would be caught when tested with the rule tester:

export default {
	messages: {
		id: "{{ expected }}"
	},
	create(context) {
		context.report({ messageId: "id", data: { actual: true });
	},
}

This is makes sure that all placeholders can be replaced (especially now that message and messageId cannot be specified for the same error/suggestion.

mdjermanovic · 2024-01-20T21:04:27Z

Looks like one test is failing now.

 1) RuleTester
       should throw an error when the expected output is not null and the output does not differ from the code:
     AssertionError: expected [Function] to throw error matching /Use 'output: null' if the rule does …/u but got 'Either omit the \'output\' property o…'

mdjermanovic · 2024-01-20T21:08:00Z

Further I have an ideas which I think should be part of this PR (but which are not part of the RFC): If data is not specified for an error or suggestion, then the rule tester should test whether the placeholders are available in the message. For example this would be caught when tested with the rule tester:
export default {
	messages: {
		id: "{{ expected }}"
	},
	create(context) {
		context.report({ messageId: "id", data: { actual: true });
	},
}
This is makes sure that all placeholders can be replaced (especially now that message and messageId cannot be specified for the same error/suggestion.

I think this would need another RFC. Could you please open a separate issue to discuss this?

docs/src/integrate/nodejs-api.md

…te its docs

tests/lib/rule-tester/rule-tester.js

lib/rule-tester/rule-tester.js

DMartens · 2024-01-26T16:43:23Z

Sorry for the delay. I fixed up the mentioned error messages and added a test case for when the suggestions property is neither an array or number which was missing.

lib/rule-tester/rule-tester.js

mdjermanovic · 2024-01-30T09:48:05Z

I think the case described in #17654 (comment) hasn't been addressed.

docs/src/integrate/nodejs-api.md

mdjermanovic · 2024-01-30T11:53:57Z

lib/rule-tester/rule-tester.js

+                            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 {


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.

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).

mdjermanovic · 2024-01-31T12:59:08Z

Looks good so far. I think the only remaining thing is to check if the received message has suggestions in this case:

eslint/lib/rule-tester/rule-tester.js

Lines 959 to 962 in 10485e8

    
           if (typeof error === "string" || error instanceof RegExp) { 
        
               // Just an error message. 
        
               assertMessageMatches(message.message, error);

…atchers

mdjermanovic · 2024-02-01T10:36:11Z

lib/rule-tester/rule-tester.js

@@ -916,6 +920,7 @@ class RuleTester {

                        // Just an error message.
                        assertMessageMatches(message.message, error);
+                        assert.ok(message.suggestions === void 0 || message.suggestions.length === 0, `The message has untested. suggestions. Please convert the error at index ${i} into an object by assigning this message to a 'message' property.`);


Suggested change

assert.ok(message.suggestions === void 0 || message.suggestions.length === 0, `The message has untested. suggestions. Please convert the error at index ${i} into an object by assigning this message to a 'message' property.`);

assert.ok(message.suggestions === void 0, `Error at index ${i} has suggestions. Please convert the test error into an object and specify 'suggestions' property on it to test suggestions.`);

For consistency with the check and the error message in the other case.

…message

mdjermanovic

LGTM, thanks!

bmish · 2024-02-01T21:21:38Z

Great work on this!

eslint-github-bot bot added breaking This change is backwards-incompatible feature This change adds a new feature to ESLint labels Oct 16, 2023

github-actions bot added Stale and removed Stale labels Nov 12, 2023

github-actions bot added the Stale label Nov 24, 2023

github-actions bot closed this Dec 1, 2023

mdjermanovic reopened this Dec 2, 2023

mdjermanovic removed the Stale label Dec 2, 2023

bmish reviewed Dec 5, 2023

View reviewed changes

github-actions bot added the Stale label Dec 15, 2023

bmish assigned DMartens Dec 15, 2023

bmish removed the Stale label Dec 15, 2023

bmish mentioned this pull request Jan 4, 2024

feat!: detect duplicate test cases #17955

Merged

3 tasks

feat!: rule tester require suggestion matchers

1e3529a

chore: tweak message for omitting output if there is no autofix

afb1a9e

mdjermanovic reviewed Jan 20, 2024

View reviewed changes

docs/src/integrate/nodejs-api.md Outdated Show resolved Hide resolved

chore: fixup using old error message for the output property and upda…

820405e

…te its docs

DMartens mentioned this pull request Jan 20, 2024

Change Request: Always check data for message in rule tester #18016

Closed

1 task

mdjermanovic reviewed Jan 21, 2024

View reviewed changes

tests/lib/rule-tester/rule-tester.js Outdated Show resolved Hide resolved

mdjermanovic reviewed Jan 21, 2024

View reviewed changes

tests/lib/rule-tester/rule-tester.js Outdated Show resolved Hide resolved

mdjermanovic reviewed Jan 21, 2024

View reviewed changes

lib/rule-tester/rule-tester.js Outdated Show resolved Hide resolved

mdjermanovic reviewed Jan 21, 2024

View reviewed changes

lib/rule-tester/rule-tester.js Outdated Show resolved Hide resolved

nzakas linked an issue Jan 25, 2024 that may be closed by this pull request

Change Request: Stricter rule test validation #15104

Closed

1 task

fix: fixup reported error messages

3fb10c5

mdjermanovic reviewed Jan 30, 2024

View reviewed changes

lib/rule-tester/rule-tester.js Outdated Show resolved Hide resolved

lib/rule-tester/rule-tester.js Outdated Show resolved Hide resolved

mdjermanovic reviewed Jan 30, 2024

View reviewed changes

DMartens added 3 commits January 30, 2024 17:33

fix: refine more assertion messages

d57d3c1

docs: document suggestion can also be a number

4453f41

chore: simplify suggestion existence checks

cb2f783

feat: check string and regexp error matchers for missing suggestion m…

d4cca7f

…atchers

nzakas approved these changes Jan 31, 2024

View reviewed changes

mdjermanovic reviewed Feb 1, 2024

View reviewed changes

chore: cleanup message for missing suggestions when testing only the …

80d190f

…message

mdjermanovic approved these changes Feb 1, 2024

View reviewed changes

mdjermanovic merged commit 47e60f8 into eslint:main Feb 1, 2024
18 checks passed

mdjermanovic mentioned this pull request Feb 5, 2024

docs: add stricter rule test validations to v9 migration guide #18085

Merged

1 task

JoshuaKGoldberg mentioned this pull request Apr 28, 2024

Enhancement(rule-tester): Stricter rule test validations typescript-eslint/typescript-eslint#9016

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Stricter rule test validations #17654

feat!: Stricter rule test validations #17654

DMartens commented Oct 16, 2023 •

edited by bmish

netlify bot commented Oct 16, 2023 •

edited

bmish commented Oct 17, 2023

DMartens commented Oct 26, 2023

github-actions bot commented Nov 12, 2023

github-actions bot commented Nov 24, 2023

github-actions bot commented Dec 1, 2023

bmish commented Dec 3, 2023

DMartens commented Dec 3, 2023

bmish commented Dec 3, 2023

DMartens commented Dec 4, 2023

bmish left a comment

bmish Dec 5, 2023

DMartens Dec 5, 2023

bmish Dec 5, 2023

bmish Jan 13, 2024 •

edited

github-actions bot commented Dec 15, 2023

mdjermanovic commented Dec 19, 2023

bmish commented Dec 31, 2023

DMartens commented Jan 2, 2024

bmish commented Jan 2, 2024

bmish commented Jan 4, 2024

DMartens commented Jan 20, 2024 •

edited

mdjermanovic commented Jan 20, 2024

mdjermanovic commented Jan 20, 2024

DMartens commented Jan 26, 2024

mdjermanovic commented Jan 30, 2024

mdjermanovic Jan 30, 2024

DMartens Jan 30, 2024

mdjermanovic commented Jan 31, 2024

mdjermanovic Feb 1, 2024

mdjermanovic left a comment

bmish commented Feb 1, 2024

	assert.notStrictEqual(item.code, item.output, "Use 'output: null' if the rule does not fix this case instead of copying the code.");
	assert.notStrictEqual(item.code, item.output, "Omit 'output' or use 'output: null' if there is no autofix.");

	assert.ok(message.suggestions === void 0 \|\| message.suggestions.length === 0, `The message has untested. suggestions. Please convert the error at index ${i} into an object by assigning this message to a 'message' property.`);
	assert.ok(message.suggestions === void 0, `Error at index ${i} has suggestions. Please convert the test error into an object and specify 'suggestions' property on it to test suggestions.`);

feat!: Stricter rule test validations #17654

feat!: Stricter rule test validations #17654

Conversation

DMartens commented Oct 16, 2023 • edited by bmish

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

netlify bot commented Oct 16, 2023 • edited

✅ Deploy Preview for docs-eslint canceled.

bmish commented Oct 17, 2023

DMartens commented Oct 26, 2023

github-actions bot commented Nov 12, 2023

github-actions bot commented Nov 24, 2023

github-actions bot commented Dec 1, 2023

bmish commented Dec 3, 2023

DMartens commented Dec 3, 2023

bmish commented Dec 3, 2023

DMartens commented Dec 4, 2023

bmish left a comment

Choose a reason for hiding this comment

bmish Dec 5, 2023

Choose a reason for hiding this comment

DMartens Dec 5, 2023

Choose a reason for hiding this comment

bmish Dec 5, 2023

Choose a reason for hiding this comment

bmish Jan 13, 2024 • edited

Choose a reason for hiding this comment

github-actions bot commented Dec 15, 2023

mdjermanovic commented Dec 19, 2023

bmish commented Dec 31, 2023

DMartens commented Jan 2, 2024

bmish commented Jan 2, 2024

bmish commented Jan 4, 2024

DMartens commented Jan 20, 2024 • edited

mdjermanovic commented Jan 20, 2024

mdjermanovic commented Jan 20, 2024

DMartens commented Jan 26, 2024

mdjermanovic commented Jan 30, 2024

mdjermanovic Jan 30, 2024

Choose a reason for hiding this comment

DMartens Jan 30, 2024

Choose a reason for hiding this comment

mdjermanovic commented Jan 31, 2024

mdjermanovic Feb 1, 2024

Choose a reason for hiding this comment

mdjermanovic left a comment

Choose a reason for hiding this comment

bmish commented Feb 1, 2024

DMartens commented Oct 16, 2023 •

edited by bmish

netlify bot commented Oct 16, 2023 •

edited

bmish Jan 13, 2024 •

edited

DMartens commented Jan 20, 2024 •

edited