Skip to content

Commit 23d52f9

Browse files
os-litantclaude
andauthored
fix(cli-gate): name it.skip / it.only / it.each blocks instead of collapsing them onto (top-level) (#12577)
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. Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd Co-authored-by: Claude <noreply@anthropic.com>
1 parent 5a22dd7 commit 23d52f9

1 file changed

Lines changed: 210 additions & 13 deletions

File tree

‎scripts/check-cli-test-child-env.mjs‎

Lines changed: 210 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -510,17 +510,104 @@ function isBulkUse(node) {
510510
return true;
511511
}
512512

513+
/**
514+
* The three vitest block roots whose modifier chains name a real test block.
515+
*
516+
* ⛔ A ROSTER rather than "any identifier", and the population is why: this
517+
* directory calls `it.name`, `it.type`, `it.next` and `it.prevVersion` on
518+
* ordinary loop variables that happen to be spelled `it`. Gating on the root
519+
* alone would name a callback site after one of those.
520+
*/
521+
const TEST_BLOCK_ROOTS = new Set(['it', 'test', 'describe']);
522+
523+
/**
524+
* The vitest modifiers that keep a chain a TEST BLOCK, so `it.skip(...)` names
525+
* a site and `rows.map(...)` does not.
526+
*
527+
* ⛔ Also a roster, and this is the half that does the refusing. Measured under
528+
* {@link POPULATION_ROOT}: a blanket "property access whose base is an
529+
* identifier" rule matches **209 call sites across 87 distinct callees** here
530+
* -- `literals.filter`, `child.on`, `entries.map`, `run.then`, `rows.map`,
531+
* `vi.fn`, `Array.from` -- against the **22** this roster matches (`it.each`
532+
* x20, `describe.each` x2, the whole modifier population of this directory).
533+
* Those 87 are not test blocks, their callee is a WORSE name than the enclosing
534+
* block the walk reaches today, and naming them would MOVE the key of every
535+
* site nested inside one. Re-keying is a data migration wearing a bug fix's
536+
* clothes; the roster is what keeps this a rename of nothing.
537+
*
538+
* `each`/`for` and `skipIf`/`runIf` are the curried forms -- `it.each(table)(
539+
* 'title', fn)` -- and are unwrapped one call deeper by {@link calleeSiteName}.
540+
*
541+
* ⛔ `extend` is deliberately absent: `test.extend(fixtures)` builds a NEW test
542+
* function rather than a block, and that product is bound to a name the
543+
* identifier branch already reads.
544+
*/
545+
const TEST_BLOCK_MODIFIERS = new Set([
546+
'skip', 'only', 'todo', 'fails', // verdict
547+
'concurrent', 'sequential', 'shuffle', // scheduling
548+
'each', 'for', // table (curried)
549+
'skipIf', 'runIf', // conditional (curried)
550+
]);
551+
552+
/**
553+
* The dotted name of a test-block chain -- `it.skip`, `describe.each`,
554+
* `it.skip.each` -- or `null` for anything that is not one.
555+
*
556+
* Every segment after the root must be on {@link TEST_BLOCK_MODIFIERS}, so a
557+
* chain stays a block however deep it nests, and `a.b.c(...)` never becomes a
558+
* site name.
559+
*/
560+
function testBlockChainName(expr) {
561+
const modifiers = [];
562+
let cursor = expr;
563+
while (ts.isPropertyAccessExpression(cursor)) {
564+
if (!ts.isIdentifier(cursor.name)) return null;
565+
modifiers.unshift(cursor.name.text);
566+
cursor = cursor.expression;
567+
}
568+
// A bare identifier is the pre-existing branch, not this one.
569+
if (modifiers.length === 0 || !ts.isIdentifier(cursor)) return null;
570+
if (!TEST_BLOCK_ROOTS.has(cursor.text)) return null;
571+
if (!modifiers.every((part) => TEST_BLOCK_MODIFIERS.has(part))) return null;
572+
return [cursor.text, ...modifiers].join('.');
573+
}
574+
575+
/**
576+
* The callee half of a site name, or `null` when the call does not name a site.
577+
*
578+
* ⚠️ The identifier branch is FIRST and byte-identical to what it always was.
579+
* That is load-bearing: {@link DELIBERATE_REROUTE} holds keys this function
580+
* produced for plain `it("...")` blocks, and both registries are pinned in both
581+
* directions, so a green live-tree run is the proof that no existing key moved.
582+
*/
583+
function calleeSiteName(expr) {
584+
if (ts.isIdentifier(expr)) return expr.text;
585+
const chain = testBlockChainName(expr);
586+
if (chain) return chain;
587+
// The curried table/conditional forms: `it.each(table)('title', fn)` is a
588+
// call whose CALLEE is itself a call of a test-block chain. Only one level --
589+
// vitest defines no deeper currying, and unbounded unwrapping would start
590+
// guessing again.
591+
if (ts.isCallExpression(expr)) return testBlockChainName(expr.expression);
592+
return null;
593+
}
594+
513595
/**
514596
* The name a call-argument arrow takes from the call it is passed to:
515597
* `it("boots the child")`, or `beforeAll` when the call carries no title.
516598
*
517599
* DOUBLE quotes around the title, deliberately: the product is a
518600
* {@link DELIBERATE} key, and this file's string literals are single-quoted, so
519601
* a key pasted into that registry needs no escaping.
602+
*
603+
* The title is read from the OUTER call's arguments, which is what makes the
604+
* curried form come out right: `it.each(table)('boots', fn)` is named for
605+
* `'boots'`, never for a string that happens to sit in `table`.
520606
*/
521607
function callbackSiteName(owner) {
522-
if (!owner || !ts.isCallExpression(owner) || !ts.isIdentifier(owner.expression)) return null;
523-
const callee = owner.expression.text;
608+
if (!owner || !ts.isCallExpression(owner)) return null;
609+
const callee = calleeSiteName(owner.expression);
610+
if (!callee) return null;
524611
const title = owner.arguments.find((arg) => ts.isStringLiteralLike(arg));
525612
return title ? `${callee}("${title.text.replace(/\s+/g, ' ').trim()}")` : callee;
526613
}
@@ -560,12 +647,15 @@ function callbackSiteName(owner) {
560647
*
561648
* ⚠️ Three limits, named here so they are pinned rather than discovered:
562649
*
563-
* 1. The callee must be an IDENTIFIER. `it.skip('...')` and
564-
* `it.each(table)('...')` call through a property access or through
565-
* another call, and still walk past to `(top-level)`. That is a choice:
566-
* widening to a property-access callee would also capture
567-
* `promise.then(() => ...)` and `rows.map(() => ...)`, whose callee is a
568-
* WORSE site name than the enclosing test block the walk reaches today.
650+
* 1. The callee must be an identifier, or a ROSTERED vitest block chain.
651+
* `it.skip('...')`, `describe.each(table)('...')` and `it.skip.each(...)`
652+
* are named (#12545); `promise.then(() => ...)`, `rows.map(() => ...)` and
653+
* every other `foo.bar(cb)` are still walked past to the enclosing block,
654+
* because their callee is a WORSE site name than the block above them.
655+
* ⛔ The refusal is the point, not a leftover: see
656+
* {@link TEST_BLOCK_MODIFIERS} for the 209-vs-22 measurement that decides
657+
* it. A blanket property-access widening would RE-KEY existing sites,
658+
* which is a data migration disguised as a bug fix.
569659
* 2. The title's whitespace is collapsed so the name stays one line, but it
570660
* is ⛔ never truncated -- two long titles sharing a prefix would collide
571661
* again, which is the defect being closed.
@@ -1752,12 +1842,119 @@ export function selfTest() {
17521842
siteNames('site-title-wrap', { 'a.e2e.test.ts': suite(`it(\`boots\nthe child\`, () => {\n${BULK}\n});`) })
17531843
=== names('it("boots the child")'));
17541844

1755-
// The named limit, pinned rather than discovered: a property-access callee
1756-
// is deliberately NOT captured, because `promise.then(...)` and `rows.map(
1757-
// ...)` would be worse site names than the test block the walk reaches.
1758-
t('an it.skip() callee is a property access, not an identifier, so it still reads (top-level)',
1759-
siteNames('site-property-callee', { 'a.e2e.test.ts': suite(`it.skip('x', () => {\n${BULK}\n});`) })
1845+
// -- (8c) THE MODIFIER FAMILY (#12545): a rostered vitest chain names a
1846+
// site; every other property-access callee still does not.
1847+
//
1848+
// Until this landed, the callee had to be a bare IDENTIFIER, so
1849+
// `it.skip(...)` (property access) and `it.each(table)(...)` (a call)
1850+
// walked past to `(top-level)` and every skipped/table-driven block in
1851+
// one file collided on ONE key -- the same defect #12531 closed for the
1852+
// identifier branch, one modifier over. The case below that used to pin
1853+
// `it.skip` AS `(top-level)` is retargeted to the name it now produces;
1854+
// what that case's comment was really protecting -- the refusal of
1855+
// `promise.then` and `rows.map` -- is pinned outright further down,
1856+
// where it is a measurement rather than a side effect.
1857+
1858+
t('an it.skip() block is named, not (top-level)',
1859+
siteNames('site-mod-skip', { 'a.e2e.test.ts': suite(`it.skip('x', () => {\n${BULK}\n});`) })
1860+
=== names('it.skip("x")'));
1861+
t('an it.only() block is named',
1862+
siteNames('site-mod-only', { 'a.e2e.test.ts': suite(`it.only('x', () => {\n${BULK}\n});`) })
1863+
=== names('it.only("x")'));
1864+
t('a describe.skip() block is named -- the root roster is not it-only',
1865+
siteNames('site-mod-desc-skip', { 'a.e2e.test.ts': suite(`describe.skip('x', () => {\n${BULK}\n});`) })
1866+
=== names('describe.skip("x")'));
1867+
t('a test.concurrent() block is named -- test is a root too',
1868+
siteNames('site-mod-test-conc', { 'a.e2e.test.ts': suite(`test.concurrent('x', () => {\n${BULK}\n});`) })
1869+
=== names('test.concurrent("x")'));
1870+
t('a modifier with NO string title falls back to the chain alone',
1871+
siteNames('site-mod-untitled', { 'a.e2e.test.ts': suite(`it.skip(() => {\n${BULK}\n});`) })
1872+
=== names('it.skip'));
1873+
1874+
// The CURRIED forms: the callee is itself a call. The title must come from
1875+
// the OUTER call, never from the table.
1876+
t('a curried it.each(table)() block is named after the OUTER title',
1877+
siteNames('site-mod-each', { 'a.e2e.test.ts': suite(`it.each([1])('x', () => {\n${BULK}\n});`) })
1878+
=== names('it.each("x")'));
1879+
t('a curried describe.each(table)() block is named',
1880+
siteNames('site-mod-desc-each', { 'a.e2e.test.ts': suite(`describe.each([1])('x', () => {\n${BULK}\n});`) })
1881+
=== names('describe.each("x")'));
1882+
t('...and a string INSIDE the table never becomes the title',
1883+
siteNames('site-mod-each-table-string', {
1884+
'a.e2e.test.ts': suite(`it.each(['from the table'])('from the block', () => {\n${BULK}\n});`),
1885+
}) === names('it.each("from the block")'));
1886+
t('a curried it.skipIf(cond)() block is named -- the conditional form curries too',
1887+
siteNames('site-mod-skipif', { 'a.e2e.test.ts': suite(`it.skipIf(flag)('x', () => {\n${BULK}\n});`) })
1888+
=== names('it.skipIf("x")'));
1889+
t('a CHAINED it.skip.each(table)() block is named through every segment',
1890+
siteNames('site-mod-chain', { 'a.e2e.test.ts': suite(`it.skip.each([1])('x', () => {\n${BULK}\n});`) })
1891+
=== names('it.skip.each("x")'));
1892+
1893+
// ⭐ THE case for this change, the modifier twin of `site-siblings`: two
1894+
// sibling it.each blocks in ONE file must not share a key either.
1895+
t('two it.each() blocks in one file get TWO DISTINCT names',
1896+
siteNames('site-mod-siblings', {
1897+
'a.e2e.test.ts': suite(
1898+
`it.each([1])('first', () => {\n${BULK}\n});\nit.each([2])('second', () => {\n${BULK}\n});`),
1899+
}) === names('it.each("first")', 'it.each("second")'));
1900+
1901+
// ⭐ THE NEGATIVE PIN. Without it, nothing in this suite distinguishes
1902+
// "the modifiers now work" from "everything got renamed". The plain
1903+
// identifier branch must be byte-identical to what it always was,
1904+
// because DELIBERATE_REROUTE holds keys it produced.
1905+
t('⭐ a plain it() block STILL yields exactly it("x") -- the identifier branch did not move',
1906+
siteNames('site-mod-plain-unmoved', { 'a.e2e.test.ts': suite(`it('x', () => {\n${BULK}\n});`) })
1907+
=== names('it("x")'));
1908+
t('...and a plain describe() > it() nesting is unmoved too',
1909+
siteNames('site-mod-plain-nested', {
1910+
'a.e2e.test.ts': suite(`describe('outer', () => {\n it('inner', () => {\n${BULK}\n });\n});`),
1911+
}) === names('it("inner")'));
1912+
1913+
// ⛔ THE REFUSALS -- the half that keeps this a targeted unwrap. A blanket
1914+
// property-access widening matches 209 call sites across 87 callees in
1915+
// the live population against this roster's 22, and would RE-KEY every
1916+
// site nested inside one of them.
1917+
t('⛔ promise.then(cb) is NOT a site name -- the walk still reaches past it',
1918+
siteNames('site-refuse-then', { 'a.e2e.test.ts': suite(`p.then(() => {\n${BULK}\n});`) })
1919+
=== names('(top-level)'));
1920+
t('⛔ rows.map(cb) is NOT a site name',
1921+
siteNames('site-refuse-map', { 'a.e2e.test.ts': suite(`rows.map(() => {\n${BULK}\n});`) })
17601922
=== names('(top-level)'));
1923+
t('⛔ an arbitrary a.b.c(cb) chain is NOT a site name',
1924+
siteNames('site-refuse-chain', { 'a.e2e.test.ts': suite(`a.b.c('x', () => {\n${BULK}\n});`) })
1925+
=== names('(top-level)'));
1926+
t('⛔ a NON-rostered member of a real root is refused -- it.name(cb) is a loop variable, not a block',
1927+
siteNames('site-refuse-nonroster', { 'a.e2e.test.ts': suite(`it.name('x', () => {\n${BULK}\n});`) })
1928+
=== names('(top-level)'));
1929+
t('⛔ a rostered MODIFIER on a non-rostered root is refused -- only it/test/describe are roots',
1930+
siteNames('site-refuse-nonroot', { 'a.e2e.test.ts': suite(`suite.each([1])('x', () => {\n${BULK}\n});`) })
1931+
=== names('(top-level)'));
1932+
t('⛔ a curried call whose inner callee is a bare identifier is refused',
1933+
siteNames('site-refuse-curried-ident', { 'a.e2e.test.ts': suite(`makeSuite('t')('x', () => {\n${BULK}\n});`) })
1934+
=== names('(top-level)'));
1935+
1936+
// The nearest NAMED binding still wins over a modifier block, exactly as it
1937+
// does over a plain one -- limit 3 of the walk, unchanged by this.
1938+
t('a named helper inside an it.skip() block still wins over the block',
1939+
siteNames('site-mod-helper', {
1940+
'a.e2e.test.ts': suite(`it.skip('x', () => {\n function build() {\n return { ...process.env };\n }\n void build;\n});`),
1941+
}) === names('build'));
1942+
1943+
// ⭐ The collision END TO END for the modifier family, through audit():
1944+
// one synthesised entry keyed to ONE it.each block silences that block
1945+
// and leaves its sibling a finding. Before this, both keyed to
1946+
// `(top-level)` and one entry silenced BOTH.
1947+
const modSiblingRoot = tree('carve-mod-siblings', {
1948+
'a.e2e.test.ts': suite(
1949+
`it.each([1])('first', () => {\n${BULK}\n});\nit.each([2])('second', () => {\n${BULK}\n});`),
1950+
});
1951+
const modOnlyFirst = audit(modSiblingRoot, { 'packages/cli/test/a.e2e.test.ts::it.each("first")': { why: 'synthetic' } });
1952+
t('a registry entry for ONE it.each() block silences THAT block and leaves its sibling a finding',
1953+
JSON.stringify({
1954+
deliberate: modOnlyFirst.deliberate.map((row) => row.fn),
1955+
findings: modOnlyFirst.findings.map((row) => row.fn),
1956+
}) === JSON.stringify({ deliberate: ['it.each("first")'], findings: ['it.each("second")'] }),
1957+
JSON.stringify(modOnlyFirst.findings.map((row) => row.fn)));
17611958

17621959
// ⭐ The collision END TO END, through the real classification path in
17631960
// audit(). One synthesised entry, two sibling blocks. Before this fix

0 commit comments

Comments
 (0)