test(storybook): execute every story play function as a gate - #323
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR makes Storybook interaction assertions “gate-grade” by executing every story’s play function inside the existing Vitest suite, and updates documentation/indexes to record the decision and close change 0040.
Changes:
- Adds a Vitest-based story runner (
src/__tests__/stories.test.tsx) that composes all*.stories.tsxvia portable stories and executesplayfunctions (with explicit handling for jsdom-unevaluable geometry stories). - Fixes/strengthens several story assertions and helpers (WeatherCard, MediaPlayerCard, FanCard, EntityDetailDialog, CameraCard) based on the first full run results.
- Updates Storybook spec + change doc + repo indexes/AGENTS to document the new gate and mark change 0040 complete, plus tracks the remaining browser-only geometry assertions in
docs/tasks.md.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/tests/stories.test.tsx | New Vitest “story runner” that composes stories and runs all play functions; includes browser-only exemption list and camera img-loader shim. |
| src/components/WeatherCard/WeatherCard.stories.tsx | Updates exceptional-condition assertion to check the rendered icon glyph rather than an emoji. |
| src/components/MediaPlayerCard/MediaPlayerCard.stories.tsx | Corrects docblock and scopes transport() helper to the “Media controls” group to avoid counting non-transport pills. |
| src/components/FanCard/FanCard.stories.tsx | Fixes WithoutPercentage story expectation to match preset-first state line behavior. |
| src/components/EntityDetailDialog/EntityDetailDialog.stories.tsx | Switches from asserting UI state flips to asserting a single service dispatch (with recorded promise-shaped mock). |
| src/components/CameraCard/CameraCard.stories.tsx | Removes a race by asserting LIVE within the same waitFor as the overlay render. |
| docs/specs/storybook/index.md | Records the decision: play-function assertions are gate-grade via Vitest portable stories; documents jsdom limitations and runner rules. |
| docs/changes/0040-test-harness-reliability.md | Marks change complete and records PR6 outcome/triage details; checks off PR6 task. |
| docs/tasks.md | Adds backlog item to decide where to enforce jsdom-unevaluable geometry assertions (likely e2e). |
| docs/index.yml | Flips change 0040 status to complete. |
| docs/index.md | Flips change 0040 row to complete. |
| AGENTS.md | Adds a repo-conventions note that story play functions run in npm test, including two traps from the first run. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #323 +/- ##
==========================================
+ Coverage 87.83% 88.13% +0.29%
==========================================
Files 232 232
Lines 8536 8536
Branches 2881 2881
==========================================
+ Hits 7498 7523 +25
+ Misses 819 797 -22
+ Partials 219 216 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Storybook play functions asserted and nothing executed them: build-storybook
proves they compile, *.stories.tsx is excluded from coverage, and Vitest
collects *.{test,spec}.* only. Roughly 340 assertions across 656 stories read
as tests to every reviewer while being unable to fail.
Settle the question the change document asks: stories are gate-grade. The
runner is portable stories inside the Vitest suite that already runs, not a
browser job — composeStories with the real .storybook/preview annotations, so
nothing about a story changes to be testable. Measured price, three paired
full-suite runs: the file costs ~9s of CPU and is one worker among 253, so the
suite's wall clock rises by under a second in steady state.
The first run surfaced 23 failures. Ten were wrong assertions, now corrected:
six where MediaPlayerCard's transport() helper read every pill on the card and
the mute toggle broke each exact list, one asserting a weather emoji retired
along with getConditionEmoji, one asserting ON where the spec gives the primary
slot to the preset, one expecting a state-driven switch to flip against a mock
that pushes no state back, one racing a badge outside its own waitFor. Five
measure rendered geometry jsdom cannot lay out and are named in a
self-verifying BROWSER_ONLY map — each still executes, and must still throw
with the message its entry pins. Seven were the runner's own gap, closed by
reproducing the workshop's camera-readiness substitution and answering for its
frames from the workshop's own static directory. One was the process-wide
dispatch guard admitting a later identical command as a success after a story
left one in flight.
Implements PR 6 of docs/changes/0040-test-harness-reliability.md, and records
PR 7 there: the rule PR 3 installed is still blinded in two src/theme story
helpers by a blanket exhaustive-deps suppression, which the React compiler
reads as a whole-function bail.
…ed distinction has a witness
777606a to
68a79ac
Compare
Summary
Implements PR 6 of
docs/changes/0040-test-harness-reliability.md— "Story assertions run, or stop pretending to".It does not close the change. A seventh task exists and is added here: the rule PR 3 installed is still blinded in two files, so 0040 stays
draftand its closure lands with PR 7. See "PR 7, recorded here" below.The change document asked for a decision and forbade deferring it. The decision is gate-grade, and the cost is what settled it rather than taste: executing these assertions does not need a browser. Storybook's portable-stories API composes a story with the real
.storybook/previewannotations, sosrc/__tests__/stories.test.tsxrenders and plays every story inside the Vitest suite that already runs — no second CI job, no Playwright install, no static build to serve.Cost, measured
Three paired full-suite runs, without → with the runner: 25.1 s → 37.1 s on the first cold pair, then 22.3 s → 22.2 s and 22.2 s → 23.1 s. The story file's own tests cost about 9 s of CPU, and it is one worker among 253, so the suite's wall clock rises by under a second in steady state. Documentation-grade was the other live option and loses on the same arithmetic: it would have retired ~340 assertions and rewritten the survivors into
__tests__for days of work, to end with strictly less checking than a second of CI buys.The first run: 23 failures, each triaged
No assertion was deleted to go green.
Ten wrong assertions, corrected. Six in
MediaPlayerCard.stories.tsx, where thetransport()helper read every.liebe-pillon the card, so the volume mute pill broke every exact-list assertion the moment mute shipped — the helper is now scoped to theMedia controlsgroup.WeatherCard/ExceptionalConditionasserted a⚠️emoji that change 0030 retired along withgetConditionEmoji; the card draws the line-artTriangleAlert.FanCard/WithoutPercentageassertedONwhereoptions/fan.mdgives the primary slot to the preset, and the fixture has one.EntityDetailDialog/BooleanControlclicked the switch and expected it to flip, which a control driven by entity state cannot do against a workshop that pushes no state back — it now asserts exactly oneinput_boolean.toggledispatch.CameraCard/OverlayWithoutStateasserted theLIVEbadge outside thewaitForthat waits for the overlay.Five jsdom cannot evaluate, named in a
BROWSER_ONLYmap with a reason and the message each must throw with. They measure rendered boxes. This is the class the change document already had a named instance of —ForecastsMaxCountOnMinimumWidthTile, written to gate-grade standard by change 0030 and never executed — and the run shows why it never could have passed here: its>= 44pxassertion fails on a 0-wide box while its no-overflow neighbour passes on0 <= 1. Their correctness in a real browser stays unverified in both directions — the runner establishes only that jsdom cannot judge them, not that they are right. Left unenforced and tracked indocs/tasks.md. The map is self-verifying in both directions: each listed story is still executed and must still throw, and must throw with the pinned message, so neither a stale entry nor an unrelated regression in the same story can hide behind it.Seven were the runner's own gap, now closed: every
CameraCardstream story sat inCONNECTING, because the workshop substitutes the stream-readiness hook through a Vite plugin the runner did not reproduce, and because jsdom neither firesloadon an<img>nor reports a decoded size. The runner applies the same substitution and answers for the mock's frames from the workshop's ownstaticDirslisting — the real set, so the deliberately-missing frame still produceserror. Scoped to those frames only, so adata:avatar or a remote album cover is left to jsdom's silence rather than told it 404'd.One is a finding worth keeping.
ActionCard/Activatingrendered the success check while asserting the in-flight spinner, and only when other stories ran first. Not the card:guardedDispatch's pending set is process-wide by design, so the story's deliberately-unsettling call left that command in flight and the next identical command was admitted as a success. Any suite that lets a service call hang inherits this;resetDispatchGuard()per story is the fix, and it is recorded inAGENTS.mdbecause the failure it produces looks exactly like a card defect.No card defect surfaced — the harness defects above are real and are the run's findings; what the ten corrected assertions did not turn up is a single case of product code behaving wrongly. Each was checked against the spec or an existing
__tests__case before being corrected.PR 7, recorded here — the change stays open
src/theme/tokens.stories.tsx:36andsrc/theme/customCss.stories.tsx:34each carry a blanket// eslint-disable-next-line react-hooks/exhaustive-deps, and the React compiler reads that comment as "the author knows they are breaking the rules of React" — so it bails on the entire enclosing function and every compiler-backed rule goes quiet with it,set-state-in-effectincluded. That is the same mechanism that madeCardConfig'sModalinvisible in PR 4, verified there in both directions.Neither effect is a defect; the problem is that a future violation beside them lands unreported, and the blind spot is self-concealing — the suppression that proves the rule once worked there afterwards reads as though it never needed to. So
**Status:** complete→draft,docs/index.ymland thedocs/index.mdrow go back with it, and the task line carries the mechanism and its scope limit (onlyexhaustive-depswas tested as a bail trigger;set-state-in-effect's own suppression does not bail, which is what makes the replacement a fix;rules-of-hooksis untested).docs/index.ymlreads 35status: complete, matchingmain.Probes
Every probe read the reported test count and every mutation was verified applied before the result was read.
presentation.ts:exceptionalresolves the generic cloud instead ofAlertTriangleFanCardstory assertsONagain (the literal "wrong assertion" the change document names)BROWSER_ONLYresetDispatchGuard()removed frombeforeEachnaturalWidthshim returns 0BROWSER_ONLYmessage pin weakened to a pattern the assertion never producesawait Story.load()removed (portable-story lifecycle hooks)load(the served/not-served distinction erased)Two results are reported rather than scored. H is a genuine "not caught": no story's assertions currently depend on the
beforeEachhooksload()runs, so the call is correctness by construction rather than something the suite can feel — it stays because the hooks exist and would otherwise silently not run. I is discarded: the mutation landed and changed the component, but the second dispatch is absorbed byuseServiceCall's own at-most-once handling, so the assertion is satisfied by a guarantee below the component. I2 probes the direction the assertion can actually see.J changed the diff.
CameraCard/StreamErrorhad noplayfunction at all, so nothing witnessed the one distinction the static-file answer exists to preserve — the PR could have claimed "the stream-error story stays honest" against a story that asserted nothing. It now asserts the error surface and the absent badge, and the probe fails as it should.Gates
Run unpiped, exit status read, on the pushed tree.
npm testnpm run lintMigrationCutoffswarningnpm run typechecknpm run test:coveragenpm run build-storybookcodecov/patchhas nothing to gate: every changed source file is*.stories.tsxor under__tests__/, both already excluded from the coverage scope byvitest.config.ts.Review passes
codex review --base mainraised three findings, all fixed: theBROWSER_ONLYexemption accepting any exception (now pins the message), the image shim answering for every image rather than the mock camera frames, and the story lifecycle hooks not running (nowawait Story.load()). Local CodeRabbit raised five; three fixed (promise-shaped recorder, exactly-one dispatch, "no real defect" scoped to card behaviour), one addressed in the spec as a documented residual (a browser-only story stops at its first unevaluable assertion, so keep geometry last), and one rejected on evidence — it claimedload()is not part of the composed-story API, which the runtime disagrees with (composeStoriesexposes bothloadandrun, and every story passes).Copilot's one thread is resolved. Its
__dirnamefinding was right to act on and wrong as stated:__dirnameis defined here by Vitest's SSR transform, and the suggestedfileURLToPath(new URL(..., import.meta.url))is the form that actually breaks — underjsdom,import.meta.urlis the page URL, so it throwsThe URL must be of scheme fileat module scope and the suite collects zero tests. The path question is gone instead: the loader asks Vite's module graph which files.storybook/publicholds, so neither__dirnamenorimport.meta.urlis involved.