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.

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

Test on Node 22. #2347

Merged
merged 17 commits into from
Dec 19, 2024
+31 −5
Merged

Test on Node 22. #2347

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
19b21be
Node 22
sobolk Dec 17, 2024
486b8b1
this
sobolk Dec 17, 2024
1206b37
this
sobolk Dec 17, 2024
c68ed7c
this
sobolk Dec 17, 2024
b909e6a
this
sobolk Dec 17, 2024
476231d
this
sobolk Dec 17, 2024
3beb341
Revert "this"
sobolk Dec 19, 2024
1d5deca
Revert "this"
sobolk Dec 19, 2024
74f707c
Revert "this"
sobolk Dec 19, 2024
2e4b689
Revert "this"
sobolk Dec 19, 2024
dcb7873
Revert "this"
sobolk Dec 19, 2024
36ca26a
try this
sobolk Dec 19, 2024
6cf54eb
try this
sobolk Dec 19, 2024
fee70e3
Merge remote-tracking branch 'origin/main' into node-22
sobolk Dec 19, 2024
8e60001
boop
sobolk Dec 19, 2024
ddb7c4d
boop
sobolk Dec 19, 2024
fbb44f2
try this
sobolk Dec 19, 2024
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
12 changes: 8 additions & 4 deletions .github/workflows/health_checks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ on:
description: 'Node versions list (as JSON array).'
required: false
type: string
default: '["18", "20"]'
default: '["18", "22"]'
Copy link
Contributor

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

Copy link
Member Author

@sobolk sobolk Dec 19, 2024 •
edited
Loading

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?

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Member Author

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 ?

Copy link
Contributor

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.

Copy link
Member Author

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.

workflow_call:
inputs:
desired-cdk-version:
Expand Down Expand Up @@ -78,7 +78,7 @@ on:
description: 'Node versions list (as JSON array).'
required: false
type: string
default: '["18", "20"]'
default: '["18", "22"]'

env:
# Health checks can run on un-released code. Often work in progress.
Expand Down Expand Up @@ -121,7 +121,7 @@ jobs:
echo "os=$os" >> "$GITHUB_OUTPUT"
echo "os_for_e2e=$os_for_e2e" >> "$GITHUB_OUTPUT"
if [ -z "${{ inputs.node }}" ]; then
echo 'node=["18", "20"]' >> "$GITHUB_OUTPUT"
echo 'node=["18", "22"]' >> "$GITHUB_OUTPUT"
else
echo 'node=${{ inputs.node }}' >> "$GITHUB_OUTPUT"
fi
Expand Down Expand Up @@ -512,6 +512,10 @@ jobs:
node-version: 20
Copy link
Contributor

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?

Copy link
Member Author

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.

- os: windows-latest
node-version: 20
- os: macos-14
node-version: 22
- os: windows-latest
node-version: 22
runs-on: ${{ matrix.os }}
timeout-minutes: ${{ matrix.os == 'windows-latest' && 35 || 25 }}
needs:
Expand All @@ -538,7 +542,7 @@ jobs:
matrix:
os: ${{ fromJSON(needs.resolve_inputs.outputs.os) }}
pkg-manager: [npm, yarn-classic, yarn-modern, pnpm]
node-version: ['20']
node-version: ['22']
env:
PACKAGE_MANAGER: ${{ matrix.pkg-manager }}
runs-on: ${{ matrix.os }}
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
"test": "npm run test:dir $(tsx scripts/get_unit_test_dir_list.ts)",
"test:coverage:generate": "NODE_V8_COVERAGE=coverage/ npm run test",
"test:coverage:threshold": "c8 npm run test",
"test:dir": "tsx --test --test-reporter spec",
"test:dir": "tsx scripts/run_tests.ts",
"test:scripts": "npm run test:dir $(glob --cwd=scripts --absolute **/*.test.ts)",
"update:api": "tsx scripts/concurrent_workspace_script.ts update:api --if-present",
"update:tsconfig-refs": "tsx scripts/update_tsconfig_refs.ts",
Expand Down
1 change: 1 addition & 0 deletions scripts/get_unit_test_dir_list.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@ result = result.filter((result) => !result.includes('integration-tests'));
result.push(
path.join('packages', 'integration-tests', 'lib', 'test-in-memory')
);

console.log(result.join(' '));
21 changes: 21 additions & 0 deletions scripts/run_tests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { execa } from 'execa';
import fs from 'fs';
import semver from 'semver';

let testPaths = process.argv.slice(2);

const nodeVersion = semver.parse(process.versions.node);
if (nodeVersion && nodeVersion.major >= 21) {
// Starting from version 21. Node test runner's cli changed how inputs to test CLI work.
// See https://github.com/nodejs/node/issues/50219.
testPaths = testPaths.map((path) => {
if (fs.existsSync(path) && fs.statSync(path).isDirectory()) {
return `${path}/**/*.test.?(c|m)js`;
}
return path;
});
}

await execa('tsx', ['--test', '--test-reporter', 'spec'].concat(testPaths), {
stdio: 'inherit',
});
Loading