Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds Bun runtime execution coverage to the E2E CLI harness so the arkor bundled CLI can be validated under Bun (in addition to using Bun as a package manager), and ensures CI installs Bun so those tests actually run.
Changes:
- Extended the E2E CLI spawner (
runCli) to support selecting the runtime (nodevsbun) and added Bun binary detection (findBunBin()). - Added a new E2E suite that runs
arkor initandarkor buildunder the Bun runtime (auto-skips when Bun isn’t available locally). - Updated CI build jobs to install a pinned Bun version so the Bun-runtime suite doesn’t auto-skip in CI.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| e2e/cli/src/spawn-cli.ts | Adds runtime selection to spawned CLI processes and implements Bun detection/caching. |
| e2e/cli/src/bun-runtime.test.ts | New E2E tests that execute core CLI flows under the Bun runtime. |
| .github/workflows/ci.yaml | Provisions a pinned Bun version in the build matrix so Bun-runtime tests run in CI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+29
to
+31
| * machines without bun aren't penalised. CI provisions bun | ||
| * explicitly for the bun-runtime job, so the cache hits on the | ||
| * runner's pre-installed binary. |
| // runtime) instead of `node` and assert it produces the same | ||
| // observable output. They auto-skip when `bun` isn't on PATH so | ||
| // local developers without bun installed don't see spurious | ||
| // failures; CI provisions bun for the dedicated bun-runtime job. |
Comment on lines
243
to
247
| argv: string[], | ||
| cwd: string, | ||
| extraEnv: NodeJS.ProcessEnv = {}, | ||
| runtime: Runtime = "node", | ||
| ): Promise<RunResult> { |
Comment on lines
+40
to
+55
| export function findBunBin(): string | undefined { | ||
| if (bunBinPath !== undefined) return bunBinPath ?? undefined; | ||
| // Probe by trying to invoke `bun --version` directly. We can't use | ||
| // `command -v` (a shell builtin, not an executable, so `spawnSync` | ||
| // can't run it) or `which` (not standard on Windows). `spawnSync` | ||
| // does its own PATH lookup, so a successful exit with version-shaped | ||
| // stdout is sufficient evidence that `bun` is invocable from the | ||
| // same env the test suite will spawn under. Use `shell: true` on | ||
| // Windows for the same reason `runCli`'s underlying `spawn` does: | ||
| // bun ships as `bun.exe`, but `setup-bun` adds `~/.bun/bin` to | ||
| // PATH where some Windows shells need `cmd` to resolve the lookup. | ||
| const probe = spawnSync("bun", ["--version"], { | ||
| stdio: ["ignore", "pipe", "ignore"], | ||
| shell: process.platform === "win32", | ||
| }); | ||
| if (probe.status !== 0 || probe.stdout === null) { |
Comment on lines
+56
to
+60
| // same env the test suite will spawn under. Use `shell: true` on | ||
| // Windows for the same reason `runCli`'s underlying `spawn` does: | ||
| // bun ships as `bun.exe`, but `setup-bun` adds `~/.bun/bin` to | ||
| // PATH where some Windows shells need `cmd` to resolve the lookup. | ||
| const probe = spawnSync("bun", ["--version"], { |
…lity with pnpm workspaces
Comment on lines
+1
to
3
| import { spawn, spawnSync } from "node:child_process"; | ||
| import { cpSync, mkdtempSync, readdirSync, rmSync } from "node:fs"; | ||
| import { tmpdir } from "node:os"; |
Comment on lines
+106
to
+109
| - name: Setup bun | ||
| uses: oven-sh/setup-bun@v2 | ||
| with: | ||
| bun-version: '1.3.13' |
Comment on lines
+507
to
+511
| child = spawn(runtimeBin, [binPath, ...argv], { | ||
| cwd, | ||
| // Defer to the shell only for the bun runtime on Windows, where | ||
| // PATH may resolve `bun` via a `.cmd` shim that direct spawn | ||
| // can't execute (same policy as `cli-internal`'s install |
The spawn() options object literal was under-indented (aligned with
the opening `{` instead of nested one level deeper), left over from
wrapping the spawn call in a try/catch without re-indenting the body.
Reindent the options object and its nested `env` to match surrounding
formatting. No behaviour change.
Comment on lines
+49
to
+53
| const spawnMock = vi.hoisted(() => vi.fn()); | ||
| vi.mock("node:child_process", () => ({ spawn: spawnMock })); | ||
| const spawnSyncMock = vi.hoisted(() => vi.fn()); | ||
| vi.mock("node:child_process", async () => ({ | ||
| ...(await vi.importActual<typeof nodeChildProcess>("node:child_process")), | ||
| spawn: spawnMock, |
… compliance with linting rules
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
| cleanup(); | ||
| reject( | ||
| new Error( | ||
| "runCli({ runtime: 'bun' }) was called but `findBunBin()` " + |
Comment on lines
+1042
to
+1043
| // PR #159 Copilot review (follow-up): when `runCli({ runtime: | ||
| // "bun" })` is invoked but `findBunBin()` returns undefined (the |
Comment on lines
+914
to
+923
| function ok(stdout: string) { | ||
| return { | ||
| status: 0, | ||
| stdout: Buffer.from(stdout), | ||
| stderr: Buffer.from(""), | ||
| pid: 1, | ||
| output: ["", Buffer.from(stdout), Buffer.from("")], | ||
| signal: null, | ||
| } as SpawnSyncReturns<Buffer>; | ||
| } |
Comment on lines
+924
to
+933
| function fail() { | ||
| return { | ||
| status: 1, | ||
| stdout: null, | ||
| stderr: Buffer.from(""), | ||
| pid: 1, | ||
| output: ["", null, Buffer.from("")], | ||
| signal: null, | ||
| } as unknown as SpawnSyncReturns<Buffer>; | ||
| } |
…onsistency with real spawnSync behavior
Comment on lines
+1080
to
+1087
| spawnSyncMock.mockReturnValueOnce({ | ||
| status: 1, | ||
| stdout: null, | ||
| stderr: Buffer.from(""), | ||
| pid: 1, | ||
| output: ["", null, Buffer.from("")], | ||
| signal: null, | ||
| } as unknown as SpawnSyncReturns<Buffer>); |
…l process exit behavior
Comment on lines
+934
to
+937
| // Model the "ran and exited non-zero" shape here since the | ||
| // `--version` probe never hits the spawn-failed branch (the spawn | ||
| // succeeds, the child just exits 1 because there's no usable | ||
| // `bun`). Keeps the mock honest about which `findBunBin` branch |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request adds end-to-end (E2E) tests to ensure the
arkorCLI works correctly when run under the Bun JavaScript runtime, in addition to Node.js. It introduces runtime selection logic to the CLI test harness, provisions Bun in the CI matrix, and adds tests that verify core CLI flows under Bun. These changes improve cross-runtime compatibility and increase test coverage for real-world usage scenarios.Bun runtime support and E2E test coverage:
bun-runtime.test.ts) that runs keyarkorCLI commands under the Bun runtime, verifying that project scaffolding and building work as expected and that no Node-only APIs leak into the CLI bundle. The tests automatically skip if Bun is not available, preventing local developer friction.spawn-cli.ts) to support selecting between Node and Bun as the runtime for spawned CLI processes. Added runtime selection logic, Bun binary detection (findBunBin()), and updatedrunCli/runCliOnceto accept a runtime parameter. [1] [2] [3] [4] [5] [6]CI improvements:
ci.yaml) to provision Bun (oven-sh/setup-bun@v2) on every build-matrix job, ensuring the Bun runtime E2E suite runs in CI and exercises the same Bun version as the install-matrix.Summary by CodeRabbit
New Features
Tests
Chores