Skip to content

tests: add tests for Node 17 and 19 #707

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 5, 2023
Merged

tests: add tests for Node 17 and 19 #707

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
08a92c3
feat: add support for Node v19
Belco90 Dec 23, 2022
be35454
ci: remove unnecessary string quotes
Belco90 Dec 23, 2022
9adac02
Merge branch 'main' into support_node_v19
Belco90 Jan 5, 2023
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
3 changes: 2 additions & 1 deletion .github/workflows/pipeline.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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']
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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
Suggested change
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]

Copy link
Member Author

@Belco90 Belco90 Dec 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

v16.0.0 is a minimum version, but I'm not going to spend my day convincing you of that 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

eslint: [7.5, 7, 8]

steps:
Expand Down