Skip to content

chore(check): type-check scripts/, and fix an assertion that could not fail - #432

Merged
PathGao merged 1 commit into
masterfrom
chore/type-check-the-tests
Aug 3, 2026
Merged

chore(check): type-check scripts/, and fix an assertion that could not fail#432
PathGao merged 1 commit into
masterfrom
chore/type-check-the-tests

Conversation

@PathGao

@PathGao PathGao commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Two defects in the test tooling. They are independent; they share a PR because both were found by reading the same file.

1. The test files were never type-checked

npm run check is svelte-check --tsconfig ./tsconfig.json, and that config extends .svelte-kit/tsconfig.json. SvelteKit generates that file's include from a fixed list of directories:

"include": [ "../src/**/*.ts", "../src/**/*.svelte", "../test/**/*.ts", "../tests/**/*.ts", ... ]

scripts/ is not on it, and never was. All 89 test files sat outside the program.

Measured, both ways

A deliberate const injectedTypeError: number = "not a number"; appended to scripts/foldKeys.test.ts:

result
before COMPLETED 439 FILES 0 ERRORS 0 WARNINGS 0 FILES_WITH_PROBLEMS
after ERROR "scripts/foldKeys.test.ts" 31:7 "Type 'string' is not assignable to type 'number'."

The practical cost of the gap: renaming an exported function gave a contributor a green npm run check and dozens of red tests with no tooling pointing at the cause. In one measured rename (loadMarkdownopenDocument), 26 of 36 failures were TypeError: … is not a function.

Why the fix is in the root tsconfig

.svelte-kit/tsconfig.json is regenerated by svelte-kit sync on every check, so editing it is not an option. SvelteKit exposes no configuration for extra include directories. The supported path is the one the root tsconfig.json already documents in its own comment:

If you want to overwrite includes/excludes, make sure to copy over the relevant includes/excludes from the referenced tsconfig.json — TypeScript does not merge them in

So the root config now carries a copy of the generated list, rewritten one level up, plus scripts/. test/ and tests/ are kept even though neither directory exists: dropping them would silently re-create this exact bug for whoever adds one. The comment says to re-sync on a SvelteKit upgrade.

Two supporting changes:

  • @types/node (devDependency). Every test file imports node:test, node:fs and node:assert/strict. None of them were typed — the first run reported 343 errors, ~300 of which were Cannot find module 'node:test'.
  • allowImportingTsExtensions. 18 imports across 7 test files name their subject with an explicit .ts extension (await import('../src/lib/utils/markdown.ts')), which is what tsx — the loader npm test uses — resolves. The flag requires noEmit, which this project already sets, so there is no output path for the extension to be wrong in. The alternative, rewriting 18 working imports to .js to satisfy a config default, is churn with runtime risk on the side that actually runs.

The 38 errors it surfaced

Across 13 files. None suppressed — no any, no @ts-ignore, no exclusions.

count error fix
18 An import path can only end with a '.ts' extension… allowImportingTsExtensions, above
9 Conversion of type 'TransferableTab' to 'Record<string, unknown>' may be a mistake the tests corrupt one field of a snapshot and re-serialize. { ...snapshotTab(tab) } is an anonymous object type, which does get an implicit index signature — no cast at all
3 Could not find a declaration file for module 'monaco-editor/esm/vs/…' monaco publishes types for its public surface only. scripts/monaco-internals.d.ts declares the three internals pasteUrlContext.test.ts drives, with the shapes it uses. This removes an existing as any on MonarchTokenizer
2 '{ id: string; … }' is not assignable to type 'Tab' genuine fixture bugs: makeTab() in tabTransfer.test.ts and lossyDecodeSaveGuard.test.ts both declare : Tab while omitting the required collapsedHeaders: Set<string>
2 Argument of type 'FakeElement' is not assignable to 'Element' one asElement() helper, matching the as unknown as ParentNode the same file already used for FakeRoot
2 Conversion of type 'Record<string, unknown>' to 'SettingsStore'… the Proxy target was cast to Record<string, unknown> going in and back to SettingsStore coming out. Dropping both casts types it correctly
1 Unused '@ts-expect-error' directive (vite.config.js) // @ts-expect-error process is a nodejs global became stale the moment @types/node landed

