Add bun test --shard=M/N for splitting tests across CI jobs - #29366

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

test/cli/test/test-shard.test.ts (2)

34-174: ⚠️ Potential issue | 🟠 Major

Missing --changed + --shard coverage leaves a contract gap.

The suite still has no e2e case that runs bun test --changed --shard=M/N and verifies shard partitioning after changed-file filtering. That behavior is part of this PR’s contract and can regress unnoticed without a dedicated test.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/cli/test/test-shard.test.ts` around lines 34 - 174, Add an end-to-end
test in test/cli/test/test-shard.test.ts that covers the combination of
--changed and --shard: use makeFixture to create a repo with multiple test files
and mark one or more files as "changed", then invoke runShard(cwd, "M/N",
["--changed"]) (or runShard with a third args array) for the shard values you
expect to exercise; assert that the selected ran files are the correct shard
partition of the filtered (changed-only) test list, that stderr contains the
"--shard=" summary, and that exitCode is 0 to prevent regressions in the
changed+shard contract.

---

76-97: ⚠️ Potential issue | 🟠 Major

--randomize composition test does not actually validate randomization behavior.

This test passes --seed but never enables --randomize, and runShard() sorts ran before returning, so “same seed ⇒ same order” is not testable here. As written, it only validates set membership, not shuffle semantics.

Suggested patch

-async function runShard(cwd: string, shard: string, extra: string[] = []) {
+async function runShard(cwd: string, shard: string, extra: string[] = [], sortRan = true) {
   await using proc = Bun.spawn({
     cmd: [bunExe(), "test", `--shard=${shard}`, ...extra],
@@
-  const ran = stdout
+  const ran = stdout
     .split("\n")
     .filter(l => l.startsWith("RAN "))
-    .map(l => l.slice(4))
-    .sort();
+    .map(l => l.slice(4));
-  return { ran, stderr, exitCode };
+  return { ran: sortRan ? [...ran].sort() : ran, stderr, exitCode };
 }
@@
-    const plain = await runShard(cwd, "2/4");
-    const seeded1 = await runShard(cwd, "2/4", ["--seed=123"]);
-    const seeded2 = await runShard(cwd, "2/4", ["--seed=123"]);
-    const otherSeed = await runShard(cwd, "2/4", ["--seed=999999"]);
+    const plain = await runShard(cwd, "2/4");
+    const seeded1 = await runShard(cwd, "2/4", ["--randomize", "--seed=123"], false);
+    const seeded2 = await runShard(cwd, "2/4", ["--randomize", "--seed=123"], false);
+    const otherSeed = await runShard(cwd, "2/4", ["--randomize", "--seed=999999"], false);
@@
-    expect(seeded1.ran).toEqual(plain.ran);
-    expect(seeded2.ran).toEqual(plain.ran);
-    expect(otherSeed.ran).toEqual(plain.ran);
+    expect([...seeded1.ran].sort()).toEqual(plain.ran);
+    expect([...seeded2.ran].sort()).toEqual(plain.ran);
+    expect([...otherSeed.ran].sort()).toEqual(plain.ran);
@@
-    // Same seed → same result.
+    // Same seed → same order.
+    expect(seeded1.ran).toEqual(seeded2.ran);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/cli/test/test-shard.test.ts` around lines 76 - 97, The test claims to
validate --randomize but never enables it and runShard currently sorts ran
before returning, so it only checks set membership; fix by calling runShard with
the --randomize flag for the seeded runs (e.g. runShard(cwd, "2/4",
["--randomize","--seed=123"]) and runShard(cwd, "2/4",
["--randomize","--seed=999999"])) and ensure runShard does not unconditionally
sort the returned ran array (or remove the sort there) so the test can assert
deterministic ordering for the same seed via seeded1.ran === seeded2.ran and
different ordering for otherSeed.

Overview

The PR adds bun test --shard=M/N across five files: CLI type definition (src/cli.zig), argument parsing (src/cli/Arguments.zig), test command runtime (src/cli/test_command.zig), shell completions (completions/bun.zsh), and a new test file (test/cli/test/test-shard.test.ts). The sharding logic sorts test files by path for determinism, applies round-robin selection, prints a summary line, and gates on test_files.len > 0 to avoid confusing 0/0 output.

