test: fold the suites onto the option each one covers - #725
Merged
Conversation
Twenty-four files for seventeen subjects: `generate` was spread over three, `minify` over three, `minimizerOptions` over two, and `imagemin` over two. Each option now has one file -- watch, the filesystem cache and `imageminGenerate` join `generate`, css and swc-html join `minify`, `terserOptions` joins the alias it is deprecated for, and `imageminNormalizeConfig` joins the minimizer that calls it. The helpers those files each carried a copy of are now written once. Merging needed the environment gate to move first. Four of them were skipped whole by `testPathIgnorePatterns`, so folding one into a file that runs everywhere would have taken it to Node 10 and to Windows, where its minimizers do not install. `test/helpers/env.js` now holds what this environment can run as one answer: `jest.config.js` reads it to skip a whole file, and a merged file reads it to skip only the block that needs more. Two files still need the whole-file kind, both because they reach for what they need at module scope. `getEcmaVersion` is covered by builds now rather than by calling it: each case sets `output.environment` and reads back the `ecma` that reached the minimizer, which is the wiring the option is for. Every flag is stated false first, since webpack fills an unset one from the target and would raise the answer behind the case; and the minimizer runs in process, or the recording happens in a worker and never comes back. Snapshots moved with their tests and were re-keyed for the describe level the gating adds. Same 573 tests, same 825 snapshots.
The image minimizers, the query in an asset name and what identifies a minimizer to the cache are all the `minify` option: which assets it is offered and what it does with them. They join it, and the worker joins `parallel`, which is the option that decides whether there is one. That leaves one whole-file skip instead of two. The image file was skipped wholesale where `sharp` and friends are absent; nothing in it reaches them at module scope, so it is gated on its block instead and every other case in `minify-option` now runs on the legacy rows too. Only `embedded-source` is still skipped whole, because it requires a deep webpack path at module scope that ships from 5.110. Thirteen files where there were twenty-four, same 573 tests and 825 snapshots. `minify-option` carries 157 of them, which is the cost of the option being this large a subject.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #725 +/- ##
=======================================
Coverage 94.91% 94.91%
=======================================
Files 4 4
Lines 1120 1120
Branches 391 397 +6
=======================================
Hits 1063 1063
Misses 50 50
Partials 7 7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
CI caught what local runs could not: on Node 20 every test passed and the job still failed, with 51 obsolete snapshots. A snapshot belonging to a block skipped inside a file that still runs has no test to match, so jest reports it obsolete, and the rows from Node 20 up pass `--ci` without the `-u` that quietly prunes it on the older ones. A whole-file skip never had the problem because the pack was not read at all. So the CSS, swc-html and image suites go back to their own files and their own file-level skips, and `jest.config.js` says why. What stays merged is what takes no snapshot: the watch and filesystem-cache cases on `generate`, `imageminGenerate` beside the generator it belongs to, `terserOptions` on the option it is an alias of, the worker on `parallel`, and the query and version cases on `minify`. `imageminNormalizeConfig` joins the image file, which shares its gate. Sixteen files rather than thirteen, and the block-level gate is kept only where it is sound. Verified by forcing each gate off and re-running: 553 then 389 tests, no obsolete snapshot either time.
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
1 participant
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.
Summary
test/had grown a file per situation rather than per option —css-minify-option,swc-html-minify-optionandimage-minify-optionwere all theminifyoption;terserOptions-optionis its own deprecated alias;generate-watchandgenerate-filesystem-cacheweregenerateseen from two angles. Twenty-four files become thirteen, with each one named for the option it covers.The reason this is worth more than tidiness is the gating. Five of those files were skipped whole by
testPathIgnorePatternswherever their packages or webpack version were missing, so on the legacy Node rows and on Windows every case in them disappeared — including the ones needing nothing special.test/helpers/env.jsnow holds those environment facts as one source of truth,jest.config.jsreads it for whole-file skips and a merged file reads it to skip only the blocks that need more. One whole-file skip is left where it is genuinely unavoidable:embedded-sourcerequires a deep webpack path at module scope that ships from 5.110, so it throws before anydescriberuns.getEcmaVersionloses its unit test and gains nineteen build-driven cases instead, reading back theecmaa recording minimizer actually received — which tests the wiring rather than the lookup.No test was deleted, rewritten or skipped: 573 cases and 825 snapshots before and after.
What kind of change does this PR introduce?
test
Did you add tests for your changes?
This is only tests — moved, not added, apart from
getEcmaVersion's cases becoming builds. Snapshots moved with their tests and were re-keyed for the addeddescribelevel (61 keys), verified with--ciso nothing could be silently rewritten and nothing is reported obsolete.Does this PR introduce a breaking change?
No — no source change.
If relevant, what needs to be documented once your changes are merged or what have you already documented?
n/a —
test/helpers/env.jsdocuments each gate next to the flag it sets, which is where a contributor adding a suite will look.Use of AI
Claude Code did the consolidation. It checked what actually blocked each merge rather than moving text:
embedded-source's module-scoperequireis why that one file keeps its whole-file skip and whyembedded-protocolcould not join it, andimage-minify-optionwas confirmed to reach its packages only inside test bodies, which is what made per-block gating safe there. Two mistakes it made are worth knowing, since both would have passed review while testing nothing: the firstgetEcmaVersionbuild returnedecma: undefinedbecause the recording minimizer ran in a worker, and the flags had to be set explicitlyfalsebecause webpack fills unset ones from the target defaults and would otherwise raise the answer behind the assertion.🤖 Generated with Claude Code
https://claude.ai/code/session_016TQeNpahUSDUjD2Crugy5H
Generated by Claude Code