chrisleekr / chrisleekr/github-app
perf(testing): replace bash-loop test-isolated.sh with native bun test --parallel (~54% wall-time cut, --shard CI matrix)
- Dominant language
- TypeScript
- Stars
- 1
- Forks
- 0
- Avg merge
- 6h 39m
- Merged PRs (30d)
- 27
Description
## Finding
The test runner (`scripts/test-isolated.sh:18-19`) shells out to `bun test` once per `*.test.ts` file inside a serial bash for-loop. The script's opening comment (`scripts/test-isolated.sh:1-4`) names the exact reason it exists: `mock.module()` is process-global and bleeds between files when they share a process, so per-file isolation was reimplemented in shell. Bun 1.3.14 (the pinned `.tool-versions` version) now ships native flags that close this gap: `bun test --parallel` runs files across N worker processes and **implies `--isolate`** (every file gets a fresh global object), and `bun test --shard=M/N` splits files across CI runners for matrix sharding (both verified by `bun test --help` on the pinned version).
The current runner is measurably leaving compute on the floor. Running `time bash scripts/test-isolated.sh` over the 150 test files on an 8-core box completes in **41.9s wall / 54% CPU**, because the loop is single-threaded by construction. The same suite under `time bun test --parallel` completes in **19.3s wall / 67% CPU** (1495 pass, 220 skip, 0 fail; same outcome), a **~54% wall-time reduction** with no source changes. A single `bun test --parallel --shard=1/4` finishes in **~4.5s** locally, so a 4-way CI matrix would take CI test wall time to ~5s instead of ~42s, which is a real fraction of the 10-minute `timeout-minutes` budget on `lint-and-test` in `.github/workflows/ci.yml:31`.
There is one load-bearing behaviour the bash loop adds that `bun test --parallel` does not: a per-file "silent skip = fail" guard (`scripts/test-isolated.sh:33-35`) that catches the case where a `describe.skipIf(sql === null)` block (e.g. `test/webhook/events/issue-comment.test.ts:117`, plus 45 other `*Skip*` sites grep finds) trivially satisfies `0 fail` because the test DB was unreachable. In CI the DB is up via `services:` (`.github/workflows/ci.yml:42-66`), so a non-zero skip count there should be treated as a regression, not as green. Any migration must preserve this contract, either via an aggregated post-run skip-rate gate or via `--reporter junit` per-file parsing.
## Diagram
```mermaid
flowchart TB
subgraph CURRENT["Current: scripts/test-isolated.sh (sequential bash loop)"]
direction LR
BASH["bash for-loop
over 150 .test.ts files"]:::bad
SPAWN["spawn 1 bun process
per file in sequence"]:::bad
WALL1["~41.9s wall time
54% CPU on 8 cores"]:::bad
BASH --> SPAWN --> WALL1
end
subgraph PROPOSED["Proposed: bun test --parallel (native)"]
direction LR
NATIVE["bun test --parallel"]:::good
WORKERS["N worker processes
implies --isolate
work-stealing queue"]:::good
WALL2["~19.3s wall time
67% CPU on 8 cores"]:::good
NATIVE --> WORKERS --> WALL2
end
subgraph CIM["CI matrix with --shard=M/N (4 runners)"]
direction LR
SH1["shard 1/4
~38 files, ~5s"]:::good
SH2["shard 2/4
~38 files, ~5s"]:::good
SH3["shard 3/4
~38 files, ~5s"]:::good
SH4["shard 4/4
~38 files, ~5s"]:::good
end
GUARD["preserve guard:
silent-skip = fail
currently in bash lines 33-35"]:::warn
CURRENT -->|"migrate to"| PROPOSED
PROPOSED -->|"split across CI"| CIM
PROPOSED -.->|"must reimplement"| GUARD
classDef bad fill:#a93226,color:#ffffff,stroke:#7b241c
classDef good fill:#196f3d,color:#ffffff,stroke:#0e6251
classDef warn fill:#b9770e,color:#ffffff,stroke:#7e5109
```
## Rationale
**Developer feedback loop.** `bun run test` (the gate in `package.json` `scripts.check`) is the slowest leg of pre-commit / pre-push verification for contributors: 150 separate `bun` process spawns each pay full module-graph cold-start overhead before running any assertion. A 54% local wall-time drop is the kind of cut that changes whether engineers run the suite locally before pushing vs. waiting on CI.
**CI minutes.** The `Test` step in `.github/workflows/ci.yml` runs on every PR and every push to `main`. At ~42s × the PR/push rate, parallelising in a single runner cuts ~22s per run; sharding 4-way across the existing free runners (`ubuntu-24.04`) cuts ~37s per run AND parallelises the longest job under the workflow-level `timeout-minutes: 10` budget, which the integration suites + future test growth will eventually pressure.
**Code surface reduction.** The bash loop is 67 lines (`scripts/test-isolated.sh:1-67`) of process spawn + summary parsing + skip detection that exists solely because Bun lacked `--isolate` when the runner was written. Migrating to the native flag deletes ~50 LoC of shell logic that the project then no longer has to maintain or audit (the runner has already needed one corrective fix, the "skip = fail" guard, and one CI guard, `scripts/check-test-globs.ts`, to defend against its own glob drift, issue #201).
**Risk surface.** The new code path is Bun's own test runner under flags it already executes for thousands of repos, replacing a hand-rolled shell loop that has its own failure modes (e.g. the documented globstar / dotglob / pipeline-buffering quirks the file's comment block already calls out). The one behaviour that does NOT come for free, the silent-skip guard, is a small post-run check on the aggregate summary line.
## References
**Internal:**
- `scripts/test-isolated.sh:1-67` — sequential per-file runner the proposal replaces.
- `scripts/test-isolated.sh:18-19` — the bash for-loop that spawns one `bun test` per file.
- `scripts/test-isolated.sh:33-35` — the "silent skip = fail" guard that must be preserved post-migration.
- `bunfig.toml` — `[test]` block with `preload`, `timeout`, and `coverage` settings that `bun test --parallel` will pick up unchanged.
- `package.json` `scripts.test` — `"bash scripts/test-isolated.sh"`, the single entry point both local dev and CI call.
- `.github/workflows/ci.yml:158-164` — the CI `Test` step (single-job, no matrix today).
- `.github/workflows/ci.yml:42-66` — the Postgres + Valkey `services:` block so non-zero skips in CI are a regression signal.
- `scripts/check-test-globs.ts:1-30` — context on the previously-shipped glob-drift fix (issue #201) the runner already needed.
**External:**
- [Bun test runner docs (concurrency + isolation flags)](https://bun.com/docs/test) — `--concurrent`, `--max-concurrency`, `--isolate`, `--parallel`, `--shard` semantics.
- [Bun test deep dive: parallel execution, isolation, sharding](https://bunjs.run/bun-test-runner-deep-dive) — work-stealing scheduling, atomic per-test output buffering, sort-by-path determinism that makes `--shard=M/N` reproducible on every runner.
- [Bun on X: `bun test --parallel`](https://x.com/bunjavascript/status/2044694547763191885) — public release note for the file-level parallel runner referenced above.
## Suggested Next Steps
1. Add a benchmark commit that runs `time bun test --parallel` and `time bash scripts/test-isolated.sh` in CI just once, on the current `main` SHA, to record a project-specific baseline (the 41.9s / 19.3s numbers above were measured locally on an 8-core box).
2. Spike a replacement `scripts/test-isolated.sh` (keep the filename for `package.json` stability) that just wraps `bun test --parallel "$@"` and re-parses the final summary line for non-zero skip counts to preserve the guard at `scripts/test-isolated.sh:33-35`. Confirm `bunfig.toml` `preload` + `coverage` still flow through (Bun applies `[test]` settings to `--parallel` workers).
3. Update `.github/workflows/ci.yml` `Test` step to either (a) the in-place faster runner from step 2, or (b) a `strategy.matrix: shard: [1, 2, 3, 4]` that invokes `bun test --parallel --shard=${{ matrix.shard }}/4`. Option (b) requires duplicating the `services:` Postgres + Valkey block per matrix leg.
4. Decide policy for the 49 `mock.module`-using files vs. the 101 that don't: `--parallel` handles both via `--isolate` so no split is required, but document the decision in `scripts/test-isolated.sh`'s header so the original "process-global mock" reason for the bash loop doesn't get re-discovered and reintroduced.
5. Remove the `scripts/check-test-globs.ts` guard's reference to a `tests=( ... )` array literal (`scripts/check-test-globs.ts:42-62`) once the bash loop is gone, or migrate it to parse the new wrapper.
## Areas Evaluated
- Read `CLAUDE.md` (testing section, CI-enforced doc gates).
- Read `scripts/test-isolated.sh` (full file, 67 lines).
- Read `scripts/check-test-globs.ts` (full file, to verify the static-guard companion).
- Read `bunfig.toml` and `package.json` `scripts` block.
- Read `.github/workflows/ci.yml` (lint-and-test job, services block, test step).
- Read `test/preload.ts` to verify what worker processes inherit.
- Inspected `test/integration/` (7 files) for DB-conflict potential under parallel runs.
- Sampled `mock.module` usage across 49 of 150 test files via grep.
- Timed `bash scripts/test-isolated.sh`, `bun test --parallel`, and `bun test --parallel --shard=1/4` on the current `main` checkout (Bun 1.3.14, 8-core box, deps installed via `bun install`).
- Cross-checked against the 30+ existing `research`-labelled issues; the two testing-area ones (`#16611` ESLint `.only`/`.skip` gate, the closed test-globs glob drift fix) target unrelated problems.
*Generated by the scheduled research action on 2026-06-23*
Contributor guide
Research direction
Read scripts/test-isolated.sh:1-67, especially the loop at 18-19 and skip guard at 33-35, then run the existing test entry point from package.json and compare it with bun test --parallel. Check bunfig.toml and .github/workflows/ci.yml:42-66,158-164 to confirm settings and services remain covered. Done means native parallel execution, preserved silent-skip failure behavior, and a documented or implemented CI sharding decision.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- bash, bun, github-actions, typescript
- Domain
- ci-cd, performance, testing, tooling
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100