diff --git a/tests/unit/dashboard-boundaries.test.js b/tests/unit/dashboard-boundaries.test.js index a5a940f0..55d61cd8 100644 --- a/tests/unit/dashboard-boundaries.test.js +++ b/tests/unit/dashboard-boundaries.test.js @@ -9,7 +9,7 @@ // typing the node: imports would need @types/node — a deferred decision). import { describe, expect, it } from 'vitest'; -import { readdirSync, readFileSync, writeFileSync, unlinkSync, existsSync, statSync } from 'node:fs'; +import { readdirSync, readFileSync, existsSync, statSync } from 'node:fs'; import { dirname, join, relative, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -40,8 +40,8 @@ function collectFiles(dir) { const SPECIFIER = /\bimport\s+(?:type\s+)?[\w*{}\s,]*\s*from\s*['"]([^'"]+)['"]|\bexport\s+(?:type\s+)?[\w*{}\s,]*\s*from\s*['"]([^'"]+)['"]/g; -function relativeSpecifiers(file) { - const source = readFileSync(file, 'utf8'); +function relativeSpecifiers(file, sourceOverride) { + const source = sourceOverride ?? readFileSync(file, 'utf8'); const specs = []; let match; SPECIFIER.lastIndex = 0; @@ -60,11 +60,20 @@ function resolveSpec(fromFile, spec) { return relative(repoRoot, found).split('\\').join('/'); } -function violations(dir, forbidden = FORBIDDEN) { +/** `virtualFiles` are `[repoRelativePath, source]` pairs checked alongside the + * real ones on disk. They let a sabotage probe exercise this walk without + * writing a file into the working tree (#554 review): the previous probe + * created and deleted a real `src/core/*.ts`, which a crash or a kill between + * write and `unlink` would have left behind, and which file watchers see. */ +function violations(dir, forbidden = FORBIDDEN, virtualFiles = []) { const abs = join(repoRoot, dir); const found = []; - for (const file of collectFiles(abs)) { - for (const spec of relativeSpecifiers(file)) { + const entries = [ + ...collectFiles(abs).map((file) => [file, undefined]), + ...virtualFiles.map(([rel, source]) => [join(repoRoot, rel), source]), + ]; + for (const [file, source] of entries) { + for (const spec of relativeSpecifiers(file, source)) { const resolved = resolveSpec(file, spec); const hit = forbidden.find((f) => resolved === f || resolved.startsWith(`${f}/`)); if (hit) found.push(`${relative(repoRoot, file)} → ${spec} (${hit})`); @@ -108,18 +117,32 @@ describe('dashboard dependency boundaries', () => { // Sabotage check: the check above only proves the CURRENT tree is clean — // it says nothing about whether the detector would actually catch a - // regression. Plant a real, throwaway file with a forbidden import inside - // src/core, confirm the same detection logic flags it, then remove it - // regardless of outcome so the source tree is left exactly as found. + // regression. Feed the same walk a virtual `src/core` file with a forbidden + // import and confirm it is flagged. Virtual, not written to disk, so the + // working tree is never touched even if this process dies mid-test. it('flags a src/core import that reaches into src/workspace (proves the rule above is not vacuous)', () => { - const probe = join(repoRoot, 'src/core/__boundary_probe_455__.ts'); - writeFileSync(probe, "import { nothing } from '../workspace/does-not-exist.js';\nexport const probe = nothing;\n"); - try { - const found = violations('src/core', FORBIDDEN_CORE); - expect(found.some((line) => line.includes('__boundary_probe_455__') && line.includes('src/workspace'))).toBe(true); - } finally { - unlinkSync(probe); - } + const found = violations('src/core', FORBIDDEN_CORE, [ + ['src/core/__boundary_probe_455__.ts', "import { nothing } from '../workspace/does-not-exist.js';\n"], + ]); + expect(found.some((line) => line.includes('__boundary_probe_455__') && line.includes('src/workspace'))).toBe(true); + }); + + // ...and the walk above is only a MIRROR of the real gate, which lives in + // `build/check-boundaries.mjs`. A mirror can drift from what it mirrors: with + // the `src/core` entry deleted from the production `RULES`, this entire spec + // stayed green (60/60) and `check:arch` happily reported "OK … 8 active + // rules" — #455's whole deliverable was removable without a single failure + // (#554 review). Bind the two so that cannot happen silently. + // + // Read as TEXT rather than imported on purpose: `check-boundaries.mjs` runs + // its entire check at module top level and exits non-zero on violations, so + // importing it here would run the production gate inside the test process. + it('build/check-boundaries.mjs still declares the src/core rule this spec mirrors (#455)', () => { + const checkerSource = readFileSync(join(repoRoot, 'build/check-boundaries.mjs'), 'utf8'); + const entry = checkerSource.match(/\{\s*dir:\s*'src\/core',\s*forbidden:\s*\[([^\]]*)\]/); + expect(entry, "build/check-boundaries.mjs has no `dir: 'src/core'` rule — #455 regressed").not.toBeNull(); + const forbidden = [...entry[1].matchAll(/'([^']+)'/g)].map((m) => m[1]); + expect(forbidden.slice().sort()).toEqual(FORBIDDEN_CORE.slice().sort()); }); it('does not restore the retired saved-query repair planner or its vocabulary', () => { diff --git a/tests/unit/typography-contract.test.js b/tests/unit/typography-contract.test.js index fb47249d..d67162c1 100644 --- a/tests/unit/typography-contract.test.js +++ b/tests/unit/typography-contract.test.js @@ -42,6 +42,20 @@ const declarations = css.replace(/\/\*[\s\S]*?\*\//g, ''); /** Everything after the token block: the rules that must consume tokens. */ const rules = declarations.slice(declarations.indexOf('*, *::before')); +/** Every class name that has a real SELECTOR somewhere in the stylesheet. + * + * Built from `declarations`, never from raw `css`, for exactly the reason the + * comment above gives about px values: this file is heavily commented, and + * those comments cross-reference sibling rules by name (e.g. `.dash-tree-confirm`'s + * prose points at "`.qtab-close-confirm*` and `.dash-tile-confirm*`"). Scanning + * raw CSS counted that prose as styling, so a class whose real rule was deleted + * still looked styled as long as ANY comment mentioned it — the whole + * "no class the UI renders may be left with no CSS rule" gate silently passed. + * Demonstrated on `.qtab-close-confirm`: deleting both of its real rules while + * leaving the comment kept all 338 assertions in this file green. */ +const styledClassNames = (source = declarations) => + new Set([...source.matchAll(/\.([a-zA-Z][\w-]*)/g)].map((m) => m[1])); + const tokenValues = (prefix) => { const out = {}; for (const m of rootBlock.matchAll(new RegExp(`(--${prefix}-[\\w-]+):\\s*([^;]+);`, 'g'))) { @@ -327,7 +341,7 @@ describe('every text-bearing class the UI renders has a rule', () => { 'src/ui/library-assign-menu.ts', ].map((f) => readFileSync(resolve(root, f), 'utf8')).join('\n'); - const styled = new Set([...css.matchAll(/\.([a-zA-Z][\w-]*)/g)].map((m) => m[1])); + const styled = styledClassNames(); const unstyled = [...sources.matchAll(/class:\s*'([^']+)'/g)] .map((m) => m[1].split(/\s+/).filter(Boolean)) // A group is fine if ANY class in it is styled — modifier hooks like @@ -341,9 +355,20 @@ describe('every text-bearing class the UI renders has a rule', () => { // shortcut-section :
wrapper; its children are styled // docs-field* : wrapper; .docs-field-label/-text are styled // kpi-value-number : span inside the styled .kpi-value + // dash-row : flow@1 row wrapper; grid set INLINE (below) // Anything NOT on this list renders in user-agent chrome. + // + // `dash-row` (#554 review): surfaced the moment this scan stopped counting + // comment prose as styling — it had no rule and never has, and was only + // ever "covered" by the two `src/styles.css` comments that name it. That is + // correct by design, not an oversight to paper over: `.dash-grid`'s comment + // spells out that each row "is itself the grid that owns its column count … + // set inline by the flow renderer", and that the container must NOT impose + // `grid-template-columns` or the rows lay out side by side. A `.dash-row` + // rule would either duplicate the inline style or reintroduce that bug, so + // this is a genuine inline-presentation hook. const ALLOWED = new Set([ - 'surface-label', 'shortcut-section', 'exp-label', 'kpi-value-number', + 'surface-label', 'shortcut-section', 'exp-label', 'kpi-value-number', 'dash-row', 'docs-field', 'docs-field docs-syntax', 'docs-field docs-facts', 'docs-field docs-related', ]); expect([...new Set(unstyled)].filter((g) => !ALLOWED.has(g))).toEqual([]); @@ -399,7 +424,7 @@ describe('openMenu extension classes have CSS rules', () => { return groups; } - const styledClasses = new Set([...css.matchAll(/\.([a-zA-Z][\w-]*)/g)].map((m) => m[1])); + const styledClasses = styledClassNames(); it('every literal menuClass/extraClass value (incl. openConfirmMenu goClass/cancelClass) has a stylesheet rule', () => { const groups = literalClassGroups(uiSource); @@ -423,13 +448,46 @@ describe('openMenu extension classes have CSS rules', () => { // real, single-purpose, and currently styled: a safe target to remove. const target = 'dash-tile-menu-danger'; expect(found.has(target)).toBe(true); - const sabotagedCss = css.replace(/\.dash-tile-menu-danger\s*\{[^}]*\}\n?/, ''); - expect(sabotagedCss).not.toEqual(css); // the removal must actually have taken effect - const sabotagedClasses = new Set([...sabotagedCss.matchAll(/\.([a-zA-Z][\w-]*)/g)].map((m) => m[1])); - const missing = [...found].filter((cls) => !sabotagedClasses.has(cls)); + const sabotagedCss = declarations.replace(/\.dash-tile-menu-danger\s*\{[^}]*\}\n?/, ''); + expect(sabotagedCss).not.toEqual(declarations); // the removal must actually have taken effect + const missing = [...found].filter((cls) => !styledClassNames(sabotagedCss).has(cls)); expect(missing).toEqual([target]); }); + // The regression this gate has to survive, and did NOT before (#554 review). + // The sabotage above picks a class no comment happens to name, so it passed + // while the general contract leaked: every confirm menu's prose + // cross-references its siblings by name, so `.dash-tree-confirm`'s comment + // mentions "`.qtab-close-confirm*` and `.dash-tile-confirm*`" and vice versa. + // Scanning raw CSS let that prose stand in for a rule. Verified against the + // real stylesheet: deleting BOTH real `.qtab-close-confirm` rules while + // leaving the comment kept all 338 assertions in this file green. + it('sabotage check: a comment naming a class does not stand in for its rule', () => { + const target = 'qtab-close-confirm'; + expect(literalClassGroups(uiSource).flat()).toContain(target); + // The masking is real in the shipped stylesheet, not hypothetical: the + // sibling confirm menu's prose names this class. + expect(css).toMatch(new RegExp(`\\.${target}\\*`)); + + // Build the bait deterministically rather than regex-surgering the real + // file: strip the class's rules from the COMMENT-FREE text (so no prose can + // confuse the match), then prepend a comment that names it. This is exactly + // the shape `src/styles.css` has today. + const withoutRules = declarations.replace( + new RegExp(`\\.${target}(?![\\w-])[^{}]*\\{[^}]*\\}\\n?`, 'g'), '', + ); + expect(withoutRules).not.toEqual(declarations); + const baited = `/* see \`.${target}*\` for the sibling rule */\n${withoutRules}`; + + // The OLD raw-CSS scan was fooled by precisely that comment — it still + // called the class styled. Pinned so the regression cannot come back + // unnoticed. + expect(new Set([...baited.matchAll(/\.([a-zA-Z][\w-]*)/g)].map((m) => m[1])).has(target)) + .toBe(true); + // The comment-stripped scan this file now uses is not fooled. + expect(styledClassNames(baited.replace(/\/\*[\s\S]*?\*\//g, '')).has(target)).toBe(false); + }); + // Existing generic scanner keeps covering these directly, unaffected by the // menuClass/extraClass extraction above (#498 acceptance: they stay covered). it('still covers .file-menu / .fm-item / .fm-section directly (unaffected by this test)', () => {