Two of these are real defects rather than annotation noise. The collapsedHeaders omissions mean both fixtures have been claiming to be Tab while not being one — precisely the class of drift this change exists to catch.

2. An assertion that could not fail

scripts/issue261EditorPdf.test.ts:76:

const viewerPrintStyles = viewer.slice(viewer.indexOf('\t@media print {'));

src/lib/MarkdownViewer.svelte contains no @media rule of any kind. Verified directly:

indexOf = -1
slice length = 1
slice JSON = "\n"

slice(-1) takes the last character, not zero characters. So assert.doesNotMatch(viewerPrintStyles, …) at line 91 was testing a one-character string against a multi-line pattern. It could not fail.

It was degenerate from birth. 37b3693 (#359) removed this block from MarkdownViewer.svelte:

@media print {
	.markdown-body {
		height: auto !important;
		overflow: visible !important;
		padding: 0 !important;
	}
}

and added the assertion guarding against its return — in the same commit. Deleting the subject is what made the anchor miss.

What the claim was, and whether it still holds

It holds. src/styles.css sets .markdown-body { padding: 0.75in !important } for print. A component-scoped rule in MarkdownViewer.svelte compiles to .markdown-body.svelte-xxxx, which outranks it on specificity even with both !important. Re-introducing that block would silently drop the page margins from PDF export again.

So the assertion is re-anchored, not deleted: same regex intent, searched over the whole component instead of over a slice that does not exist.

assert.doesNotMatch(viewer, /@media print\s*\{[\s\S]*?\.markdown-body\s*\{[\s\S]*?padding:\s*0\s*!important;/);

Nothing new is asserted — the pattern is the old one with the slice's anchor folded into it.

Falsifiability, checked by re-introducing the regression

The block was re-inserted into MarkdownViewer.svelte two ways and the file restored afterwards:

regression re-introduced old (sliced) form new (whole-file) form
exactly as removed in 37b3693, tab-indented catches it catches it
same rule, space-indented passes — bug undetected catches it

The second row is the point. The old form depended on a literal \t in the anchor; any re-introduction that indented differently, or that lived outside a @media print { line matching that exact prefix, sailed through. And with the marker absent entirely, as today, it could not fail at all.

The sweep

Every .slice() / .substring() / .substr() in scripts/ whose bounds come from .indexOf() / .lastIndexOf() / .search(), found by walking each file's TypeScript AST rather than grepping (several span multiple lines):

72 sites across 23 files.

Degenerate-vs-fragile was then measured rather than eyeballed: String.prototype.slice/substring/substr were wrapped to record any call receiving -1 as a bound, and the full suite was run.

count
degenerate (search string absent today) 1
fragile (present today, unguarded) 71

The one degenerate site is issue261EditorPdf.test.ts:76.slice(-1) against a 149,066-character subject. The only other -1 bounds observed anywhere in the suite were literal slice(0, -1) / slice(1, -1) quote-trimming on 2–4 character strings.

A separate instrumented run confirmed all 72 sites execute during npm test, so "not degenerate" means "the anchor was found", not "never ran".

Helper, not per-site guards

The identical guarded helper was already copied verbatim into two files (findCollapsedMatches.test.ts:7-13, lossyDecodeSaveGuard.test.ts:26-33). It moves into scripts/sourceTree.ts — the module whose own header records that three copies of walk() were collapsed into it for the same reason — and gains the shape it was missing:

  • sliceBetween(source, start, end) — the lifted helper, both duplicates deleted and routed through it (12 call sites).
  • sliceFrom(source, start) — new, for the tail-of-file shape. This is the shape that produced both known instances of this bug, and neither existing copy covered it.

Migrated in this PR: the two de-duplicated files plus every site in issue261EditorPdf.test.ts — 17 anchors, verified individually (below).

The remaining 66 sites in 22 files are reported, not migrated. Justification: they are fragile, not broken; the helper's two shapes do not cover all of them (several search the end anchor from a bespoke offset); and rewriting 66 call sites across 22 files inside a PR that also changes tsconfig.json would make the diff unreviewable. Several of those files are also being deleted on chore/drop-tests-that-only-detect-renames, so migrating them now would conflict for no benefit. The helper is in the shared module and has 3 users on day one, so the next one is a one-line import rather than a fourth copy.

Every migrated anchor, broken and confirmed loud

Each of the 17 anchors was replaced with 'NO SUCH ANCHOR ZZZ' in turn, that test file run, and the file restored:

anchors broken: 17   loud failures: 17   silent passes: 0

Each failed with expected to find "NO SUCH ANCHOR ZZZ" rather than an assertion about the wrong subject.

Not covered

  • 66 fragile slices in 22 files, listed above by count: checkedReadMigration 7, externalChangeReload 8, windowStateRestore 7, truncatedBufferGuard 6, viewModeWithoutSaving 5, reopenDirtyDocument/tabTransferHandoff/windowClosePerTab 4 each, editorPdfExport/settingsPersistence/tabTransfer 3 each, printFindHighlight 2, and 1 each in 10 more. All find their anchors today.
  • scrollSyncInput.test.ts is still half-guarded. It got a top-level assert.notEqual(index, -1) on its start anchor; its end anchor (editor.indexOf('\n\t$effect(() => {', syncStart + 1)) is unguarded, and a miss there would slice to the end of the file rather than to the end of the effect. Left alone — it is one of the 66, and fixing it in isolation would repeat the pattern this section is about.
  • build-test-bundle.mjs is inside the new include glob but has no type errors to report; nothing was done to it.
  • AGENTS.md is untouched. It still says "Frontend: No test framework currently configured", which was already stale before this PR and is reported in AGENTS.md: three stale statements send contributors the wrong way #385. Not this PR's file to correct.
  • The claim that a Svelte-scoped print rule outranks the global one is argued from specificity, not measured in a browser. It is the same reasoning 37b3693 acted on when it removed the block.
  • No Rust touched, so cargo test was not run.

Verification

npm test 562 passed, 0 failed · npm run check 644 FILES 0 ERRORS 0 WARNINGS 0 FILES_WITH_PROBLEMS · npm run build clean · type-error injection caught (644 FILES 1 ERRORS) and removed · 17/17 broken anchors fail loudly · working tree clean, MarkdownViewer.svelte and foldKeys.test.ts restored byte-identical after every experiment.

🤖 Generated with Claude Code

…t fail

`npm run check` runs `svelte-check --tsconfig ./tsconfig.json`, which inherits
`.svelte-kit/tsconfig.json`. SvelteKit generates that file's `include` list from
a fixed set of directories — `src/`, `test/`, `tests/` — and `scripts/` is not
among them. All 89 test files were therefore invisible to the type checker.

Measured: a deliberate `const x: number = "not a number"` in
scripts/foldKeys.test.ts still produced `438 FILES 0 ERRORS`. After this change
the same injection reports `ERROR "scripts/foldKeys.test.ts" 31:7 "Type 'string'
is not assignable to type 'number'."`.

TypeScript does not merge `include` across `extends`, so the root tsconfig now
carries a copy of the generated list plus `scripts/`. `@types/node` is added —
the suite imports `node:test`, `node:fs` and `node:assert/strict`, none of which
were typed. `allowImportingTsExtensions` is enabled because the tests import
their subjects with an explicit `.ts` extension, which is what `tsx` resolves;
the project is `noEmit`, so there is no output path for the extension to be
wrong in.

That surfaced 38 pre-existing errors across 13 files, all fixed rather than
suppressed: two `makeTab()` fixtures declared `Tab` while omitting the required
`collapsedHeaders`; monaco-editor's ESM internals had no declarations and are
now declared with the shapes the test uses (removing an `as any`); a `Proxy`
was cast in and back out for no reason; a `@ts-expect-error` in vite.config.js
became stale once `process` was typed.

Second, unrelated to the above but found by the same reading:
scripts/issue261EditorPdf.test.ts sliced the viewer from `@media print {`, a
marker MarkdownViewer.svelte does not contain. `indexOf` returns -1,
`slice(-1)` yields the file's last character, and the `assert.doesNotMatch`
against it could not fail. 37b3693 removed that print block from the component
and added this assertion in the same commit, so it was degenerate from birth.
The claim is still real — a component-scoped print rule would outrank the
global one — so it is re-anchored on the whole component instead of a slice.

A guarded `slice()` helper already existed, copied verbatim into two test
files. It moves to scripts/sourceTree.ts as `sliceBetween`, joined by
`sliceFrom` for the tail-of-file shape that produced both known instances of
this bug.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@PathGao
PathGao merged commit 4251f0a into master Aug 3, 2026
4 checks passed
@PathGao
PathGao deleted the chore/type-check-the-tests branch August 3, 2026 09:24
PathGao pushed a commit that referenced this pull request Aug 3, 2026
…bounds

Two holes in the test tooling, both demonstrated before they were fixed.

1. `enclosingFunctionName` matched `\n\t…function NAME(` and nothing else, so a
   call inside `const f = async () => {}` was attributed to whichever classic
   `function` preceded it. Planting

       const renderRawBypass = async (raw: string) => {
           return (await invoke('render_markdown', { content: raw })) as string;
       };

   in MarkdownViewer.svelte left all 524 tests green. It is now answered from
   the real AST via `svelte/compiler`, which the suite already depends on, so
   every declaration form in `src/` — 451 classic, 74 arrow, class methods,
   object shorthand — and every form nobody has written yet is covered by
   construction. Same for `$effect` bodies: the regex needed a tab-indented
   `});` to terminate, so a one-line effect merged into its successor and its
   ungated `editor` read was masked by the next effect's `editorReady`.

2. 71 `indexOf`-derived slice and ordering bounds had no `-1` guard, the defect
   #432 fixed in two places. `sliceFrom`/`sliceBetween` cover the slices;
   `offsetOf` is added for the ordering comparisons, where `a < b` is also
   satisfied by `a === -1`. 110 helper call sites replace 155 raw `indexOf`
   bounds; 194 anchors were each corrupted in turn and 193 produced a failure
   naming the missing anchor.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PathGao added a commit that referenced this pull request Aug 3, 2026
…bounds (#440)

Two holes in the test tooling, both demonstrated before they were fixed.

1. `enclosingFunctionName` matched `\n\t…function NAME(` and nothing else, so a
   call inside `const f = async () => {}` was attributed to whichever classic
   `function` preceded it. Planting

       const renderRawBypass = async (raw: string) => {
           return (await invoke('render_markdown', { content: raw })) as string;
       };

   in MarkdownViewer.svelte left all 524 tests green. It is now answered from
   the real AST via `svelte/compiler`, which the suite already depends on, so
   every declaration form in `src/` — 451 classic, 74 arrow, class methods,
   object shorthand — and every form nobody has written yet is covered by
   construction. Same for `$effect` bodies: the regex needed a tab-indented
   `});` to terminate, so a one-line effect merged into its successor and its
   ungated `editor` read was masked by the next effect's `editorReady`.

2. 71 `indexOf`-derived slice and ordering bounds had no `-1` guard, the defect
   #432 fixed in two places. `sliceFrom`/`sliceBetween` cover the slices;
   `offsetOf` is added for the ordering comparisons, where `a < b` is also
   satisfied by `a === -1`. 110 helper call sites replace 155 raw `indexOf`
   bounds; 194 anchors were each corrupted in turn and 193 produced a failure
   naming the missing anchor.

Co-authored-by: PathGao <gaoyanbo@gaoyanbodeMacBook-Air.local>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant