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

feat: Introduce ecosystem tests for popular plugins #127

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
d55fe1a
feat: Introduce ecosystem tests for popular third-party plugins
JoshuaKGoldberg Nov 26, 2024
9baeb50
Update designs/2024-repo-ecosystem-plugin-tests/README.md
JoshuaKGoldberg Dec 3, 2024
a52ca5e
on all PRs
JoshuaKGoldberg Dec 3, 2024
1329c1a
Split issue/PR steps for main vs. PR
JoshuaKGoldberg Dec 3, 2024
cdb2677
Out of Scope: automation
JoshuaKGoldberg Dec 3, 2024
071532d
Added eslint-plugin-eslint-comments
JoshuaKGoldberg Dec 3, 2024
7d61947
if (they) meet - typo
JoshuaKGoldberg Dec 3, 2024
d1ab08b
Mention examples of fixed breakages within a week
JoshuaKGoldberg Dec 3, 2024
994a1a8
Note 1-week fix examples and add to selection criteria
JoshuaKGoldberg Dec 3, 2024
2dcf2be
Small fixes: clarification, typo
JoshuaKGoldberg Dec 3, 2024
5a79c72
Clarification: 'those' steps
JoshuaKGoldberg Dec 3, 2024
e663bf3
Added: dogfooding not being enough; 'all' enablements
JoshuaKGoldberg Dec 6, 2024
91aa502
Apply suggestions from code review
JoshuaKGoldberg Dec 10, 2024
4612800
Expand to first-(second-?)party plugins
JoshuaKGoldberg Dec 10, 2024
8fd5da4
Automate updates
JoshuaKGoldberg Dec 10, 2024
0f518bf
Explain the off-main branch
JoshuaKGoldberg Dec 13, 2024
c176a7e
Switch cron job to be actually just a cron job
JoshuaKGoldberg Dec 18, 2024
7fef2ef
Apply suggestions from code review
JoshuaKGoldberg Jan 20, 2025
c767f0f
Clarifications: rollout and some small points
JoshuaKGoldberg Jan 20, 2025
8ddf9ea
Noted internal/private API usage as out of scope
JoshuaKGoldberg Jan 20, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
139 changes: 139 additions & 0 deletions designs/2024-repo-ecosystem-plugin-tests/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
- 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).
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
Copy link
Member

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

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:

  • In eslint-remote-tester.config.js/ts you'll configure ESLint config (identical to ESLint's overrideConfig) and repositories that should be cloned on the file system.
  • When run, it spins up x-amount of node:worker_threads that each handle a single repository parallel.
  • A single thread clones the assigned repository and run ESLint using Node API against it. Results are reported back to main thread.
  • Main thread constructs markdown or plain-text files and writes them to file system or outputs to CLI

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.

I know a number of popular plugins use it like:

Also worth to mention: eslint-plugin-unicorn, eslint-plugin-jest, eslint-plugin-testing-library and eslint-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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL, thanks for the reference! I think it would actually make a lot of sense to use eslint-remote-tester. It does roughly what we're suggesting, but with much better parallelization and reporting. Nice! 🔥

I put up a PoC branch here: https://github.com/JoshuaKGoldberg/eslint/tree/eslint-remote-tester-poc/tests/remote.

I'm in favor, but am hesitant to change the RFC until the TSC weighs in.

Copy link
Member

Choose a reason for hiding this comment

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

This is an interesting idea, but I think we should first decide if we want to run plugin tests instead (#127 (comment)).


The new CI job will, for each plugin:

1. Create a new directory containing a `package.json`, `eslint.config.js`, and small set of files known to be parsed and not cause lint reports with the plugin
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
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 unicorn`.
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

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.
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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:

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?

Copy link
Member

Choose a reason for hiding this comment

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

Got it now.

I'm a bit skeptical that this approach will give us the data that we want for a few reasons:

  1. We can't guarantee that code we'll come up with will properly exercise each rule.
  2. It's possible that rules could work fine when there are no violations but actually crash when there is a violation. If we're not testing all the code paths, we can't have high confidence that the rules actually work.
  3. Not all plugins have an all config that enables most of the rules.
  4. Even if we enable all rules, we won't have coverage of all possible options, which also leaves it possible that linting will pass but a different option that we aren't using might fail.

That why's I think running npm test for each plugin is a better approach. The tests for each rule are (mostly) guaranteed to exercise the important paths for both valid and invalid code.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @nzakas that running the plugin's own rule tests seems like a better approach. If it's doable.


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

In the case of a breakage being discovered, 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. The plugin will be removed 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
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

### 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-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 meet the following criteria:
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

- &gt;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: 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 and 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 branches, alongside existing CI jobs
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

At least one month should be held between steps to make sure the job is consistently passing.
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

## Open Questions

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.
Is that accurate?
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

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)