-
-
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?
Changes from 12 commits
d55fe1a
9baeb50
a52ca5e
1329c1a
cdb2677
071532d
7d61947
d1ab08b
994a1a8
2dcf2be
5a79c72
e663bf3
91aa502
4612800
8fd5da4
0f518bf
c176a7e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,157 @@ | ||||||||||||||
- Repo: eslint/eslint | ||||||||||||||
- Start Date: 2024-11-25 | ||||||||||||||
- RFC PR: <https://github.com/eslint/rfcs/pull/127> | ||||||||||||||
- Authors: [Josh Goldberg](https://github.com/JoshuaKGoldberg) | ||||||||||||||
|
||||||||||||||
# Introduce ecosystem tests for popular third-party plugins | ||||||||||||||
|
||||||||||||||
## Summary | ||||||||||||||
|
||||||||||||||
Adding an CI job to the `eslint/eslint` repo that checks changes against a small selection of third-party plugins. | ||||||||||||||
|
||||||||||||||
## Motivation | ||||||||||||||
|
||||||||||||||
Changes in ESLint occasionally break downstream plugins in unexpected ways. | ||||||||||||||
Those changes might be unintentional breaking changes, or even non-breaking changes that happen to touch edge case behaviors relied on by plugins. | ||||||||||||||
|
||||||||||||||
[Bug: Error while loading rule '@typescript-eslint/no-unused-expressions](https://github.com/eslint/eslint/issues/19134) is an example change in ESLint's that caused downstream breakages in third-party plugins. | ||||||||||||||
At least two popular plugins -[`eslint-plugin-unicorn`](https://github.com/sindresorhus/eslint-plugin-unicorn/issues/2496) and [`typescript-eslint`](https://github.com/typescript-eslint/typescript-eslint/issues/10338)- were broken by that change. | ||||||||||||||
|
||||||||||||||
The plugins broke because they were relying on non-public implementation details of ESLint rules per [Docs: Formalize recommendation against plugins calling to rules via use-at-your-own-risk](https://github.com/eslint/eslint/issues/19169). | ||||||||||||||
ESLint core's [`eslint-config-eslint`](https://github.com/eslint/eslint/tree/main/packages/eslint-config-eslint) does not use all rules of downstream plugins and is not always up-to-date with their latest versions, so its internal usage of plugins is not sufficient to flag all high visibility compatibility issues. | ||||||||||||||
When the root cause is a bug in the downstream plugins, an "early warning" system would help them fix their issues before the incompatible changes to ESLint are published. | ||||||||||||||
|
||||||||||||||
## Detailed Design | ||||||||||||||
|
||||||||||||||
### CI Job | ||||||||||||||
|
||||||||||||||
The new CI job will, for each plugin: | ||||||||||||||
|
||||||||||||||
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. | ||||||||||||||
|
||||||||||||||
This will all be runnable locally with a `package.json` script like `npm run test:ecosystem --plugin eslint-plugin-unicorn`. | ||||||||||||||
|
||||||||||||||
An addition to `.github/workflows/ci.yml` under `jobs` would approximately look like: | ||||||||||||||
|
||||||||||||||
```yml | ||||||||||||||
test_ecosystem: | ||||||||||||||
name: Test Ecosystem Plugins | ||||||||||||||
runs-on: ubuntu-latest | ||||||||||||||
strategy: | ||||||||||||||
matrix: | ||||||||||||||
plugin: | ||||||||||||||
- eslint-plugin-unicorn | ||||||||||||||
- eslint-plugin-vue | ||||||||||||||
- typescript-eslint | ||||||||||||||
steps: | ||||||||||||||
- uses: actions/checkout@v4 | ||||||||||||||
- uses: actions/setup-node@v4 | ||||||||||||||
with: | ||||||||||||||
node-version: "lts/*" | ||||||||||||||
- name: Install Packages | ||||||||||||||
run: npm install | ||||||||||||||
- name: Test ${{ matrix.plugin }} | ||||||||||||||
run: npm run test:ecosystem --plugin ${{ matrix.plugin }} | ||||||||||||||
``` | ||||||||||||||
|
||||||||||||||
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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 commentThe reason will be displayed to describe this comment to others. Learn more.
Can you explain how that would work? Are you running There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Is that not clear? Is there a different phrasing you'd suggest? |
||||||||||||||
|
||||||||||||||
Asserting that plugins successfully produce reports will not be part of this job. | ||||||||||||||
Depending on specifics of plugin rule reports would make the job prone to failure on arbitrary plugin rule updates. | ||||||||||||||
|
||||||||||||||
### Failure Handling | ||||||||||||||
|
||||||||||||||
It is theoretically possible that the ecosystem CI job will occasionally be broken by updates to ecosystem plugins. | ||||||||||||||
However, this RFC believes that case will be exceedingly rare and short-lived: | ||||||||||||||
|
||||||||||||||
- Per [Plugin Selection](#plugin-selection), only very stable plugins that test on multiple ESLint versions including the latest will be selected | ||||||||||||||
- Today, plugin breakages are typically resolved within a week - even without this RFC's proposed "early warning" detection | ||||||||||||||
JoshuaKGoldberg marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
- Example: [typescript-eslint#10191](https://github.com/typescript-eslint/typescript-eslint/issues/10338) was reported on October 21st, 2024 and a fix published on October 28th, 2024 | ||||||||||||||
JoshuaKGoldberg marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
- Example: [typescript-eslint#10338](https://github.com/typescript-eslint/typescript-eslint/issues/10338) was reported on November 15th, 2024 and a fix published on November 18th, 2024 | ||||||||||||||
- Example: [eslint-plugin-unicorn#10191](https://github.com/sindresorhus/eslint-plugin-unicorn/issues/2496) was reported on November 15th, 2024 and a fix published on November 19th, 2024 | ||||||||||||||
JoshuaKGoldberg marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
|
||||||||||||||
In the case of a breakage being discovered on the `main` branch, this RFC proposes the following process: | ||||||||||||||
|
||||||||||||||
1. An ESLint team member should file a bug report on the plugin's repository -if it doesn't yet exist-, as well as an issue on `eslint/eslint` linking to that bug report | ||||||||||||||
2. If the issue isn't resolved within two weeks: | ||||||||||||||
1. An ESLint team member should send a PR to resolve the issue that removes the plugin from ESLint's ecosystem CI job | ||||||||||||||
2. An ESLint team member should file a followup issue to re-add it once the breakage is fixed | ||||||||||||||
|
||||||||||||||
In the case of a breaking being discovered on a PR branch, this RFC proposes the following process: | ||||||||||||||
|
||||||||||||||
1. If the failure is an indication of an issue in the PR, the PR should be updated as usual | ||||||||||||||
2. Otherwise, if the failure is an indication the plugin needs to be updated, the PR's author should file a bug report on the plugin's repository - if it doesn't yet exist | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could mention that a plugin using an internal/private ESLint API is an example of why the plugin would need to be updated. We don't want to stop ESLint from being able to modify its own internal/private APIs. Relevant comment about this: eslint/eslint#19139 (comment) |
||||||||||||||
3. If the issue isn't resolved within two weeks: | ||||||||||||||
1. The PR's author should remove the plugin from ESLint's ecosystem CI job in the PR | ||||||||||||||
2. The PR's author should file a followup issue to re-add it once the breakage is fixed | ||||||||||||||
|
||||||||||||||
### Major Releases | ||||||||||||||
|
||||||||||||||
Upcoming new major versions of ESLint are an expected failure case for ecosystem plugins. | ||||||||||||||
The ecosystem CI job will skip running any plugin that doesn't explicitly support the version of ESLint being tested. | ||||||||||||||
|
||||||||||||||
Plugin version support will be determined by the maximum `eslint` peer dependency range in the plugin's published `package.json`, if it exists. | ||||||||||||||
JoshuaKGoldberg marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
Otherwise the ESLint repository will assume only supporting up to the currently stable version of ESLint. | ||||||||||||||
|
||||||||||||||
### Plugin Selection | ||||||||||||||
JoshuaKGoldberg marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
|
||||||||||||||
The plugins that will be included to start will be: | ||||||||||||||
|
||||||||||||||
- [`eslint-plugin-eslint-comments`](https://github.com/eslint-community/eslint-plugin-eslint-comments): to capture an `eslint-community` project and AST edge cases around comments | ||||||||||||||
- [`eslint-plugin-unicorn`](https://github.com/sindresorhus/eslint-plugin-unicorn): to capture a large selection of miscellaneous rules | ||||||||||||||
- [`eslint-plugin-vue`](https://github.com/vuejs/eslint-plugin-vue): to capture support for a framework with nested parsing of a non-JavaScript/TypeScript-standard syntax | ||||||||||||||
- [`typescript-eslint`](https://github.com/typescript-eslint/typescript-eslint): to capture testing TypeScript APIs and intricate uses of parsing in general | ||||||||||||||
|
||||||||||||||
Plugins will be selectively added if they meet all of the following criteria: | ||||||||||||||
|
||||||||||||||
- >1 million npm downloads a week: arbitrary large size threshold to avoid small packages | ||||||||||||||
- Adding a notable new API usage not yet covered: to avoid duplicate equivalent plugins | ||||||||||||||
- Has had a breakage reported on ESLint: to be cautious in adding to the list | ||||||||||||||
- Is under active maintenance and has taken a week or less to fix any ESLint breakages within the last year: to avoid packages that won't be updated quickly on failures | ||||||||||||||
|
||||||||||||||
The number of plugins should remain small. | ||||||||||||||
Each added plugin brings adds the risk of third-party breakage, so plugins will only be added after filing a new issue and gaining team consensus. | ||||||||||||||
|
||||||||||||||
### Rollout | ||||||||||||||
|
||||||||||||||
This RFC expects the added ecosystem CI job to _likely_ consistently pass. | ||||||||||||||
However, to be safe, this RFC proposes adding a CI job in three steps: | ||||||||||||||
|
||||||||||||||
1. On a branch that is manually updated from `main` several times a week | ||||||||||||||
JoshuaKGoldberg marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
2. On the `main` branch only | ||||||||||||||
3. On all PRs targeting the `main` branch, alongside existing CI jobs | ||||||||||||||
|
||||||||||||||
At least one month should be held between those steps to make sure the job is consistently passing. | ||||||||||||||
|
||||||||||||||
## Out of Scope | ||||||||||||||
|
||||||||||||||
Automation could be added for at least the filing of issues on plugin failures. | ||||||||||||||
That does not seem worth the time expenditure given how rarely plugins are expected to fail. | ||||||||||||||
This RFC's discussion settled on it not being worth it. | ||||||||||||||
|
||||||||||||||
## Open Questions | ||||||||||||||
|
||||||||||||||
Are there other plugins we should include that satisfy the criteria? | ||||||||||||||
JoshuaKGoldberg marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
|
||||||||||||||
## Help Needed | ||||||||||||||
|
||||||||||||||
I expect to implement this change. | ||||||||||||||
|
||||||||||||||
## Frequently Asked Questions | ||||||||||||||
|
||||||||||||||
### Given ESLint respects semver, why add tests for plugins that are relying on internals? | ||||||||||||||
|
||||||||||||||
It's exceedingly difficult to be sure when changes to a large published package break contracts with downstream consumers. | ||||||||||||||
Even when all packages in an ecosystem are well-tested the way ESLint and its major plugins are, the sheer project size and duration of maintenance make unfortunate edge cases likely to happen. | ||||||||||||||
|
||||||||||||||
> [Venerable xkcd "Workflow" comic](https://xkcd.com/1172) | ||||||||||||||
|
||||||||||||||
## Related Discussions | ||||||||||||||
|
||||||||||||||
- [Repo: add end-to-end/integration tests for popular 3rd party plugins](https://github.com/eslint/eslint/issues/19139) |
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.
Should we utilize eslint-remote-tester for testing against other repositories? If not, can we mention it under "alternatives considered" or at least mention it as prior art?
I know a number of popular plugins use it like:
@AriPerkkio
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.
Thanks for ping @bmish. Here's a quick summary of what
eslint-remote-tester
does:eslint-remote-tester.config.js/ts
you'll configure ESLint config (identical to ESLint'soverrideConfig
) and repositories that should be cloned on the file system.node:worker_threads
that each handle a single repository parallel.There are some examples of bugs it can find automatically listed here: AriPerkkio/eslint-remote-tester#3. I used to run it against most popular community plugins for a while couple of years ago.
Also worth to mention:
eslint-plugin-unicorn
,eslint-plugin-jest
,eslint-plugin-testing-library
andeslint-plugin-vitest
.For the other ecosystem CI setups I would recommend to check how Vite and Vitest does this. There has also been some thoughts about making a generic ecosystem-ci that all Javascript ecosystem packages could utilize. It would not be strictly tied to Vite-ecosystem like the current ones are.