From d2f78fed4be99004900ef1abff5ae299b64c563e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 21:34:17 +0800 Subject: [PATCH] test(guards): re-anchor nine assertions that could not fail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A survey instrumented all 82 test files and applied 35 source mutations, one full suite run each. Nine of the assertions it flagged as unable to fail sit on real guards; those nine are fixed here. Sixteen of the eighteen affected test files did not exist at the last release — this is debt the recent PR series produced, not debt it inherited. The failure mode is not the one #440 fixed. #440 fixed anchors that were *missing*, so a slice degenerated to "". These are anchors that are too far apart: the slice swallows a neighbouring function and something else in it satisfies the match. Six had a mutation that left the suite green, reproduced here before being fixed and re-applied afterwards: * canTransfer's "&& !tab.isTruncated" and handleDetach's whole ensureFullContent guard, each deletable — the transfer-a-truncated- buffer-then-auto-save-truncates-the-file path truncatedBufferGuard is named for. The two slices ran through canDetach and moveTabToWindow, whose character-identical guards satisfied the match. * appExit's unsaved-tab review, movable verbatim into a helper nobody calls: the slice spanned 969 lines for a 16-line function. * previewSanitize's "all {@html} sinks" list, which captured bare identifiers only, so a second raw sink spelled as a member expression was invisible. * the Destroyed handler's window_registry removal, deletable because the two tokens matched ~460 lines apart. * the recent-files storage key and cap, each compared with itself through an export that existed for that comparison alone. * every context-menu label in 26 languages, restated 26 times: t() falls back to English, so deleting the German entry was green. Two more could not fail by construction and are deleted rather than repaired: an assertion on a template literal declared in the same file, and a count of a pattern the line above had already proved absent. A third of that kind, the config's URI regexp compared with the constant it is assigned from, turned out to be reachable — rebuilding the regexp from the same source and flags fails it, which is the hand-copied-pattern regression the file exists for — so it stays. New: sourceTree.ts gains functionSource(text, name), which extracts a function from the AST by its own name instead of by naming whatever text follows it, and fails loudly on a rename or a duplicate. Four guards that were only ever stood in front of — canDetach, moveTabToWindow, and each of the two independent recent-list caps — are now asserted about directly, and appExit's confirmation must be acted on rather than merely present. Source changes are limited to dropping "export" from RECENT_FILES_KEY and RECENT_FILES_LIMIT, whose only importer was the test that compared them with themselves. No behaviour changes. Co-Authored-By: Claude Opus 5 --- scripts/editorContextMenuI18n.test.ts | 58 +++++++++++++++++++----- scripts/previewSanitize.test.ts | 55 +++++++++++++++++++---- scripts/recentFilesMultiWindow.test.ts | 62 +++++++++++++++++++++----- scripts/sourceTree.test.ts | 41 ++++++++++++++++- scripts/sourceTree.ts | 28 ++++++++++++ scripts/truncatedBufferGuard.test.ts | 33 +++++++++++--- scripts/viewModeWithoutSaving.test.ts | 13 +++++- scripts/windowOrganization.test.ts | 18 +++++++- src/lib/utils/recentFiles.ts | 9 +++- 9 files changed, 273 insertions(+), 44 deletions(-) diff --git a/scripts/editorContextMenuI18n.test.ts b/scripts/editorContextMenuI18n.test.ts index 2c38f3f..cdb78b1 100644 --- a/scripts/editorContextMenuI18n.test.ts +++ b/scripts/editorContextMenuI18n.test.ts @@ -2,7 +2,7 @@ import assert from 'node:assert/strict'; import { readFileSync } from 'node:fs'; import test from 'node:test'; -import { getSupportedLanguages, t, type LanguageCode } from '../src/lib/utils/i18n.js'; +import { getSupportedLanguages, t, translations, type LanguageCode, type Translation } from '../src/lib/utils/i18n.js'; // WHAT THIS FILE COVERS, AND WHAT IT DOES NOT // @@ -24,6 +24,33 @@ function count(source: string, pattern: RegExp): number { return source.match(pattern)?.length ?? 0; } +/** + * Does `lang`'s own dictionary define `key`, without the English fallback? + * + * `t()` falls back to English, so `assert.notEqual(t(key, lang), key)` can only + * fail when ENGLISH lacks the key — which each caller below already asserts one + * line earlier. The 26-language loop around it restated that 26 times and could + * not see the regression it was written for: deleting the German + * `menu.inlineCode` entry left the whole suite green while the German context + * menu rendered the English label. + * + * These labels are held to a stricter bar than the dictionary at large. + * i18nCoverage.test.ts reports per-locale gaps rather than failing on them, + * because 19 locales are missing >100 keys and nobody should have to translate + * a new English string 25 times before landing it. That trade does not apply + * here: this is a fixed list of a dozen context-menu entries, all 26 languages + * define all of them today, and adding an eleventh action means adding a row to + * FORMATTING_ACTIONS by hand — so the cost is visible where it is incurred. + */ +function defines(lang: LanguageCode, key: string): boolean { + let node: string | Translation | undefined = translations[lang]; + for (const part of key.split('.')) { + if (typeof node !== 'object' || node === null || !(part in node)) return false; + node = node[part]; + } + return typeof node === 'string'; +} + // The markdown formatting entries that used to be English string literals. const FORMATTING_ACTIONS: ReadonlyArray<[actionId: string, key: string, englishLabel: string]> = [ ['fmt-inline-code', 'menu.inlineCode', 'Inline Code'], @@ -60,13 +87,16 @@ test('every context-menu label is translated, none is an English literal', () => ); }); -test('the formatting labels exist in English and resolve in every language', () => { +test('the formatting labels exist in English and are translated in every language', () => { for (const [, key, englishLabel] of FORMATTING_ACTIONS) { assert.equal(t(key, 'en'), englishLabel, `${key} is defined for English`); for (const lang of supported) { - const label = t(key, lang as LanguageCode); - assert.notEqual(label, key, `${key} resolves for ${lang} instead of echoing the key`); - assert.ok(label.length > 0, `${key} is non-empty for ${lang}`); + // `defines`, not `t(...) !== key` — see the note on the helper. + assert.ok( + defines(lang as LanguageCode, key), + `${key} is translated for ${lang}; it currently falls back to the English label`, + ); + assert.ok(t(key, lang as LanguageCode).length > 0, `${key} is non-empty for ${lang}`); } } }); @@ -92,7 +122,11 @@ test('toggle-occurrences-highlight has its own label, not Show Whitespace', () = for (const lang of supported) { const occurrences = t('settings.occurrencesHighlight', lang as LanguageCode); const whitespace = t('settings.showWhitespace', lang as LanguageCode); - assert.notEqual(occurrences, 'settings.occurrencesHighlight', `defined for ${lang}`); + // Same substitution as above: `occurrences !== 'settings.occurrencesHighlight'` + // only ever failed if English lacked the key, which the assertions at the + // top of this test already cover. + assert.ok(defines(lang as LanguageCode, 'settings.occurrencesHighlight'), `translated for ${lang}`); + assert.ok(defines(lang as LanguageCode, 'settings.showWhitespace'), `translated for ${lang}`); assert.notEqual( occurrences, whitespace, @@ -156,13 +190,13 @@ test('actions are re-registered when the UI language changes', () => { ); assert.match(editor, /onDestroy\(disposeLocalizedActions\)/, 'teardown releases them too'); - // The old design read a `uiLanguage` snapshot captured inside onMount. + // The old design read a `uiLanguage` snapshot captured inside onMount. The + // narrower "no label is still bound to the snapshot" count that used to + // follow this line is gone: it counted occurrences of a pattern containing + // `uiLanguage`, so the line above has to pass for it to be reached and, + // once it has, the count is zero by construction. There is no state of the + // component in which it, and not the line above, is the failure. assert.doesNotMatch(editor, /uiLanguage/, 'no captured language snapshot remains'); - assert.equal( - count(editor, /label: t\('[^']+', uiLanguage\)/g), - 0, - 'no label is still bound to the snapshot', - ); }); test('no action id is registered more than once', () => { diff --git a/scripts/previewSanitize.test.ts b/scripts/previewSanitize.test.ts index b21ced0..9e9866d 100644 --- a/scripts/previewSanitize.test.ts +++ b/scripts/previewSanitize.test.ts @@ -3,7 +3,7 @@ import { readFileSync } from 'node:fs'; import test from 'node:test'; import { MARKDOWN_SANITIZE_CONFIG, ALLOWED_MARKDOWN_URI_REGEXP } from '../src/lib/utils/sanitize.js'; -import { SANITIZER_FILES, callSiteOffsets, enclosingFunctionName, filesMatching, readSourceFiles } from './sourceTree.js'; +import { SANITIZER_FILES, callSiteOffsets, enclosingFunctionName, filesMatching, readSourceFiles, sliceBetween } from './sourceTree.js'; // The preview is the path the ` +// +// It used to be a `POC_STYLE` constant with an `assert.match(POC_STYLE, +// /^'; const SOURCES = readSourceFiles('src'); const viewerSource = readFileSync('src/lib/MarkdownViewer.svelte', 'utf8'); @@ -62,12 +71,37 @@ test('the preview sanitizes through the shared policy, not a local config', () = 'the viewer must import the shared sanitizer', ); - // The sink itself: every bare `{@html ident}` in the viewer injects the - // sanitizer's output and nothing else. Stated over *all* of them rather than - // over one known-good spelling, so adding a second injection point of the raw - // document is a failure instead of an unnoticed addition. - const injected = [...new Set([...viewerSource.matchAll(/\{@html\s+([A-Za-z_$][\w$]*)\s*\}/g)].map((m) => m[1]))]; - assert.deepEqual(injected, [sanitizedSinkName()], 'the preview sink must inject the shared sanitizer output'); + // The sinks: every `{@html …}` in the viewer, whatever the expression looks + // like. Stated over *all* of them rather than over one known-good spelling, + // so adding an injection point of the raw document is a failure instead of + // an unnoticed addition. + // + // The capture used to be `([A-Za-z_$][\w$]*)`, bare identifiers only, which + // made the claim unfalsifiable in exactly the direction it is about. The + // list it compared was `[sanitizedSinkName()]` by construction: the second + // sink already in the file, `{@html tooltip.html}`, was invisible to it, and + // a planted `{@html unsafe.rawHtml}` beside the sanitized one — the raw + // rendered document, injected — left the whole suite green. + // + // So the set is an allowlist now, and `tooltip.html` is on it with a reason + // rather than by accident: the footnote tooltip is a clone of a subtree of + // `markdownBody`, i.e. of the DOM the sanitized sink already injected. It is + // not a second policy, it is the same bytes read back out of the document — + // which is what the next two assertions pin. + const injected = [...new Set([...viewerSource.matchAll(/\{@html\s+([^}]+?)\s*\}/g)].map((m) => m[1]))].sort(); + assert.deepEqual( + injected, + [sanitizedSinkName(), 'tooltip.html'].sort(), + 'unexpected {@html} sink — what the preview injects is the shared sanitizer output', + ); + + // Why the footnote sink is allowed, pinned as a direction rather than as a + // spelling: the tooltip body is read out of the rendered document, never + // built from a string the sanitizer has not seen. + const footnote = sliceBetween(viewerSource, "anchor.hasAttribute('data-footnote-ref')", 'isFootnote: true'); + assert.match(footnote, /markdownBody\?\.querySelector/, 'the footnote body is found in the rendered document'); + assert.match(footnote, /\.innerHTML/, 'and taken from the DOM the shared sanitizer already filtered'); + assert.doesNotMatch(footnote, /rawContent/, 'never from the unrendered buffer'); // The regression itself — a DOMPurify call with a config assembled at the // call site — is caught for the whole tree by the call-site allowlist below; @@ -88,8 +122,11 @@ test('the shared policy the preview now gets is the one that forbids author styl assert.match(sanitizeSource, /return DOMPurify\.sanitize\(html, MARKDOWN_SANITIZE_CONFIG\)/); assert.deepEqual(Object.keys(MARKDOWN_SANITIZE_CONFIG).sort(), ['ALLOWED_URI_REGEXP', 'FORBID_TAGS']); assert.deepEqual(MARKDOWN_SANITIZE_CONFIG.FORBID_TAGS, ['style']); + // Identity, not equality: the regression this file exists for is a *copy* of + // the URI pattern, and a copy that happens to be spelled the same today is + // the thing that drifts tomorrow. Measured — rebuilding the config's regexp + // from the exported one's source and flags fails here. assert.equal(MARKDOWN_SANITIZE_CONFIG.ALLOWED_URI_REGEXP, ALLOWED_MARKDOWN_URI_REGEXP); - assert.match(POC_STYLE, /^