From caeea11706d899a6b6124d3ebec7eef214184492 Mon Sep 17 00:00:00 2001 From: Don Isaac Date: Mon, 14 Apr 2025 19:29:05 -0700 Subject: [PATCH] fix(bun/test): it.failing for tests using done callbacks (#19018) --- packages/bun-types/test.d.ts | 7 +++- src/bun.js/bindings/JSGlobalObject.zig | 13 ++++++ src/bun.js/bindings/bindings.cpp | 5 +++ src/bun.js/test/jest.zig | 41 ++++++++++++++---- .../failing-test-done-test-fails.fixture.ts | 34 +++++++++++++++ ...failing-test-done-test-succeeds.fixture.ts | 42 +++++++++++++++++++ .../fixtures/failing-test-timeout.fixture.ts | 20 ++++++++- test/js/bun/test/test-failing.test.ts | 32 +++++++++++++- .../third_party/express/express.text.test.ts | 2 +- 9 files changed, 182 insertions(+), 14 deletions(-) create mode 100644 test/js/bun/test/fixtures/failing-test-done-test-fails.fixture.ts create mode 100644 test/js/bun/test/fixtures/failing-test-done-test-succeeds.fixture.ts diff --git a/packages/bun-types/test.d.ts b/packages/bun-types/test.d.ts index 5e77272b38..a3717b06be 100644 --- a/packages/bun-types/test.d.ts +++ b/packages/bun-types/test.d.ts @@ -426,8 +426,13 @@ declare module "bun:test" { * * @param label the label for the test * @param fn the test function + * @param options the test timeout or options */ - failing(label: string, fn?: (() => void | Promise) | ((done: (err?: unknown) => void) => void)): void; + failing( + label: string, + fn?: (() => void | Promise) | ((done: (err?: unknown) => void) => void), + options?: number | TestOptions, + ): void; /** * Runs this test, if `condition` is true. * diff --git a/src/bun.js/bindings/JSGlobalObject.zig b/src/bun.js/bindings/JSGlobalObject.zig index 109f3c44a0..85fc040329 100644 --- a/src/bun.js/bindings/JSGlobalObject.zig +++ b/src/bun.js/bindings/JSGlobalObject.zig @@ -460,6 +460,18 @@ pub const JSGlobalObject = opaque { return JSGlobalObject__clearException(this); } + /// Clear the currently active exception off the VM unless it is a + /// termination exception. + /// + /// Returns `true` if the exception was cleared, `false` if it was a + /// termination exception. Use `clearException` to unconditionally clear + /// exceptions. + /// + /// It is safe to call this function when no exception is present. + pub fn clearExceptionExceptTermination(this: *JSGlobalObject) bool { + return JSGlobalObject__clearExceptionExceptTermination(this); + } + /// Clears the current exception and returns that value. Requires compile-time /// proof of an exception via `error.JSError` pub fn takeException(this: *JSGlobalObject, proof: bun.JSError) JSValue { @@ -727,6 +739,7 @@ pub const JSGlobalObject = opaque { extern fn JSC__JSGlobalObject__vm(*JSGlobalObject) *VM; extern fn JSC__JSGlobalObject__deleteModuleRegistryEntry(*JSGlobalObject, *const ZigString) void; extern fn JSGlobalObject__clearException(*JSGlobalObject) void; + extern fn JSGlobalObject__clearExceptionExceptTermination(*JSGlobalObject) bool; extern fn JSGlobalObject__clearTerminationException(this: *JSGlobalObject) void; extern fn JSGlobalObject__hasException(*JSGlobalObject) bool; extern fn JSGlobalObject__setTimeZone(this: *JSGlobalObject, timeZone: *const ZigString) bool; diff --git a/src/bun.js/bindings/bindings.cpp b/src/bun.js/bindings/bindings.cpp index 435b614421..4666d5c40c 100644 --- a/src/bun.js/bindings/bindings.cpp +++ b/src/bun.js/bindings/bindings.cpp @@ -6272,6 +6272,11 @@ extern "C" void JSGlobalObject__clearException(JSC::JSGlobalObject* globalObject DECLARE_CATCH_SCOPE(globalObject->vm()).clearException(); } +extern "C" bool JSGlobalObject__clearExceptionExceptTermination(JSC::JSGlobalObject* globalObject) +{ + return DECLARE_CATCH_SCOPE(globalObject->vm()).clearExceptionExceptTermination(); +} + extern "C" JSC::EncodedJSValue JSGlobalObject__tryTakeException(JSC::JSGlobalObject* globalObject) { auto scope = DECLARE_CATCH_SCOPE(globalObject->vm()); diff --git a/src/bun.js/test/jest.zig b/src/bun.js/test/jest.zig index ac4ac1fad2..533c3eebb3 100644 --- a/src/bun.js/test/jest.zig +++ b/src/bun.js/test/jest.zig @@ -259,6 +259,7 @@ pub const TestRunner = struct { status: Status = Status.pending, pub const ID = u32; + pub const null_id: ID = std.math.maxInt(Test.ID); pub const List = std.MultiArrayList(Test); pub const Status = enum(u4) { @@ -633,21 +634,35 @@ pub const TestScope = struct { if (JSC.getFunctionData(function)) |data| { var task = bun.cast(*TestRunnerTask, data); + const expect_count = expect.active_test_expectation_counter.actual; + const current_test = task.testScope(); + const no_err_result: Result = if (current_test.tag == .fail) + .{ .fail_because_failing_test_passed = expect_count } + else + .{ .pass = expect_count }; JSC.setFunctionData(function, null); if (args.len > 0) { const err = args.ptr[0]; if (err.isEmptyOrUndefinedOrNull()) { debug("done()", .{}); - task.handleResult(.{ .pass = expect.active_test_expectation_counter.actual }, .callback); + task.handleResult(no_err_result, .callback); } else { debug("done(err)", .{}); - _ = globalThis.bunVM().uncaughtException(globalThis, err, true); - task.handleResult(.{ .fail = expect.active_test_expectation_counter.actual }, .callback); + const result: Result = if (current_test.tag == .fail) failing_passed: { + break :failing_passed if (globalThis.clearExceptionExceptTermination()) + Result{ .pass = expect_count } + else + Result{ .fail = expect_count }; // what is the correct thing to do when terminating? + } else passing_failed: { + _ = globalThis.bunVM().uncaughtException(globalThis, err, true); + break :passing_failed Result{ .fail = expect_count }; + }; + task.handleResult(result, .callback); } } else { debug("done()", .{}); - task.handleResult(.{ .pass = expect.active_test_expectation_counter.actual }, .callback); + task.handleResult(no_err_result, .callback); } } @@ -1142,7 +1157,7 @@ pub const DescribeScope = struct { if (end == 0) { var runner = allocator.create(TestRunnerTask) catch unreachable; runner.* = .{ - .test_id = std.math.maxInt(TestRunner.Test.ID), + .test_id = TestRunner.Test.null_id, .describe = this, .globalThis = globalObject, .source_file_path = source.path.text, @@ -1174,8 +1189,8 @@ pub const DescribeScope = struct { pub fn onTestComplete(this: *DescribeScope, globalThis: *JSGlobalObject, test_id: TestRunner.Test.ID, skipped: bool) void { // invalidate it - this.current_test_id = std.math.maxInt(TestRunner.Test.ID); - if (test_id != std.math.maxInt(TestRunner.Test.ID)) this.pending_tests.unset(test_id); + this.current_test_id = TestRunner.Test.null_id; + if (test_id != TestRunner.Test.null_id) this.pending_tests.unset(test_id); globalThis.bunVM().onUnhandledRejectionCtx = null; if (!skipped) { @@ -1277,6 +1292,10 @@ pub const TestRunnerTask = struct { fulfilled, }; + pub inline fn testScope(this: *TestRunnerTask) *TestScope { + return &this.describe.tests.items[this.test_id]; + } + pub fn onUnhandledRejection(jsc_vm: *VirtualMachine, globalObject: *JSGlobalObject, rejection: JSValue) void { var deduped = false; const is_unhandled = jsc_vm.onUnhandledRejectionCtx == null; @@ -1309,7 +1328,11 @@ pub const TestRunnerTask = struct { if (jsc_vm.onUnhandledRejectionCtx) |ctx| { var this = bun.cast(*TestRunnerTask, ctx); jsc_vm.onUnhandledRejectionCtx = null; - this.handleResult(.{ .fail = expect.active_test_expectation_counter.actual }, .unhandledRejection); + const result: Result = if (this.testScope().tag == .fail) + .{ .pass = expect.active_test_expectation_counter.actual } + else + .{ .fail = expect.active_test_expectation_counter.actual }; + this.handleResult(result, .unhandledRejection); } else if (Jest.runner) |runner| { if (!deduped) runner.unhandled_errors_between_tests += 1; @@ -1343,7 +1366,7 @@ pub const TestRunnerTask = struct { jsc_vm.last_reported_error_for_dedupe = .zero; const test_id = this.test_id; - if (test_id == std.math.maxInt(TestRunner.Test.ID)) { + if (test_id == TestRunner.Test.null_id) { describe.onTestComplete(globalThis, test_id, true); Jest.runner.?.runNextTest(); this.deinit(); diff --git a/test/js/bun/test/fixtures/failing-test-done-test-fails.fixture.ts b/test/js/bun/test/fixtures/failing-test-done-test-fails.fixture.ts new file mode 100644 index 0000000000..d55b30663d --- /dev/null +++ b/test/js/bun/test/fixtures/failing-test-done-test-fails.fixture.ts @@ -0,0 +1,34 @@ +import { describe, test, expect } from "bun:test"; + +describe("test.failing with a done callback", () => { + test.failing("fails when done is called with no args", done => { + done(); + }); + + test.failing("fails when done is called with undefined", done => { + done(undefined); + }); + + test.failing("fails when all expectations are met and done is called without an error", done => { + expect(1).toBe(1); + done(); + }); + + describe("when test fn is async", () => { + // NOTE: tests that resolve/reject immediately hit a different code path + test.failing("fails when done() is called immediately", async done => { + done(); + }); + + test.failing("fails when done() is called on the next tick", async done => { + await new Promise(resolve => process.nextTick(resolve)); + done(); + }); + + test.failing("fails when all expectations are met and done is called", async done => { + await Bun.sleep(5); + expect(1).toBe(1); + done(); + }); + }); +}); diff --git a/test/js/bun/test/fixtures/failing-test-done-test-succeeds.fixture.ts b/test/js/bun/test/fixtures/failing-test-done-test-succeeds.fixture.ts new file mode 100644 index 0000000000..10b80e6db3 --- /dev/null +++ b/test/js/bun/test/fixtures/failing-test-done-test-succeeds.fixture.ts @@ -0,0 +1,42 @@ +import { describe, test } from "bun:test"; + +describe("test.failing with a done callback", () => { + test.failing("passes when an error is thrown", done => { + throw new Error("test error"); + }); + + test.failing("passes when done() is called with an error", done => { + done(new Error("test error")); + }); + + describe("when test fn is asynchronous but does not return a promise", () => { + test.failing("passes when done(err) is called on next tick", done => { + process.nextTick(() => { + done(new Error("test error")); + }); + }); + + test.failing("passes when done(err) is called on next event loop cycle", done => { + setTimeout(() => { + done(new Error("test error")); + }, 0); + }); + }); + + describe("when test fn is async", () => { + // NOTE: tests that resolve/reject immediately hit a different code path + test.failing("passes when a promise rejects", async _done => { + await Bun.sleep(5); + throw new Error("test error"); + }); + + test.failing("passes when a promise rejects immediately", async _done => { + throw new Error("test error"); + }); + + test.failing("passes when done() is called with an error", async done => { + await Bun.sleep(5); + done(new Error("test error")); + }); + }); +}); diff --git a/test/js/bun/test/fixtures/failing-test-timeout.fixture.ts b/test/js/bun/test/fixtures/failing-test-timeout.fixture.ts index a6934e2fd8..14b1001420 100644 --- a/test/js/bun/test/fixtures/failing-test-timeout.fixture.ts +++ b/test/js/bun/test/fixtures/failing-test-timeout.fixture.ts @@ -1,4 +1,20 @@ +import { isCI, isWindows } from "harness"; + jest.setTimeout(5); -test.failing("timeouts still count as failures", async () => { - await Bun.sleep(1000); + +describe("test.failing", () => { + test.failing("Timeouts still count as failures", async () => { + await Bun.sleep(1000); + }); + + // fixme: hangs on windows. Timer callback never fires + describe.skipIf(isWindows && isCI)("when using a done() callback", () => { + test.failing("fails when an async test never calls done()", async _done => { + // nada + }); + + test.failing("fails when a sync test never calls done()", _done => { + // nada + }); + }); }); diff --git a/test/js/bun/test/test-failing.test.ts b/test/js/bun/test/test-failing.test.ts index 3090d1e7f8..5c06bb0d8f 100644 --- a/test/js/bun/test/test-failing.test.ts +++ b/test/js/bun/test/test-failing.test.ts @@ -39,7 +39,37 @@ describe("test.failing", () => { if (result.exitCode === 0) { fail("Expected exit code to be non-zero\n\n" + stderr); } - expect(stderr).toContain(" 1 fail\n"); + expect(stderr).toContain(" 0 pass\n"); expect(stderr).toMatch(/timed out after \d+ms/i); }); + + describe("when using a done() callback", () => { + it("when a test throws, rejects, or passes an error to done(), the test passes", async () => { + const result = await $.cwd( + fixtureDir, + ).nothrow()`${bunExe()} test ./failing-test-done-test-succeeds.fixture.ts`.quiet(); + const stderr = result.stderr.toString(); + try { + expect(stderr).toContain("0 fail"); + expect(result.exitCode).toBe(0); + } catch (e) { + console.error(stderr); + throw e; + } + }); + + it("when the test doesn't throw, or otherwise fail, the test does not pass", async () => { + const result = await $.cwd( + fixtureDir, + ).nothrow()`${bunExe()} test ./failing-test-done-test-fails.fixture.ts`.quiet(); + const stderr = result.stderr.toString(); + try { + expect(stderr).toContain("0 pass"); + expect(result.exitCode).not.toBe(0); + } catch (e) { + console.error(stderr); + throw e; + } + }); + }); }); diff --git a/test/js/third_party/express/express.text.test.ts b/test/js/third_party/express/express.text.test.ts index 9b7fad6017..096ed48f21 100644 --- a/test/js/third_party/express/express.text.test.ts +++ b/test/js/third_party/express/express.text.test.ts @@ -509,7 +509,7 @@ describe("express.text()", function () { }); }); -function createApp(options) { +function createApp(options?) { var app = express(); app.use(express.text(options));