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

chore(ci): add Node.js 22 #2026

Merged
merged 1 commit into from
Jun 10, 2024
Merged

chore(ci): add Node.js 22 #2026

merged 1 commit into from
Jun 10, 2024

Conversation

XhmikosR
Copy link
Contributor

@XhmikosR XhmikosR commented Jun 8, 2024

The matrix is getting a little big but better test everything. We could probably just test Node.js 16 on Ubuntu to save some time, or exclude some things. Let me know if you have any suggestions.

@SethFalco
Copy link
Member

SethFalco commented Jun 9, 2024

We could probably just test Node.js 16 on Ubuntu to save some time

Regarding time specifically, it's not an immediate concern since they run in parallel, and we have unlimited built minutes. Though, I do still value the idea to reduce running unnecessary tasks. (Wasted energy.)

When I added Windows to CI in #1853, I was thinking it might be best to only run the Windows task on the latest LTS version of Node.js (so 20 atm). I opted against it to play it safe, but thinking more about it, I don't really see the value.

In practice, I don't expect to encounter issues that differ in both Node.js version and operating system (and haven't so far), though it probably can happen. We definitely want to test each Node.js version, and we want to test each operating system for platform specific behavior, but I don't think we need every combination of the two. 🤔

Rather than only dropping Windows on Node.js v16, it might be worth only testing Windows on Node.js v20. (Node.js v22 isn't officially LTS until October 2024.)

@XhmikosR
Copy link
Contributor Author

Actually, if you check the CI run, the slowness is coming from regression tests. Maybe we could leave it as is for now?

@SethFalco
Copy link
Member

SethFalco commented Jun 10, 2024

Regarding the speed, that's because I recently merged the PR that uses svg/svgo-test-suite.

TL;DR: We no longer only test for regressions on the however many W3C test files, but also another 4,000+ icons from Oxygen Icons (KDE icon set). I was hoping to improve performance more before merging, but I decided to get it in already to improve QA before making the SVGO v4 release.

Reference:

It's a nice icon set to test against, since it has a lot of more complex shapes. But unfortunately, takes the regression test task from 1-2 minutes to 18-20 minutes, but seems worth it to me.

Some examples (these are the PNG versions, though):




Sure though, we can leave it. Later, I may still evaluate disabling some Windows builds, but we'll figure it out later.

@SethFalco SethFalco marked this pull request as ready for review June 10, 2024 14:58
@SethFalco SethFalco merged commit 7593b32 into svg:main Jun 10, 2024
12 checks passed
@XhmikosR XhmikosR deleted the patch-1 branch June 10, 2024 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants