Skip to content

Commit 1ca622e

Browse files
committed
ci(test): clarify test loop remediation
1 parent 1feaf2b commit 1ca622e

7 files changed

Lines changed: 68 additions & 30 deletions

File tree

.github/workflows/codebase-growth-guardrails.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ jobs:
140140
set -euo pipefail
141141
node --experimental-strip-types tools/growth-guardrails/test-conditionals.mts
142142
143-
- name: Require changed test files not to add table-test candidate loops
143+
- name: Require changed test files not to increase test-loop counts
144144
env:
145145
GH_TOKEN: ${{ github.token }}
146146
PR_NUMBER: ${{ github.event.pull_request.number }}

scripts/growth-guardrails/find-test-loops.mts

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,10 @@
22
// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
33
// SPDX-License-Identifier: Apache-2.0
44
//
5-
// Finds `for` loops that make test cases or generated test definitions
6-
// iterative. Independent rows should use it.each or test.each so each failure
7-
// identifies one behavior. Required iteration can stay in a named helper
8-
// outside the test callback.
5+
// Finds `for` loops inside test callbacks and loops that generate test
6+
// definitions. Required iteration can stay in a named helper outside the test
7+
// callback. Independent rows should use it.each or test.each so each failure
8+
// identifies one behavior.
99

