-
-
Notifications
You must be signed in to change notification settings - Fork 70
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: Introduce ecosystem tests for popular plugins #127
base: main
Are you sure you want to change the base?
feat: Introduce ecosystem tests for popular plugins #127
Conversation
That none of the At least one of eg I also note that the selection criteria is similar to those outlined in the suggested |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My apologies, I started a review last week, left a bunch of comments, and then forgot to hit "Submit Review".
I think this outlines a good infrastructure, just looking for some clarifications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me! This is an excellent starting point. We can proceed to run it and see how it performs. 🚀🚀🚀
``` | ||
|
||
A `test/ecosystem` directory will be created with a directory for each plugin. | ||
The `test:ecosystem` script will copy the contents of the provided `--plugin` directory into a clean `test/${plugin}-scratch` directory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized we're not really defining what "breakage" is anywhere. Are we just running npm test
on each package with the local ESLint changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't planning on suggesting including source code from plugins in this proposal. To start I was thinking of just verifying that npm lint
doesn't error out - regardless of rule reports. Meanwhile, the instigating issues mentioned in the RFC were all runtime crashes that would be caught by it.
Relying on tests makes me a little nervous. It'd be a lot slower -especially if the plugins have build steps- and we'd need to make sure none of them have tests that rely on specifics of rule reports.
My vote would be to just use the plugins as end-users until we have a breakage that would have been caught by source code level checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To start I was thinking of just verifying that
npm lint
doesn't error out - regardless of rule reports.
Can you explain how that would work? Are you running npm lint
in the ESLint repo? If so, does that mean we can only test plugins that ESLint itself uses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I intend by this:
rfcs/designs/2024-repo-ecosystem-plugin-tests/README.md
Lines 30 to 35 in 8fd5da4
1. Create a new directory containing: | |
- `package.json` | |
- `eslint.config.js` with the closest equivalent to an _"enable all rules"_ preset from the plugin | |
- A small set of files known to be parsed and not cause lint reports with the plugin | |
2. Run a lint command (i.e. `npx eslint .`) in that directory | |
3. Assert that the lint command passed with 0 lint reports. |
Is that not clear? Is there a different phrasing you'd suggest?
Co-authored-by: Milos Djermanovic <[email protected]>
Summary
Adding an CI job to the
eslint/eslint
repo that checks changes against a small selection of third-party plugins.Related Issues
eslint/eslint#19139