test: add angular test matrix#85
Conversation
Coverage report
Test suite run success10 tests passing in 2 suites. Report generated by 🧪jest coverage report action from be5df1e Show full coverage report
|
Orkuncakilkaya
requested review from
JuroUhlar,
TheUnderScorer and
ilfa
as code owners
There was a problem hiding this comment.
Pull request overview
Adds an Angular version test matrix to validate the library across Angular 15–21, with supporting Node/pnpm configuration for CI and local test execution.
Changes:
- Adds a GitHub Actions workflow for Angular matrix testing.
- Adds a local
bin/test-matrix.shhelper script with similar matrix logic. - Updates Node/test configuration and ignores generated test artifacts.
Reviewed changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/test-matrix.yml |
Adds PR workflow to build and test the library across Angular versions. |
bin/test-matrix.sh |
Adds local shell script for running the same Angular matrix. |
package.json |
Adds Node engine requirement. |
.node-version |
Sets the repository Node version to 22. |
.gitignore |
Ignores pnpm store and matrix test logs. |
projects/fingerprintjs-pro-angular/tsconfig.spec.json |
Adds Node types for specs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
TheUnderScorer
left a comment
TheUnderScorer
left a comment
There was a problem hiding this comment.
Looks good, I think the JS approach is way more readable compared to bash, so thanks for working on this!
One more suggestion: currently, the version matrix lives in two places - in the test-matrix.yml and in the constants.mjs. Perhaps we could introduce a single source of truth with a mapping of specific Node.js <-> Angular versions and use it in both places?
| exec('pnpm --version', { stdio: 'ignore' }) | ||
| } catch (e) { | ||
| console.log('pnpm not found, installing...') | ||
| exec('npm install -g pnpm', { stdio: 'inherit' }) |
There was a problem hiding this comment.
I'd prefer for it to throw an error rather than install the pnpm globally - this should be handled manually IMO.
There was a problem hiding this comment.
nit: most of these methods are just wrappers for fs and add too much noise, consider removing them and just using fs directly when possible.
| import { setupLogDir } from './utils/setupLogDir.mjs' | ||
| import { logErrorSummary } from './utils/logErrorSummary.mjs' | ||
|
|
||
| process.env.CI = 'true' |
There was a problem hiding this comment.
Shouldn't this be up to the environment to declare whether it's CI or not? When running locally (not in CI), this could produce unexpected behaviors.
| async function main() { | ||
| setupLogDir() | ||
|
|
||
| const args = process.argv.slice(2) | ||
| const versionArgs = [] | ||
| for (let i = 0; i < args.length; i++) { | ||
| if (args[i].startsWith('--version=')) { | ||
| versionArgs.push(args[i].split('=')[1]) | ||
| } else if (args[i] === '--version' && i + 1 < args.length) { | ||
| versionArgs.push(args[++i]) | ||
| } | ||
| } | ||
|
|
||
| const versionsToTest = versionArgs.length > 0 ? versionArgs : VERSIONS | ||
|
|
||
| console.log(`Starting tests for Angular versions: ${versionsToTest.join(', ')}`) | ||
| console.log(`Using pnpm store at: ${PNPM_STORE_DIR}`) | ||
| console.log(`Logs: ${LOG_DIR}`) | ||
| console.log('------------------------------------------------------------') | ||
|
|
||
| ensurePnpm() | ||
|
|
||
| const results = await Promise.all(versionsToTest.map((v) => testVersion(v))) | ||
| const failed = results.some((code) => code !== 0) | ||
|
|
||
| console.log('------------------------------------------------------------') | ||
| if (failed) { | ||
| console.log('CI Pipeline Failed') | ||
| process.exit(1) | ||
| } else { | ||
| console.log('Success: All Angular versions passed') | ||
| process.exit(0) | ||
| } | ||
| } | ||
|
|
||
| main() |
There was a problem hiding this comment.
nit: since it's a module, the async/await is available in top-level, so we could remove main and just run the code inline for better readability
pnpm exec changesetto create a changeset. |
This PR solves INTER-2234
CI/CD Improvements:
.github/workflows/test-matrix.yml) to run tests across Angular versions 15–21, each paired with the appropriate Node.js version, enhancing automated compatibility checks.Node.js Version Management:
.node-versionfile specifying Node.js version 18 for local development consistency.package.jsonto require Node.js version 18 or higher using theenginesfield, ensuring developers use a compatible Node.js version.Development Tooling:
package.jsonto include.mjsfiles in addition to.jsand.ts, improving code quality checks for modern JavaScript modules.test:matrixscript topackage.jsonto facilitate running matrix tests locally or in CI.