mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
fd978f302d
Fixes #1076 **Cause** - suites that redirect `os.tmpdir()` for a whole file hoist the restorer, and the scratch directory it points at, into uninitialized `let`s that `beforeEach` assigns partway through. Vitest still runs `afterEach` when `beforeEach` throws, so teardown calls `undefined()` and hands `undefined` to `fs.rm` - bogus `TypeError`s reported beside the real failure, and a reader chases the wrong one. **Fix** - initialize what the teardown reads (`= () => {}` / `= ""`; `fs.rm("", { force: true })` is a no-op), plus a guard in `tmpdir-env.test.ts` that scans the suite for the shape so a new `redirectTmpdir` caller cannot reintroduce it. **Verified** - injected a throw ahead of the assignment in each affected `beforeEach`: before, the teardown `TypeError` rode alongside the injected error; after, only the injected error is reported. The guard lists all 9 declarations against `origin/main` and passes with the fix. `npx tsc --build`, `typecheck:tests -w @argent/tool-server`, and the full package suite (433 files, 6164 tests) are green. Docs: none - test-only change, no tool, CLI, config key, flow file or user-facing capability touched. <details> <summary>Repro, before and after</summary> `packages/tool-server/test/react-profiler/dump.test.ts`, throw injected as the first statement of `beforeEach`: Before: ``` ⎯⎯⎯⎯⎯⎯ Failed Tests 17 ⎯⎯⎯⎯⎯⎯⎯ Error: SIMULATED mkdtemp failure before assignment TypeError: restoreTmpdir is not a function ``` With only the restorer initialized - the second half of the same hook still throws: ``` Error: SIMULATED mkdtemp failure before assignment TypeError: The "path" argument must be of type string or an instance of Buffer or URL. Received undefined ``` After (same for `screen-recording`, `profiler-load-list-e2e`, `native-profiler-freshness-e2e`, `flow-visual`): ``` Error: SIMULATED failure before assignment ``` </details> <details> <summary>Guard against origin/main</summary> ``` - [] + [ + "android-perfetto/native-profiler-freshness-e2e.test.ts: let tempDir: string;", + "android-perfetto/native-profiler-freshness-e2e.test.ts: let restoreTmpdir: () => void;", + "android-perfetto/profiler-load-list-e2e.test.ts: let scratch: string;", + "android-perfetto/profiler-load-list-e2e.test.ts: let restoreTmpdir: () => void;", + "flows/flow-visual.test.ts: let tmpDir: string;", + "react-profiler/dump.test.ts: let restoreTmpdir: () => void;", + "react-profiler/dump.test.ts: let scratch: string;", + "screen-recording.test.ts: let restoreTmpdir: () => void;", + "screen-recording.test.ts: let scratch: string;", + ] ``` </details> <details> <summary>Class check - what was left alone</summary> - `packages/telemetry/test/index.test.ts:89` `let restoreOptOut: () => void;` - same shape, no window: the assignment is the first statement of the file's first `beforeEach`, so nothing can throw ahead of it. - Roughly 90 other hoisted `let`s across tool-server tests are read in `afterEach` without an initializer (`tmpDir`, `handle`, `exitSpy`, ...) in suites that never redirect the tmpdir. That is a repo-wide pattern for a lint rule, not for this fix; the guard is scoped to the `redirectTmpdir` suites the issue reports. - Local `npx eslint` on the changed files could not run: `packages/docs/tsconfig.json` fails to resolve `@docusaurus/tsconfig` because the root install skips that workspace. Pre-existing and unrelated; CI's ESLint job is green on this branch. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
119 lines
4.1 KiB
TypeScript
119 lines
4.1 KiB
TypeScript
import { describe, it, expect, afterEach } from "vitest";
|
|
import { readdirSync, readFileSync, statSync } from "node:fs";
|
|
import { join, relative } from "node:path";
|
|
import { redirectTmpdir } from "./helpers/tmpdir-env";
|
|
|
|
// The variable os.tmpdir() consults is platform-dependent — POSIX reads TMPDIR,
|
|
// TMP, TEMP in that order and Windows reads TEMP then TMP — so a redirect that
|
|
// covers only the local platform's name leaves the other one resolving into the
|
|
// machine-wide temp directory. The files that scope the tmpdir to catch a leak
|
|
// then scan a directory nothing writes to and pass whatever the code does.
|
|
const LOOKUPS = {
|
|
posix: ["TMPDIR", "TMP", "TEMP"],
|
|
win32: ["TEMP", "TMP"],
|
|
};
|
|
|
|
describe("redirectTmpdir", () => {
|
|
const saved = new Map<string, string | undefined>();
|
|
for (const k of ["TMPDIR", "TEMP", "TMP"]) saved.set(k, process.env[k]);
|
|
|
|
afterEach(() => {
|
|
for (const [k, v] of saved) {
|
|
if (v === undefined) delete process.env[k];
|
|
else process.env[k] = v;
|
|
}
|
|
});
|
|
|
|
it.each(Object.entries(LOOKUPS))(
|
|
"redirects the whole %s lookup chain, not just its first entry",
|
|
(_platform, chain) => {
|
|
const restore = redirectTmpdir("/scratch/owned-by-this-run");
|
|
try {
|
|
for (const key of chain) {
|
|
expect(process.env[key]).toBe("/scratch/owned-by-this-run");
|
|
}
|
|
} finally {
|
|
restore();
|
|
}
|
|
}
|
|
);
|
|
|
|
it("restores an absent variable by deleting it rather than setting undefined", () => {
|
|
delete process.env.TEMP;
|
|
process.env.TMPDIR = "/before";
|
|
|
|
redirectTmpdir("/scratch")();
|
|
|
|
expect("TEMP" in process.env).toBe(false);
|
|
expect(process.env.TMPDIR).toBe("/before");
|
|
});
|
|
});
|
|
|
|
/**
|
|
* These suites call the restorer from `afterEach` unconditionally, and Vitest
|
|
* runs `afterEach` even when `beforeEach` threw. Whatever the teardown reads
|
|
* that the assigning hook never got to is then `undefined`, so the teardown
|
|
* throws a second TypeError beside the genuine failure - the one a reader
|
|
* chases first.
|
|
*/
|
|
|
|
const TEST_ROOT = __dirname;
|
|
|
|
function testFiles(dir: string, out: string[] = []): string[] {
|
|
for (const entry of readdirSync(dir)) {
|
|
const p = join(dir, entry);
|
|
if (statSync(p).isDirectory()) testFiles(p, out);
|
|
else if (p.endsWith(".test.ts")) out.push(p);
|
|
}
|
|
return out;
|
|
}
|
|
|
|
/** Body of every `name(...)` call in `src`, matched by balancing parentheses. */
|
|
function callBodies(src: string, name: string): string {
|
|
const bodies: string[] = [];
|
|
const calls = new RegExp(String.raw`\b${name}\s*\(`, "g");
|
|
for (let call = calls.exec(src); call; call = calls.exec(src)) {
|
|
const start = call.index + call[0].length;
|
|
let depth = 1;
|
|
let i = start;
|
|
for (; i < src.length && depth > 0; i++) {
|
|
if (src[i] === "(") depth++;
|
|
else if (src[i] === ")") depth--;
|
|
}
|
|
bodies.push(src.slice(start, i - 1));
|
|
}
|
|
return bodies.join("\n");
|
|
}
|
|
|
|
/** Hoisted `let`/`var` declarations carrying no initializer - `=>` is not one. */
|
|
function uninitialized(src: string): Map<string, string> {
|
|
const found = new Map<string, string>();
|
|
for (const [decl, name] of src.matchAll(/^[ \t]*(?:let|var)[ \t]+(\w+)[^;\n]*;$/gm)) {
|
|
if (!decl.replace(/=>/g, "").includes("=")) found.set(name, decl.trim());
|
|
}
|
|
return found;
|
|
}
|
|
|
|
const RESTORER = /(?:(const|let|var)[ \t]+)?\w+[ \t]*=[ \t]*redirectTmpdir\(/g;
|
|
|
|
describe("suites that redirect the tmpdir for a whole file", () => {
|
|
it("initialize every hoisted variable their teardown reads", () => {
|
|
const offenders: string[] = [];
|
|
let scanned = 0;
|
|
|
|
for (const file of testFiles(TEST_ROOT)) {
|
|
const src = readFileSync(file, "utf8");
|
|
// A block-scoped restorer cannot outlive the block that assigned it.
|
|
if (![...src.matchAll(RESTORER)].some((m) => !m[1])) continue;
|
|
scanned++;
|
|
const teardown = callBodies(src, "afterEach");
|
|
for (const [name, decl] of uninitialized(src))
|
|
if (new RegExp(String.raw`\b${name}\b`).test(teardown))
|
|
offenders.push(`${relative(TEST_ROOT, file)}: ${decl}`);
|
|
}
|
|
|
|
expect(scanned).toBeGreaterThan(0);
|
|
expect(offenders).toEqual([]);
|
|
});
|
|
});
|