mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-09 14:38:59 +02:00
fix(test): prepush hook test builds PATH with a POSIX-only separator
`test/redact-prepush-hook.test.ts` shadows `git` with a stub by prepending a
temp dir to PATH, built as `${stubDir}:${process.env.PATH}`. On Windows the
separator is `;`, so that produces one unparseable entry, the stub is never
found, and the REAL git runs — the diff succeeds, `gitStrict` never throws, and
the hook exits 0 where the test expects 1. It fails as a wrong assertion rather
than as a portability problem, which is what made it hard to place.
Replace it with a `prependPath` helper mirroring the one already in
test/gstack-brain-context-load.test.ts, which handles both platform details:
`path.delimiter`, and a case-insensitive lookup of the existing env key —
Windows commonly spells it `Path`, and adding a second `PATH` alongside an
inherited `Path` leaves the winner up to the spawn implementation.
On POSIX the helper resolves to `{ PATH: binDir + ":" + process.env.PATH }`,
byte-identical to the expression it replaces, so behaviour there is unchanged.
Fixing the separator alone does not make the test pass on Windows, and it
cannot: the premise is that a signal-killed child yields `spawnSync`
status === null, and Windows has no equivalent (a force-killed process reports
a non-zero exit code). The stub is also a `#!/bin/sh` file named `git`, which
Windows will not execute, since process creation resolves through PATHEXT and
ignores the shebang. A Windows variant would assert the non-zero-exit branch
instead — a different branch than the test name claims — so the test is gated
with test.skipIf(process.platform === "win32"), matching
test/session-runner-timeout.test.ts and test/setup-emoji-font.test.ts.
Windows before: 14 pass, 1 fail. After: 14 pass, 1 skip, 0 fail (3 consecutive
runs). Unchanged on POSIX, where it should still run and pass — worth
confirming in CI, since I can only verify the Windows half here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
7ed17bf76d
commit
890fcacde5
@@ -44,6 +44,24 @@ function runHook(
|
||||
return { code: r.status ?? 0, stderr: r.stderr ?? "" };
|
||||
}
|
||||
|
||||
/**
|
||||
* Env override that puts `binDir` first on the search path, for tests that
|
||||
* shadow a real binary with a stub.
|
||||
*
|
||||
* Two portability details, both of which a hardcoded `PATH: "dir:" + ...`
|
||||
* gets wrong (mirrors `prependPath` in test/gstack-brain-context-load.test.ts):
|
||||
* - The separator is `;` on Windows, not `:`. Using the literal produces one
|
||||
* unparseable entry, so the stub is never found and the REAL binary runs —
|
||||
* a test that silently passes through rather than failing loudly.
|
||||
* - Windows env keys are case-insensitive and commonly spelled `Path`. Adding
|
||||
* a second `PATH` key alongside an inherited `Path` leaves which one wins up
|
||||
* to the spawn implementation, so reuse whichever key already exists.
|
||||
*/
|
||||
function prependPath(binDir: string): Record<string, string> {
|
||||
const pathKey = Object.keys(process.env).find((k) => k.toLowerCase() === "path") || "PATH";
|
||||
return { [pathKey]: `${binDir}${path.delimiter}${process.env[pathKey] || ""}` };
|
||||
}
|
||||
|
||||
const ZERO = "0000000000000000000000000000000000000000";
|
||||
|
||||
// Assembled at runtime so the LITERAL never appears in a pushed diff — the
|
||||
@@ -147,30 +165,44 @@ describe("fail closed on unscannable diffs (#1946)", () => {
|
||||
expect(stderr).not.toContain("could not compute the pushed diff");
|
||||
});
|
||||
|
||||
test("a diff killed by a signal (null status — the maxBuffer/kill class) BLOCKS", () => {
|
||||
// Stub git: probes delegate to the real git; the diff invocation kills
|
||||
// itself, producing spawnSync status === null. This is the exact branch
|
||||
// gitStrict's docstring names (oversized-diff overflow is delivered the
|
||||
// same way) — pre-landing review flagged it as untested.
|
||||
const realGit = Bun.which("git") || "/usr/bin/git";
|
||||
const stubDir = fs.mkdtempSync(path.join(os.tmpdir(), "prepush-stubgit-"));
|
||||
try {
|
||||
const stub = `#!/bin/sh\nif [ "$1" = "diff" ]; then kill -KILL $$; fi\nexec "${realGit}" "$@"\n`;
|
||||
fs.writeFileSync(path.join(stubDir, "git"), stub);
|
||||
fs.chmodSync(path.join(stubDir, "git"), 0o755);
|
||||
// POSIX-only, for two independent reasons. `spawnSync` reports
|
||||
// `status === null` only when a child dies from a signal, and Windows has no
|
||||
// equivalent — a force-killed process surfaces a non-zero exit code there — so
|
||||
// the branch this test names is unreachable. The stub below is also a
|
||||
// `#!/bin/sh` file named `git`, which Windows will not execute at all, since
|
||||
// process creation resolves commands through PATHEXT (.exe/.cmd/.bat) and
|
||||
// ignores the shebang. A Windows variant would have to assert the non-zero
|
||||
// exit path instead, i.e. a different branch than the name claims, so it is
|
||||
// skipped rather than rewritten. Gate style follows
|
||||
// test/session-runner-timeout.test.ts and test/setup-emoji-font.test.ts.
|
||||
test.skipIf(process.platform === "win32")(
|
||||
"a diff killed by a signal (null status — the maxBuffer/kill class) BLOCKS",
|
||||
() => {
|
||||
// Stub git: probes delegate to the real git; the diff invocation kills
|
||||
// itself, producing spawnSync status === null. This is the exact branch
|
||||
// gitStrict's docstring names (oversized-diff overflow is delivered the
|
||||
// same way) — pre-landing review flagged it as untested.
|
||||
const realGit = Bun.which("git") || "/usr/bin/git";
|
||||
const stubDir = fs.mkdtempSync(path.join(os.tmpdir(), "prepush-stubgit-"));
|
||||
try {
|
||||
const stub = `#!/bin/sh\nif [ "$1" = "diff" ]; then kill -KILL $$; fi\nexec "${realGit}" "$@"\n`;
|
||||
fs.writeFileSync(path.join(stubDir, "git"), stub);
|
||||
fs.chmodSync(path.join(stubDir, "git"), 0o755);
|
||||
|
||||
const base = git(["rev-parse", "HEAD"]);
|
||||
const head = commit("clean.txt", "clean content\n", "clean commit");
|
||||
const { code, stderr } = runHook(`refs/heads/main ${head} refs/heads/main ${base}\n`, {
|
||||
PATH: `${stubDir}:${process.env.PATH}`,
|
||||
});
|
||||
expect(code).toBe(1);
|
||||
expect(stderr).toContain("could not compute the pushed diff");
|
||||
expect(stderr).toContain("GSTACK_REDACT_PREPUSH=skip");
|
||||
} finally {
|
||||
fs.rmSync(stubDir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
const base = git(["rev-parse", "HEAD"]);
|
||||
const head = commit("clean.txt", "clean content\n", "clean commit");
|
||||
const { code, stderr } = runHook(
|
||||
`refs/heads/main ${head} refs/heads/main ${base}\n`,
|
||||
prependPath(stubDir),
|
||||
);
|
||||
expect(code).toBe(1);
|
||||
expect(stderr).toContain("could not compute the pushed diff");
|
||||
expect(stderr).toContain("GSTACK_REDACT_PREPUSH=skip");
|
||||
} finally {
|
||||
fs.rmSync(stubDir, { recursive: true, force: true });
|
||||
}
|
||||
},
|
||||
);
|
||||
});
|
||||
|
||||
describe("install UX surfaces (#1946 / eng review D3+D10)", () => {
|
||||
|
||||
Reference in New Issue
Block a user