diff --git a/.changeset/sg-rule-tests-directory.md b/.changeset/sg-rule-tests-directory.md new file mode 100644 index 00000000..238f55a1 --- /dev/null +++ b/.changeset/sg-rule-tests-directory.md @@ -0,0 +1,9 @@ +--- +"@taskless/cli": patch +--- + +Stop a rule with no tests from failing every other rule's ast-grep test run. + +Migration `0005` created a rule's `.tests/` only as a side effect of moving a test file into it, so an ast-grep rule that had no test at version 3 — or one whose test file did not match the `-YYYYMMDD-test.yml` shape the migration can attribute to a rule — arrived in the new layout with no tests directory at all. Assembly then named that directory as a `testConfigs` entry anyway, and ast-grep 0.41.0 treats a `testDir` it cannot read as fatal to the whole invocation rather than to the one rule: `taskless test` on _any_ rule died with `Cannot read rule directory .taskless/rules/sg//.tests` and exit 6, naming a rule the author had never touched. `--filter` does not scope that away, so there was no way to run one rule's tests around it. + +`0005` now gives every `rules/sg//` a `.tests/`, holding a committed `.gitkeep` when it would otherwise be empty — git does not track empty directories, so without one the repair would not survive a commit and the failure would come back in CI. Assembly separately omits any `testDir` that is not on disk, which is what rescues a project a nightly already stamped at version 5: migrations short-circuit once the manifest is at the latest version, so those installs never re-run the amended `0005`, and the same state is reachable at any version by creating a rule directory by hand. Neither change turns a missing test into a pass — `verify` still reports "No test file found" and `test` still reports "Skipped: no test file found", both reading the rule directory rather than the generated config. diff --git a/packages/cli/src/filesystem/migrations/0005-rule-directories.ts b/packages/cli/src/filesystem/migrations/0005-rule-directories.ts index 47340a66..2a10602c 100644 --- a/packages/cli/src/filesystem/migrations/0005-rule-directories.ts +++ b/packages/cli/src/filesystem/migrations/0005-rule-directories.ts @@ -177,6 +177,15 @@ async function moveEngineTests( } } +/** Create `path` and drop a `.gitkeep` in it when it would otherwise be empty. */ +async function ensureTrackedDirectory(path: string): Promise { + await mkdir(path, { recursive: true }); + const entries = await entriesOf(path); + if (entries.length === 0) { + await writeFile(join(path, ".gitkeep"), "", "utf8"); + } +} + /** * Create `rules//` for every engine, tracked when empty. * @@ -188,12 +197,42 @@ async function moveEngineTests( */ async function scaffoldEngineDirectories(directory: string): Promise { for (const engine of ENGINES) { - const path = join(directory, RULES_DIRECTORY, engine); - await mkdir(path, { recursive: true }); - const entries = await entriesOf(path); - if (entries.length === 0) { - await writeFile(join(path, ".gitkeep"), "", "utf8"); - } + await ensureTrackedDirectory(join(directory, RULES_DIRECTORY, engine)); + } +} + +/** + * Give every ast-grep rule a `.tests/`, tracked when it holds nothing else. + * + * Up to here `.tests/` only ever appeared as a side effect of moving a test + * *into* it, so a rule that had no test at version 3 — or one whose test file + * did not match the `-YYYYMMDD-test.yml` shape `moveEngineTests` can + * attribute — arrived at version 5 with no tests directory at all. + * + * **That is not a cosmetic gap.** Assembly names every rule's `.tests/` as a + * `testConfigs` entry, and ast-grep 0.41.0 aborts the entire invocation when + * one of them is missing (`Cannot read rule directory ...`, exit 6) — which + * `--filter` does not scope away. One rule with no tests therefore failed + * `taskless test` for every *other* rule in the project, with an error naming a + * rule the author had never touched. Measured against 0.41.0: an empty + * `.tests/`, and one holding only a `.gitkeep`, are both accepted by `sg test` + * and `sg scan`. + * + * The `.gitkeep` is what makes the repair survive a commit. Git does not track + * empty directories, so a `.tests/` created here and left empty would never + * reach CI or a fresh clone, and the failure would come back there. It is + * committed rather than ignored — `.taskless/.gitignore` carries only the two + * generated configs — and never reads as a test: `verify` counts a test file + * only when it matches `-*-test.yml`, so such a rule still reports "No test + * file found" rather than quietly passing. + */ +async function ensureSgTestDirectories(directory: string): Promise { + const engineRoot = join(directory, RULES_DIRECTORY, "sg"); + for (const entry of await entriesOf(engineRoot)) { + if (!entry.isDirectory()) continue; + await ensureTrackedDirectory( + join(engineRoot, entry.name, RULE_TESTS_DIRECTORY) + ); } } @@ -370,6 +409,7 @@ const migration: Migration = async (directory) => { await rm(join(directory, config), { force: true }); } + await ensureSgTestDirectories(directory); await scaffoldEngineDirectories(directory); await ignoreGeneratedConfigs(directory); diff --git a/packages/cli/src/rules/assemble.ts b/packages/cli/src/rules/assemble.ts index 0334b010..36f79448 100644 --- a/packages/cli/src/rules/assemble.ts +++ b/packages/cli/src/rules/assemble.ts @@ -1,4 +1,4 @@ -import { mkdir, readFile, writeFile } from "node:fs/promises"; +import { mkdir, readFile, stat, writeFile } from "node:fs/promises"; import { dirname, join, posix, relative, sep } from "node:path"; import { @@ -41,6 +41,16 @@ function toPosix(path: string): string { return path.split(sep).join(posix.sep); } +/** Whether `path` is a directory on disk. */ +async function isDirectory(path: string): Promise { + try { + const stats = await stat(path); + return stats.isDirectory(); + } catch { + return false; + } +} + /** * The header every assembled Vale config carries. * @@ -137,6 +147,19 @@ export async function assembleValeConfig( * * `testConfigs` gets one entry per rule, because each rule keeps its tests * inside its own directory. Sorted with the rule ids, so the file is stable. + * + * A rule whose `.tests/` is not on disk is left out of `testConfigs` + * entirely. **ast-grep 0.41.0 treats a missing `testDir` as fatal to the whole + * invocation** — `Cannot read rule directory ...`, exit 6 — and `--filter` does + * not scope that away, so emitting the entry regardless would let one rule fail + * every other rule's test run, with an error naming a rule its author never + * touched. Migration `0005` now creates the directory, but that does not make + * this check redundant: `runMigrations` short-circuits once the manifest reads + * version 5, so a project a nightly already stamped never re-runs the amended + * migration, and a hand-made `mkdir .taskless/rules/sg//` reaches the same + * state on any version. Nothing becomes a silent pass — `verify` still reports + * "No test file found" and `test` still reports "Skipped: no test file found", + * both of which read the rule directory rather than this config. */ export async function assembleSgConfig( cwd: string @@ -145,9 +168,15 @@ export async function assembleSgConfig( if (ruleIds.length === 0) return undefined; const rulesDirectory = tasklessRelative(RULES_DIRECTORY, "sg"); - const testDirectories = ruleIds.map((ruleId) => - toPosix(relative(join(cwd, ".taskless"), ruleTestsDirectory(cwd, "sg", ruleId))) + const candidates = ruleIds.map((ruleId) => + ruleTestsDirectory(cwd, "sg", ruleId) + ); + const present = await Promise.all( + candidates.map((path) => isDirectory(path)) ); + const testDirectories = candidates + .filter((_, index) => present[index]) + .map((path) => toPosix(relative(join(cwd, ".taskless"), path))); const contents = [ "ruleDirs:", diff --git a/packages/cli/test/assemble.test.ts b/packages/cli/test/assemble.test.ts index e1b13c71..07ed537a 100644 --- a/packages/cli/test/assemble.test.ts +++ b/packages/cli/test/assemble.test.ts @@ -1,12 +1,16 @@ -import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { + mkdir, + mkdtemp, + readFile, + rm, + stat, + writeFile, +} from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; -import { - assembleSgConfig, - assembleValeConfig, -} from "../src/rules/assemble"; +import { assembleSgConfig, assembleValeConfig } from "../src/rules/assemble"; import { ruleDirectory, ruleTestsDirectory } from "../src/rules/engines"; let cwd: string; @@ -32,13 +36,23 @@ async function valeRule(id: string, config: string): Promise { /** Lay down an ast-grep rule with a test file. */ async function sgRule(id: string): Promise { + await sgRuleWithoutTests(id); + await mkdir(ruleTestsDirectory(cwd, "sg", id), { recursive: true }); +} + +/** + * Lay down an ast-grep rule that has no `.tests/` at all. + * + * The state a nightly-migrated project is in, and the state a hand-made + * `mkdir .taskless/rules/sg//` reaches on any version. + */ +async function sgRuleWithoutTests(id: string): Promise { const directory = ruleDirectory(cwd, "sg", id); await mkdir(directory, { recursive: true }); await writeFile( join(directory, `${id}.yml`), `id: ${id}\nlanguage: TypeScript\nseverity: error\nmessage: x\nrule:\n pattern: eval($A)\n` ); - await mkdir(ruleTestsDirectory(cwd, "sg", id), { recursive: true }); } describe("Vale config assembly", () => { @@ -150,4 +164,51 @@ describe("ast-grep config assembly", () => { it("writes nothing when there are no ast-grep rules", async () => { expect(await assembleSgConfig(cwd)).toBeUndefined(); }); + + // ast-grep 0.41.0 aborts the whole invocation on a `testDir` it cannot read + // (exit 6), and `--filter` does not scope that away — so a single rule with + // no `.tests/` would fail every *other* rule's test run, naming a rule its + // author never touched. + it("omits a testDir whose directory is not on disk", async () => { + await sgRule("has-tests"); + await sgRuleWithoutTests("no-tests"); + + const path = await assembleSgConfig(cwd); + const contents = await readFile(join(cwd, path ?? ""), "utf8"); + + expect(contents).toContain("- testDir: rules/sg/has-tests/.tests"); + expect(contents).not.toContain("no-tests"); + }); + + it("emits no testDir that is missing from disk, for any rule", async () => { + await sgRule("alpha"); + await sgRuleWithoutTests("beta"); + await sgRule("gamma"); + + const path = await assembleSgConfig(cwd); + const contents = await readFile(join(cwd, path ?? ""), "utf8"); + + const prefix = " - testDir: "; + const emitted = contents + .split("\n") + .filter((line) => line.startsWith(prefix)) + .map((line) => line.slice(prefix.length)); + expect(emitted.length).toBeGreaterThan(0); + for (const directory of emitted) { + const stats = await stat(join(cwd, ".taskless", directory)); + expect(stats.isDirectory(), `${directory} is a directory`).toBe(true); + } + }); + + // `testConfigs:` with no entries under it is accepted by ast-grep 0.41.0 — + // measured for both `sg test` and `sg scan` — so a project where no rule has + // tests yet still gets a config both commands can read. + it("still emits a config when no rule has a tests directory", async () => { + await sgRuleWithoutTests("only-rule"); + + const path = await assembleSgConfig(cwd); + const contents = await readFile(join(cwd, path ?? ""), "utf8"); + + expect(contents).toBe("ruleDirs:\n - rules/sg\ntestConfigs:\n"); + }); }); diff --git a/packages/cli/test/migrate-engine-layout.test.ts b/packages/cli/test/migrate-engine-layout.test.ts index e141374a..f7afc1cc 100644 --- a/packages/cli/test/migrate-engine-layout.test.ts +++ b/packages/cli/test/migrate-engine-layout.test.ts @@ -2,6 +2,7 @@ import { createHash } from "node:crypto"; import { mkdir, mkdtemp, + readdir, readFile, rm, stat, @@ -314,6 +315,62 @@ describe("migrations 0004 + 0005 — one directory per rule", () => { ).not.toContain("sgconfig.yml"); }); + // Assembly names every rule's `.tests/` as a `testConfigs` entry, and + // ast-grep 0.41.0 aborts the whole invocation on one it cannot read (exit 6, + // `Cannot read rule directory ...`) — which `--filter` does not scope away. + // A migrated rule with no tests directory therefore failed `taskless test` + // for every *other* rule in the project. The fixture above already produces + // one: `no-eval`'s test file carries no timestamp, so 0005 cannot attribute + // it and the rule arrives with nothing moved into it. + it("gives every ast-grep rule a tests directory", async () => { + await seedLegacyLayout(); + + await ensureTasklessDirectory(temporaryDirectory); + + const sgRoot = join(tasklessDirectory, "rules", "sg"); + const entries = await readdir(sgRoot, { withFileTypes: true }); + const ruleIds = entries + .filter((entry) => entry.isDirectory()) + .map((entry) => entry.name); + expect(ruleIds).toContain("no-eval"); + + for (const ruleId of ruleIds) { + const tests = join(sgRoot, ruleId, ".tests"); + expect(await exists(tests), `${ruleId} has a .tests/`).toBe(true); + } + + // Git does not track empty directories, so a `.tests/` created here and + // left empty would never reach CI or a fresh clone and the failure would + // come back there. The `.gitkeep` is committed, not gitignored — + // `.taskless/.gitignore` carries only the two generated configs. + expect(await exists(join(sgRoot, "no-eval", ".tests", ".gitkeep"))).toBe( + true + ); + const gitignore = await readFile( + join(tasklessDirectory, ".gitignore"), + "utf8" + ); + expect(gitignore).not.toContain(".gitkeep"); + }); + + // A `.gitkeep` is not a test. `verify` counts a test file only when it + // matches `-*-test.yml`, so the repaired rule still reports honestly + // rather than turning into a silent pass. + it("does not gitkeep a tests directory that received a real test", async () => { + await seedLegacyLayout(); + await writeTree(tasklessDirectory, { + "rule-tests/no-var-20250101-test.yml": "id: no-var\nvalid:\n - foo()\n", + "rules/no-var.yml": + "id: no-var\nlanguage: typescript\nrule:\n pattern: var $A = $B\n", + }); + + await ensureTasklessDirectory(temporaryDirectory); + + const tests = join(tasklessDirectory, "rules", "sg", "no-var", ".tests"); + expect(await exists(join(tests, "no-var-20250101-test.yml"))).toBe(true); + expect(await exists(join(tests, ".gitkeep"))).toBe(false); + }); + it("is idempotent — a second run changes nothing", async () => { await seedLegacyLayout(); await ensureTasklessDirectory(temporaryDirectory); @@ -322,9 +379,8 @@ describe("migrations 0004 + 0005 — one directory per rule", () => { ); // Re-run 0005 directly (runMigrations would short-circuit on version). - const { default: migration } = await import( - "../src/filesystem/migrations/0005-rule-directories" - ); + const { default: migration } = + await import("../src/filesystem/migrations/0005-rule-directories"); await migration(tasklessDirectory); expect(