-
Notifications
You must be signed in to change notification settings - Fork 74
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
Test on Node 22. #2347
Test on Node 22. #2347
Conversation
|
.github/workflows/health_checks.yml
Outdated
@@ -47,7 +47,7 @@ on: | |||
description: 'Node versions list (as JSON array).' | |||
required: false | |||
type: string | |||
default: '["18", "20"]' | |||
default: '["18", "22"]' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whats support guidance for 20? since we don't pull 18 up to 20
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a note in PR description about this.
The 18 is minimum we support. The 22 then becomes the max we know we support. I think this is enough. The presence of 18 should assert that we can handle "old node".
We definitely don't want to run all e2e tests on 18,20 and 22. These would run only on 18, 22 by default.
A lightweight solution that we could do is to run install-build-unit-tests on 18, 20 and 22, but I'm still not convinced we need this.
@Amplifiyer @rtpascual wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If most of the customers are on node 20, then I see some value of running at least some sanity tests with it. Though it's true that if the library functions well on 22
and on 18
, every version in between is covered, the real life situations situations such as special logic or considerations for 18
might break that.
My thought is to have some coverage for the majority use case. As soon as the 20
is no longer the majority shareholder, we can drop it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. While testing on 22
should cover 20
, there may be edge cases we might not catch if we're not explicitly testing on 20
and this goes for whenever future node versions are released. Where we set the line for dropping testing a node version we need to decide at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
Would install-build-unit-tests
be good enough or do we want to bring some e2e into the mix ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, look like majority is node 18
so we definitely don't need extensive coverage for 20
. Just the build and unit tests sounds good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, we discussed this offline a bit.
Decided to try just adding Node 22 for now. We'll re-evaluate approach later. We still have room for ~164 more jobs.
@@ -512,6 +512,10 @@ jobs: | |||
node-version: 20 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we remove old reference to 20 here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw this and left it alone. One can still trigger workflow with "20" included in parameter list, if this is removed the exclusion won't work.
Problem
Node 22 is active stable. Customers are using that. We're not testing it.
Changes
Move upper bound Node to 22 in test matrix.
Change unit test resolution script due to nodejs/node#50219 .
Note: This PR makes an assumption that testing on lower and upper boundaries (18 and 22) is sufficient.
Validation
PR checks.
Canaries: https://github.com/aws-amplify/amplify-backend/actions/runs/12417044898
CDK validation: https://github.com/aws-amplify/amplify-backend/actions/runs/12417254468
Checklist
run-e2e
label set.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.