Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/codebase-growth-guardrails.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ jobs:
set -euo pipefail
node --experimental-strip-types tools/growth-guardrails/test-conditionals.mts

- name: Require changed test files not to add table-test candidate loops
- name: Require changed test files not to increase test-loop counts
env:
GH_TOKEN: ${{ github.token }}
PR_NUMBER: ${{ github.event.pull_request.number }}
Expand Down
12 changes: 6 additions & 6 deletions scripts/growth-guardrails/find-test-loops.mts
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@
// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
// SPDX-License-Identifier: Apache-2.0
//
// Finds `for` loops that make test cases or generated test definitions
// iterative. Independent rows should use it.each or test.each so each failure
// identifies one behavior. Required iteration can stay in a named helper
// outside the test callback.
// Finds `for` loops inside test callbacks and loops that generate test
// definitions. Required iteration can stay in a named helper outside the test
// callback. Independent rows should use it.each or test.each so each failure
// identifies one behavior.

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

export function formatReport(report: TestLoopReport, options: Pick<CliOptions, "top">): string {
const lines = [
`Scanned ${report.summary.scannedFiles} test files; found ${report.summary.loopCount} table-test candidate loop(s) in ${report.summary.filesWithLoops} file(s).`,
`Scanned ${report.summary.scannedFiles} test files; found ${report.summary.loopCount} test loop(s) in ${report.summary.filesWithLoops} file(s).`,
"",
"Top files by loop count:",
];
for (const file of report.files.slice(0, options.top)) {
lines.push(`- ${file.file}: loops=${file.count}`);
}
lines.push("", "Table-test candidate loops:");
lines.push("", "Test loops:");
for (const occurrence of report.occurrences.slice(0, options.top)) {
lines.push(
`- ${occurrence.file}:${occurrence.line}:${occurrence.column} [${formatContext(occurrence)}] ${occurrence.kind}`,
Expand Down
9 changes: 6 additions & 3 deletions test/growth-guardrails-entrypoints.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ describe("growth-guardrails executable entrypoints (#6953)", () => {

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

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

expect(result.status).toBe(1);
expect(result.stderr).toContain("FAIL: changed test files add table-test candidate loops.");
expect(result.stderr).toContain("FAIL: changed test files increase test-loop counts.");
expect(result.stderr).toContain(
"test/a.test.ts: 1 table-test candidate loop(s), up from 0",
"test/a.test.ts: 1 test loop(s), up from 0",
);
expect(result.stderr).toContain(
"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.",
);
});

Expand Down
12 changes: 6 additions & 6 deletions test/growth-guardrails-test-loops.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,17 +61,17 @@ describe("growth-guardrails test-loops: pure policy", () => {
displayName: path,
});

it("flags a test file that adds a table-test candidate loop", () => {
it("flags a test file that adds a test loop", () => {
const result = evaluateLoopViolations(
[same("test/a.test.ts")],
blobs({ "test/a.test.ts": NO_LOOP }),
blobs({ "test/a.test.ts": ONE_LOOP }),
);
expect(result.details).toEqual(["test/a.test.ts: 1 table-test candidate loop(s), up from 0"]);
expect(result.details).toEqual(["test/a.test.ts: 1 test loop(s), up from 0"]);
expect([result.baseTotal, result.headTotal]).toEqual([0, 1]);
});

it("passes a test file that removes table-test candidate loops", () => {
it("passes a test file that removes test loops", () => {
const result = evaluateLoopViolations(
[same("test/a.test.ts")],
blobs({ "test/a.test.ts": TWO_LOOPS }),
Expand Down Expand Up @@ -103,7 +103,7 @@ describe("growth-guardrails test-loops: pure policy", () => {
blobs({ "test/adder.test.ts": ONE_LOOP, "test/remover.test.ts": NO_LOOP }),
);
expect(result.details).toEqual([
"test/adder.test.ts: 1 table-test candidate loop(s), up from 0",
"test/adder.test.ts: 1 test loop(s), up from 0",
]);
expect([result.baseTotal, result.headTotal]).toEqual([2, 1]);
});
Expand All @@ -128,14 +128,14 @@ describe("growth-guardrails test-loops: orchestration", () => {
REPO: "NVIDIA/NemoClaw",
} as const;

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

it("ignores changed source files that are not tests", async () => {
Expand Down
43 changes: 39 additions & 4 deletions test/test-loops-scanner.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@

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

import { scanTextForTestLoops } from "../scripts/growth-guardrails/find-test-loops.mts";
import {
formatReport,
scanTextForTestLoops,
} from "../scripts/growth-guardrails/find-test-loops.mts";

describe("test loop scanner", () => {
it("detects each for-loop form inside a test callback", () => {
Expand Down Expand Up @@ -51,22 +54,35 @@ describe("test loop scanner", () => {
});
});

it("ignores fixture text and loop forms that do not represent table-test candidates", () => {
it("allows required iteration in a named helper outside the test callback", () => {
const occurrences = scanTextForTestLoops(
"test/virtual-helper-loops.test.ts",
`
const fixture = \`for (const row of rows) { expect(row).toBeDefined(); }\`;
function collect(values) {
for (const value of values) consume(value);
}
it("collects values", () => {
collect(values);
expect(values).toBeDefined();
});
`,
);

expect(occurrences).toEqual([]);
});

it("ignores fixture text, hooks, and loop forms outside the rule", () => {
const occurrences = scanTextForTestLoops(
"test/virtual-ignored-loops.test.ts",
`
const fixture = \`for (const row of rows) { expect(row).toBeDefined(); }\`;
afterEach(() => {
for (const resource of resources) resource.close();
});
it("waits", () => {
while (pending()) wait();
do { wait(); } while (pending());
values.forEach(consume);
collect(values);
expect(fixture).toContain("for");
});
`,
Expand Down Expand Up @@ -96,4 +112,23 @@ describe("test loop scanner", () => {
contextName: "iterates in interpolation",
});
});

it("reports findings as test loops", () => {
const occurrences = scanTextForTestLoops(
"test/virtual-report.test.ts",
'it("iterates", () => { for (const value of values) consume(value); });',
);
const report = formatReport(
{
summary: { scannedFiles: 1, filesWithLoops: 1, loopCount: 1 },
files: [{ file: "test/virtual-report.test.ts", count: 1 }],
occurrences,
},
{ top: 1 },
);

expect(report).toContain("found 1 test loop(s) in 1 file(s)");
expect(report).toContain("\nTest loops:\n");
expect(report).not.toContain("table-test candidate");
});
});
16 changes: 8 additions & 8 deletions tools/growth-guardrails/test-loops.mts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
// SPDX-License-Identifier: Apache-2.0
//
// Trusted policy evaluator: a changed test file may not add table-test
// candidate loops. The workflow reads pull-request blobs as data and parses
// them with the TypeScript AST. It never executes pull-request code.
// Trusted policy evaluator: a changed test file must not increase its test-loop
// count. The workflow reads pull-request blobs as data and parses them with the
// TypeScript AST. It never executes pull-request code.

import { scanTextForTestLoops } from "../../scripts/growth-guardrails/find-test-loops.mts";
import {
Expand Down Expand Up @@ -54,7 +54,7 @@ export function evaluateLoopViolations(
headTotal += headCount;
if (headCount > baseCount) {
details.push(
`${change.headPath ?? change.displayName}: ${headCount} table-test candidate loop(s), up from ${baseCount}`,
`${change.headPath ?? change.displayName}: ${headCount} test loop(s), up from ${baseCount}`,
);
}
}
Expand Down Expand Up @@ -120,13 +120,13 @@ async function main(): Promise<void> {
const client = createPrBlobClient({ token: env.GH_TOKEN });
const result = await runTestLoops(client, env);
if (!result.ok) {
console.error("FAIL: changed test files add table-test candidate loops.");
console.error("FAIL: changed test files increase test-loop counts.");
console.error(
`Changed test files contain ${result.headTotal} table-test candidate loop(s) at the latest PR commit vs ${result.baseTotal} at base.`,
`Across all changed test files: ${result.headTotal} test loop(s) at the latest PR commit vs ${result.baseTotal} at base.`,
);
console.error("");
console.error(
"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.",
"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.",
);
console.error("");
console.error("Files with increased test loop counts:");
Expand All @@ -136,7 +136,7 @@ async function main(): Promise<void> {
process.exit(1);
}
console.log(
`PASS: changed test files did not add table-test candidate loops (${result.headTotal} at the latest PR commit vs ${result.baseTotal} at base).`,
`PASS: no changed test file increased its test-loop count (${result.headTotal} total at the latest PR commit vs ${result.baseTotal} at base).`,
);
}

Expand Down
4 changes: 2 additions & 2 deletions tools/growth-guardrails/workflow-boundary.mts
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ const APPROVED_STEP_SHAPES = [
sha256: "991a7036d27aeb45fde2f3178936fba6ac87b90200b79c933015f9d63525d3cf",
},
{
name: "Require changed test files not to add table-test candidate loops",
sha256: "6432b36d3c5ed34c648d782e5fc2302c15e76aea88b0ed319dca080ef6ef9431",
name: "Require changed test files not to increase test-loop counts",
sha256: "b31484bda2065644a63d1fb5fdd36d4a98f307e9b6514eeb236952e689a64106",
},
] as const;

Expand Down
Loading