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

Add ability to test rule schemas #13434

Open
bmish opened this issue Jun 22, 2020 · 7 comments · May be fixed by #16823
Open

Add ability to test rule schemas #13434

bmish opened this issue Jun 22, 2020 · 7 comments · May be fixed by #16823
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint

Comments

@bmish
Copy link
Sponsor Member

bmish commented Jun 22, 2020

The version of ESLint you are using.

Latest, 7.3.0.

The problem you want to solve.

ESLint rules can have complex schemas. Some schemas can reach 100 lines long, and often allow various formats, such as in no-restricted-imports which allows either an array of strings or an array of objects.

Today, unit tests for a rule will ideally ensure that the rule behavior is correct for all possible combinations of valid rule options, but it is not currently possible to test that a rule correctly disallows invalid rules schemas.

For example, with the rule no-restricted-imports, I would like to test that the rule schema fails validation when passed:

  • Something that isn't an array
  • An empty array
  • An array containing an item that isn't a string nor object
  • An array containing an object that is missing required properties like name
  • Any other invalid combinations of input

Note that I am obviously not trying to test that JSON Schema works properly, but I am trying to test that I as a rule developer have written out my schema correctly. It can be tricky to get schemas to perfectly represent what the allowed input should be, especially when we don't even try to test the invalid cases.

The goal of improved testing here is to improve the quality of our rule schemas, thus reducing the chance of rule consumers configuring rules improperly (which can result in rules silently misbehaving or crashing).

Your take on the correct solution to problem.

In addition to the valid / invalid test case arrays in a rule unit test file, there could be an error array of invalid schemas.

Are you willing to submit a pull request to implement this change?

Likely yes.

@bmish bmish added core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint triage An ESLint team member will look at this issue soon labels Jun 22, 2020
@kaicataldo kaicataldo added 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 Jun 23, 2020
@kaicataldo
Copy link
Member

kaicataldo commented Jun 23, 2020

Since this would be a change to core, do you mind creating an RFC? I agree that this would be a useful thing to be able to test for, and it would be especially nice for cases where one might want to use custom keyword validators (which we don't support now, but could be useful to support in the future!).

Thoughts on figuring out a way to include these in the invalid cases array? I'm not sure there's much value in adding a new error array, and I believe this could be done in a backwards-compatible fashion. As a quick illustration of what I'm envisioning, I think something like the following might allow us to use the current API (though changes would need to happen under the hood):

var rule = require("../../../lib/rules/my-rule");
var RuleTester = require("eslint").RuleTester;

var ruleTester = new RuleTester();
ruleTester.run("my-rule", rule, {
    valid: ["bar", "baz"],
    invalid: [
        {
            code: "foo",
            options: ["some-option", { someOtherOption: true }]
            errors: [
                {
                    messageId: "invalidConfiguration":
                    data: {
                        message: "Expected configuration option someOtherOption to be a string. Found boolean instead."
                    }
                }
            ]
        }
    ]
});

@bmish
Copy link
Sponsor Member Author

bmish commented Jun 24, 2020

@kaicataldo sure, I will work on putting an RFC together.

Using the invalid array sounds good. In the interest of keeping this lightweight and leaving out irrelevant properties, what do you think about:

  1. Disallowing code from being passed when the config is being tested (since it can't affect the results).
  2. Encouraging messageId: "invalidConfiguration" to be used by itself without the exact message string. I'm not trying to test JSON Schema and don't really care what message it outputs. In fact, we might even want to disallow testing the exact string so that future JSON Schema string changes won't break our test cases.
{
    invalid: [
        {
            options: ["some-option", { someOtherOption: true }],
            errors: [
                {
                    messageId: "invalidConfiguration"
                }
            ]
        }
    ]
}

@kaicataldo
Copy link
Member

Both your points make a lot of sense to me!

@eslint-deprecated eslint-deprecated bot added the auto closed The bot closed this issue label Jul 27, 2020
@eslint-deprecated
Copy link

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jan 24, 2021
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jan 24, 2021
@bmish
Copy link
Sponsor Member Author

bmish commented Dec 10, 2022

Reopening this as something I may try to write an RFC for at some point.

@bmish bmish reopened this Dec 10, 2022
@bmish bmish self-assigned this Dec 10, 2022
@bmish bmish removed archived due to age This issue has been archived; please open a new issue for any further discussion auto closed The bot closed this issue labels Dec 15, 2022
@bmish
Copy link
Sponsor Member Author

bmish commented Jan 5, 2023

@aladdin-add
Copy link
Member

moved to "ready for implement", as the rfc has been merged. :)

@fasttime fasttime added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jun 16, 2023
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 core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
Status: Ready to Implement
Development

Successfully merging a pull request may close this issue.

4 participants