refactor(v4): migrate the spec suite onto the shipped test module - #864
Merged
Conversation
…module The specs predate `src/test/index.ts` and imported the same three helpers from `src/test-utils.ts`. Point all 74 of them at the shipped module instead, and delete the twelve hand-rolled `render(html)` copies that `mount()` now covers. `TrackEvent.spec.ts` and `withTransition.spec.ts` keep a one-line local wrapper, because both assert on the component's own element rather than on the wrapper `mount()` returns. `getInstance` and `countRequestedFrames` still come from `test-utils.ts`: `getInstance` reads the raw instances map with no `$isMounted` filter, so it is not the public `getInstances`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnepYhqjPMcFCSb43PoHRM
…ver settle Fifteen files carried a local `observed()`, `settled()`, `ticked()` or `included()` — a loop over `settle()` sized by guesswork, because an observer's delivery and a frame-driven write land on nobody's promise. Where the spec waits for a *condition*, `waitFor()` now polls for that exact condition and the wait is deterministic. The three hand-copied `waitForClass()` bodies go the same way, since a class landing is just another condition. Where the assertion is an *absence* — nothing emitted, nothing imported, nothing mounted, a scroll that never moved — the loop survives under the name `quiet()`, with a comment saying why. An absence cannot be polled for: the predicate is true before anything has happened. Three waits had to move to a later signal than the one they named, because the value lands a scheduler lane before the DOM write it feeds: `Draggable`'s transform, `Carousel`'s `--carousel-progress`, and `smoothTo`'s `isMoving`. The four `Slider*` `ready()` helpers stay: they compose the shipped `settle`/`frames` with per-file instance lookups rather than re-implement a wait. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnepYhqjPMcFCSb43PoHRM
…t writes to Thirty-four of the forty-one `vi.spyOn(console, 'warn')` sites become `captureDiagnostics()`. A spy on the sink cannot see the code, the severity or the reporting component, passes for a diagnostic carrying the wrong code, and breaks when the sink's wording changes. Where a site only asserted "something was warned", the assertion is now the code itself. `Track.spec.ts`'s local `recordDiagnostics()` was `captureDiagnostics()` written out by hand, and is deleted. Seven spies stay, all of them assertions about the sink rather than about a diagnostic: the three in `diagnostics.spec.ts`, the two `$warn`/`$error` sink tests in `Base.spec.ts`, `negotiated-events.spec.ts`'s "cancelling suppresses the sink", and the test module's own spec. `captureDiagnostics()` cancels every event it sees, so using it in those would make the assertion vacuous. `Base.spec.ts` keeps one hand-rolled listener: it asserts the element a diagnostic *started on*, which `captureDiagnostics()` does not expose. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnepYhqjPMcFCSb43PoHRM
Six spec files kept their own listener-attaching recorder, in two shapes:
one that pushed the type alone and one that pushed `{ type, detail }`.
`recordEvents()` returns the richer shape, so the four files that only
want the names map down at the call site, and `Fetch` and `Draggable` —
which already asserted on payloads — read it directly.
`Timer.spec.ts` keeps a two-line `record()` over the shipped helper,
because its eight call sites each name a different set of types and the
mapping reads better once than eight times.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NnepYhqjPMcFCSb43PoHRM
… is left `test-utils.ts` was three things at once: a wait helper, an instance reader, and a registered component tree. The waits now come from `src/test/index.ts`, so the tree moves to `src/todo.fixtures.ts` — where its `registerComponent(TodoList)` side effect is visible in the filename — and `test-utils.ts` keeps only what the shipped module deliberately does not export. `getInstance()` stays because it reads the raw instances map with no `$isMounted` filter, which is what lets a spec look at an instance before it mounts or after it unmounts. `countRequestedFrames()` stays because it replaces a global. No build or packaging change: `scripts/build.js` already excludes `!test-utils.ts` and `**/*.fixtures.ts`, and `check-package.js` already guards both. Verified — `npm run build` emits neither, and `npm run check:package` passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnepYhqjPMcFCSb43PoHRM
`resetRegistry()` is the obvious replacement for a per-test counter, and it is the wrong one here: the state that survives `resetDom()` in these three files is the page-wide `DataRegistry` on the root context, whose group records keep their values after the elements are gone. `resetRegistry()` clears the component registry and never touches it. Say so, so the next reader does not swap one for the other. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnepYhqjPMcFCSb43PoHRM
Export sizeBundled per export with peer dependencies left external, dynamic imports excluded and the output minified; sizes are gzipped. ✅ No export size changes. Unchanged (396)@studiometa/js-toolkit
@studiometa/js-toolkit-v4
|
Code ReviewRisk: Low — No concrete defects were found in the files opened; the refactor is safe to merge aside from the explicitly skipped and unopened diffs. This change migrates v4 specs from hand-rolled helpers to the shipped test module, replaces guessed scheduler waits with condition-based polling, and moves todo fixtures into a dedicated module. It also changes diagnostic and event assertions to use the framework channels directly. I reviewed Review usage: 302,234 in (233,834 cached) / 2,023 out tokens — $0.0605 (openrouter/openai/gpt-5.6-luna, thinking: low) Reviewed by @weareikko/code-review v0.9.5 for commit 5011010. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #864 +/- ##
=======================================
Coverage 97.06% 97.06%
=======================================
Files 176 176
Lines 4561 4561
Branches 1331 1331
=======================================
Hits 4427 4427
Misses 122 122
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
v4 mount benchmarksBase and head measured on this runner, alternating over 3 rounds each; every value is the median of the round medians. Running both sides on one machine is what removes cross-machine noise — a cached baseline from another runner would put it back. A move under 25%, or on a benchmark under 5 ms, is not reported as a change: it is inside the measured noise of a shared runner. No benchmark moved beyond the noise floor. Within noise (18)
|
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.
Moves the v4 spec suite onto
@studiometa/js-toolkit-v4/test(packages/v4/src/test/index.ts, merged in #862) and deletes the helpers the specs had hand-rolled before it existed.1558 tests, 114 files, green at every commit — the same count as
main, so nothing was dropped.What was deleted, and what replaced it
settle/frames/resetDomre-exported fromsrc/test-utils.tssrc/test/index.jsasync function render(html)copiesmount()settle()" helpers (observed,settled,ticked,included)waitFor(), or a renamedquiet()where an absence is assertedwaitForClass()copieswaitFor()vi.spyOn(console, 'warn')sitescaptureDiagnostics()recordEvents()recordDiagnostics()inTrack.spec.tscaptureDiagnostics()src/test-utils.tsis down togetInstanceandcountRequestedFrames. The todo component tree moved to a newsrc/todo.fixtures.ts, where theregisterComponent(TodoList)side effect is visible in the filename.Three assertions that were passing for the wrong reason
mockRestore()clears call history as well as restoring the implementation, soexpect(warn).not.toHaveBeenCalled()after a restore is always true. Three sites insrc/decorators.spec.tshad exactly that shape:merges a static config field on the same class instead of dropping one—warn.mockRestore()ran beforeexpect(warn).not.toHaveBeenCalled()unions refs declared on both sides— samemerges both declarations on a decorated subclass of a decorated class— sameAll three now read
expect(log.codes).toEqual([])with the capture still open, and all three pass for real: declaring the same name on both sides genuinely reports nothing, and refs genuinely union without a conflict. The behaviour they meant to assert is correct — they simply were not asserting it.The swap and the fix could not be separated into two commits: replacing
mockRestore()(clears history) withlog.stop()(does not) is the fix, and there is no intermediate state that both compiles and stays vacuous. They are in b362fea, atsrc/decorators.spec.tslines 514, 562 and 635 of the pre-change file.One dead silencer, and the product behaviour behind it
Track.spec.ts→falls back to an empty payload when 'data-option-payload' is invalid JSONspied onconsole.warnand never asserted on it. UndercaptureDiagnostics()the channel turns out to be completely silent for that case, unlike its two sibling tests which do reporttrack.invalid-json.The reason is deliberate and documented in the framework:
readJSON()insrc/Base.tsswallows a malformed option attribute and hands back the declared default ("Unparsable JSON is not a failure either"), soAbstractTrack.optionPayload's owntry/catchnever runs on the attribute path. No product change made — the asymmetry is pre-existing and intentional at theBaselayer. The test now assertsexpect(log.codes).toEqual([])with a comment naming the cause.Waits:
waitForwhere there is a condition,quiet()where there is notThe loop helpers existed because observer delivery is not synchronous. Every positive wait is now a
waitFor()poll for the exact state the assertion reads — deterministic, and it fails with a useful message instead of a stale read.An absence cannot be polled for: the predicate is true before anything has happened. Those sites keep the bounded loop, renamed
quiet()with a comment. That is the same asymmetryREPORT.md:900records for transition classes, and the threewaitForClassconversions follow it exactly — the positive assertions became polls, the negative one (AnchorNav, "the class is gone afterleaveTransition()") stayed a direct assertion.Three waits had to move to a later signal than the one they named, because the value lands a scheduler lane before the DOM write it feeds:
Draggable— poll the writtentransform, notprops.dampedXCarousel— poll--carousel-progress, notcarousel.progresssmoothTo—waitFor(() => !motion.isMoving)replaces a 300-frame bounded loopDeliberately left alone
getInstanceand its 187 call sites. It reads the rawINSTANCESsymbol with no$isMountedfilter; the publicgetInstances()filters. Not interchangeable.console.warnspies, because each one asserts about the sink, not about a diagnostic: three indiagnostics.spec.ts, the$warn/$errorsink pair inBase.spec.ts,negotiated-events.spec.ts's "cancelling suppresses the sink", and the test module's own spec.captureDiagnostics()cancels every event it sees, which would make all seven vacuous.uniqueGroupcounters.resetRegistry()is the obvious replacement and the wrong one: the state that survivesresetDom()inmigration/Data/*is the page-wideDataRegistryprovided on the root context, whose group records keep their values after the elements are gone.resetRegistry()clears the component registry and never touches it. Documented in 5011010 so the next reader does not swap one for the other.Slider*ready()helpers, which compose the shippedsettle/frameswith per-file instance lookups rather than re-implement a wait.render(html)helpers (coexistence,context-subscription,responsive-options) that deliberately do not settle: their call sites inspect the DOM before mount, andcontext-subscriptionrenders no components at all, somount()would add 5×10ms per call for nothing.Where the shipped helper did not fit
captureDiagnostics()does not expose the event target.Base.spec.tskeeps one hand-rolled listener for the test that asserts a diagnostic started on the component's element (the property that lets a subtree-scoped listener filter). Everything else in that describe block converted.mount()returns the wrapper.TrackEvent.spec.tsandwithTransition.spec.tsassert on the component's own element, so each keeps a one-line wrapper that unwraps.firstElementChild. The duplicated DOM-building and settling is gone from both.Not done here
Twenty-three spec files still attach a diagnostic listener by hand outside a
console.warnspy. Those are the samecaptureDiagnostics()opportunity and are out of this PR's scope.Verification
Run in
packages/v4unless noted.check:packageneeds the main checkout'snode_modulessymlinked into the worktree root; the symlink was removed afterwards and the tree is clean.No build or packaging change was needed, as expected:
scripts/build.jsalready excludes!test-utils.tsand**/*.fixtures.ts, andcheck-package.jsalready guards both. Verified rather than assumed —dist/contains neithertest-utilsnortodo.fixtures.🤖 Generated with Claude Code
https://claude.ai/code/session_01NnepYhqjPMcFCSb43PoHRM