test: run the runes-module tests through the compiler that owns runes - #615
Merged
Conversation
`npm test` is `node --test`, which cannot load a `.svelte.ts` file. The three suites whose subject is a runes store got there anyway by installing their own `$state`/`$derived`/`$effect` onto `globalThis` — 84 lines of hand-written reactivity across the three, plus hand-written `window` and `localStorage`. A hand-written rune is not the compiler's. The shim `$effect` recorded its callback and ran it once, so `settingsPersistence`'s central claim — changing one setting rewrites one localStorage key, which IS Svelte's dependency tracking — was asserted against a hand-rolled simulation of that tracking rather than the thing itself. It could describe the intended shape; it could not fail when the shape was wrong. So: vitest alongside `node --test`, not instead of it. `scripts/*.spec.ts` for the files vitest owns, `scripts/*.test.ts` for the rest; the globs do not overlap, so a file is run by exactly one runner and moving one across is a rename. `reopenDirtyDocument`, `tabPathIdentity` and `settingsPersistence` move, and their shims are deleted: the stores compile through the Svelte plugin, jsdom supplies the browser globals, and `flushSync()` runs the real effects. `resolve.conditions: ['browser']` is what makes that true. Without it Svelte resolves to its SSR build and `$effect` never flushes — five of the fifty-four assertions go green against nothing, which is precisely the failure mode being removed. It is set before anything else in the config for that reason. Two smaller edges, both recorded where they bite: `Reflect.get` must not forward the receiver, because the compiler backs each `$state` with a private field and a getter invoked on a Proxy cannot read one; and `import.meta.url` is an `http://` URL under vitest, so `readSource` takes the cwd-relative form. AGENTS.md carries the criteria, including the two categories that must never move — cross-language contracts and single-implementation conventions. Both are claims about text: running the one implementation that exists cannot observe a second one, so a runtime test cannot express them at all. Timings on this machine: `npm test` 22.3s before, 18.2s after (three files left), `npm run test:vitest` 2.8s. 953 + 54 assertions, 0 failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PathGao
enabled auto-merge (squash)
August 12, 2026 06:23
This was referenced Aug 12, 2026
Merged
PathGao
added a commit
that referenced
this pull request
Aug 12, 2026
…ing away (#624) `SettingsStore`'s constructor calls `$effect.root()` and drops what it returns. In the app that is harmless and deliberate: `settings` is a singleton, one per webview, and it is meant to keep its ~30 write effects and its `storage` listener until the window closes. There is nothing to dispose because nothing ever stops being needed. In the suite it is a leak. Every spec file shares one jsdom, so every store a test constructs adds another live effect per persisted key and another `storage` listener to that one environment, and they outlive the test that made them. `settingsPersistence.spec.ts` already builds a store per test -- two in the multi-window one -- and nothing has bitten yet only because no file reconstructs a store and flushes often enough for an abandoned one to answer first. That is a property of how few files run under vitest today, not of the code: the pilot in #615 is spreading it to another 25, and the failure it produces is a test writing a value some earlier test's store owns, which reads as flakiness rather than as a leak. Worth knowing before the tenth file, not after. So the disposer is kept and exposed as `dispose()`, the same stop-function shape `observeFoldLayout` returns, on the instance because a constructor cannot return a second value. The app calls it nowhere and its behaviour is unchanged to the character. The `window` listener comes off with it: it is registered by an `$effect` inside the same root, and destroying a root runs its effects' teardowns, so `removeEventListener` fires without a seam of its own. Nothing in the constructor registers anything outside the root, so there is no second half left holding the environment. The one thing `dispose()` does not reach is the `initOSType()` promise, which can still land on a disposed store's fields -- inert, because with the effects gone no write follows it. The spec constructs through a `createStore()` helper paired with vitest's `onTestFinished`, so a construction site cannot forget. The new test asserts both halves separately: a disposed store's field change writes nothing over the live store's key, and a `storage` event no longer reaches its fields. Falsified by dropping the `dispose()` call: the write assertion goes red with 12 in the key the live store had just set to 30, and with that assertion also removed the listener assertion goes red with the abandoned store's font size folded to 40. Both halves fail on their own, so neither is carried by the other. Co-authored-by: PathGao <gaoyanbo@gaoyanbodeMacBook-Air.local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
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.
114 test files carry 678 source-shape assertions and 88 of them import
scripts/sourceTree.ts, which readssrc/as text. The reason is in that file's own header:node --testcannot import.svelte. Extracting more modules does not move that number — the last round of refactoring moved it by 5.So this changes the runner, for one specific class of test, as a pilot.
The class
.svelte.tsrunes modules — stores, sessions, tab identity, save guards. Real logic that currently cannot be executed at all, so the tests hand-write a$stateproxy shim ontoglobalThisand hope it matches the compiler.Three files migrated:
reopenDirtyDocument,tabPathIdentity,settingsPersistence. They keep their names but move to*.spec.ts, which the existingscripts/*.test.tsglob does not match — two runners, zero overlap, either can be stopped.What the pilot had to prove
1. The shims come out — 84 lines. No
.spec.tsinstalls$state/$derived/$effectontoglobalThisany more. The stores compile through the Svelte plugin; jsdom supplieswindow/localStorage/navigatorfor real.settingsPersistencelost ~62 of the 84 (full rune shim pluslocalStorageShimandwindowShim).2.
$effectreally flushes.settingsPersistence.spec.tsnow constructs a realSettingsStore— whose constructor installs ~30 effects inside$effect.root— callsflushSync(), mutates one field, flushes again, and diffs actuallocalStorage. It asserts the changed-key set is exactly['editor.minimap'], then exactly['editor.fontSize','editor.language'], then[]for a no-op.The old version hand-simulated the flush loop and then asserted on its own simulation, so it could not fail. The
registeredEffects.length === entryCount + 1counting test is gone with it — subsumed, because the behaviour is now observed rather than counted.Negative control: setting
resolve.conditions: []makes exactly 5 of 54 fail — precisely the$effect-dependent ones. That is simultaneously proof the trap is real and proof the effects are running.3. CI does not get slower.
npm test22.3s → 18.2s (three files left the glob).npm run test:vitest2.8s. Combined 21.0s, marginally under the baseline. vitest's cost is per-process, not per-file, so further migrations add much less than linearly.The shims had already drifted, and nobody knew
createRecordingStore()forwarded withReflect.get(target, prop, receiver). The real compiler backs every$statewith a#privatefield plus a getter, and invoking that getter with the Proxy asthisthrowsCannot read private member #minimap. Under the identity shim these were plain properties, so it passed.This is the concrete form of the concern: a shim passing is not the same as the code passing.
Other potholes, for the next person
import.meta.urlishttp://under vitest (Vite serves the test files), soreadSource(new URL(…, import.meta.url))dies with The URL must be of scheme file. The cwd-relative string form, whichreadSourcealready documents, works. Noted in its doc comment.storagefor same-document writes (correct per spec). Tests now dispatch a realStorageEvent, which exercises the store's actualaddEventListenerinstead of a captured callback array.vite.config.jsexports an async factory, somergeConfigmust call it first. It merges cleanly withsveltekit()included — no need to drop to a baresvelte()plugin.vitest.config.tswas invisible tonpm run check(tsconfigincludelistedvite.config.*only). Adding it immediately caught a wrongUserConfigimport path.The boundary is written down, with reasons
AGENTS.mdgains a rewritten Testing section:*.spec.tswhen the subject is a.svelte.tsrunes module or needs a real DOM;*.test.tsotherwise, because moving a passing file buys nothing; do not migrate.sveltecomponent tests, because jsdom standing in for Monaco/mermaid/KaTeX is a swamp and "the handler is wired up" is weaker than what it would replace.Then a never-migrate section that gives the reasoning rather than just the list:
That is what makes the two-runner split a defensible end state rather than an unfinished migration.
Known before rolling out further
SettingsStorediscards the disposer$effect.rootreturns, so every store a test constructs leaks live effects into that file's environment. It did not bite here, but a file that constructs stores and flushes repeatedly could see cross-test writes. The fix belongs insrc/and should land before file Border Around Window on Windows 10 #10.foldStatePerDocument.test.tsis not in this class. It slices functions out of a.sveltefile with a hand-written brace matcher; a runner swap does not fix that. The subject needs extracting into a.svelte.tsmodule first.25 files still install rune shims. Each was ~15 minutes once the config was right, and the API delta is one import line.
Verification
npm test953 pass / 0 failnpm run test:vitest54 pass / 0 failnpm run check766 files / 0 errorssrc/orsrc-tauri/was touched.🤖 Generated with Claude Code