tests: add tests for Node 17 and 19 #707
Conversation
.github/workflows/pipeline.yml
Outdated
| @@ -54,7 +54,8 @@ jobs: | |||
|
|
|||
| strategy: | |||
| matrix: | |||
| node: [12.22.0, 12, 14.17.0, 14, '16.0', 16, '18.0', 18] | |||
| # The .x indicates "the most recent one" | |||
| node: [19.x, 18.x, 17.x, 16.x, 14.x, '14.17.0', 12.x, '12.22.0'] | |||
There was a problem hiding this comment.
We only needed 14.17.0 and 12.22.0 because they are the min ones from ESLint. All remaining majors can be checked against the most recent one.
There was a problem hiding this comment.
A couple of remarks:
- There's no need to add the
x, as just the number will already take the most recent one. - Also no need to put numbers in quotes if you have a specific patch version
- Node 16.0.0 is a minimum version as well
- Node 17 is EOL
| node: [19.x, 18.x, 17.x, 16.x, 14.x, '14.17.0', 12.x, '12.22.0'] | |
| node: [12.22.0, 12, 14.17.0, 14, 16.0.0, 16, 18, 19] |
There was a problem hiding this comment.
- There's no need to add the
x, as just the number will already take the most recent one.
Yep, but it makes it more explicit. It's confusing to see just 19 in there.
- Also no need to put numbers in quotes if you have a specific patch version
Good point! Removed for consistency.
- Node 16.0.0 is a minimum version as well
Not sure what this means. We are supporting the whole range for v16, v17, v18 and v19, hence testing only the latest version for simplicity. But we are supporting a restricted range for v12 and v14, hence resting both the min and the most recent version. This was inspired by ESLint CI itself.
- Node 17 is EOL
It is indeed, but by the versions indicated in our package.json we were already providing compatibility. I'm just adding tests here to make it official and sync CI with the indicated engines. I won't drop v17 since that would be a breaking change.
There was a problem hiding this comment.
But we are supporting a restricted range for v12 and v14, hence resting both the min and the most recent version
v16.0.0 is also necessary to test, as otherwise we won't know if it's failing or not (just like we do with v12 & v14)
I won't drop v17
Odd numbered Node versions shouldn't be used by the public, it's only meant for library maintainers to tests if everything is still going to work in next even major updates
We shouldn't actively support them, but also not necessary to exclude them in our engines (for now) until ESLint does so imo
There was a problem hiding this comment.
v16.0.0is also necessary to test, as otherwise we won't know if it's failing or not (just like we do with v12 & v14)
I don't agree. Otherwise we would have to test every single minor and patch of node versions. We only want the min one for v12 and v14 for the mentioned reasons. We can keep it simple for other majos and just test the recent one as ESLint does.
Odd numbered Node versions shouldn't be used by the public, it's only meant for library maintainers to tests if everything is still going to work in next even major updates
We shouldn't actively support them, but also not necessary to exclude them in our
engines(for now) until ESLint does so imo
The point is our plugin is flagged right now as compatible with v17. I'm just adding tests for it, that's it.
There was a problem hiding this comment.
v16.0.0 is a minimum version, but I'm not going to spend my day convincing you of that 😅
There was a problem hiding this comment.
Sorry I don't get it. My point was v16 is supported entirely, so we don't need to test it in both ends; any v16 is fine.
MichaelDeBoey
changed the title
Belco90
changed the title
Belco90
added
the
chore
|
Merging after waiting for reviews for 2 weeks. I need to carry on with other tickets and I have little spare time. |
|
🎉 This PR is included in version 5.10.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 6.0.0-alpha.15 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Checks
Changes
Context
n/a