Skip to content

Commit 5c19a47

Browse files
theCodeDriftclaude
andcommitted
fix(cli): count a fixture only when the file's own id is this rule
Three review findings on #156, all in the same shape: a claim stated twice diverges, and the second copy is the one nobody reads. `fixtureCoverage` counted a test file by filename alone, while `sg test --filter ^<id>$` resolves cases against the file's `id:` field. A draft copied from another rule — right filename, wrong `id:` — had its buckets counted toward coverage while ast-grep never ran them, reopening the "never shown to fire" gap this branch exists to close through the filename door rather than the empty-bucket one. The "what counts as this rule's test file" predicate was also written twice in one `verifyRule()` pass. It now lives in one `discoverRuleTestFiles` used by both `validateRequirements` and `fixtureCoverage`. Also drops the duplicate `describe("the language field")` suite that the rebase left behind. The base branch carries the fuller three-test version; commit 44f4850 removed the duplicate `atLanguage` helper but missed the suite itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cyga14bww8rmazH2XrF8ms
1 parent 44f4850 commit 5c19a47

3 files changed

Lines changed: 71 additions & 65 deletions

File tree

‎packages/cli/src/rules/verify.ts‎

Lines changed: 46 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -165,17 +165,8 @@ async function validateRequirements(
165165
}
166166
}
167167

