ci: point the Jest job at test:ci, and correct TESTING.md - #8045
Merged
Conversation
`eform-client/TESTING.md` already documented `npm run test:ci` as what CI runs. The workflows were the outlier, still calling `test:unit`. This aligns them. Behaviourally this is close to neutral, and the doc now says so rather than implying a benefit: - `--ci` is redundant on GitHub Actions. Jest auto-detects CI via `ci-info`, which lists `GITHUB_ACTIONS`; `jest --showConfig` resolves `ci: true` on a runner without the flag. It is kept so the script stays correct anywhere detection does not fire. - `--maxWorkers=2` is dropped from `test:ci`. It was an unexamined number, and it buys nothing measurable: benchmarked on a 4-CPU cgroup with a cold cache, capped 239.7s vs default 240.1s over two runs each, with the two modes trading places. Peak RSS ~2.65 GB against 16 GB either way. Removing it lets Jest scale to whatever runner the repo gets rather than pinning a constant nobody re-measured. TESTING.md's CI section now names the job and both workflow files, states the worker behaviour accurately, and records that these jobs are not required status checks — so a failing Jest run does not by itself block a merge. Not fixed here, but worth knowing: the rest of TESTING.md is stale in ways a consistency touch-up should not absorb. Its entire "Testing Patterns" section teaches Jasmine (`jasmine.createSpyObj`), which no spec in the repo uses and `src/setup-jest.ts` explicitly says was removed; "Common Issues" and "Debugging Tests" reference `karma.conf.js` and `src/test.ts`, neither of which exists. That is a doc rewrite, not this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018qJL2WhHwhZ5CGZehZF2ro
There was a problem hiding this comment.
🟢 Approval recommended
The changes are low-risk consistency updates aligning workflows and documentation, with only minor doc maintainability nits noted.
Pull request overview
Aligns the CI Jest invocation with the documented test:ci entry point, and updates the testing documentation to reflect the CI job/script used.
Changes:
- Switch GitHub Actions Jest steps from
npm run test:unittonpm run test:ci. - Simplify
test:ciby removing the explicit--maxWorkers=2cap. - Update
eform-client/TESTING.mdCI section to name the relevant jobs/workflows and reflect current script behavior.
File summaries
| File | Description |
|---|---|
| eform-client/TESTING.md | Updates the CI testing documentation to reference the actual workflows/job and the test:ci script behavior. |
| eform-client/package.json | Removes the hardcoded Jest worker cap from test:ci. |
| .github/workflows/dotnet-core-pr.yml | Runs the Jest CI script in the PR workflow. |
| .github/workflows/dotnet-core-master.yml | Runs the Jest CI script in the master workflow. |
Review details
Suppressed comments (1)
eform-client/TESTING.md:339
- This statement depends on repository settings (required status checks) that can change independently of the code, so it can become outdated. Consider qualifying it with a time-bound phrasing (e.g., "At time of writing").
These jobs are not configured as required status checks, so a failing Jest run does not by itself
block a merge.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+334
to
+336
| - Jest's default worker count. There is no explicit `--maxWorkers` cap, so Jest scales to | ||
| whatever runner it gets (3 workers on the current 4-vCPU `ubuntu-latest` runner). Capping at 2 | ||
| measured no faster on this suite, so there is no cap left to maintain. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
eform-client/TESTING.mdalready documentednpm run test:cias the CI entry point. The workflows were the outlier, still callingtest:unit. This aligns them and corrects the doc.This is a consistency fix, not a performance or correctness one
I want to be explicit, because the obvious justifications for it are both wrong:
--cichanges nothing on GitHub Actions. Jest auto-detects CI viaci-info, which listsGITHUB_ACTIONS;jest --showConfigresolvesci: trueon a runner without the flag. The snapshot-writing hole--ciwould close is also empty here — the suite has 0 snapshots. The flag is kept only so the script stays correct anywhere detection does not fire.--maxWorkers=2buys nothing measurable, so it is dropped fromtest:ci. Benchmarked on a 4-CPU cgroup with a cold cache, two runs each: capped 239.7s vs default 240.1s — 0.16%, with the two modes trading places between runs. Peak RSS ~2.65 GB against 16 GB either way. It was an unexamined constant; removing it lets Jest scale to whatever runner the repo gets.What was actually wrong
The docs and the workflow disagreed about how tests run in CI, and the docs were right. The doc also claimed a 2-worker cap that is now gone, and said nothing about these jobs not gating a merge.
TESTING.md's CI section now names the job and both workflow files, describes the worker behaviour accurately, and records that no required status checks are configured — so a failing Jest run does not by itself block a merge. (masterdoes have rulesets — an org-levelcopilot_code_reviewon all refs, and a repo ruleset restricting deletion/non-fast-forward onrefs/heads/stable— they simply carry norequired_status_checks.)Verified
35 tracked spec files, 147 tests, all passing, run as CI would (excluding the gitignored
src/app/plugins/dev-mode copies, which do not exist in a CI checkout).Stale, and deliberately not touched here
The rest of
TESTING.mdis wrong in ways a consistency touch-up should not absorb:jasmine.createSpyObj,jasmine.SpyObj). No spec in the repo uses it — 80 usejest.fn/jest.spyOn, andsrc/setup-jest.tsstates outright that the Jasmine compatibility layer was removed. The documented pattern would not run.karma.conf.jsand runng test --source-map. Neitherkarma.conf.jsnorsrc/test.tsexists.That is a doc rewrite and deserves its own PR.
Related
While establishing the above I found that 60 unit spec files across 8 Angular plugin repos are executed by no pipeline at all — filed as #8044. That is the substantive version of "we have tests we do not run"; this PR is the small tidy-up next to it.
🤖 Generated with Claude Code
https://claude.ai/code/session_018qJL2WhHwhZ5CGZehZF2ro