From 613aea1787f1c8ef78d54a4dbc322b9fe59d1743 Mon Sep 17 00:00:00 2001 From: pfg Date: Tue, 30 Sep 2025 21:47:31 -0700 Subject: [PATCH] run beforeAll before the first file and afterAll after the last file (#23113) Fixes #23066 Reverts the breaking change to this order made in #22534 --- src/bun.js/test/Order.zig | 26 +++++++++------- src/bun.js/test/bun_test.zig | 28 +++++++++++------ src/cli/test_command.zig | 11 +++---- test/js/bun/test/bun_test.test.ts | 31 +++++++++++++++++++ .../bun/test/scheduling/multi-file/preload.ts | 17 ++++++++++ .../scheduling/multi-file/test1.fixture.ts | 5 +++ .../scheduling/multi-file/test2.fixture.ts | 5 +++ test/regression/issue/12782.test.ts | 18 +++-------- 8 files changed, 100 insertions(+), 41 deletions(-) create mode 100644 test/js/bun/test/scheduling/multi-file/preload.ts create mode 100644 test/js/bun/test/scheduling/multi-file/test1.fixture.ts create mode 100644 test/js/bun/test/scheduling/multi-file/test2.fixture.ts diff --git a/src/bun.js/test/Order.zig b/src/bun.js/test/Order.zig index de48849fa8..ed9d021378 100644 --- a/src/bun.js/test/Order.zig +++ b/src/bun.js/test/Order.zig @@ -4,11 +4,13 @@ groups: std.ArrayList(ConcurrentGroup), sequences: std.ArrayList(ExecutionSequence), arena: std.mem.Allocator, previous_group_was_concurrent: bool = false, +cfg: Config, -pub fn init(gpa: std.mem.Allocator, arena: std.mem.Allocator) Order { +pub fn init(gpa: std.mem.Allocator, arena: std.mem.Allocator, cfg: Config) Order { return .{ .groups = std.ArrayList(ConcurrentGroup).init(gpa), .sequences = std.ArrayList(ExecutionSequence).init(gpa), + .cfg = cfg, .arena = arena, }; } @@ -17,10 +19,10 @@ pub fn deinit(this: *Order) void { this.sequences.deinit(); } -pub fn generateOrderSub(this: *Order, current: TestScheduleEntry, cfg: Config) bun.JSError!void { +pub fn generateOrderSub(this: *Order, current: TestScheduleEntry) bun.JSError!void { switch (current) { - .describe => |describe| try generateOrderDescribe(this, describe, cfg), - .test_callback => |test_callback| try generateOrderTest(this, test_callback, cfg), + .describe => |describe| try generateOrderDescribe(this, describe), + .test_callback => |test_callback| try generateOrderTest(this, test_callback), } } pub const AllOrderResult = struct { @@ -39,7 +41,7 @@ pub const Config = struct { always_use_hooks: bool, randomize: ?std.Random, }; -pub fn generateAllOrder(this: *Order, entries: []const *ExecutionEntry, _: Config) bun.JSError!AllOrderResult { +pub fn generateAllOrder(this: *Order, entries: []const *ExecutionEntry) bun.JSError!AllOrderResult { const start = this.groups.items.len; for (entries) |entry| { if (bun.Environment.ci_assert and entry.added_in_phase != .preload) bun.assert(entry.next == null); @@ -54,29 +56,29 @@ pub fn generateAllOrder(this: *Order, entries: []const *ExecutionEntry, _: Confi const end = this.groups.items.len; return .{ .start = start, .end = end }; } -pub fn generateOrderDescribe(this: *Order, current: *DescribeScope, cfg: Config) bun.JSError!void { +pub fn generateOrderDescribe(this: *Order, current: *DescribeScope) bun.JSError!void { if (current.failed) return; // do not schedule any tests in a failed describe scope - const use_hooks = cfg.always_use_hooks or current.base.has_callback; + const use_hooks = this.cfg.always_use_hooks or current.base.has_callback; // gather beforeAll - const beforeall_order: AllOrderResult = if (use_hooks) try generateAllOrder(this, current.beforeAll.items, cfg) else .empty; + const beforeall_order: AllOrderResult = if (use_hooks) try generateAllOrder(this, current.beforeAll.items) else .empty; // shuffle entries if randomize flag is set - if (cfg.randomize) |random| { + if (this.cfg.randomize) |random| { random.shuffle(TestScheduleEntry, current.entries.items); } // gather children for (current.entries.items) |entry| { if (current.base.only == .contains and entry.base().only == .no) continue; - try generateOrderSub(this, entry, cfg); + try generateOrderSub(this, entry); } // update skip_to values for beforeAll to skip to the first afterAll beforeall_order.setFailureSkipTo(this); // gather afterAll - const afterall_order: AllOrderResult = if (use_hooks) try generateAllOrder(this, current.afterAll.items, cfg) else .empty; + const afterall_order: AllOrderResult = if (use_hooks) try generateAllOrder(this, current.afterAll.items) else .empty; // update skip_to values for afterAll to skip the remaining afterAll items afterall_order.setFailureSkipTo(this); @@ -104,7 +106,7 @@ const EntryList = struct { } }; -pub fn generateOrderTest(this: *Order, current: *ExecutionEntry, _: Config) bun.JSError!void { +pub fn generateOrderTest(this: *Order, current: *ExecutionEntry) bun.JSError!void { bun.assert(current.base.has_callback == (current.callback != null)); const use_each_hooks = current.base.has_callback; diff --git a/src/bun.js/test/bun_test.zig b/src/bun.js/test/bun_test.zig index 9167ac5bfe..3cb212ec7d 100644 --- a/src/bun.js/test/bun_test.zig +++ b/src/bun.js/test/bun_test.zig @@ -136,14 +136,14 @@ pub const BunTestRoot = struct { bun.assert(this.active_file == null); } - pub fn enterFile(this: *BunTestRoot, file_id: jsc.Jest.TestRunner.File.ID, reporter: *test_command.CommandLineReporter, default_concurrent: bool) void { + pub fn enterFile(this: *BunTestRoot, file_id: jsc.Jest.TestRunner.File.ID, reporter: *test_command.CommandLineReporter, default_concurrent: bool, first_last: FirstLast) void { group.begin(@src()); defer group.end(); bun.assert(this.active_file.get() == null); this.active_file = .new(undefined); - this.active_file.get().?.init(this.gpa, this, file_id, reporter, default_concurrent); + this.active_file.get().?.init(this.gpa, this, file_id, reporter, default_concurrent, first_last); } pub fn exitFile(this: *BunTestRoot) void { group.begin(@src()); @@ -164,6 +164,11 @@ pub const BunTestRoot = struct { var clone = this.active_file.clone(); return clone.take(); } + + pub const FirstLast = struct { + first: bool, + last: bool, + }; }; pub const BunTest = struct { @@ -180,6 +185,7 @@ pub const BunTest = struct { result_queue: ResultQueue, /// Whether tests in this file should default to concurrent execution default_concurrent: bool, + first_last: BunTestRoot.FirstLast, extra_execution_entries: std.ArrayList(*ExecutionEntry), phase: enum { @@ -190,7 +196,7 @@ pub const BunTest = struct { collection: Collection, execution: Execution, - pub fn init(this: *BunTest, outer_gpa: std.mem.Allocator, bunTest: *BunTestRoot, file_id: jsc.Jest.TestRunner.File.ID, reporter: *test_command.CommandLineReporter, default_concurrent: bool) void { + pub fn init(this: *BunTest, outer_gpa: std.mem.Allocator, bunTest: *BunTestRoot, file_id: jsc.Jest.TestRunner.File.ID, reporter: *test_command.CommandLineReporter, default_concurrent: bool, first_last: BunTestRoot.FirstLast) void { group.begin(@src()); defer group.end(); @@ -213,6 +219,7 @@ pub const BunTest = struct { .reporter = reporter, .result_queue = .init(this.gpa), .default_concurrent = default_concurrent, + .first_last = first_last, .extra_execution_entries = .init(this.gpa), }; } @@ -546,19 +553,20 @@ pub const BunTest = struct { .collection => { this.phase = .execution; try debug.dumpDescribe(this.collection.root_scope); - var order = Order.init(this.gpa, this.arena); - defer order.deinit(); const has_filter = if (this.reporter) |reporter| if (reporter.jest.filter_regex) |_| true else false else false; const should_randomize: ?std.Random = if (this.reporter) |reporter| reporter.jest.randomize else null; - const cfg: Order.Config = .{ + + var order = Order.init(this.gpa, this.arena, .{ .always_use_hooks = this.collection.root_scope.base.only == .no and !has_filter, .randomize = should_randomize, - }; - const beforeall_order: Order.AllOrderResult = if (cfg.always_use_hooks or this.collection.root_scope.base.has_callback) try order.generateAllOrder(this.buntest.hook_scope.beforeAll.items, cfg) else .empty; - try order.generateOrderDescribe(this.collection.root_scope, cfg); + }); + defer order.deinit(); + + const beforeall_order: Order.AllOrderResult = if (this.first_last.first) try order.generateAllOrder(this.buntest.hook_scope.beforeAll.items) else .empty; + try order.generateOrderDescribe(this.collection.root_scope); beforeall_order.setFailureSkipTo(&order); - const afterall_order: Order.AllOrderResult = if (cfg.always_use_hooks or this.collection.root_scope.base.has_callback) try order.generateAllOrder(this.buntest.hook_scope.afterAll.items, cfg) else .empty; + const afterall_order: Order.AllOrderResult = if (this.first_last.last) try order.generateAllOrder(this.buntest.hook_scope.afterAll.items) else .empty; afterall_order.setFailureSkipTo(&order); try this.execution.loadFromOrder(&order); diff --git a/src/cli/test_command.zig b/src/cli/test_command.zig index 318cb518c7..8c0daab27c 100644 --- a/src/cli/test_command.zig +++ b/src/cli/test_command.zig @@ -1763,18 +1763,17 @@ pub const TestCommand = struct { const reporter = this.reporter; const vm = this.vm; var files = this.files; - const allocator = this.allocator; bun.assert(files.len > 0); if (files.len > 1) { - for (files[0 .. files.len - 1]) |file_name| { - TestCommand.run(reporter, vm, file_name.slice(), allocator) catch |err| handleTopLevelTestErrorBeforeJavaScriptStart(err); + for (files[0 .. files.len - 1], 0..) |file_name, i| { + TestCommand.run(reporter, vm, file_name.slice(), .{ .first = i == 0, .last = false }) catch |err| handleTopLevelTestErrorBeforeJavaScriptStart(err); reporter.jest.default_timeout_override = std.math.maxInt(u32); Global.mimalloc_cleanup(false); } } - TestCommand.run(reporter, vm, files[files.len - 1].slice(), allocator) catch |err| handleTopLevelTestErrorBeforeJavaScriptStart(err); + TestCommand.run(reporter, vm, files[files.len - 1].slice(), .{ .first = files.len == 1, .last = true }) catch |err| handleTopLevelTestErrorBeforeJavaScriptStart(err); } }; @@ -1792,7 +1791,7 @@ pub const TestCommand = struct { reporter: *CommandLineReporter, vm: *jsc.VirtualMachine, file_name: string, - _: std.mem.Allocator, + first_last: bun_test.BunTestRoot.FirstLast, ) !void { defer { js_ast.Expr.Data.Store.reset(); @@ -1832,7 +1831,7 @@ pub const TestCommand = struct { var bun_test_root = &jest.Jest.runner.?.bun_test_root; // Determine if this file should run tests concurrently based on glob pattern const should_run_concurrent = reporter.jest.shouldFileRunConcurrently(file_id); - bun_test_root.enterFile(file_id, reporter, should_run_concurrent); + bun_test_root.enterFile(file_id, reporter, should_run_concurrent, first_last); defer bun_test_root.exitFile(); reporter.jest.current_file.set(file_title, file_prefix, repeat_count, repeat_index); diff --git a/test/js/bun/test/bun_test.test.ts b/test/js/bun/test/bun_test.test.ts index 147295d983..9782cdeb33 100644 --- a/test/js/bun/test/bun_test.test.ts +++ b/test/js/bun/test/bun_test.test.ts @@ -235,3 +235,34 @@ test("cross-file safety", async () => { expect(stderr).toInclude("Snapshot matchers cannot be used outside of a test"); expect(exitCode).toBe(1); }); + +test("multi-file", async () => { + const result = await Bun.spawn({ + cmd: [ + bunExe(), + "test", + import.meta.dir + "/scheduling/multi-file/test1.fixture.ts", + import.meta.dir + "/scheduling/multi-file/test2.fixture.ts", + "--preload", + import.meta.dir + "/scheduling/multi-file/preload.ts", + ], + stdio: ["pipe", "pipe", "pipe"], + env: bunEnv, + }); + + const exitCode = await result.exited; + const stdout = await result.stdout.text(); + const stderr = await result.stderr.text(); + expect(exitCode).toBe(0); + expect(normalizeBunSnapshot(stdout)).toMatchInlineSnapshot(` + "bun test () + preload: before first file + preload: beforeEach + test1 + preload: afterEach + preload: beforeEach + test2 + preload: afterEach + preload: after last file" + `); +}); diff --git a/test/js/bun/test/scheduling/multi-file/preload.ts b/test/js/bun/test/scheduling/multi-file/preload.ts new file mode 100644 index 0000000000..eebcffd246 --- /dev/null +++ b/test/js/bun/test/scheduling/multi-file/preload.ts @@ -0,0 +1,17 @@ +import { beforeAll, beforeEach, afterAll, afterEach } from "bun:test"; + +beforeAll(() => { + console.log("preload: before first file"); +}); + +afterAll(() => { + console.log("preload: after last file"); +}); + +beforeEach(() => { + console.log("preload: beforeEach"); +}); + +afterEach(() => { + console.log("preload: afterEach"); +}); diff --git a/test/js/bun/test/scheduling/multi-file/test1.fixture.ts b/test/js/bun/test/scheduling/multi-file/test1.fixture.ts new file mode 100644 index 0000000000..35f72250ba --- /dev/null +++ b/test/js/bun/test/scheduling/multi-file/test1.fixture.ts @@ -0,0 +1,5 @@ +import { test } from "bun:test"; + +test("test1", () => { + console.log("test1"); +}); diff --git a/test/js/bun/test/scheduling/multi-file/test2.fixture.ts b/test/js/bun/test/scheduling/multi-file/test2.fixture.ts new file mode 100644 index 0000000000..5c617db116 --- /dev/null +++ b/test/js/bun/test/scheduling/multi-file/test2.fixture.ts @@ -0,0 +1,5 @@ +import { test } from "bun:test"; + +test("test2", () => { + console.log("test2"); +}); diff --git a/test/regression/issue/12782.test.ts b/test/regression/issue/12782.test.ts index 2df2fa0ae7..9cf64acecf 100644 --- a/test/regression/issue/12782.test.ts +++ b/test/regression/issue/12782.test.ts @@ -34,20 +34,12 @@ test("12782", async () => { (fail) (unnamed) test/regression/issue/12782.bar.fixture.ts: - 1 | import { beforeAll } from "bun:test"; - 2 | - 3 | const FOO = process.env.FOO ?? ""; - 4 | - 5 | beforeAll(() => { - 6 | if (!FOO) throw new Error("Environment variable FOO is not set"); - ^ - error: Environment variable FOO is not set - at (file:NN:NN) - (fail) (unnamed) + (pass) bar > should not run + (pass) bar > inner describe > should not run - 0 pass - 2 fail - Ran 2 tests across 2 files." + 2 pass + 1 fail + Ran 3 tests across 2 files." `); expect(exitCode).toBe(1); });