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 |
Co-authored-by: Francesco Trotta <github@fasttime.org>
nzakas
left a comment
There was a problem hiding this comment.
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.
aladdin-add
left a comment
There was a problem hiding this comment.
Looks great to me! This is an excellent starting point. We can proceed to run it and see how it performs. 🚀🚀🚀
nzakas
left a comment
There was a problem hiding this comment.
I think the direction of this makes sense now. Nice work.
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
| The new CI job will, for each plugin: | ||
|
|
||
| 1. Clone the plugin into a directory named `test/ecosystem/${plugin}` | ||
| 2. Run the plugin's package installation and build commands with [ni](https://github.com/antfu-collective/ni) |
There was a problem hiding this comment.
How will ni be installed? And will ni automatically install npm (or another package manager) if it's not found locally, and does this also work in a CI environment?
There was a problem hiding this comment.
installed
npm i -g ni,
automatically install
The nci command will.
I'll make those two points explicit.
Aside: I filed antfu-collective/ni#263 as a docs issue on ni to support this.
| 2. Run the plugin's package installation and build commands with [ni](https://github.com/antfu-collective/ni) | ||
| 3. Link the plugin to the current eslint installation | ||
| - This will have to be done manually, as ni does not support linking ([ni#85](https://github.com/antfu-collective/ni/issues/85 "ni issue 85: Maybe xxx link can join ni project")) | ||
| 4. Run the plugin's `test:eslint-compat` script with [ni](https://github.com/antfu-collective/ni) |
There was a problem hiding this comment.
Does ni also install Node.js in the correct version for the package?
There was a problem hiding this comment.
No, but last I checked, these packages are all fine with running on approximately the same Node.js versions as ESLint's CI. As in, they might specify preferred packageManager / .nvmrc versions, but they should pass CI.
There was a problem hiding this comment.
Yes, I think we should be fine using actions/setup-node@v4 with Node.js lts/* like it's done in other jobs.
- uses: actions/setup-node@v4
with:
node-version: "lts/*"This will also make sure that npm is installed so we can run npm i -g ni.
|
|
||
| See [Plugin Selection](#plugin-selection) below for specifics on which plugins will be included. | ||
|
|
||
| > ⚠️ Plugins are currently being asked for feedback on the `test:eslint-compat` script. |
There was a problem hiding this comment.
That's good. If there are any questions I think we can also discuss them in this RFC.
|
@JoshuaKGoldberg just a reminder there are a few questions left. |
|
👍 I'd been hoping for an answer on the Discord messaging, #127 (comment). I just went ahead now and pushed it as part of the RFC for review. |
|
@mdjermanovic @fasttime any other comments on this? I feel like there's enough here now to move forward with a PoC. |
|
Moving to Final Commenting based on two TSC approvals |
mdjermanovic
left a comment
There was a problem hiding this comment.
Looks good to me, thanks!
|
Final commenting period has ended, merging. |
Summary
Adding an CI job to the
eslint/eslintrepo that checks changes against a small selection of third-party plugins.Related Issues
eslint/eslint#19139