[DO NOT REVIEW - TurboSnap 2.0] Sample for canary testing - #1431
Draft
codykaup wants to merge 83 commits into
Draft
[DO NOT REVIEW - TurboSnap 2.0] Sample for canary testing#1431codykaup wants to merge 83 commits into
codykaup wants to merge 83 commits into
Conversation
The v2 module reported 97.7% statement coverage, but mutation testing showed
that number was hiding real gaps: graph.ts, storybookFiles.ts and fileHashes.ts
had no test file of their own, and every hash assertion in the suite was
relational ("this hash moves when that file changes"). Six deliberate breakages
survived the whole suite, including two that the source comments explicitly warn
against — dropping the length prefixes in hashEntryIdentity, and sharing one
accumulator across preview subtrees.
Add direct tests for the three untested modules, covering the hash encoding,
sort-independence, cycle termination, missing graph nodes, and the three-homes
file attribution.
Add manifest.golden.test.ts, which pins the exact published hashes for a frozen
fixture. Relational assertions cannot see a change to the hash recipe itself, and
a recipe change is not local to one build: the backend compares each build
against its baselines, so every branch with a baseline recorded by an older CLI
full-snapshots on its next run until it re-baselines. The file carries its own
mock harness so the golden values cannot drift with a shared fixture, and its
header spells out that a diff there is a fleet-wide cache invalidation rather
than a value to regenerate.
Close the three remaining uncovered lines: writeManifest's file write, the
concatenated-child names in statsAnchor's evidence sweep, and the unreadable
directory branch in the out-of-graph sweep. The last one needed a new
unreadable-directory hook in the fake fs, since the existing mock could only
simulate a missing directory and the sweep handles the two cases separately.
All six previously surviving mutations are now caught, and manifest.ts,
statsAnchor.ts and outOfGraphFiles.ts reach 100% line and function coverage.
V2 runs purely for its side effects now: no result inspection, no logging, and errors are swallowed so a v2 failure can never block or alter v1's outcome. Updates tests accordingly and tidies up test boilerplate.
`collectStoryImporters` matched the entry allowlist against the raw stats spelling, so a builder that names its entry without a `./` prefix matched nothing. `storybook-builder-rsbuild` 3.x is such a builder: its `withChromaticMinimalContract` shim re-derives names via `path.relative(cwd, …)`, which leaves both spellings in one graph — the config entry is named `./storybook-config-entry.js` but referenced as bare `storybook-config-entry.js`, and the require-context is named bare. No reason ever matched, no story importer was found, and every build bailed `noStoryFiles`. Every other part of v2 compares canonical keys, so compare these on canonical keys too. `storyImporters` and `entryFiles` now hold nothing else, which drops `stripConcatenatedModuleSuffix` from the file entirely — `normalizeStatsPath` already calls it. `buildManifest` canonicalises `reasons[].moduleName` once and reuses the result for both the story test and the dependency edges, where the second call site already normalised. Real builder 3.3.4 stats, no synthetic name transform: 0 story files before, 3 after. Canonicalising also let three `external "__STORYBOOK_MODULE_*"` modules into `storyImporters` — the config entry imports them and they have no on-disk file, which is the shape of a require-context. Measured inert (an external is a graph leaf; nothing lists one as a reason), but gated out anyway, since that set is what someone reads when debugging story detection. `statsAnchor`'s `BUILDER_ENTRY_FILES` is deliberately untouched: it matches basenames over absolute names because it needs the entry's directory to compare against `projectRoot`, and normalising would relativise that away.
`chromatic turbosnap-manifest` calls buildManifest directly, so it runs none of v2's guards. That left every bail unmeasurable locally, and every finding on the rsbuild map is a bail. turbosnap-bail runs the production traceChangedFiles instead, with a stub Index client, and prints the bail reason as JSON. Only the network is faked, so every stats-shaped and emptiness guard fires as it would in a build. The verdict names the builder generation the stats came from, because a bail on one generation of a builder says nothing about another. --project-json and --builder-name exist for the layouts a checkout cannot build on its own: a synthetically transformed stats file no longer sits beside its metadata. The input assembly both commands share moves to turbosnapInput, so the two cannot derive the same run two different ways.
The widened AST pattern newly populated builder, refs and staticDir on ctx.storybook for those projects, and TurboSnap v1 reads staticDir to decide its static-file bails. readMainConfig now takes the pattern, so v2's readStorybookDirectories widens it for itself and v1's inputs are unchanged.
Three rounds running, a change made for v2 widened the shared metadata path without anyone noticing v1 read it. The field set is now asserted per config shape, so a fourth occurrence fails loudly rather than silently.
turbosnap-bail ran the full stats pass twice: once in readTurbosnapInput and again in production traceChangedFiles. The fallback comment also claimed a reachability that only holds behind v2's anchor bail, which turbosnap-manifest does not run.
TurboSnap 2.0 gains one module that owns its disk reads, reached through the input objects the subsystem already threads. The recursive directory sweep moves into it: the module, not the caller, knows that symlinks are followed and named by the link path, that a cycle terminates, and that a missing or unreadable directory yields nothing rather than an error. The adapter is a required field on the entry point's input and on the manifest builder's input, with no default: a default would let a caller omit it and silently get the real disk, which is the failure the seam exists to prevent. Production and both local diagnostic commands pass the real adapter explicitly. The six circular sweep tests are gone. They covered symlinks, cycles and unreadable directories against a hand-built fake that simulated symlink semantics, so they proved the fake follows symlinks and could not fail when the real sweep was wrong. Those rules are now pinned against real temporary directories in projectFiles.test.ts, and the manifest suites describe their disk as a value through the in-memory adapter instead of mocking fs/promises. No production behaviour changes; the golden manifest hashes are unmoved.
The remaining three disk rules move into the ProjectFiles module, and the two helper modules that held them are deleted. Only the real adapter now names a Node filesystem primitive for reading, apart from the stats anchor, which ticket 03 moves. Content hashing gains one improvement: the shared hashing utility rejects with whatever the underlying read produced, losing which of the N paths failed, so the module wraps it and names the path. An on-call engineer reading the Sentry event under a manifest-build failure now learns which file could not be read. Throwing stays: it propagates one level, is caught at the entry point and becomes an internal-error bail, after which TurboSnap v1 traces the build. A returned failure list would give a caller a legitimate-looking way to continue without the failures, which silently under-captures. The hashable-file rule — a directory is not a file, as storybook-builder-rsbuild 3.3.0/3.3.1 requires — now states itself once inside the module rather than at the one call site we happened to hit. Package versions resolve the package's own manifest, so a package that does not export ./package.json reports no version; the Storybook-core and builder-Vite helpers keep their own policy and only the disk read moved. Two rules gain their first test ever: the directory-instead-of-a-file case and the non-exported-package-manifest case. The manifest, Storybook version and builder-Vite suites stop mocking fs, module and the hashing utility; they describe their disk as a value through the in-memory adapter instead, which now carries content hashes and installed package versions alongside the directory tree. The writeManifest tests write into a real temporary directory, writing being the one disk operation the module does not own. No production behaviour changes; the golden manifest hashes are unmoved.
The anchor used one existence primitive for two different questions. Each becomes the question it is: the source-module check asks whether a path is a file, and the owning-project walk asks whether a path is a directory. No general existence method is offered on ProjectFiles — a name that tells a caller nothing about what they may then do with the path is the kind of width this work exists to remove. The anchor module no longer imports any fs primitive, and its input carries the adapter as a required field with no default, like the other inputs. This carries the one intended behaviour change in the refactor, and it is a narrowing: a directory named like a source file previously satisfied the anchor's existence check and now does not, and a plain file named like the config directory no longer makes its directory the project that owns the stats. Both cases are pathological and answering them correctly is a fix — reading either path throws EISDIR or yields no config. A test pins each. Everything else about stats-root resolution — which roots are tried, that the project root wins whenever any source module resolves there, and the repository-root fallback — is unchanged. The anchor suite now passes realProjectFiles() explicitly; its fixtures are real temporary directories, so the real disk is the disk under test. The two bin-src suites gain a statSync fake alongside the existsSync one they already had, modelling the same disk they always described. Ticket 04 replaces those mocks with the in-memory adapter.
The suite mocked six sibling modules. Three of them — the manifest builder, the anchor module and the builder-compatibility gate — were mocked only to control what the disk answers, so the suite asserted against stub shapes kept up by hand. Those three go, and the api mock goes with them: the network is faked through the injected GraphQL client instead, so the assertions are on the mutation input the Index receives. Only error reporting stays mocked. The disk is a value now, and the manifest is read back from a real temporary directory, because writing is not reading and ProjectFiles does not model it. Two assertions changed shape rather than being dropped. That nothing is read off a disproven anchor is now proven by outcome: the stats are Vite's with no builder-vite installed, so the builder gate would have bailed had it run. That the anchor check and the manifest share one pass over the stats is structural in index.ts and covered end to end by index.statsRoot.test.ts, which stays untouched as the wiring check. Adds the one bail path that had no test: a disk that fails while the stats root is being resolved.
The written manifest now leads with the whole-Storybook hash and its inputs, then the story hashes, and keeps the large `files` graph last, so a person opening the JSON reads the summary before the bulk. The synthetic `storybookFiles` keys lose their angle brackets. They kept the brackets to prove they cannot collide with a real file; a canonical path always starts with `./` or `../`, so a bare name proves the same thing and reads better. The keys feed hashEntryIdentities, so the golden storybookHash moves.
The rule that decides what a story file is was split across two exports and three steps buildManifest performed itself: it filtered the importers against the story-importer set, checked the file had a hash, and only then asked isStoryFile. Nothing could ask the question without rebuilding that loop, so the rule had no suite of its own — its 23 tests reached it through the whole manifest pipeline, paying for a disk, a config directory and a Storybook version to assert which modules are stories. detectStoryFiles now answers it in one call and absorbs all three steps. It takes the paths that have a file on disk as a `has`-only interface rather than the hash map, because that is the only thing it ever asked of them; buildManifest passes `hashes` straight through, so nothing is copied. The module loop is left building graph edges. It costs one more pass over stats.modules. Measured against the 664-module fixture in bin-src/__mocks__, that pass is under 3ms where reading and parsing the same file is 30ms. No behaviour change: old and new detection produce the same 96 story files over that fixture, and the same unrecognized entries. The detection tests move to storyDetection.test.ts and take a stats fixture and a set of paths — no disk, no hashing, no config directory. manifest.attribution.test.ts keeps what it is named for. Two of the moved tests changed what they exercise: the relocated-entry test now lists the entry in realFiles, as a real build cache would, so the basename check is what carries it rather than the absent-importer fallback; and the external module is now absent from realFiles, so the guard the test describes is the one that fires.
buildManifest did two unrelated jobs. It read a stats file, deciding what the builder actually emitted across webpack, rspack and Vite spellings, and it rolled that up into the manifest the Index compares. The two fail differently and are tested differently, but both were only observable through the same function, so "rspack puts the real file name in nameForCondition" and "a roll-up is path-sensitive" shared one 662-line suite and every builder-shape test paid for a disk, a config directory and a Storybook version to assert which module the graph rooted at. readStatsGraph now answers the first question and buildManifest only rolls up. The seam is the graph value: files, hashes, story files and the unrecognized entries. Hashing stays inside the reader rather than being lifted out for purity, because detectStoryFiles already depends on it — telling a story file apart from a require-context glob is asking whether there is a file on disk, which is what the hashes answer. Hiding that ordering is most of what the module is for. pruneSyntheticFiles stays private to manifest.ts and still runs last. A synthetic node is a reachable member of the subtrees the roll-ups walk, so it contributes a [path, ''] entry; pruning any earlier would move every hash that walked through it. That was the one fact the suite could not see, so it is pinned first: a synthetic leaf hanging off a story now has to move that story's hash even though pruning erases it from the serialized graph. Verified by moving the prune call up and watching it fail. countNodeModulesFiles moves too. Its own comment already said it is a property of the builder's output rather than of the manifest, and the noNodeModulesFiles bail is asking the stats, not the manifest. No behaviour change: the serialized manifest is byte-identical over the 664-module fixture in bin-src/__mocks__, and the golden hashes are untouched. The builder-spelling tests move to statsGraph.test.ts and assert on the graph — that the right edge exists and the right bytes were hashed — rather than on a story hash two steps downstream. Where a pair of tests proved an edge exists and then that its content reaches the roll-up, the pair collapses: the second claim is graph.test.ts's job, and end to end the leaf-inclusion and golden tests already cover it. The relocation and order-independence tests stay end to end in manifest.hashing.test.ts, because the claim there is about the whole pipeline being path-canonical, which the primitives being path-sensitive does not imply.
storybookFiles -> storybookFileHashes and storyFiles -> storyFileHashes, in the in-memory manifest and in the file written to disk. The story-file set that story detection returns keeps its name; it holds paths, not hashes.
codykaup
marked this pull request as draft
August 10, 2026 17:23
Contributor
|
📦 Package Size: 7192 KB |
The `storybookFileHashes` section mixed a path key with its four bare keys. Nothing keys off the preview path — the backend only asks whether an entry moved — so the section now reads uniformly. Multiple config dirs (a monorepo case) fold into the one entry. That costs no precision: a move in any entry means recapture everything either way. A preview rename is still visible, because a file's own path reaches its roll-up. The golden `storybookHash` moves with the key, as the header there requires. v2 hashes are not published yet, so no baseline re-capture follows.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1431 +/- ##
==========================================
+ Coverage 83.88% 85.62% +1.74%
==========================================
Files 257 277 +20
Lines 4932 5509 +577
Branches 1386 1523 +137
==========================================
+ Hits 4137 4717 +580
+ Misses 686 683 -3
Partials 109 109 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
codykaup
force-pushed
the
cody/turbosnap-v2
branch
from
August 12, 2026 22:07
83a7658 to
cc363b9
Compare
The CJS proxy importer-edge fix has landed, so v2 no longer has to refuse Vite stats by builder version. This deletes the whole untrustedBuilderStats bail family: packageNotFound and invalidVersion only existed to support the version read, and CHROMATIC_TURBOSNAP_TRUST_BUILDER_STATS was always meant to go with the gate. isBuilderViteStats stays, since the anchor check still compares the builder that produced the stats against the one the project declares. It moves into statsAnchor.ts, its only remaining consumer. Also fixes two test suites that were already failing on this branch: the mutation payload assertion left over from passing storybookConfigHashes, and a context log mock with no debug method.
codykaup
changed the title
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.
Important
This is just for the canary build of the CLI. This will not be merged into production. It will be split up into smaller PRs then cleaned up.
We need a canary build of the CLI to do some testing against the Index changes. This has zero intention of being merged.
📦 Published PR as canary version:
18.2.1--canary.1431.31646871325.0✨ Test out this PR locally via:
npm install chromatic@18.2.1--canary.1431.31646871325.0 # or yarn add chromatic@18.2.1--canary.1431.31646871325.0