Skip to content

Speed up shell leak test #25880

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Jarred-Sumner merged 2 commits into from
Jan 8, 2026
Merged

Speed up shell leak test #25880

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8a803ef
Speed up shell leak test
Jarred-Sumner Jan 8, 2026
e48f099
Update leak.test.ts
Jarred-Sumner Jan 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 60 additions & 41 deletions test/js/bun/shell/leak.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
import { $ } from "bun";
import { heapStats } from "bun:jsc";
import { describe, expect, test } from "bun:test";
import { bunEnv, isPosix, tempDirWithFiles } from "harness";
import { appendFileSync, closeSync, openSync, writeFileSync } from "node:fs";
import { devNull, tmpdir } from "os";
import { bunEnv, isPosix, tempDir, tempDirWithFiles } from "harness";
import { join } from "path";
import { bunExe } from "./test_builder";
import { createTestBuilder } from "./util";
Expand Down Expand Up @@ -53,22 +51,50 @@ const TESTS: [name: string, builder: () => TestBuilder, runs?: number][] = [
],
];

describe("fd leak", () => {
describe.concurrent("fd leak", () => {
function fdLeakTest(name: string, builder: () => TestBuilder, runs: number = 1000, threshold: number = 5) {
test(`fdleak_${name}`, async () => {
Bun.gc(true);
const baseline = openSync(devNull, "r");
closeSync(baseline);
const testcode = await Bun.file(join(import.meta.dirname, "./test_builder.ts")).text();

for (let i = 0; i < runs; i++) {
await builder().quiet().run();
const impl = /* ts */ `
import { openSync, closeSync } from "node:fs";
import { devNull } from "os";
const TestBuilder = createTestBuilder(import.meta.path);

const runs = ${runs};
const threshold = ${threshold};

Bun.gc(true);
const baseline = openSync(devNull, "r");
closeSync(baseline);

for (let i = 0; i < runs; i++) {
await ${builder.toString().slice("() =>".length)}.quiet().run();
}
// Run the GC, because the interpreter closes file descriptors when it
// deinitializes when its finalizer is called
Bun.gc(true);
const fd = openSync(devNull, "r");
closeSync(fd);
if (fd - baseline > threshold) {
console.error('FD leak detected:', fd - baseline, 'leaked (threshold:', threshold, ')');
process.exit(1);
}
`;

using dir = tempDir("fdleak", {
"script.ts": testcode + impl,
});

const { exited, stderr: stream } = Bun.spawn([process.argv0, "--smol", "test", join(dir, "script.ts")], {

@coderabbitai coderabbitai Bot Jan 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider using bunExe() for consistency.

Line 89 uses process.argv0 to reference the Bun executable, while other parts of this file use bunExe() (e.g., lines 371, 408). For consistency and clarity, consider using bunExe() throughout.

Suggested change 
-      const { exited, stderr: stream } = Bun.spawn([process.argv0, "--smol", "test", join(dir, "script.ts")], {
+      const { exited, stderr: stream } = Bun.spawn([bunExe(), "--smol", "test", join(dir, "script.ts")], {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const { exited, stderr: stream } = Bun.spawn([process.argv0, "--smol", "test", join(dir, "script.ts")], {
const { exited, stderr: stream } = Bun.spawn([bunExe(), "--smol", "test", join(dir, "script.ts")], {
🤖 Prompt for AI Agents 
In @test/js/bun/shell/leak.test.ts at line 89, Replace usage of process.argv0 in
the Bun.spawn call with the project's helper bunExe() for consistency; update
the spawn invocation that currently passes process.argv0 (inside the test
invoking Bun.spawn with ["process.argv0", "--smol", "test", join(dir,
"script.ts")]) to call bunExe() instead so it matches other tests that use
bunExe() (see other occurrences around bunExe() at lines 371 and 408) and
preserves existing arguments and options.

env: bunEnv,
stderr: "pipe",
});
const [exitCode, stderr] = await Promise.all([exited, stream.text()]);
if (exitCode != 0) {
console.log("\n\nSTDERR:", stderr);
}
// Run the GC, because the interpreter closes file descriptors when it
// deinitializes when its finalizer is called
Bun.gc(true);
const fd = openSync(devNull, "r");
closeSync(fd);
expect(fd - baseline).toBeLessThanOrEqual(threshold);
expect(exitCode).toBe(0);
}, 100_000);
}

Expand All @@ -79,12 +105,7 @@ describe("fd leak", () => {
threshold: number = DEFAULT_THRESHOLD,
) {
test(`memleak_${name}`, async () => {
const tempfile = join(tmpdir(), "script.ts");

const filepath = import.meta.dirname;
const testcode = await Bun.file(join(filepath, "./test_builder.ts")).text();

writeFileSync(tempfile, testcode);
const testcode = await Bun.file(join(import.meta.dirname, "./test_builder.ts")).text();

const impl = /* ts */ `
import { heapStats } from "bun:jsc";
Expand Down Expand Up @@ -119,16 +140,17 @@ describe("fd leak", () => {
}
`;

appendFileSync(tempfile, impl);

// console.log("THE CODE", readFileSync(tempfile, "utf-8"));
using dir = tempDir("memleak", {
"script.ts": testcode + impl,
});

const { stdout, stderr, exitCode } = Bun.spawnSync([process.argv0, "--smol", "test", tempfile], {
const { exited, stderr: stream } = Bun.spawn([process.argv0, "--smol", "test", join(dir, "script.ts")], {
env: bunEnv,
stderr: "pipe",
});
// console.log("STDOUT:", stdout.toString(), "\n\nSTDERR:", stderr.toString());
const [exitCode, stderr] = await Promise.all([exited, stream.text()]);
if (exitCode != 0) {
console.log("\n\nSTDERR:", stderr.toString());
console.log("\n\nSTDERR:", stderr);
}
expect(exitCode).toBe(0);
}, 100_000);
Expand Down Expand Up @@ -182,12 +204,7 @@ describe("fd leak", () => {
test.if(runTheTest)(
`memleak_protect_${name}`,
async () => {
const tempfile = join(tmpdir(), "script.ts");

const filepath = import.meta.dirname;
const testcode = await Bun.file(join(filepath, "./test_builder.ts")).text();

writeFileSync(tempfile, testcode);
const testcode = await Bun.file(join(import.meta.dirname, "./test_builder.ts")).text();

const impl = /* ts */ `
import { heapStats } from "bun:jsc";
Expand All @@ -211,16 +228,18 @@ describe("fd leak", () => {
}
`;

appendFileSync(tempfile, impl);

// console.log("THE CODE", readFileSync(tempfile, "utf-8"));
using dir = tempDir("memleak_protect", {
"script.ts": testcode + impl,
});

const { stdout, stderr, exitCode } = Bun.spawnSync([process.argv0, "--smol", "test", tempfile], {
const { stderr: stream, exited } = Bun.spawn([process.argv0, "--smol", "test", join(dir, "script.ts")], {

@coderabbitai coderabbitai Bot Jan 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider using bunExe() for consistency.

Same inconsistency as lines 89 and 147: consider using bunExe() instead of process.argv0 for consistency.

Suggested change 
-        const { stderr: stream, exited } = Bun.spawn([process.argv0, "--smol", "test", join(dir, "script.ts")], {
+        const { stderr: stream, exited } = Bun.spawn([bunExe(), "--smol", "test", join(dir, "script.ts")], {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const { stderr: stream, exited } = Bun.spawn([process.argv0, "--smol", "test", join(dir, "script.ts")], {
const { stderr: stream, exited } = Bun.spawn([bunExe(), "--smol", "test", join(dir, "script.ts")], {
🤖 Prompt for AI Agents 
In @test/js/bun/shell/leak.test.ts at line 235, Replace the direct use of
process.argv0 with the helper bunExe() to keep spawn invocation consistent; in
the Bun.spawn call that destructures { stderr: stream, exited } and currently
passes [process.argv0, "--smol", "test", join(dir, "script.ts")], swap
process.argv0 for bunExe() so the array becomes [bunExe(), "--smol", "test",
join(dir, "script.ts")] to match the other tests using bunExe().

env: bunEnv,
stdout: "ignore",
stderr: "pipe",
});
// console.log("STDOUT:", stdout.toString(), "\n\nSTDERR:", stderr.toString());
const [exitCode, stderr] = await Promise.all([exited, stream.text()]);
if (exitCode != 0) {
console.log("\n\nSTDERR:", stderr.toString());
console.log("\n\nSTDERR:", stderr);
}
expect(exitCode).toBe(0);
},
Expand Down Expand Up @@ -373,7 +392,7 @@ describe("fd leak", () => {
true,
);

describe("#11816", async () => {
describe.serial("#11816", async () => {

@coderabbitai coderabbitai Bot Jan 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Unnecessary async keyword on describe callback.

The async on this describe callback is unnecessary since there's no await in the callback body. Line 414 correctly omits it.

Suggested fix 
-  describe.serial("#11816", async () => {
+  describe.serial("#11816", () => {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
describe.serial("#11816", async () => {
describe.serial("#11816", () => {
🤖 Prompt for AI Agents 
In @test/js/bun/shell/leak.test.ts at line 381, Remove the unnecessary async
from the describe.serial callback for "#11816" (i.e., change
describe.serial("#11816", async () => { ... }) to describe.serial("#11816", ()
=> { ... })), since the callback contains no await and the async keyword is
redundant.

function doit(builtin: boolean) {
test(builtin ? "builtin" : "external", async () => {
const files = tempDirWithFiles("hi", {
Expand Down Expand Up @@ -406,7 +425,7 @@ describe("fd leak", () => {
doit(true);
});

describe("not leaking ParsedShellScript when ShellInterpreter never runs", async () => {
describe.serial("not leaking ParsedShellScript when ShellInterpreter never runs", () => {
function doit(builtin: boolean) {
test(builtin ? "builtin" : "external", async () => {
const files = tempDirWithFiles("hi", {
Expand Down
16 changes: 0 additions & 16 deletions test/js/bun/shell/test_builder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,6 @@ import { join } from "node:path";
export function createTestBuilder(path: string) {
var { describe, test, afterAll, beforeAll, expect, beforeEach, afterEach } = Bun.jest(path);

var insideTestScope = false;
beforeEach(() => {
insideTestScope = true;
});
afterEach(() => {
insideTestScope = false;
});

class TestBuilder {
_testName: string | undefined = undefined;

Expand Down Expand Up @@ -247,14 +239,6 @@ export function createTestBuilder(path: string) {
}

async run(): Promise<undefined> {
if (!insideTestScope) {
const err = new Error("TestBuilder.run() must be called inside a test scope");
test("TestBuilder.run() must be called inside a test scope", () => {
throw err;
});
return Promise.resolve(undefined);
}

try {
let finalPromise = Bun.$(this._scriptStr, ...this._expresssions);
if (this.tempdir) finalPromise = finalPromise.cwd(this.tempdir);
Expand Down