Security risks

None. The feature parses two unsigned integers from a CLI argument and does a modulo-based array selection. No user-controlled data reaches sensitive operations, file paths, or system calls beyond what bun test already does.

Level of scrutiny

Moderate — this is new user-facing functionality touching argument parsing and test file selection. However the logic is simple (sort + round-robin), all error cases produce clear messages with non-zero exit codes, and the test suite covers partitioning invariants, determinism, empty shards, and all invalid-input forms.

Other factors

A prior review cycle caught a real UX bug (misleading running 0/0 test files when no test files exist); the author fixed it promptly. The remaining open items from prior inline comments — missing Output.flush() before five Global.exit(1) calls in the shard parser, describe vs describe.concurrent in the test file, and the seed-order test only asserting set equality — are nits. The flush omission is pre-existing pattern in adjacent option parsers, flush-on-exit is reliable in practice, and all test suite invariants that matter (no overlap, no gaps, correct round-robin, correct exit codes) are verified. The --watch + --shard edge case was deliberately left as-is with a documented rationale that is reasonable for a CI-oriented flag.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Overview

This PR adds bun test --shard=M/N to split test files across CI jobs, touching CLI argument parsing (Arguments.zig, cli.zig), test runner logic (test_command.zig), zsh completions, and a comprehensive E2E test file (test-shard.test.ts).

Security risks

None. This is a purely additive CLI flag that filters an already-discovered list of local test files. No user input reaches network, filesystem writes, or privilege boundaries beyond the existing test runner.

Level of scrutiny

Moderate. The feature touches the test runner's file-dispatch path but is well-isolated: the shard block is a contained sort + round-robin loop on a local slice, guarded by test_files.len > 0. The parsing adds validation-gated early exits consistent with adjacent option parsers.

Other factors

• 13 E2E tests cover partitioning correctness (union == all files, intersection == ∅), determinism, 1/1, empty-shard exit 0, summary line, no-files case, and all invalid-input error messages.
• All inline review concerns from prior runs were resolved: the 0/0 misleading output guard, describe.concurrent, and the overclaimed randomize test comment.
• No bugs found by the automated bug-hunting system.

Overview

This PR adds bun test --shard=M/N across 5 files: zsh completions, CLI type definition, argument parser, test command runtime logic, and a new E2E test suite.

Security risks

None. This is a purely additive CLI flag that filters an already-discovered set of test file paths. No auth, crypto, or I/O boundary changes.

Level of scrutiny

The implementation is straightforward — sort, round-robin index, truncate slice. The tricky edge cases (empty shard, empty project, --changed interaction, --watch interaction) were raised in review and either fixed or consciously accepted with documented rationale. The test suite is comprehensive (13 tests, switched to describe.concurrent, timeout removed, randomize composition clarified).

Other factors

All my prior review comments have been addressed: the empty-shard guard was added, describe.concurrent was applied, the Output.flush() concern was resolved (Global.exit already flushes), the --watch --shard watcher behavior was consciously left as-is with a clear explanation, and the --randomize test description was corrected. The one remaining inline comment (pluralization of "test file" using denominator instead of numerator) is a cosmetic grammar nit that does not affect correctness.

Overview

The PR adds --shard=M/N support across five files: CLI type definitions (cli.zig), argument parsing (Arguments.zig), test execution (test_command.zig), zsh completion, and a new E2E test suite (test-shard.test.ts). The shard logic sorts discovered test files lexicographically then applies a round-robin filter in-place on the mutable test_files slice before test execution.

Security risks

No security concerns — the feature is a test-file selection filter with no I/O beyond what bun test already does. Argument parsing validates both index and count before use.

Level of scrutiny

The change modifies the core test execution flow in test_command.zig, a production-critical path. Several rounds of review found real bugs (empty-shard messaging, describe.concurrent omission, overclaimed test assertions) that have all been fixed. The implementation is now clean and the test coverage is solid, but the test runner is high-impact code and a human pass is appropriate.

Other factors

All my previous inline feedback has been resolved or reasonably declined with clear explanations. No bugs were found by the automated hunting system in the final revision. The PR is well-documented and consistent with existing --changed patterns.