diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fc8c0ad5..6b2f3564 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -32,6 +32,14 @@ jobs: - name: Build run: npm run build + - name: Type-check changed tests + if: github.event_name == 'pull_request' + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: | + git fetch --no-tags --depth=1 origin "$BASE_SHA" + node scripts/typecheck-changed-tests.mjs "$BASE_SHA" + - name: Sync advisory database (required by e2e tests using --offline) run: node dist/index.js advisories sync diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d1fe12f9..b42f8b50 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -34,6 +34,7 @@ diagnosed a problem in the release notes, not only the author of the merged PR. ``` `advisories sync` populates the local advisory database. E2E tests that scan with `--offline` read from it. Without the sync step, those tests fail while the rest of the suite passes. +- CI also type-checks the test files a pull request adds or changes. Run it locally with `node scripts/typecheck-changed-tests.mjs ` (for example, `upstream/main`) before opening the PR. - Keep changes tightly scoped to the issue. Do not refactor unrelated code in the same PR. - New utility functions belong in a focused module under `src/utils/` - not inlined in `src/index.ts`. - Use `node:fs`, `node:path`, etc. for Node built-in imports. diff --git a/scripts/typecheck-changed-tests.mjs b/scripts/typecheck-changed-tests.mjs new file mode 100644 index 00000000..e64c2b86 --- /dev/null +++ b/scripts/typecheck-changed-tests.mjs @@ -0,0 +1,168 @@ +#!/usr/bin/env node +/** + * Type-check the test files a pull request adds or changes. + * + * Jest transpiles tests without type-checking them, so a test can assert a + * shape the compiler would reject. This gate type-checks changed test files + * only: errors attached to unchanged files are ignored so a PR is not blocked + * by existing debt, while configuration and compiler failures always fail. + * + * Usage: node scripts/typecheck-changed-tests.mjs [--root ] + */ + +import { execFileSync } from "node:child_process"; +import { dirname, resolve } from "node:path"; +import { fileURLToPath } from "node:url"; +import ts from "typescript"; + +const defaultRoot = resolve(dirname(fileURLToPath(import.meta.url)), ".."); + +/** + * List the test files the branch added, copied, modified or renamed. + * + * `-z` keeps Git from quoting paths, so filenames with spaces, Unicode or + * newlines survive intact. Splitting on NUL is the only safe delimiter here. + */ +function changedTestFiles(baseRef, root = defaultRoot) { + const output = execFileSync( + "git", + ["diff", "-z", "--name-only", "--diff-filter=ACMR", baseRef, "HEAD", "--", "tests"], + { cwd: root, encoding: "utf8" }, + ); + + return output + .split("\0") + .filter(Boolean) + .filter((file) => file.endsWith(".ts")) + .sort(); +} + +function formatDiagnostic(diagnostic) { + const message = ts.flattenDiagnosticMessageText(diagnostic.messageText, "\n"); + if (!diagnostic.file) { + return `error TS${diagnostic.code}: ${message}`; + } + const position = diagnostic.file.getLineAndCharacterOfPosition(diagnostic.start ?? 0); + return `${diagnostic.file.fileName}(${position.line + 1},${position.character + 1}): error TS${diagnostic.code}: ${message}`; +} + +function typeCheckChangedTests(baseRef, root = defaultRoot) { + const files = changedTestFiles(baseRef, root); + if (files.length === 0) { + return { skipped: true, changedFiles: [], failures: [], global: [], ignored: 0 }; + } + + const configPath = resolve(root, "tsconfig.test.json"); + const config = ts.readConfigFile(configPath, ts.sys.readFile); + if (config.error) { + return { skipped: false, changedFiles: files, failures: [], global: [config.error], ignored: 0 }; + } + + const parsed = ts.parseJsonConfigFileContent(config.config, ts.sys, root, undefined, configPath); + if (parsed.errors.length > 0) { + return { skipped: false, changedFiles: files, failures: [], global: parsed.errors, ignored: 0 }; + } + + const changed = new Set(files.map((file) => resolve(root, file))); + const options = { ...parsed.options, noEmit: true }; + + let diagnostics; + try { + const program = ts.createProgram({ + rootNames: [...changed], + options, + projectReferences: parsed.projectReferences, + }); + diagnostics = ts.getPreEmitDiagnostics(program); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + return { + skipped: false, + changedFiles: files, + failures: [], + global: [{ code: 0, category: ts.DiagnosticCategory.Error, messageText: message }], + ignored: 0, + }; + } + + const failures = []; + const global = []; + let ignored = 0; + + for (const diagnostic of diagnostics) { + if (diagnostic.category !== ts.DiagnosticCategory.Error) continue; + + if (!diagnostic.file) { + global.push(diagnostic); + continue; + } + + if (changed.has(resolve(root, diagnostic.file.fileName))) { + failures.push(diagnostic); + } else { + ignored++; + } + } + + return { skipped: false, changedFiles: files, failures, global, ignored }; +} + +function parseArgs(argv) { + let baseRef; + let root = defaultRoot; + + for (let index = 0; index < argv.length; index++) { + const arg = argv[index]; + if (arg === "--root") { + const value = argv[++index]; + if (!value) throw new Error("--root requires a path"); + root = resolve(value); + } else if (!baseRef) { + baseRef = arg; + } + } + + return { baseRef, root }; +} + +function main() { + let args; + try { + args = parseArgs(process.argv.slice(2)); + } catch (error) { + console.error(error instanceof Error ? error.message : String(error)); + process.exit(2); + } + + if (!args.baseRef) { + console.error("usage: node scripts/typecheck-changed-tests.mjs [--root ]"); + process.exit(2); + } + + const result = typeCheckChangedTests(args.baseRef, args.root); + + if (result.skipped) { + console.log("No changed test files; skipping test-tree type-check."); + process.exit(0); + } + + const diagnostics = [...result.failures, ...result.global]; + + if (diagnostics.length > 0) { + console.error(`Test type-check failed (${diagnostics.length}):\n`); + for (const diagnostic of diagnostics) console.error(formatDiagnostic(diagnostic)); + if (result.ignored > 0) { + console.error( + `\nIgnored ${result.ignored} error(s) in unchanged files; those belong to the existing backlog.`, + ); + } + process.exit(1); + } + + console.log( + `Type-checked ${result.changedFiles.length} changed test file(s): no errors.` + + (result.ignored > 0 ? ` Ignored ${result.ignored} error(s) in unchanged files.` : ""), + ); +} + +main(); diff --git a/tests/scripts/typecheck-changed-tests.test.ts b/tests/scripts/typecheck-changed-tests.test.ts new file mode 100644 index 00000000..fd0a967f --- /dev/null +++ b/tests/scripts/typecheck-changed-tests.test.ts @@ -0,0 +1,168 @@ +import { execFileSync, spawnSync } from "node:child_process"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { fileURLToPath } from "node:url"; + +/** + * The gate must see the exact files Git reports. Filenames are the one place + * where a naive newline split breaks: spaces survive trim(), but a Unicode + * path or a path with an embedded newline does not. These tests run the real + * script against throwaway repositories so the git plumbing is exercised too. + */ + +const script = fileURLToPath(new URL("../../scripts/typecheck-changed-tests.mjs", import.meta.url)); +const roots: string[] = []; + +function git(cwd: string, ...args: string[]): void { + execFileSync("git", args, { + cwd, + stdio: "ignore", + env: { + ...process.env, + GIT_AUTHOR_NAME: "test", + GIT_AUTHOR_EMAIL: "test@example.com", + GIT_COMMITTER_NAME: "test", + GIT_COMMITTER_EMAIL: "test@example.com", + }, + }); +} + +function commit(cwd: string, message: string): void { + git(cwd, "add", "-A"); + git(cwd, "commit", "-q", "-m", message); +} + +function write(root: string, relativePath: string, contents: string): void { + const fullPath = join(root, relativePath); + mkdirSync(join(fullPath, ".."), { recursive: true }); + writeFileSync(fullPath, contents, "utf8"); +} + +function makeRepo(): string { + const root = mkdtempSync(join(tmpdir(), "cve-lite-typecheck-")); + roots.push(root); + git(root, "init", "-q"); + write( + root, + "tsconfig.test.json", + JSON.stringify( + { + compilerOptions: { + target: "ES2022", + module: "NodeNext", + moduleResolution: "NodeNext", + strict: true, + noEmit: true, + skipLibCheck: true, + types: [], + }, + include: ["tests/**/*.ts"], + }, + null, + 2, + ), + ); + mkdirSync(join(root, "tests"), { recursive: true }); + return root; +} + +function runGate(root: string, base: string) { + return spawnSync(process.execPath, [script, base, "--root", root], { encoding: "utf8" }); +} + +afterEach(() => { + for (const root of roots.splice(0)) { + rmSync(root, { recursive: true, force: true }); + } +}); + +describe("typecheck-changed-tests", () => { + it("fails a normal changed file with a type error", () => { + const root = makeRepo(); + commit(root, "base"); + write(root, "tests/broken.test.ts", 'const broken: number = "not a number";\n'); + commit(root, "head"); + + const result = runGate(root, "HEAD~1"); + + expect(result.status).toBe(1); + expect(result.stderr).toContain("tests/broken.test.ts"); + expect(result.stderr).toContain("error TS2322"); + }); + + it("fails a Unicode filename with a type error", () => { + const root = makeRepo(); + commit(root, "base"); + write(root, "tests/café-测试.test.ts", 'const broken: number = "not a number";\n'); + commit(root, "head"); + + const result = runGate(root, "HEAD~1"); + + expect(result.status).toBe(1); + expect(result.stderr).toContain("café-测试.test.ts"); + expect(result.stderr).toContain("error TS2322"); + }); + + it("fails a filename containing spaces even when an imported unchanged file also has errors", () => { + const root = makeRepo(); + write(root, "tests/shared-helper.ts", 'export const shared: number = "debt";\n'); + commit(root, "base"); + write( + root, + "tests/space name.test.ts", + 'import { shared } from "./shared-helper.js";\nconst broken: number = "not a number";\nexport const value = shared + broken;\n', + ); + commit(root, "head"); + + const result = runGate(root, "HEAD~1"); + + expect(result.status).toBe(1); + expect(result.stderr).toContain("space name.test.ts"); + expect(result.stderr).toContain("Ignored 1 error"); + }); + + it("passes a valid changed file despite unchanged-file debt", () => { + const root = makeRepo(); + write(root, "tests/legacy-debt.ts", 'export const legacy: number = "debt";\n'); + commit(root, "base"); + write( + root, + "tests/valid.test.ts", + 'import { legacy } from "./legacy-debt.js";\nexport const value: number = legacy;\n', + ); + commit(root, "head"); + + const result = runGate(root, "HEAD~1"); + + expect(result.status).toBe(0); + expect(result.stdout).toContain("Type-checked 1 changed test file(s): no errors."); + expect(result.stdout).toContain("Ignored 1 error"); + }); + + it("skips successfully when no test files changed", () => { + const root = makeRepo(); + commit(root, "base"); + + const result = runGate(root, "HEAD"); + + expect(result.status).toBe(0); + expect(result.stdout).toContain("No changed test files; skipping test-tree type-check."); + }); +}); + +describe("typecheck-changed-tests configuration failures", () => { + it("fails when the test TypeScript config cannot be parsed", () => { + const root = makeRepo(); + commit(root, "base"); + write(root, "tsconfig.test.json", "{ this is not json"); + write(root, "tests/valid.test.ts", "export const value = 1;\n"); + commit(root, "head"); + + const result = runGate(root, "HEAD~1"); + + expect(result.status).toBe(1); + expect(result.stderr).toContain("Test type-check failed"); + expect(result.stderr).toContain("error TS"); + }); +});