1010
import { existsSync, readdirSync, readFileSync, statSync } from "node:fs";
1111
import path from "node:path";
@@ -264,14 +264,14 @@ function formatContext(occurrence: TestLoopOccurrence): string {
264264

265265
export function formatReport(report: TestLoopReport, options: Pick<CliOptions, "top">): string {
266266
const lines = [
267-
`Scanned ${report.summary.scannedFiles} test files; found ${report.summary.loopCount} table-test candidate loop(s) in ${report.summary.filesWithLoops} file(s).`,
267+
`Scanned ${report.summary.scannedFiles} test files; found ${report.summary.loopCount} test loop(s) in ${report.summary.filesWithLoops} file(s).`,
268268
"",
269269
"Top files by loop count:",
270270
];
271271
for (const file of report.files.slice(0, options.top)) {
272272
lines.push(`- ${file.file}: loops=${file.count}`);
273273
}
274-
lines.push("", "Table-test candidate loops:");
274+
lines.push("", "Test loops:");
275275
for (const occurrence of report.occurrences.slice(0, options.top)) {
276276
lines.push(
277277
`- ${occurrence.file}:${occurrence.line}:${occurrence.column} [${formatContext(occurrence)}] ${occurrence.kind}`,

test/growth-guardrails-entrypoints.test.ts

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ describe("growth-guardrails executable entrypoints (#6953)", () => {
9494

9595
expect(result.status).toBe(0);
9696
expect(result.stdout).toContain(
97-
"PASS: changed test files did not add table-test candidate loops (0 at the latest PR commit vs 0 at base).",
97+
"PASS: no changed test file increased its test-loop count (0 total at the latest PR commit vs 0 at base).",
9898
);
9999
});
100100

@@ -106,9 +106,12 @@ describe("growth-guardrails executable entrypoints (#6953)", () => {
106106
]);
107107

108108
expect(result.status).toBe(1);
109-
expect(result.stderr).toContain("FAIL: changed test files add table-test candidate loops.");
109+
expect(result.stderr).toContain("FAIL: changed test files increase test-loop counts.");
110110
expect(result.stderr).toContain(
111-
"test/a.test.ts: 1 table-test candidate loop(s), up from 0",
111+
"test/a.test.ts: 1 test loop(s), up from 0",
112+
);
113+
expect(result.stderr).toContain(
114+
"Move iteration needed for one behavior into a named helper outside the test callback. Use it.each or test.each when loop rows are independent cases.",
112115
);
113116
});
114117

test/growth-guardrails-test-loops.test.ts

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -61,17 +61,17 @@ describe("growth-guardrails test-loops: pure policy", () => {
6161
displayName: path,
6262
});
6363

64-
it("flags a test file that adds a table-test candidate loop", () => {
64+
it("flags a test file that adds a test loop", () => {
6565
const result = evaluateLoopViolations(
6666
[same("test/a.test.ts")],
6767
blobs({ "test/a.test.ts": NO_LOOP }),
6868
blobs({ "test/a.test.ts": ONE_LOOP }),
6969
);
70-
expect(result.details).toEqual(["test/a.test.ts: 1 table-test candidate loop(s), up from 0"]);
70+
expect(result.details).toEqual(["test/a.test.ts: 1 test loop(s), up from 0"]);
7171
expect([result.baseTotal, result.headTotal]).toEqual([0, 1]);
7272
});
7373

74-
it("passes a test file that removes table-test candidate loops", () => {
74+
it("passes a test file that removes test loops", () => {
7575
const result = evaluateLoopViolations(
7676
[same("test/a.test.ts")],
7777
blobs({ "test/a.test.ts": TWO_LOOPS }),
@@ -103,7 +103,7 @@ describe("growth-guardrails test-loops: pure policy", () => {
103103
blobs({ "test/adder.test.ts": ONE_LOOP, "test/remover.test.ts": NO_LOOP }),
104104
);
105105
expect(result.details).toEqual([
106-
"test/adder.test.ts: 1 table-test candidate loop(s), up from 0",
106+
"test/adder.test.ts: 1 test loop(s), up from 0",
107107
]);
108108
expect([result.baseTotal, result.headTotal]).toEqual([2, 1]);
109109
});
@@ -128,14 +128,14 @@ describe("growth-guardrails test-loops: orchestration", () => {
128128
REPO: "NVIDIA/NemoClaw",
129129
} as const;
130130

131-
it("fails a PR whose changed test adds a table-test candidate loop", async () => {
131+
it("fails a PR whose changed test adds a test loop", async () => {
132132
const client = fakeClient([{ filename: "test/a.test.ts", status: "modified" }], {
133133
"NVIDIA/NemoClaw base test/a.test.ts": NO_LOOP,
134134
"fork/repo head test/a.test.ts": ONE_LOOP,
135135
});
136136
const result = await runTestLoops(client, ENV);
137137
expect(result.ok).toBe(false);
138-
expect(result.details).toEqual(["test/a.test.ts: 1 table-test candidate loop(s), up from 0"]);
138+
expect(result.details).toEqual(["test/a.test.ts: 1 test loop(s), up from 0"]);
139139
});
140140

141141
it("ignores changed source files that are not tests", async () => {

test/test-loops-scanner.test.ts

Lines changed: 39 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,10 @@
33

44
import { describe, expect, it } from "vitest";
55

6-
import { scanTextForTestLoops } from "../scripts/growth-guardrails/find-test-loops.mts";
6+
import {
7+
formatReport,
8+
scanTextForTestLoops,
9+
} from "../scripts/growth-guardrails/find-test-loops.mts";
710

811
describe("test loop scanner", () => {
912
it("detects each for-loop form inside a test callback", () => {
@@ -51,22 +54,35 @@ describe("test loop scanner", () => {
5154
});
5255
});
5356

54-
it("ignores fixture text and loop forms that do not represent table-test candidates", () => {
57+
it("allows required iteration in a named helper outside the test callback", () => {
5558
const occurrences = scanTextForTestLoops(
5659
"test/virtual-helper-loops.test.ts",
5760
`
58-
const fixture = \`for (const row of rows) { expect(row).toBeDefined(); }\`;
5961
function collect(values) {
6062
for (const value of values) consume(value);
6163
}
64+
it("collects values", () => {
65+
collect(values);
66+
expect(values).toBeDefined();
67+
});
68+
`,
69+
);
70+
71+
expect(occurrences).toEqual([]);
72+
});
73+
74+
it("ignores fixture text, hooks, and loop forms outside the rule", () => {
75+
const occurrences = scanTextForTestLoops(
76+
"test/virtual-ignored-loops.test.ts",
77+
`
78+
const fixture = \`for (const row of rows) { expect(row).toBeDefined(); }\`;
6279
afterEach(() => {
6380
for (const resource of resources) resource.close();
6481
});
6582
it("waits", () => {
6683
while (pending()) wait();
6784
do { wait(); } while (pending());
6885
values.forEach(consume);
69-
collect(values);
7086
expect(fixture).toContain("for");
7187
});
7288
`,
@@ -96,4 +112,23 @@ describe("test loop scanner", () => {
96112
contextName: "iterates in interpolation",
97113
});
98114
});
115+
116+
it("reports findings as test loops", () => {
117+
const occurrences = scanTextForTestLoops(
118+
"test/virtual-report.test.ts",
119+
'it("iterates", () => { for (const value of values) consume(value); });',
120+
);
121+
const report = formatReport(
122+
{
123+
summary: { scannedFiles: 1, filesWithLoops: 1, loopCount: 1 },
124+
files: [{ file: "test/virtual-report.test.ts", count: 1 }],
125+
occurrences,
126+
},
127+
{ top: 1 },
128+
);
129+
130+
expect(report).toContain("found 1 test loop(s) in 1 file(s)");
131+
expect(report).toContain("\nTest loops:\n");
132+
expect(report).not.toContain("table-test candidate");
133+
});
99134
});

tools/growth-guardrails/test-loops.mts

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
22
// SPDX-License-Identifier: Apache-2.0
33
//
4-
// Trusted policy evaluator: a changed test file may not add table-test
5-
// candidate loops. The workflow reads pull-request blobs as data and parses
6-
// them with the TypeScript AST. It never executes pull-request code.
4+
// Trusted policy evaluator: a changed test file must not increase its test-loop
5+
// count. The workflow reads pull-request blobs as data and parses them with the
6+
// TypeScript AST. It never executes pull-request code.
77

88
import { scanTextForTestLoops } from "../../scripts/growth-guardrails/find-test-loops.mts";
99
import {
@@ -54,7 +54,7 @@ export function evaluateLoopViolations(
5454
headTotal += headCount;
5555
if (headCount > baseCount) {
5656
details.push(
57-
`${change.headPath ?? change.displayName}: ${headCount} table-test candidate loop(s), up from ${baseCount}`,
57+
`${change.headPath ?? change.displayName}: ${headCount} test loop(s), up from ${baseCount}`,
5858
);
5959
}
6060
}
@@ -120,13 +120,13 @@ async function main(): Promise<void> {
120120
const client = createPrBlobClient({ token: env.GH_TOKEN });
121121
const result = await runTestLoops(client, env);
122122
if (!result.ok) {
123-
console.error("FAIL: changed test files add table-test candidate loops.");
123+
console.error("FAIL: changed test files increase test-loop counts.");
124124
console.error(
125-
`Changed test files contain ${result.headTotal} table-test candidate loop(s) at the latest PR commit vs ${result.baseTotal} at base.`,
125+
`Across all changed test files: ${result.headTotal} test loop(s) at the latest PR commit vs ${result.baseTotal} at base.`,
126126
);
127127
console.error("");
128128
console.error(
129-
"Test cases should stay linear. Use it.each or test.each for independent cases. Move iteration that represents one behavior into a named helper outside the test callback.",
129+
"Keep test callbacks linear. Move iteration needed for one behavior into a named helper outside the test callback. Use it.each or test.each when loop rows are independent cases.",
130130
);
131131
console.error("");
132132
console.error("Files with increased test loop counts:");
@@ -136,7 +136,7 @@ async function main(): Promise<void> {
136136
process.exit(1);
137137
}
138138
console.log(
139-
`PASS: changed test files did not add table-test candidate loops (${result.headTotal} at the latest PR commit vs ${result.baseTotal} at base).`,
139+
`PASS: no changed test file increased its test-loop count (${result.headTotal} total at the latest PR commit vs ${result.baseTotal} at base).`,
140140
);
141141
}
142142

tools/growth-guardrails/workflow-boundary.mts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,8 +67,8 @@ const APPROVED_STEP_SHAPES = [
6767
sha256: "991a7036d27aeb45fde2f3178936fba6ac87b90200b79c933015f9d63525d3cf",
6868
},
6969
{
70-
name: "Require changed test files not to add table-test candidate loops",
71-
sha256: "6432b36d3c5ed34c648d782e5fc2302c15e76aea88b0ed319dca080ef6ef9431",
70+
name: "Require changed test files not to increase test-loop counts",
71+
sha256: "b31484bda2065644a63d1fb5fdd36d4a98f307e9b6514eeb236952e689a64106",
7272
},
7373
] as const;
7474

0 commit comments

Comments
 (0)