perf(workflow): parallelise jobs - #1429
Conversation
Coverage reportCaution Multiple errors occurred
Report generated by 🧪jest coverage report action from eef34da |
Coverage reportTotal coverage
Report generated by 🧪jest coverage report action from 499f53c |
There was a problem hiding this comment.
Hermes Agent Code Review
Verdict: Comment
This PR splits the monolithic test job in run-tests.yml into 6 parallel jobs (lint, audit, docs, build-and-smoke, unit-tests, server-tests), upgrades all actions to v4, expands the Linux matrix to Node 22 + 24, and modernises the Windows and publish workflows. The parallelisation is well-structured — each job is self-contained with its own checkout/setup/install, so there is no shared-state risk between parallel runners.
⚠️ Mergeability note: GitHub reports this PR asmergeable: false/mergeable_state: dirty— there are merge conflicts withmainthat need to be resolved before merging. All 13 CI check runs on the head SHA pass withsuccess.
Warnings
-
Missing
permissionsblock — The originaltestjob declared explicitpermissions(contents: read,checks: write,pull-requests: write,issues: write). None of the new split jobs declarepermissions, so they fall back to repo defaults. Theunit-testsjob usesjest-coverage-report-actionwithGITHUB_TOKEN, which needschecks: writeandpull-requests: writeto post coverage reports. CI is green today (repo defaults are permissive), but if the repo default is ever tightened toread(GitHub's recommendation), the coverage action will silently break. Recommend adding least-privilegepermissionstounit-testsat minimum. -
npm auditstep-order change — In the original,npm auditran before the production install. Now the order is swapped:npm ci --omit=devruns first, thennpm audit. Sincenpm auditreads the lockfile and doesn't neednode_modules, the original order served as a fast pre-check. With the new order, a production-install failure hides audit results. Minor, but worth noting. -
testPathIgnorePatternsbroadness — Addingtest-app-create-andtest-app-init-is a good fix to prevent Jest from discovering spec files inside generated app directories. These are regexes matched against the full test path; bare substrings without/delimiters could theoretically match unintended paths. Anchoring with a leading/(e.g./test-app-create-) would be more precise.
Suggestions
-
Node version inconsistency —
run-tests.ymlhardcodes[22, 24]whilerun-tests-windows.ymlandnpmpublish.ymluse thelts/jodalias. If the intent is to test across multiple majors (22 + 24), a comment explaining why literals are used here vs. the alias elsewhere would help future maintainers. -
Windows lint removal — The
Check code stylestep was removed fromrun-tests-windows.yml. This is fine since lint now runs as a dedicated parallel job on Linux, but Windows-specific lint issues (e.g., CRLF line endings) will no longer be caught. Likely acceptable. -
Redundant registry step in
npmpublish.yml—Set NPM Registry back to httpssetshttps://registry.npmjs.org, which is the same value already set in theInstall dependenciesstep. This appears pre-existing but could be cleaned up.
Looks Good
- ✅ Clean job separation — each parallel job is fully self-contained (checkout → setup-node → npm ci → task), no implicit dependencies between jobs.
- ✅ Action version upgrades:
checkout@v2→v4,setup-node@v3→v4(addresses deprecated Node 12/16 runners). - ✅
npm i -g pm2→npx pm2in server-tests — avoids unnecessary global installs. - ✅ Removing
http://registry.npmjs.orgin favour of the defaulthttps://— better security. - ✅
env.NODE_VERSION: lts/jodin Windows/publish workflows — clean single-source version pinning. - ✅ Combining unzip + pm2 start into a single step — reduces step overhead.
- ✅ No secrets leaked, no injection vectors, no unsafe inputs in workflow
runsteps.
Reviewed by Hermes Agent (GitHub App)
| uses: artiomtr/jest-coverage-report-action@v2.0-rc.2 | ||
| with: | ||
| github-token: ${{ secrets.GITHUB_TOKEN }} | ||
| test-script: npx jest --config=jest.config.js --silent --runInBand --ci --coverage --testLocationInResults --json --outputFile="report.json" |
There was a problem hiding this comment.
The unit-tests job uses jest-coverage-report-action with GITHUB_TOKEN to post coverage reports. The original job had an explicit permissions block (checks: write, pull-requests: write); this job has none. Consider adding least-privilege permissions here so the coverage action keeps working if repo defaults are ever tightened.
| - name: Install production dependencies | ||
| run: npm ci --omit=dev --ignore-scripts | ||
| - name: Check npm audit | ||
| run: npm audit --omit=dev --audit-level=low |
There was a problem hiding this comment.
Behaviour change: npm audit now runs after npm ci --omit=dev instead of before. npm audit reads the lockfile and doesn't need node_modules, so the original order was a faster pre-check. If the production install fails here, audit results are lost.
| strategy: | ||
| matrix: | ||
| node-version: [22] | ||
| node-version: [22, 24] |
There was a problem hiding this comment.
Node versions hardcoded as [22, 24] here, while run-tests-windows.yml and npmpublish.yml use lts/jod. If the intent is to test across two major versions, a brief comment would help future maintainers understand why this matrix differs from the alias-based approach elsewhere.
|
|
||
| // An array of regexp pattern strings that are matched against all test paths, matched tests are skipped | ||
| testPathIgnorePatterns: ['/node_modules/', '<rootDir>/build/'], | ||
| testPathIgnorePatterns: ['/node_modules/', '<rootDir>/build/', 'test-app-create-', 'test-app-init-'], |
There was a problem hiding this comment.
Good fix to prevent Jest from picking up spec files inside generated test-app-create-* / test-app-init-* directories. These are regexes against the full test path — consider anchoring with a leading / (e.g. /test-app-create-) for precision.
Issue
Slow workflow.
Intent
Speed up the workflow execution.
Implementation
Parallelise the jobs.
Checks
npm run lint:fix).npm test).Reviewer checks