From 605beb0b372ed573e067b449cad956967c571d52 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 11:38:10 +0000 Subject: [PATCH] fix(cli-gate): name it.skip / it.only / it.each blocks instead of collapsing them onto (top-level) callbackSiteName() required a bare-identifier callee, so `it('x', fn)` was named but `it.skip('x', fn)` (a property access) and `it.each(table)('x', fn)` (a call) returned null. enclosingFunctionName() then fell out of its loop to '(top-level)', collapsing every skipped/only/table-driven block in one file onto a single site key -- the same collision #12531 closed for the identifier branch, one modifier over. The unwrap is TARGETED, gated on two rosters rather than on node kind: the roots it/test/describe, and the vitest modifiers that keep a chain a test block. A blanket "property access whose base is an identifier" rule matches 209 call sites across 87 distinct callees under packages/cli/test (literals.filter, child.on, run.then, rows.map, vi.fn, ...) against this roster's 22, and naming those would MOVE the key of every site nested inside one. The modifier roster is also what refuses it.name / it.type / it.next -- ordinary loop variables in this directory that happen to be spelled `it`. The identifier branch is byte-identical, which DELIBERATE_REROUTE's two it("...") keys pin in both directions: a green live-tree run is the proof that no existing key moved. Self-test 132 -> 152 cases. One pre-existing case changed: the one that pinned it.skip AS (top-level), retargeted to the name it now produces; what its comment was protecting (the refusal of promise.then / rows.map) is now pinned outright. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd --- scripts/check-cli-test-child-env.mjs | 223 +++++++++++++++++++++++++-- 1 file changed, 210 insertions(+), 13 deletions(-) diff --git a/scripts/check-cli-test-child-env.mjs b/scripts/check-cli-test-child-env.mjs index 044e59d5bf..2210188b92 100644 --- a/scripts/check-cli-test-child-env.mjs +++ b/scripts/check-cli-test-child-env.mjs @@ -510,6 +510,88 @@ function isBulkUse(node) { return true; } +/** + * The three vitest block roots whose modifier chains name a real test block. + * + * ⛔ A ROSTER rather than "any identifier", and the population is why: this + * directory calls `it.name`, `it.type`, `it.next` and `it.prevVersion` on + * ordinary loop variables that happen to be spelled `it`. Gating on the root + * alone would name a callback site after one of those. + */ +const TEST_BLOCK_ROOTS = new Set(['it', 'test', 'describe']); + +/** + * The vitest modifiers that keep a chain a TEST BLOCK, so `it.skip(...)` names + * a site and `rows.map(...)` does not. + * + * ⛔ Also a roster, and this is the half that does the refusing. Measured under + * {@link POPULATION_ROOT}: a blanket "property access whose base is an + * identifier" rule matches **209 call sites across 87 distinct callees** here + * -- `literals.filter`, `child.on`, `entries.map`, `run.then`, `rows.map`, + * `vi.fn`, `Array.from` -- against the **22** this roster matches (`it.each` + * x20, `describe.each` x2, the whole modifier population of this directory). + * Those 87 are not test blocks, their callee is a WORSE name than the enclosing + * block the walk reaches today, and naming them would MOVE the key of every + * site nested inside one. Re-keying is a data migration wearing a bug fix's + * clothes; the roster is what keeps this a rename of nothing. + * + * `each`/`for` and `skipIf`/`runIf` are the curried forms -- `it.each(table)( + * 'title', fn)` -- and are unwrapped one call deeper by {@link calleeSiteName}. + * + * ⛔ `extend` is deliberately absent: `test.extend(fixtures)` builds a NEW test + * function rather than a block, and that product is bound to a name the + * identifier branch already reads. + */ +const TEST_BLOCK_MODIFIERS = new Set([ + 'skip', 'only', 'todo', 'fails', // verdict + 'concurrent', 'sequential', 'shuffle', // scheduling + 'each', 'for', // table (curried) + 'skipIf', 'runIf', // conditional (curried) +]); + +/** + * The dotted name of a test-block chain -- `it.skip`, `describe.each`, + * `it.skip.each` -- or `null` for anything that is not one. + * + * Every segment after the root must be on {@link TEST_BLOCK_MODIFIERS}, so a + * chain stays a block however deep it nests, and `a.b.c(...)` never becomes a + * site name. + */ +function testBlockChainName(expr) { + const modifiers = []; + let cursor = expr; + while (ts.isPropertyAccessExpression(cursor)) { + if (!ts.isIdentifier(cursor.name)) return null; + modifiers.unshift(cursor.name.text); + cursor = cursor.expression; + } + // A bare identifier is the pre-existing branch, not this one. + if (modifiers.length === 0 || !ts.isIdentifier(cursor)) return null; + if (!TEST_BLOCK_ROOTS.has(cursor.text)) return null; + if (!modifiers.every((part) => TEST_BLOCK_MODIFIERS.has(part))) return null; + return [cursor.text, ...modifiers].join('.'); +} + +/** + * The callee half of a site name, or `null` when the call does not name a site. + * + * ⚠️ The identifier branch is FIRST and byte-identical to what it always was. + * That is load-bearing: {@link DELIBERATE_REROUTE} holds keys this function + * produced for plain `it("...")` blocks, and both registries are pinned in both + * directions, so a green live-tree run is the proof that no existing key moved. + */ +function calleeSiteName(expr) { + if (ts.isIdentifier(expr)) return expr.text; + const chain = testBlockChainName(expr); + if (chain) return chain; + // The curried table/conditional forms: `it.each(table)('title', fn)` is a + // call whose CALLEE is itself a call of a test-block chain. Only one level -- + // vitest defines no deeper currying, and unbounded unwrapping would start + // guessing again. + if (ts.isCallExpression(expr)) return testBlockChainName(expr.expression); + return null; +} + /** * The name a call-argument arrow takes from the call it is passed to: * `it("boots the child")`, or `beforeAll` when the call carries no title. @@ -517,10 +599,15 @@ function isBulkUse(node) { * DOUBLE quotes around the title, deliberately: the product is a * {@link DELIBERATE} key, and this file's string literals are single-quoted, so * a key pasted into that registry needs no escaping. + * + * The title is read from the OUTER call's arguments, which is what makes the + * curried form come out right: `it.each(table)('boots', fn)` is named for + * `'boots'`, never for a string that happens to sit in `table`. */ function callbackSiteName(owner) { - if (!owner || !ts.isCallExpression(owner) || !ts.isIdentifier(owner.expression)) return null; - const callee = owner.expression.text; + if (!owner || !ts.isCallExpression(owner)) return null; + const callee = calleeSiteName(owner.expression); + if (!callee) return null; const title = owner.arguments.find((arg) => ts.isStringLiteralLike(arg)); return title ? `${callee}("${title.text.replace(/\s+/g, ' ').trim()}")` : callee; } @@ -560,12 +647,15 @@ function callbackSiteName(owner) { * * ⚠️ Three limits, named here so they are pinned rather than discovered: * - * 1. The callee must be an IDENTIFIER. `it.skip('...')` and - * `it.each(table)('...')` call through a property access or through - * another call, and still walk past to `(top-level)`. That is a choice: - * widening to a property-access callee would also capture - * `promise.then(() => ...)` and `rows.map(() => ...)`, whose callee is a - * WORSE site name than the enclosing test block the walk reaches today. + * 1. The callee must be an identifier, or a ROSTERED vitest block chain. + * `it.skip('...')`, `describe.each(table)('...')` and `it.skip.each(...)` + * are named (#12545); `promise.then(() => ...)`, `rows.map(() => ...)` and + * every other `foo.bar(cb)` are still walked past to the enclosing block, + * because their callee is a WORSE site name than the block above them. + * ⛔ The refusal is the point, not a leftover: see + * {@link TEST_BLOCK_MODIFIERS} for the 209-vs-22 measurement that decides + * it. A blanket property-access widening would RE-KEY existing sites, + * which is a data migration disguised as a bug fix. * 2. The title's whitespace is collapsed so the name stays one line, but it * is ⛔ never truncated -- two long titles sharing a prefix would collide * again, which is the defect being closed. @@ -1752,12 +1842,119 @@ export function selfTest() { siteNames('site-title-wrap', { 'a.e2e.test.ts': suite(`it(\`boots\nthe child\`, () => {\n${BULK}\n});`) }) === names('it("boots the child")')); - // The named limit, pinned rather than discovered: a property-access callee - // is deliberately NOT captured, because `promise.then(...)` and `rows.map( - // ...)` would be worse site names than the test block the walk reaches. - t('an it.skip() callee is a property access, not an identifier, so it still reads (top-level)', - siteNames('site-property-callee', { 'a.e2e.test.ts': suite(`it.skip('x', () => {\n${BULK}\n});`) }) + // -- (8c) THE MODIFIER FAMILY (#12545): a rostered vitest chain names a + // site; every other property-access callee still does not. + // + // Until this landed, the callee had to be a bare IDENTIFIER, so + // `it.skip(...)` (property access) and `it.each(table)(...)` (a call) + // walked past to `(top-level)` and every skipped/table-driven block in + // one file collided on ONE key -- the same defect #12531 closed for the + // identifier branch, one modifier over. The case below that used to pin + // `it.skip` AS `(top-level)` is retargeted to the name it now produces; + // what that case's comment was really protecting -- the refusal of + // `promise.then` and `rows.map` -- is pinned outright further down, + // where it is a measurement rather than a side effect. + + t('an it.skip() block is named, not (top-level)', + siteNames('site-mod-skip', { 'a.e2e.test.ts': suite(`it.skip('x', () => {\n${BULK}\n});`) }) + === names('it.skip("x")')); + t('an it.only() block is named', + siteNames('site-mod-only', { 'a.e2e.test.ts': suite(`it.only('x', () => {\n${BULK}\n});`) }) + === names('it.only("x")')); + t('a describe.skip() block is named -- the root roster is not it-only', + siteNames('site-mod-desc-skip', { 'a.e2e.test.ts': suite(`describe.skip('x', () => {\n${BULK}\n});`) }) + === names('describe.skip("x")')); + t('a test.concurrent() block is named -- test is a root too', + siteNames('site-mod-test-conc', { 'a.e2e.test.ts': suite(`test.concurrent('x', () => {\n${BULK}\n});`) }) + === names('test.concurrent("x")')); + t('a modifier with NO string title falls back to the chain alone', + siteNames('site-mod-untitled', { 'a.e2e.test.ts': suite(`it.skip(() => {\n${BULK}\n});`) }) + === names('it.skip')); + + // The CURRIED forms: the callee is itself a call. The title must come from + // the OUTER call, never from the table. + t('a curried it.each(table)() block is named after the OUTER title', + siteNames('site-mod-each', { 'a.e2e.test.ts': suite(`it.each([1])('x', () => {\n${BULK}\n});`) }) + === names('it.each("x")')); + t('a curried describe.each(table)() block is named', + siteNames('site-mod-desc-each', { 'a.e2e.test.ts': suite(`describe.each([1])('x', () => {\n${BULK}\n});`) }) + === names('describe.each("x")')); + t('...and a string INSIDE the table never becomes the title', + siteNames('site-mod-each-table-string', { + 'a.e2e.test.ts': suite(`it.each(['from the table'])('from the block', () => {\n${BULK}\n});`), + }) === names('it.each("from the block")')); + t('a curried it.skipIf(cond)() block is named -- the conditional form curries too', + siteNames('site-mod-skipif', { 'a.e2e.test.ts': suite(`it.skipIf(flag)('x', () => {\n${BULK}\n});`) }) + === names('it.skipIf("x")')); + t('a CHAINED it.skip.each(table)() block is named through every segment', + siteNames('site-mod-chain', { 'a.e2e.test.ts': suite(`it.skip.each([1])('x', () => {\n${BULK}\n});`) }) + === names('it.skip.each("x")')); + + // ⭐ THE case for this change, the modifier twin of `site-siblings`: two + // sibling it.each blocks in ONE file must not share a key either. + t('two it.each() blocks in one file get TWO DISTINCT names', + siteNames('site-mod-siblings', { + 'a.e2e.test.ts': suite( + `it.each([1])('first', () => {\n${BULK}\n});\nit.each([2])('second', () => {\n${BULK}\n});`), + }) === names('it.each("first")', 'it.each("second")')); + + // ⭐ THE NEGATIVE PIN. Without it, nothing in this suite distinguishes + // "the modifiers now work" from "everything got renamed". The plain + // identifier branch must be byte-identical to what it always was, + // because DELIBERATE_REROUTE holds keys it produced. + t('⭐ a plain it() block STILL yields exactly it("x") -- the identifier branch did not move', + siteNames('site-mod-plain-unmoved', { 'a.e2e.test.ts': suite(`it('x', () => {\n${BULK}\n});`) }) + === names('it("x")')); + t('...and a plain describe() > it() nesting is unmoved too', + siteNames('site-mod-plain-nested', { + 'a.e2e.test.ts': suite(`describe('outer', () => {\n it('inner', () => {\n${BULK}\n });\n});`), + }) === names('it("inner")')); + + // ⛔ THE REFUSALS -- the half that keeps this a targeted unwrap. A blanket + // property-access widening matches 209 call sites across 87 callees in + // the live population against this roster's 22, and would RE-KEY every + // site nested inside one of them. + t('⛔ promise.then(cb) is NOT a site name -- the walk still reaches past it', + siteNames('site-refuse-then', { 'a.e2e.test.ts': suite(`p.then(() => {\n${BULK}\n});`) }) + === names('(top-level)')); + t('⛔ rows.map(cb) is NOT a site name', + siteNames('site-refuse-map', { 'a.e2e.test.ts': suite(`rows.map(() => {\n${BULK}\n});`) }) === names('(top-level)')); + t('⛔ an arbitrary a.b.c(cb) chain is NOT a site name', + siteNames('site-refuse-chain', { 'a.e2e.test.ts': suite(`a.b.c('x', () => {\n${BULK}\n});`) }) + === names('(top-level)')); + t('⛔ a NON-rostered member of a real root is refused -- it.name(cb) is a loop variable, not a block', + siteNames('site-refuse-nonroster', { 'a.e2e.test.ts': suite(`it.name('x', () => {\n${BULK}\n});`) }) + === names('(top-level)')); + t('⛔ a rostered MODIFIER on a non-rostered root is refused -- only it/test/describe are roots', + siteNames('site-refuse-nonroot', { 'a.e2e.test.ts': suite(`suite.each([1])('x', () => {\n${BULK}\n});`) }) + === names('(top-level)')); + t('⛔ a curried call whose inner callee is a bare identifier is refused', + siteNames('site-refuse-curried-ident', { 'a.e2e.test.ts': suite(`makeSuite('t')('x', () => {\n${BULK}\n});`) }) + === names('(top-level)')); + + // The nearest NAMED binding still wins over a modifier block, exactly as it + // does over a plain one -- limit 3 of the walk, unchanged by this. + t('a named helper inside an it.skip() block still wins over the block', + siteNames('site-mod-helper', { + 'a.e2e.test.ts': suite(`it.skip('x', () => {\n function build() {\n return { ...process.env };\n }\n void build;\n});`), + }) === names('build')); + + // ⭐ The collision END TO END for the modifier family, through audit(): + // one synthesised entry keyed to ONE it.each block silences that block + // and leaves its sibling a finding. Before this, both keyed to + // `(top-level)` and one entry silenced BOTH. + const modSiblingRoot = tree('carve-mod-siblings', { + 'a.e2e.test.ts': suite( + `it.each([1])('first', () => {\n${BULK}\n});\nit.each([2])('second', () => {\n${BULK}\n});`), + }); + const modOnlyFirst = audit(modSiblingRoot, { 'packages/cli/test/a.e2e.test.ts::it.each("first")': { why: 'synthetic' } }); + t('a registry entry for ONE it.each() block silences THAT block and leaves its sibling a finding', + JSON.stringify({ + deliberate: modOnlyFirst.deliberate.map((row) => row.fn), + findings: modOnlyFirst.findings.map((row) => row.fn), + }) === JSON.stringify({ deliberate: ['it.each("first")'], findings: ['it.each("second")'] }), + JSON.stringify(modOnlyFirst.findings.map((row) => row.fn))); // ⭐ The collision END TO END, through the real classification path in // audit(). One synthesised entry, two sibling blocks. Before this fix