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 4 commits
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
27 changes: 19 additions & 8 deletions designs/2024-repo-ecosystem-plugin-tests/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@
- RFC PR: <https://github.com/eslint/rfcs/pull/127>
- Authors: [Josh Goldberg](https://github.com/JoshuaKGoldberg)

# Introduce ecosystem tests for popular third-party plugins
# Introduce ecosystem tests for popular plugins

## Summary

Adding an CI job to the `eslint/eslint` repo that checks changes against a small selection of third-party plugins.
Adding an CI job to the `eslint/eslint` repo that checks changes against `@eslint/*` plugins as well as a small selection of third-party plugins.

## Motivation

Expand Down Expand Up @@ -67,7 +67,7 @@ Depending on specifics of plugin rule reports would make the job prone to failur

### Failure Handling

It is theoretically possible that the ecosystem CI job will occasionally be broken by updates to ecosystem plugins.
It is theoretically possible that the ecosystem CI job will occasionally be broken by updates to 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
Expand Down Expand Up @@ -103,30 +103,33 @@ Otherwise the ESLint repository will assume only supporting up to the currently

The plugins that will be included to start will be:

- All `@eslint/*` plugins, including [`@eslint/css`](https://www.npmjs.com/package/@eslint/css), [`@eslint/json`](https://www.npmjs.com/package/@eslint/json), and [`@eslint/markdown`](https://www.npmjs.com/package/@eslint/markdown)
- [`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:
Third-party plugins will be selectively added if they meet all of the following criteria:

- &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 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.
The number of third-party plugins should remain small.
Each added plugin brings adds a risk of breakage, so plugins will only be added after filing a new issue and gaining team consensus.
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

### 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:
A CI job will be added to the `eslint/eslint` issue, but will not immediately be a part of `main` branch or PR branch builds.
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
To be safe, this RFC proposes rolling out CI job in three steps:

1. On a branch that is manually updated from `main` several times a week
1. On a CI cron job once a day, targeting the `main` branch but not blocking its builds
2. On the `main` branch only
3. On all PRs targeting the `main` branch, alongside existing CI jobs

Starting with a job separately from `main` ensures that unexpectedly high frequencies of breakages are caught early, without blocking `main` branch builds.
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused by this process. Is the intent that we end up with all three at the end? Or just number 3?

And I'm unclear on the value of number 2. Presumably, this is meant to run only after a PR is merged, but how will we be notified if the job fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

intent

My intent was that each step would replace the previous. As in, at the end, we end up with just 3, which is a superset of 2.

how will be notified

I was thinking the ❌ failing commit on the main branch.

I clarified the intent in c767f0f. Does that make sense?

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking the ❌ failing commit on the main branch.

The problem with this is that it's passive -- it requires someone looking at the GitHub repo Code tab. (And then it requires that someone care enough to click on it to see what's going on.) I feel like this will be easy to miss. Is there some way to have a notification of some sort?

Copy link
Member

Choose a reason for hiding this comment

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

GitHub user that merged the PR gets a notification if CI checks failed on the main branch, so perhaps that user could take further steps and/or notify the rest of the team in the team channel.

Copy link
Member

Choose a reason for hiding this comment

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

I don't get GitHub emails so I'd never know. 😄 I think we need an automated solution.

At least one month should be held between those steps to make sure the job is consistently passing.

## Out of Scope
Expand All @@ -152,6 +155,14 @@ Even when all packages in an ecosystem are well-tested the way ESLint and its ma

> [Venerable xkcd "Workflow" comic](https://xkcd.com/1172)

### What if a breakage causes rules to report incorrectly, but doesn't cause `npm lint` to crash?

Checking for incorrect rule reports is not handled by this RFC.
All recent significant downstream breakages caused rules to fully crash.

Any kind of rule report verification would necessitate ecosystem tests taking a dependency on the specific reports from downstream plugins.
This RFC does not believe the effort of keeping snapshots of those reports up-to-date as worthwhile.

## Related Discussions

- [Repo: add end-to-end/integration tests for popular 3rd party plugins](https://github.com/eslint/eslint/issues/19139)