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鈥檒l 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

@linux-foundation-easycla
Copy link

@linux-foundation-easycla linux-foundation-easycla bot commented Oct 17, 2021

CLA Signed

The committers are authorized under a signed CLA.

Loading

Copy link
Member

@aladdin-add aladdin-add left a comment

LGTM, thanks!

Loading

@ljharb
Copy link
Contributor

@ljharb 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鈥檛 useful in a reasonable time frame unless they鈥檙e backwards compatible.

Loading

nzakas
nzakas approved these changes Oct 19, 2021
Copy link
Member

@nzakas nzakas left a comment

LGTM. Thanks!

Loading

@nzakas
Copy link
Member

@nzakas nzakas commented Oct 19, 2021

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

Loading

@ljharb
Copy link
Contributor

@ljharb 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.

Loading

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Oct 19, 2021

Marked as accepted since the issue is accepted.

Loading

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Oct 19, 2021

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

Loading

@ljharb
Copy link
Contributor

@ljharb ljharb commented Oct 19, 2021

@mdjermanovic thats what i was afraid of, that means that the eslint ecosystem can鈥檛 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 鈥渢hrow on unknown properties鈥 behavior, since that鈥檚 what breaks forward compatibility?

Loading

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Oct 19, 2021

Any thought to removing the 鈥渢hrow on unknown properties鈥 behavior, since that鈥檚 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);
    }
}

Loading

@G-Rath G-Rath force-pushed the support-test-name branch from 54322ab to b2acb62 Oct 19, 2021
@nzakas
Copy link
Member

@nzakas nzakas commented Oct 20, 2021

Agree. I don鈥檛 holding back progress for the sake of forward compatibility is a good approach. You can always just not use 鈥渘ame鈥 at all in your tests and there will be no difference.

Loading

Copy link
Member

@mdjermanovic mdjermanovic left a comment

LGTM, thanks!

Loading

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

@mdjermanovic mdjermanovic commented Oct 21, 2021

Thanks for contributing!

Loading

@ilyub
Copy link

@ilyub ilyub commented Oct 21, 2021

Thx for the feature

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants