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

New: Add name to RuleTester #15179

Merged
merged 1 commit into from Oct 21, 2021
Merged

New: Add name to RuleTester #15179

merged 1 commit into from Oct 21, 2021

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Oct 17, 2021

Prerequisites checklist

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

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

What changes did you make? (Give an overview)

Implemented support for naming test cases, which can be useful when you're debugging some tests that have very similar code (especially when you're in the final cleanup stages and trying to update all those column & endColumn numbers 😅), or when your test code is very looooooooong so you want to give it a name to make it easier on your terminal if it fails.

If a name isn't provided or is an empty string, we fallback to the current behavior of using code.

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

Closes #15090

@eslint-github-bot eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label Oct 17, 2021
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 17, 2021

CLA Signed

The committers are authorized under a signed CLA.

@aladdin-add aladdin-add added core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Oct 18, 2021
Copy link
Member

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@ljharb
Copy link
Sponsor Contributor

ljharb commented Oct 18, 2021

What happens in older eslints’ RuleTesters when test cases have a name provided?

Most eslint plugins cover multiple eslint versions, so new features aren’t useful in a reasonable time frame unless they’re backwards compatible.

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@nzakas
Copy link
Member

nzakas commented Oct 19, 2021

@ljharb I think they will be ignored. I’m not sure it matters either way, as this only affects the rule developer who has control over the ESLint version used.

@ljharb
Copy link
Sponsor Contributor

ljharb commented Oct 19, 2021

@nzakas great! the rule developer has "control" in the sense that they could semver-major and drop all older eslint versions, but in practice the rule developer does not have control of this, because breaking changes cause churn and pain for their users.

@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion enhancement This change enhances an existing feature of ESLint and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Oct 19, 2021
@mdjermanovic
Copy link
Member

Marked as accepted since the issue is accepted.

@mdjermanovic
Copy link
Member

What happens in older eslints’ RuleTesters when test cases have a name provided?

Top-level properties that are not RuleTesterParameters are passed through as eslint config options, so the test case will fail due to additionalProperties: false, since there's no top-level property name in the eslintrc schema.

 Error: ESLint configuration in rule-tester is invalid:
        - Unexpected top-level property "name".

@ljharb
Copy link
Sponsor Contributor

ljharb commented Oct 19, 2021

@mdjermanovic thats what i was afraid of, that means that the eslint ecosystem can’t use this feature in RuleTester until eslint < 8 is no longer supported - given that i have plugins still supporting eslint 2, that will be a long time.

Any thought to removing the “throw on unknown properties” behavior, since that’s what breaks forward compatibility?

@mdjermanovic
Copy link
Member

Any thought to removing the “throw on unknown properties” behavior, since that’s what breaks forward compatibility?

I'd prefer not in this case, as that would silently ignore typos in test cases, while it's relatively easy to extend RuleTester and remove unsupported properties.

const { RuleTester } = require("eslint");

const supportsName = false; // ... check ESLint version 

module.exports = class MyRuleTester extends RuleTester {
    run(ruleName, rule, test) {
        if (!supportsName) {
            [...test.valid, ...test.invalid].forEach(testCase => {
                if (typeof testCase === "object") {
                      delete testCase.name;
                }
            });
        }

        super.run(ruleName, rule, test);
    }
}

@nzakas
Copy link
Member

nzakas commented Oct 20, 2021

Agree. I don’t holding back progress for the sake of forward compatibility is a good approach. You can always just not use “name” at all in your tests and there will be no difference.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mdjermanovic mdjermanovic merged commit e926b17 into eslint:main Oct 21, 2021
@mdjermanovic
Copy link
Member

Thanks for contributing!

@iliubinskii
Copy link

Thx for the feature

@G-Rath G-Rath deleted the support-test-name branch October 22, 2021 22:12
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Apr 20, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Apr 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change Request: Add "test name" option to ValidTestCase / InvalidTestCase
6 participants