chore(ci): try to add Node.js v20 to build matrix #28
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds Node.js v20 to the CI build matrix to expand test coverage, working around Node.js v20's lack of wildcard support in its test runner by introducing separate test scripts.
- Adds Node.js v20 to the GitHub Actions build matrix
- Refactors test scripts to use a common base and separate v20-specific script with unquoted wildcards
- Implements conditional test execution in the workflow based on Node.js version
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| package.json | Refactors test scripts to extract common base command and adds test:node20 script with unquoted wildcard pattern for Node.js v20 compatibility |
| .github/workflows/nodejs.yml | Adds Node.js v20 to matrix and implements conditional test execution using version-specific test scripts |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
package.json
Outdated
| "test:runtime": "node --import tsx --test --test-reporter=spec \"test/**/*.test.ts\"", | ||
| "test:watch": "node --import tsx --test --test-reporter=spec --watch \"test/**/*.test.ts\"" | ||
| "test:base": "node --import tsx --test --test-reporter=spec", | ||
| "test:node20": "npm run test:base -- test/**/*.test.ts", |
There was a problem hiding this comment.
The test:node20 script passes an unquoted wildcard pattern (test/**/*.test.ts) to the Node.js test runner. When npm run executes this command, it passes the argument as-is without shell expansion. Whether this works depends on Node.js's native glob support. If Node.js v20's test runner doesn't support wildcards (as mentioned in the PR description), this script may not find any test files. Consider using a shell wrapper (e.g., sh -c) to ensure glob expansion happens, or explicitly list the test files using a tool that performs expansion.
| "test:node20": "npm run test:base -- test/**/*.test.ts", | |
| "test:node20": "npm run test:base -- \"test/**/*.test.ts\"", |
| node-version: ${{ matrix.node-version }} | ||
| - name: Test (Node v20.x) | ||
| if: matrix.node-version == '20' | ||
| # CAUTION: this only works in POSIX environments |
There was a problem hiding this comment.
The comment states "this only works in POSIX environments" but the actual issue is not POSIX-specific. The problem is that npm doesn't perform shell glob expansion on arguments passed to scripts. The test:node20 script passes an unquoted wildcard pattern which may not be expanded correctly regardless of the operating system. Consider updating the comment to accurately reflect the actual limitation, or better yet, fix the underlying issue with how the wildcard is handled.
| # CAUTION: this only works in POSIX environments | |
| # NOTE: npm does not expand shell globs in script arguments; ensure test:node20 handles wildcards portably |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "test:base": "node --import tsx --test --test-reporter=spec", | ||
| "test:node20": "node --import tsx --test --test-reporter=spec test/*.test.ts", | ||
| "test:runtime": "npm run test:base -- \"test/*.test.ts\"", | ||
| "test:watch": "npm run test:base -- --watch \"test/*.test.ts\"" |
There was a problem hiding this comment.
The test:watch script forwards a quoted glob pattern to test:base using --, which may result in nested or escaped quotes being passed to the Node.js command. This could cause the glob pattern to be treated as a literal string rather than being expanded. Consider removing the quotes around the pattern in the forwarded argument, or test thoroughly to ensure the pattern is correctly interpreted by Node.js across different shells and platforms.
| "test:watch": "npm run test:base -- --watch \"test/*.test.ts\"" | |
| "test:watch": "npm run test:base -- --watch test/*.test.ts" |
| "test:runtime": "npm run test:base -- \"test/*.test.ts\"", | ||
| "test:watch": "npm run test:base -- --watch \"test/*.test.ts\"" |
There was a problem hiding this comment.
The test:runtime script forwards a quoted glob pattern to test:base using --, which may result in nested or escaped quotes being passed to the Node.js command. This could cause the glob pattern to be treated as a literal string rather than being expanded. Consider removing the quotes around the pattern in the forwarded argument, or test thoroughly to ensure the pattern is correctly interpreted by Node.js across different shells and platforms.
| "test:runtime": "npm run test:base -- \"test/*.test.ts\"", | |
| "test:watch": "npm run test:base -- --watch \"test/*.test.ts\"" | |
| "test:runtime": "npm run test:base -- test/*.test.ts", | |
| "test:watch": "npm run test:base -- --watch test/*.test.ts" |
The problem is a Node.js v20's lack of wildcard support for the test runner.