Skip to content

Streamline CI and coverage collection - #50

Merged
jstayton merged 2 commits into
developmentfrom
VISION-4310-streamline-ci
Oct 8, 2026
Merged

jstayton merged 2 commits into
developmentfrom
VISION-4310-streamline-ci

Conversation

@jstayton

@jstayton jstayton commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Collect coverage during the Node 26 matrix run instead of running the suite a fourth time. Keep tests on Node 22, 24, and 26, run lint/typecheck once, and use npm ci with updated pinned actions, restricted permissions, timeouts, and cancellation of superseded runs.

Validated lint, typecheck, workflow syntax, and all 37 tests on all three Node versions. Node 26 reports 100% coverage.

Fixes VISION-4310.

Comment thread .github/workflows/ci.yml

defaults:
run:
shell: bash

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: This is what keeps pipefail on for the npm run test:coverage | tee step now that set -o pipefail is gone (the job log confirms bash --noprofile --norc -e -o pipefail). On ubuntu-latest it reads like a no-op, though, so I'd add a one-line comment saying why it's here. Otherwise someone will remove it and a failing suite will pass CI.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the comment in c49567d explaining that explicit bash enables pipefail so tee cannot hide test failures.

Comment thread .github/workflows/ci.yml Outdated
strategy:
fail-fast: false
matrix:
node-version: ['22.x', '24.x', '26.x']

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: '26.x' is now hardcoded in six ifs. When Node 28 lands, it'd be easy to bump the matrix and miss one, and then the job would run neither lint nor coverage. What do you think about a matrix flag?

node-version: ['22.x', '24.x']
include:
  - node-version: '26.x'
    is-primary: true

Then if: matrix.is-primary / if: ${{ !matrix.is-primary }}. Your call though.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the is-primary matrix flag in c49567d and replaced all six version-specific conditions. Node 22 and 24 still run plain tests; the primary Node 26 entry runs lint, typecheck, and tests with coverage.

Comment thread .github/workflows/ci.yml Outdated
if:
${{ !cancelled() && matrix.node-version == '26.x' &&
hashFiles('coverage/lcov.info') != '' }}
uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: v7.0.2 (cf430e030ddbb5b0abf93d22962f4752f3646cd9) is out. Bump it while you're updating the other pins?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated upload-artifact to v7.0.2 in c49567d, pinned to cf430e030ddbb5b0abf93d22962f4752f3646cd9. Lint, typecheck, all 37 tests with coverage, and workflow validation pass.

Fixes VISION-4310.
@jstayton
jstayton merged commit cf54e01 into development Oct 8, 2026
4 checks passed
@jstayton
jstayton deleted the VISION-4310-streamline-ci branch October 8, 2026 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant