From 2a0be52f45b4c0db61f67b8ca48ec750c789af78 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 12 Aug 2026 04:12:13 +0000 Subject: [PATCH] test(shell): make TestBuilder fail on expectations that cannot assert anything TestBuilder silently accepted two patterns that never asserted anything: - Callbacks passed to .stdout()/.stderr()/.exitCode() had their return value discarded, so predicate-style callbacks such as `s => s.includes("x")` or `c => c !== 0` checked nothing. - Calling .stdout() (or .stderr()/.exitCode()/.error()) twice on one builder overwrote the earlier expectation, so only the last one in a chain ran. The builder now throws when an expectation is set twice and when a callback returns a value. run() also fails when .error() was set but the command completed, and awaits the checks on the ShellError path instead of letting a failure escape as an unhandled rejection. Fix the call sites this uncovers: the cd-in-pipeline tests in pipeline_stack.test.ts, the exit-code table, the nested pipeline stderr check and three todo'd condexpr tests in bunshell.test.ts, the two bunshell-default.test.ts stderr checks, and four chained .stdout() chains in ls.test.ts. `export` is removed from the exit-code table because `export` with no arguments succeeds (as in bash); a test for that behaviour is added instead. test_builder.test.ts pins the new builder behaviour. --- test/js/bun/shell/bunshell-default.test.ts | 5 +- test/js/bun/shell/bunshell.test.ts | 20 ++-- test/js/bun/shell/commands/ls.test.ts | 18 ++-- test/js/bun/shell/pipeline_stack.test.ts | 15 +-- test/js/bun/shell/test_builder.test.ts | 106 +++++++++++++++++++++ test/js/bun/shell/test_builder.ts | 57 ++++++++--- 6 files changed, 181 insertions(+), 40 deletions(-) create mode 100644 test/js/bun/shell/test_builder.test.ts diff --git a/test/js/bun/shell/bunshell-default.test.ts b/test/js/bun/shell/bunshell-default.test.ts index ddd78df4dba1..39a176877668 100644 --- a/test/js/bun/shell/bunshell-default.test.ts +++ b/test/js/bun/shell/bunshell-default.test.ts @@ -1,3 +1,4 @@ +import { expect, test } from "bun:test"; import { bunEnv } from "harness"; import { bunExe, createTestBuilder } from "./test_builder"; const TestBuilder = createTestBuilder(import.meta.path); @@ -24,7 +25,7 @@ test("default throw on command failure", async () => { await TestBuilder.command`echo ${code} > index.test.ts; ${bunExe()} test index.test.ts` .ensureTempDir() .stdout(`bun test ${Bun.version_with_sha}\n`) - .stderr(s => s.includes("1 pass")) + .stderr(s => expect(s).toContain("1 pass")) .env(bunEnv) .run(); }); @@ -49,7 +50,7 @@ test("ShellError has .text()", async () => { await TestBuilder.command`echo ${code} > index.test.ts; ${bunExe()} test index.test.ts` .ensureTempDir() .stdout(`bun test ${Bun.version_with_sha}\n`) - .stderr(s => s.includes("1 pass")) + .stderr(s => expect(s).toContain("1 pass")) .env(bunEnv) .run(); }); diff --git a/test/js/bun/shell/bunshell.test.ts b/test/js/bun/shell/bunshell.test.ts index 95a990ba68e7..c26930a637a1 100644 --- a/test/js/bun/shell/bunshell.test.ts +++ b/test/js/bun/shell/bunshell.test.ts @@ -62,7 +62,6 @@ describe("bunshell", () => { process.platform === "win32" ? "cat ldkfjsldf" : null, "touch -alskdjfakjfhasjfh", "mkdir", - "export", "cd lskfjlsdkjf", process.platform !== "win32" ? "echo hi > /dev/full" : null, "pwd sldfkj sfks jdflks flksd f", @@ -83,7 +82,7 @@ describe("bunshell", () => { failing_cmds.forEach(cmdstr => !!cmdstr ? TestBuilder.command`${{ raw: cmdstr }}` - .exitCode(c => c !== 0) + .exitCode(c => expect(c).not.toBe(0)) .stdout(() => {}) .stderr(() => {}) .runAsTest(cmdstr) @@ -1328,6 +1327,12 @@ describe("deno_task", () => { TestBuilder.command`export VAR=1 VAR2=testing VAR3="test this out" && echo $VAR $VAR2 $VAR3` .stdout("1 testing test this out\n") .runAsTest("exported vars 2"); + + TestBuilder.command`export FOO=bar; export` + .env({ ZED: "2", ALPHA: "1" }) + .ensureTempDir() + .stdout("ALPHA=1\nFOO=bar\nPWD=$TEMP_DIR\nZED=2\n") + .runAsTest("export with no arguments prints the exported vars sorted and succeeds"); }); describe("pipeline", async () => { @@ -1467,7 +1472,7 @@ describe("deno_task", () => { // Test nested pipeline with mixed success/failure TestBuilder.command`(echo success | echo works) | (nonexistent | echo backup) || echo final_fallback` .stdout("backup\n") - .stderr(s => s.includes("command not found")) + .stderr("bun: command not found: nonexistent\n") .runAsTest("nested pipeline mixed success failure"); TestBuilder.command`echo 0 | echo 1 | echo 2 | echo 3 | echo 4 | echo 5 | echo 6 | echo 7 | echo 8 | echo 9 | echo 10 | echo 11 | echo 12 | echo 13 | echo 14 | echo 15 | echo 16 | echo 17 | echo 18 | echo 19 | echo 20 | echo 21 | echo 22 | echo 23 | echo 24 | echo 25 | echo 26 | echo 27 | echo 28 | echo 29 | echo 30 | echo 31 | echo 32 | echo 33 | echo 34 | echo 35 | echo 36 | echo 37 | echo 38 | echo 39 | echo 40 | echo 41 | echo 42 | echo 43 | echo 44 | echo 45 | echo 46 | echo 47 | echo 48 | echo 49 | BUN_TEST_VAR=1 ${BUN} -e 'process.stdin.pipe(process.stdout)'` @@ -2394,16 +2399,14 @@ describe("condexprs", () => { [[ ! 1 -eq 1 ]]; echo $? [[ ! ! 1 -eq 1 ]]; echo $? ` - .stdout("1") - .stdout("0") + .stdout("1\n0\n") .runAsTest("! toggles on and off rather than just setting an 'invert result' flag"); TestBuilder.command` [[ ! ! ! 1 -eq 1 ]]; echo $? [[ ! ! ! ! 1 -eq 1 ]]; echo $? ` - .stdout("1") - .stdout("0") + .stdout("1\n0\n") .runAsTest("! toggles on and off rather than just setting an 'invert result' flag"); TestBuilder.command`[[ a ]]`.exitCode(0).runAsTest("parenthesized terms didn't work right until post-2.04"); @@ -2624,8 +2627,7 @@ fi if [[ "123abc" == *?(a)bc ]]; then echo ok 42; else echo bad 42; fi if [[ "123abc" == *?(a)bc ]]; then echo ok 43; else echo bad 43; fi ` - .stdout("ok 42") - .stdout("ok 43") + .stdout("ok 42\nok 43\n") .runAsTest("bug in all versions up to and including bash-2.05b"); // TestBuilder.command` diff --git a/test/js/bun/shell/commands/ls.test.ts b/test/js/bun/shell/commands/ls.test.ts index 3c7ea5176cc3..b44accab984c 100644 --- a/test/js/bun/shell/commands/ls.test.ts +++ b/test/js/bun/shell/commands/ls.test.ts @@ -110,10 +110,7 @@ describe.concurrent("bunshell ls", () => { await $`touch .hidden regular`.quiet().throws(true).cwd(tempdir); await TestBuilder.command`ls -A` .setTempdir(tempdir) - .stdout(s => expect(sortedLsOutput(s)).not.toContain(".")) - .stdout(s => expect(sortedLsOutput(s)).not.toContain("..")) - .stdout(s => expect(sortedLsOutput(s)).toContain(".hidden")) - .stdout(s => expect(sortedLsOutput(s)).toContain("regular")) + .stdout(s => expect(sortedLsOutput(s)).toEqual([".hidden", "regular"])) .run(); }); @@ -205,8 +202,9 @@ describe.concurrent("bunshell ls", () => { await $`mkdir sub; touch .hidden sub/.hidden-sub`.quiet().throws(true).cwd(tempdir); await TestBuilder.command`ls -Ra` .setTempdir(tempdir) - .stdout(s => expect(sortedLsOutput(s)).toContain(".hidden")) - .stdout(s => expect(sortedLsOutput(s)).toContain(".hidden-sub")) + .stdout(s => + expect(sortedLsOutput(s)).toEqual([".", "..", ".hidden", "sub", "./sub:", ".", "..", ".hidden-sub"].sort()), + ) .run(); }); @@ -215,9 +213,7 @@ describe.concurrent("bunshell ls", () => { await $`mkdir sub; touch .hidden sub/.hidden-sub`.quiet().throws(true).cwd(tempdir); await TestBuilder.command`ls -RA` .setTempdir(tempdir) - .stdout(s => expect(sortedLsOutput(s)).toContain(".hidden")) - .stdout(s => expect(sortedLsOutput(s)).toContain(".hidden-sub")) - .stdout(s => expect(sortedLsOutput(s)).not.toContain(".")) + .stdout(s => expect(sortedLsOutput(s)).toEqual([".hidden", "sub", "./sub:", ".hidden-sub"].sort())) .run(); }); @@ -327,9 +323,7 @@ describe.concurrent("bunshell ls", () => { await TestBuilder.command`ls -R level1` .setTempdir(tempdir) .exitCode(1) - .stdout(s => expect(sortedLsOutput(s)).toContain("file1")) - .stdout(s => expect(sortedLsOutput(s)).toContain("file2")) - .stdout(s => expect(sortedLsOutput(s)).toContain("file3")) + .stdout(s => expect(sortedLsOutput(s)).toEqual(["file1", "file2", "file3", "level2"])) .stderr(s => expect(s).toContain("Permission denied")) .run(); diff --git a/test/js/bun/shell/pipeline_stack.test.ts b/test/js/bun/shell/pipeline_stack.test.ts index 8e04ea613229..d4cf20e7ae66 100644 --- a/test/js/bun/shell/pipeline_stack.test.ts +++ b/test/js/bun/shell/pipeline_stack.test.ts @@ -75,22 +75,25 @@ describe("pipeline stack edge cases", () => { describe("cd builtin in pipelines", () => { TestBuilder.command`cd / | pwd` - .stdout(s => s.includes("$TEMP_DIR")) + .stdout("$TEMP_DIR\n") .ensureTempDir() .runAsTest("cd | pwd - cd doesn't affect next command in pipeline"); TestBuilder.command`mkdir foo; mkdir foo/bar; cd foo | cd foo/bar | pwd` - .stdout(s => s.includes("$TEMP_DIR")) + .stdout("$TEMP_DIR\n") .ensureTempDir() .runAsTest("cd | cd | pwd - multiple cd's don't affect"); + // The first pwd writes into the pipe, so only the last one reaches stdout. TestBuilder.command`pwd | cd / | pwd` - .stdout(s => { - const lines = s.trim().split("\n"); - return lines.length === 2 && lines[0].includes("$TEMP_DIR") && lines[1].includes("$TEMP_DIR"); - }) + .stdout("$TEMP_DIR\n") .ensureTempDir() .runAsTest("pwd | cd | pwd - cd in middle doesn't affect"); + + TestBuilder.command`mkdir foo; cd foo | pwd; pwd` + .stdout("$TEMP_DIR\n$TEMP_DIR\n") + .ensureTempDir() + .runAsTest("cd in a pipeline doesn't affect the commands after the pipeline either"); }); describe("mixed builtin and subprocess pipelines", () => { diff --git a/test/js/bun/shell/test_builder.test.ts b/test/js/bun/shell/test_builder.test.ts new file mode 100644 index 000000000000..7e74d55d07b4 --- /dev/null +++ b/test/js/bun/shell/test_builder.test.ts @@ -0,0 +1,106 @@ +import { $ } from "bun"; +import { describe, expect, test } from "bun:test"; +import { createTestBuilder } from "./test_builder"; + +const TestBuilder = createTestBuilder(import.meta.path); + +// Other shell test files switch the shared `$` to nothrow mode; the ShellError tests below need the default. +$.throws(true); + +describe.concurrent("TestBuilder", () => { + describe("an expectation can only be set once per builder", () => { + test("stdout", () => { + expect(() => TestBuilder.command`echo hi`.stdout("hi\n").stdout("hi\n")).toThrow( + "the stdout expectation was set more than once", + ); + }); + + test("stderr", () => { + expect(() => TestBuilder.command`echo hi`.stderr("").stderr_contains("")).toThrow( + "the stderr expectation was set more than once", + ); + }); + + test("exitCode", () => { + expect(() => TestBuilder.command`echo hi`.exitCode(0).exitCode(0)).toThrow( + "the exitCode expectation was set more than once", + ); + }); + + test("error", () => { + expect(() => TestBuilder.command`echo hi`.error("a").error("b")).toThrow( + "the error expectation was set more than once", + ); + }); + + test("different expectations on one builder are fine", async () => { + await TestBuilder.command`echo hi`.stdout("hi\n").stderr("").exitCode(0).run(); + }); + }); + + describe("expectation callbacks", () => { + test("assert by calling expect()", async () => { + await TestBuilder.command`echo hi` + .stdout(s => expect(s).toBe("hi\n")) + .stderr(s => { + expect(s).toBe(""); + }) + .exitCode(c => expect(c).toBe(0)) + .run(); + }); + + test("a failing expect() in a callback fails the run", async () => { + await expect(TestBuilder.command`echo hi`.stdout(s => expect(s).toBe("bye\n")).run()).rejects.toThrow(); + }); + + test("stdout callback must not return a value", async () => { + await expect(TestBuilder.command`echo hi`.stdout(s => s.includes("hi")).run()).rejects.toThrow( + "the .stdout() callback returned a boolean", + ); + }); + + test("stderr callback must not return a value", async () => { + await expect(TestBuilder.command`true`.stderr(s => s.length).run()).rejects.toThrow( + "the .stderr() callback returned a number", + ); + }); + + test("exitCode callback must not return a value", async () => { + await expect(TestBuilder.command`true`.exitCode(c => c === 0).run()).rejects.toThrow( + "the .exitCode() callback returned a boolean", + ); + }); + }); + + describe("error()", () => { + test("passes when the command throws the expected error", async () => { + await TestBuilder.command`echo hi |`.error("Unexpected EOF").run(); + }); + + test("fails when the command does not throw", async () => { + await expect(TestBuilder.command`echo hi`.error("Unexpected EOF").run()).rejects.toThrow( + "expected the command to throw, but it completed with exit code 0", + ); + }); + }); + + describe("when the command rejects with a ShellError", () => { + test("the expectations are checked against its output", async () => { + await TestBuilder.command`echo out; ls does-not-exist` + .stdout("out\n") + .stderr("ls: does-not-exist: No such file or directory\n") + .exitCode(1) + .run(); + }); + + test("a failing expectation fails the run", async () => { + await expect( + TestBuilder.command`echo out; ls does-not-exist` + .stdout("something else\n") + .stderr_contains("No such file or directory") + .exitCode(1) + .run(), + ).rejects.toThrow(); + }); + }); +}); diff --git a/test/js/bun/shell/test_builder.ts b/test/js/bun/shell/test_builder.ts index 64a90a805548..0db34bf1ff3a 100644 --- a/test/js/bun/shell/test_builder.ts +++ b/test/js/bun/shell/test_builder.ts @@ -1,10 +1,20 @@ -import { ShellError, ShellExpression } from "bun"; +import { $, ShellError, ShellExpression } from "bun"; // import { tempDirWithFiles } from "harness"; import * as fs from "node:fs"; import * as os from "node:os"; import { join } from "node:path"; // import { bunExe } from "harness"; +type Expectation = "stdout" | "stderr" | "exitCode" | "error"; + +function checkCallbackReturnedNothing(kind: Exclude, returned: unknown): void { + if (returned !== undefined) { + throw new Error( + `TestBuilder: the .${kind}() callback returned a ${typeof returned}, but its return value is ignored. The callback must call expect() itself.`, + ); + } +} + export function createTestBuilder(path: string) { var { describe, test, afterAll, beforeAll, expect, beforeEach, afterEach } = Bun.jest(path); @@ -16,6 +26,7 @@ export function createTestBuilder(path: string) { expected_exit_code: number | ((code: number) => void) = 0; expected_error: ShellError | string | boolean | undefined = undefined; file_equals: { [filename: string]: string | (() => string | Promise) } = {}; + _expectationsSet: Set = new Set(); _doesNotExist: string[] = []; _timeout: number | undefined = undefined; @@ -121,22 +132,36 @@ export function createTestBuilder(path: string) { return this; } + _setExpectation(kind: Expectation): void { + if (this._expectationsSet.has(kind)) { + throw new Error( + `TestBuilder: the ${kind} expectation was set more than once; only the last one would be checked. Put all of the assertions in a single callback.`, + ); + } + this._expectationsSet.add(kind); + } + /** * Expect output from stdout * - * @param expected - can either be a string or a function which itself calls `expect()` + * @param expected - can either be a string (`$TEMP_DIR` is replaced with the temp directory) or + * a function which itself calls `expect()`. A function must not return anything: its return + * value is not an assertion. */ stdout(expected: string | ((stdout: string, tempDir: string) => void)): this { + this._setExpectation("stdout"); this.expected_stdout = expected; return this; } stderr(expected: string | ((stderr: string, tempDir: string) => void)): this { + this._setExpectation("stderr"); this.expected_stderr = expected; return this; } stderr_contains(expected: string): this { + this._setExpectation("stderr"); this.expected_stderr = { contains: expected }; return this; } @@ -155,7 +180,11 @@ export function createTestBuilder(path: string) { return this; } + /** + * Expect the command to throw (e.g. a parse error). The test fails if it completes instead. + */ error(expected?: ShellError | string | boolean): this { + this._setExpectation("error"); if (expected === undefined || expected === true) { this.expected_error = true; } else if (expected === false) { @@ -167,6 +196,7 @@ export function createTestBuilder(path: string) { } exitCode(expected: number | ((code: number) => void)): this { + this._setExpectation("exitCode"); this.expected_exit_code = expected; return this; } @@ -211,21 +241,23 @@ export function createTestBuilder(path: string) { if (typeof this.expected_stdout === "string") { expect(stdout.toString()).toEqual(this.expected_stdout.replaceAll("$TEMP_DIR", tempdir)); } else { - this.expected_stdout(stdout.toString(), tempdir); + checkCallbackReturnedNothing("stdout", this.expected_stdout(stdout.toString(), tempdir)); } } if (this.expected_stderr !== undefined) { if (typeof this.expected_stderr === "string") { expect(stderr.toString()).toEqual(this.expected_stderr.replaceAll("$TEMP_DIR", tempdir)); } else if (typeof this.expected_stderr === "function") { - this.expected_stderr(stderr.toString(), tempdir); + checkCallbackReturnedNothing("stderr", this.expected_stderr(stderr.toString(), tempdir)); } else { expect(stderr.toString()).toContain(this.expected_stderr.contains); } } if (typeof this.expected_exit_code === "number") { expect(exitCode).toEqual(this.expected_exit_code); - } else if (typeof this.expected_exit_code === "function") this.expected_exit_code(exitCode); + } else { + checkCallbackReturnedNothing("exitCode", this.expected_exit_code(exitCode)); + } for (const [filename, expected_raw] of Object.entries(this.file_equals)) { const expected = typeof expected_raw === "string" ? expected_raw : await expected_raw(); @@ -239,16 +271,14 @@ export function createTestBuilder(path: string) { } async run(): Promise { + let output: $.ShellOutput; try { let finalPromise = Bun.$(this._scriptStr, ...this._expresssions); if (this.tempdir) finalPromise = finalPromise.cwd(this.tempdir); if (this._cwd) finalPromise = finalPromise.cwd(this._cwd); if (this._env) finalPromise = finalPromise.env(this._env); if (this._quiet) finalPromise = finalPromise.quiet(); - const output = await finalPromise; - - const { stdout, stderr, exitCode } = output; - await this.doChecks(stdout, stderr, exitCode); + output = await finalPromise; } catch (err_) { const err: ShellError = err_ as any; const { stdout, stderr, exitCode } = err; @@ -256,7 +286,7 @@ export function createTestBuilder(path: string) { if (stdout === undefined || stderr === undefined || exitCode === undefined) { throw err_; } - this.doChecks(stdout, stderr, exitCode); + await this.doChecks(stdout, stderr, exitCode); return; } if (this.expected_error === true) return undefined; @@ -273,7 +303,12 @@ export function createTestBuilder(path: string) { return undefined; } - // return output; + if (this.expected_error !== undefined && this.expected_error !== false) { + expect.unreachable( + `TestBuilder: expected the command to throw, but it completed with exit code ${output.exitCode}`, + ); + } + await this.doChecks(output.stdout, output.stderr, output.exitCode); } todo(reason?: string): this {