fix(jest-reporters): do not check coverageThreshold when running a shard - #16372
fix(jest-reporters): do not check coverageThreshold when running a shard#16372Aeirx wants to merge 3 commits into
Conversation
|
|
✅ Deploy Preview for jestjs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
Bot
added
the
require-changelog
babel-jest
babel-plugin-jest-hoist
babel-preset-jest
create-jest
@jest/diff-sequences
expect
@jest/expect-utils
jest
jest-changed-files
jest-circus
jest-cli
jest-config
@jest/console
@jest/core
@jest/create-cache-key-function
jest-diff
jest-docblock
jest-each
@jest/environment
jest-environment-jsdom
@jest/environment-jsdom-abstract
jest-environment-node
@jest/expect
@jest/fake-timers
@jest/get-type
@jest/globals
jest-haste-map
jest-jasmine2
jest-leak-detector
jest-matcher-utils
jest-message-util
jest-mock
@jest/pattern
jest-phabricator
jest-regex-util
@jest/reporters
jest-resolve
jest-resolve-dependencies
jest-runner
jest-runtime
@jest/schemas
jest-snapshot
@jest/snapshot-utils
@jest/source-map
@jest/test-result
@jest/test-sequencer
@jest/transform
@jest/types
jest-util
jest-validate
jest-watcher
jest-worker
pretty-format
commit: |
A shard runs a subset of the test files, but coverage is still reported for every file matching collectCoverageFrom -- the ones the shard did not exercise come out at zero. The threshold is then checked against that mix, so a `global` threshold fails on every shard no matter how well the project is actually covered. Reproduced on 29.7.0 and on 30.4.2: four test files, four sources, each fully covered by its own test, `global` thresholds at 90%. A full run reports 100% and passes. `--shard=1/4` reports 25% and fails all four thresholds, while the one test file it ran passes. Skip the check for a sharded run and say so, rather than reporting a verdict on coverage the run never set out to collect. Skipped rather than adjusted because there is no adjustment to make: what a `global` threshold describes is the whole project's coverage, and no single shard has it. Enforcing it belongs after the shards' coverage is merged. `--shard=1/1` runs every test file, so it is a complete run and keeps its thresholds -- the condition is shardCount > 1, not "shard is set". No new option and no merge mode, since collecting coverage per shard and merging it is already the documented way to get a whole-project number. Refs jestjs#12751
The e2e cases run Jest in a subprocess, so the new branch showed as uncovered on the patch. Exercise it directly through CoverageReporter as well: a run with shardCount 4 records no error and logs the warning, and one with shardCount 1 still reports the threshold error. Also point the changelog entry at this PR rather than the issue, which is what scripts/checkChangelog.mjs requires.
70b184d to
47f5975
Compare
soltonigiri
left a comment
soltonigiri
left a comment
There was a problem hiding this comment.
Please clarify that Jest does not currently provide a built-in command for applying coverageThreshold to a merged coverage file. I could merge the shard coverage-final.json files, but reproducing Jest's exact-file/directory/glob groups and negative-threshold semantics required a custom script that manually drove CoverageReporter. The docs should either show a concrete external-service/custom-script workflow and its semantic limitations, or avoid implying that this can be completed with Jest itself.
…is not Jest's to do The previous wording told the reader to merge the shards' coverage and check the thresholds against the merged report, which reads as though Jest has a step for that. It does not: there is no flag that merges coverage across runs, and none that checks a threshold against a report Jest did not just produce. Say so, name the workflow that does work -- `json` reporter per shard, merge the `coverage-final.json` files, enforce with a coverage service or `nyc check-coverage` -- and name what it costs. `nyc check-coverage` takes global and per-file thresholds, so this option's glob and path groups do not carry over, and neither does a negative threshold's "maximum uncovered items" rule. A config using those has no exact equivalent outside Jest. The runtime warning made the same implication and now says the enforcement happens outside Jest. Reported by @soltonigiri in review, who hit exactly this: merging the coverage files was straightforward, reproducing the threshold semantics was not.
|
You are right, and the wording was doing exactly what you describe. Fixed in a95ff26. I checked the claim rather than taking it on faith — the coverage flags are The docs now say that outright, name the workflow that does work, and name what it costs:
That last paragraph is your finding specifically — the part you hit was not the merging, it was that the group and negative-threshold semantics have no equivalent on the other side. Worth stating in the docs rather than leaving each person to discover it with a custom script. The runtime warning made the same implication and now ends with "enforce a whole-project threshold outside Jest, against the merged coverage from every shard" instead of implying Jest will do it. Two things I deliberately did not do, both of which I think are the maintainers' call rather than mine:
Tests, lint and format re-run after the change: 26 passing across the unit and e2e suites. |
soltonigiri
left a comment
soltonigiri
left a comment
There was a problem hiding this comment.
Thanks for addressing the documentation concern. I re-reviewed a95ff26 and verified with the built CLI that threshold checking is skipped only when shardCount > 1. Unsharded runs and --shard=1/1 still enforce global, path/directory, glob, positive-percentage, and negative-uncovered thresholds.
Multi-shard runs still produce text/JSON coverage and emit one warning, including under Circus, Jasmine, and a multi-project configuration. I also verified that the two shard coverage-final.json files merge to the complete aggregate.
The revised docs now accurately state that Jest cannot merge or re-check the reports itself, and they call out the path/glob and negative-threshold semantics that nyc check-coverage cannot reproduce. My earlier concern is resolved.
2 participants
Summary
Refs #12751.
A shard runs a subset of the test files, but coverage is still reported for every file matching
collectCoverageFrom— the ones the shard did not exercise come out at zero._checkThresholdthen runs against that mix, so aglobalthreshold fails on every shard however well the project is actually covered.Reproduced on 29.7.0 and on 30.4.2. Four sources, each fully covered by its own test file,
globalthresholds at 90%:Every test that ran passed. The failure is entirely the three files this shard was never going to run.
The change
Skip the threshold check for a sharded run and say so, rather than reporting a verdict on coverage the run never set out to collect:
Skipped rather than adjusted because there is no adjustment to make: what a
globalthreshold describes is the whole project's coverage, and no single shard has it.--shard=1/1keeps its thresholds. It runs every test file, so it is a complete run. The condition isshardCount > 1, not "shard is set" — getting that wrong would quietly stop enforcing coverage for anyone who shards by one.No new option and no merge mode. @SimenB wrote in 2022 that multiple runs are the only way to make the threshold check work and that he was unsure Jest wants coverage merging as a mode; this follows from that rather than arguing with it. Collecting per shard and merging is already the documented route to a whole-project number, and the warning points at it.
A decision worth your call
The skip covers all threshold groups, not only
global. Apathorglobthreshold on a file the shard did cover would still be meaningful, so there is a narrower fix available.I did not take it because Jest cannot tell which files a given shard should cover — the split is by test file, so any group can contain files at zero purely because their tests landed in another shard. Skipping everything is predictable; skipping some groups but not others depends on where the sequencer happened to put things. Happy to narrow it to
globalif you would rather.Tests
Three e2e cases in
coverageThreshold.test.ts:does not check the threshold when running one shard of several— the fix. Fails without it (exit 1 instead of 0); I checked by neutralising the guard and re-running.checks the threshold when the run is a single shard—--shard=1/1is unaffected.checks the threshold on a single shard that misses it— a complete run genuinely short of the threshold still fails, so the skip is about sharding and not a way to switch thresholds off.The last two pass with or without the change by design; they are there to pin the boundary, not to detect the bug.
packages/jest-reporters+ coverage/shard/summary e2ee2e/__tests__/coverage*tsc --noEmitonjest-reportersDocs updated on both sides —
coverageThresholdin Configuration.md and--shardin CLI.md — since the two are only surprising together. CHANGELOG entry under Fixes.I have not run the full suite; the above is the coverage, reporter and shard surface plus the gates.