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 17 commits into
base: main
Choose a base branch
from

Conversation

JoshuaKGoldberg
Copy link
Contributor

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

@fasttime fasttime added the Initial Commenting This RFC is in the initial feedback stage label Nov 26, 2024
@voxpelli
Copy link

That none of the eslint-community plugins are included feels a bit odd? Since it's somewhat operated under the same umbrella as ESLint itself?

At least one of eg eslint-plugin-n, eslint-plugin-promise or eslint-plugin-security would be good to include?

I also note that the selection criteria is similar to those outlined in the suggested eslint-community governance, and which have gotten feedback / objections / concerns there: eslint-community/governance#1 (comment)

Copy link
Member

@nzakas nzakas left a 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.

designs/2024-repo-ecosystem-plugin-tests/README.md Outdated Show resolved Hide resolved
designs/2024-repo-ecosystem-plugin-tests/README.md Outdated Show resolved Hide resolved
designs/2024-repo-ecosystem-plugin-tests/README.md Outdated Show resolved Hide resolved
designs/2024-repo-ecosystem-plugin-tests/README.md Outdated Show resolved Hide resolved
Copy link
Member

@aladdin-add aladdin-add left a 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.
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?

@JoshuaKGoldberg JoshuaKGoldberg changed the title feat: Introduce ecosystem tests for popular third-party plugins feat: Introduce ecosystem tests for popular plugins Dec 11, 2024
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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Each added plugin brings adds a risk of breakage, so plugins will only be added after filing a new issue and gaining team consensus.
Each added plugin adds a risk of 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:
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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.
A CI job will be added to the `eslint/eslint` repo, but will not immediately be a part of `main` branch or PR branch 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?


## 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.

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

Choose a reason for hiding this comment

The 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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Initial Commenting This RFC is in the initial feedback stage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants