ci: simplify test scripts and separate coverage from test runs#2566
ci: simplify test scripts and separate coverage from test runs#2566WilliamBergamin wants to merge 8 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: af3165f The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2566 +/- ##
=======================================
Coverage 87.41% 87.41%
=======================================
Files 62 62
Lines 10251 10251
Branches 415 415
=======================================
Hits 8961 8961
Misses 1269 1269
Partials 21 21
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
zimeg
left a comment
There was a problem hiding this comment.
@WilliamBergamin Amazing improvements to CI health! I like the pattern of checking coverage for the latest version so much 🧪 ✨
I'm approving with a few notes and question about dependencies noted? If this all seems expected please do merge:
Added glob-bin as a root devDependency to replace bash -O globstar glob expansion in test scripts (cross-platform compatibility on Windows CI).
I added af3165f during review that hinted toward this package, but was this still used?
🚢 💨
There was a problem hiding this comment.
🔭 note: I'm so curious for what happens to this file in upcoming releases!
| pull_request: | ||
|
|
||
| env: | ||
| LATEST_SUPPORTED_NODE: "24.x" |
There was a problem hiding this comment.
🧠 praise: Super nice to surface this clear!
| @@ -64,15 +67,14 @@ jobs: | |||
| run: npm run lint | |||
| - name: Build docs | |||
There was a problem hiding this comment.
| - name: Build docs | |
| - name: Build docs | |
| if: matrix.node-version == env.LATEST_SUPPORTED_NODE |
⌛ thought: We can save ~20 seconds for most runs with this? But perhaps it's adjacent to this PR!
| "test": "npm run test:unit", | ||
| "test:node18": "bash -c 'node --test --test-reporter=spec src/*.test.js'", | ||
| "test:unit": "node --experimental-test-coverage --test-reporter=spec --test-reporter-destination=stdout --test-reporter=lcov --test-reporter-destination=lcov.info --test-reporter=junit --test-reporter-destination=test-results.xml --test src/*.test.js" | ||
| "test": "bash -c 'node --test-reporter=spec --test-reporter-destination=stdout --test-reporter=junit --test-reporter-destination=test-results.xml --test src/*.test.js'", |
There was a problem hiding this comment.
| "test": "bash -c 'node --test-reporter=spec --test-reporter-destination=stdout --test-reporter=junit --test-reporter-destination=test-results.xml --test src/*.test.js'", | |
| "test": "bash -c 'node --test src/*.test.js'", |
🧪 thought(non-blocking): I like the pattern of a minimal test script and a more complex test:coverage but am uncertain which flags remain needed for the first?
| npm test --workspace=packages/web-api # all tests | ||
| npm run test:unit --workspace=packages/web-api # unit only | ||
| npm test --workspace=packages/web-api # unit only |
There was a problem hiding this comment.
🔭 thought: One of these might be good to remove since it's unclear which test suite is used at a glance?
| "lint": "npx @biomejs/biome check packages", | ||
| "lint:fix": "npx @biomejs/biome check --write packages", | ||
| "test": "npm test --workspaces --if-present", | ||
| "test:coverage": "npm run test:coverage --workspaces --if-present", |
There was a problem hiding this comment.
🏆 praise: These patterns are joined well here!
Summary
testscript and atest:coveragescript. Previously most packages aliased test to test:unit which always included coverage instrumentation.Requirements