168-
// A rule's tests live inside the rule directory, so there is one place to
169-
// look and no resolution order to get wrong.
170-
let hasTestFile = false;
171-
try {
172-
const entries = await readdir(ruleTestsDirectory(cwd, "sg", ruleId));
173-
hasTestFile = entries.some(
174-
(f) => f.startsWith(`${ruleId}-`) && f.endsWith("-test.yml")
175-
);
176-
} catch {
177-
// No tests directory — hasTestFile stays false.
178-
}
168+
const testFiles = await discoverRuleTestFiles(cwd, ruleId);
169+
const hasTestFile = testFiles.length > 0;
179170
if (!hasTestFile) {
180171
errors.push(
181172
`No test file found for rule "${ruleId}" in ` +
@@ -186,6 +177,39 @@ async function validateRequirements(
186177
return { valid: errors.length === 0, errors, hasTestFile };
187178
}
188179

180+
/**
181+
* Every file in a rule's own `.tests/` directory that the naming convention
182+
* claims for this rule, as absolute paths.
183+
*
184+
* A rule's tests live inside the rule directory, so there is one place to look
185+
* and no resolution order to get wrong. Stated once because two callers ask the
186+
* same question in the same `verifyRule()` pass — `validateRequirements` for
187+
* "are there any tests at all", `fixtureCoverage` for "what is in them" — and a
188+
* change to the convention has to move both at once or they disagree.
189+
*
190+
* The filename is all this decides. What ast-grep will actually RUN is keyed on
191+
* the file's own `id:` field, which is a separate question, asked where it
192+
* matters (see {@link fixtureCoverage}).
193+
*/
194+
async function discoverRuleTestFiles(
195+
cwd: string,
196+
ruleId: string
197+
): Promise<string[]> {
198+
const directory = ruleTestsDirectory(cwd, "sg", ruleId);
199+
let entries: string[];
200+
try {
201+
entries = await readdir(directory);
202+
} catch {
203+
// No tests directory — the rule owns no test files.
204+
return [];
205+
}
206+
return entries
207+
.filter(
208+
(entry) => entry.startsWith(`${ruleId}-`) && entry.endsWith("-test.yml")
209+
)
210+
.map((entry) => join(directory, entry));
211+
}
212+
189213
/** Classify a rule's buckets by how many sources each held. */
190214
function coverageOf(
191215
validCount: number,
@@ -209,28 +233,25 @@ function coverageOf(
209233
* A file that cannot be read or parsed contributes nothing. `sg test` reports
210234
* malformed test YAML itself, and guessing at a bucket count from a file we
211235
* could not parse would be a worse error than the one already being raised.
236+
*
237+
* Only a file whose own `id:` is this rule counts, which is a stricter test
238+
* than the filename it was found by. `sg test --filter ^<id>$` resolves cases
239+
* against that field, so a file named for `no-eval` but carrying
240+
* `id: no-alert-scratch` — a draft copied from another rule — is never executed
241+
* for `no-eval`. Counting its buckets here would report coverage for fixtures
242+
* that never ran, which is the same "never shown to fire" gap this check
243+
* exists to close, reached through the filename rather than an empty bucket.
212244
*/
213245
async function fixtureCoverage(
214246
cwd: string,
215247
ruleId: string
216248
): Promise<SgFixtureCoverage> {
217-
const directory = ruleTestsDirectory(cwd, "sg", ruleId);
218-
let entries: string[];
219-
try {
220-
entries = await readdir(directory);
221-
} catch {
222-
return "none";
223-
}
224-
225249
let validCount = 0;
226250
let invalidCount = 0;
227-
for (const entry of entries) {
228-
if (!entry.startsWith(`${ruleId}-`) || !entry.endsWith("-test.yml")) {
229-
continue;
230-
}
251+
for (const file of await discoverRuleTestFiles(cwd, ruleId)) {
231252
let parsed: unknown;
232253
try {
233-
parsed = parse(await readFile(join(directory, entry), "utf8"));
254+
parsed = parse(await readFile(file, "utf8"));
234255
} catch {
235256
continue;
236257
}
@@ -242,6 +263,7 @@ async function fixtureCoverage(
242263
continue;
243264
}
244265
const buckets = parsed as Record<string, unknown>;
266+
if (buckets.id !== ruleId) continue;
245267
if (Array.isArray(buckets.valid)) validCount += buckets.valid.length;
246268
if (Array.isArray(buckets.invalid)) invalidCount += buckets.invalid.length;
247269
}

‎packages/cli/test/ast-grep-vendor-contract.test.ts‎

Lines changed: 0 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -655,47 +655,6 @@ withSg("ast-grep vendor contract", () => {
655655
});
656656
});
657657

658-
/**
659-
* How a wrong `language:` fails — the two shapes `create-sg-rule.txt` warns
660-
* about where the field is written.
661-
*
662-
* Nothing of ours catches either one first: the vendored
663-
* `src/generated/ast-grep-rule-schema.json` types `$defs.Language` as a bare
664-
* string with no enum, and `verify` never reads the field. So the binary's
665-
* response IS the contract, and a recipe telling an author what to expect is
666-
* quoting it.
667-
*/
668-
describe("the language field", () => {
669-
it("fails the whole scan on a spelling it does not recognize", () => {
670-
// `C#` is the plausible wrong spelling of `CSharp`, and getting it wrong
671-
// is not a rule that quietly matches nothing: ast-grep cannot parse the
672-
// config, so every OTHER rule in the project goes unreported too. The
673-
// error names the enum, which is what an author sees.
674-
const result = scan(
675-
project({ rules: { "no-eval": atLanguage("C#") }, sources: evalSource })
676-
);
677-
expect(result.status).toBeGreaterThan(1);
678-
expect(result.stderr).toContain("SgLang");
679-
});
680-
681-
it("treats Tsx and TypeScript as different parsers, not aliases", () => {
682-
// The quiet half of the same field, and the reason the recipe names this
683-
// pair specifically. `TypeScript` over a `.tsx` tree exits clean with no
684-
// findings, which is indistinguishable from a codebase with nothing to
685-
// flag — the rule looks written and proves nothing.
686-
const sources = { "src/a.tsx": "const el = <div>{eval(x)}</div>;\n" };
687-
const asTypeScript = scan(
688-
project({ rules: { "no-eval": atLanguage("TypeScript") }, sources })
689-
);
690-
expect(asTypeScript.status).toBe(0);
691-
expect(asTypeScript.stdout.trim()).toBe("");
692-
expect(
693-
scan(project({ rules: { "no-eval": atLanguage("Tsx") }, sources }))
694-
.stdout
695-
).toContain("eval(x)");
696-
});
697-
});
698-
699658
/**
700659
* Relocated from `engine-layout.test.ts`, which existed only for these two.
701660
*

‎packages/cli/test/verify.test.ts‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -461,6 +461,31 @@ describe("verifyRule", () => {
461461
expect(result.tests.fixtures).toBe("both");
462462
expect(result.tests.valid).toBe(true);
463463
});
464+
465+
it("ignores a file in the rule's directory whose own id is another rule", async () => {
466+
// The filename says `no-eval`, the `id:` inside says otherwise — the
467+
// shape a draft copied from another rule arrives in. `sg test --filter
468+
// ^no-eval$` resolves cases against that `id:`, so these fixtures never
469+
// run; counting them would report coverage the rule never earned.
470+
await coverageProject(temporaryDirectory, { valid: ["const x = 1;"] });
471+
await writeFile(
472+
join(
473+
temporaryDirectory,
474+
".taskless",
475+
"sg",
476+
"rule-tests",
477+
"no-eval-20260331-test.yml"
478+
),
479+
stringify({
480+
id: "no-alert-scratch",
481+
invalid: ["eval('alert(1)')"],
482+
}),
483+
"utf8"
484+
);
485+
const result = await verifyRule(temporaryDirectory, "no-eval");
486+
expect(result.tests.fixtures).toBe("valid-only");
487+
expect(result.tests.valid).toBe(false);
488+
});
464489
});
465490
});
466491

0 commit comments

Comments
 (0)