Skip to content

test(install): serve GitHub tarball fixtures locally in bun-add.test.ts - #35149

Closed
robobun wants to merge 3 commits into
mainfrom
farm/72949f35/hermetic-github-add-tests
Closed

robobun wants to merge 3 commits into
mainfrom
farm/72949f35/hermetic-github-add-tests

Conversation

@robobun

@robobun robobun commented Jul 22, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

test/cli/install/bun-add.test.ts went red on every retry in build 77839 (and 77787/77818) with:

error: GET https://api.github.com/repos/mishoo/UglifyJS/tarball/v3.14.1 - 504
error: mishoo/UglifyJS#v3.14.1 failed to resolve

Seven tests in this file fetch real tarballs from api.github.com (via the owner/repo#ref dependency form, which alloc_github_url turns into …/repos/{owner}/{repo}/tarball/{ref}). bun already retries 5xx up to max_retry_count times, but the retries are immediate with no backoff, so a GitHub 504 window exhausts all 6 attempts. The tests themselves exercise bun add's GitHub-shorthand handling (package.json writing, bin linking, cache folder naming, resolved-commit extraction from the tarball root directory), not GitHub.

Fix

Make the affected tests hermetic:

  • test/cli/install/dummy.registry.ts: add makeGithubTarball(rootDir, files) (builds a gzip'd tar whose single top-level directory matches GitHub's <owner>-<repo>-<short-sha> layout) and setGithubTarball(owner, repo, ref, bytes). The existing server now routes /repos/:owner/:repo/tarball/:ref to those fixtures so an install under test can point GITHUB_API_URL at it.
  • test/cli/install/bun-add.test.ts: register fixtures for mishoo/UglifyJS#v3.14.1, dylan-conway/install-test-3#v1.0.{0,1,2} and liz3/empty-bun-repo in beforeAll, and pass GITHUB_API_URL for the seven GitHub-tarball tests. The fixtures reproduce the exact directory listing, package.json name/version and bin entry the tests already assert, so every existing expect (including the @GH@mishoo-UglifyJS-e219a9a@@@1 cache-folder name and the e219a9a resolved commit in stdout, which bun reads from the tarball's root directory name) is preserved unchanged.

The two tests that go through git clone (should handle Git URL in dependencies (SCP-style) and git dep without package.json and with default branch) do not use GITHUB_API_URL and are left alone; #35035 addresses the SCP-style one.

Why this is the right fix

These tests have no reason to depend on api.github.com being up: they are asserting bun's behavior, and every byte of the GitHub response they care about is now produced locally. #34419 applied the same pattern (point GITHUB_API_DOMAIN at a local server) to bun create; this extends it to the install path's GITHUB_API_URL. The new makeGithubTarball/setGithubTarball helpers are exported so the other files in the same cluster (bunx.test.ts, bun-install-lifecycle-scripts.test.ts) can adopt them in their own follow-ups.

Verification

$ bun bd test test/cli/install/bun-add.test.ts
 54 pass
 0 fail
Ran 54 tests across 1 file. [21.66s]

Release build: 54 pass / 0 fail in 3.28s (down from ~58s before, since no network round-trips to GitHub). A couple of unrelated files that import dummy.registry.ts (bun-remove.test.ts, bun-install-retry.test.ts) behave identically before and after.

No single culprit PR: the live-GitHub dependency in these tests predates the Rust port.


no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.

The GitHub-dependency tests in bun-add.test.ts fetched real tarballs from
api.github.com. When GitHub serves 5xx (as during the 2026-07-22 outage),
all 6 attempts fail and the tests go red on every lane.

dummy.registry.ts now exposes makeGithubTarball()/setGithubTarball() and
routes /repos/:owner/:repo/tarball/:ref on the existing server. The affected
tests point GITHUB_API_URL at it and consume fixtures that reproduce the
GitHub tarball layout (root dir <owner>-<repo>-<short-sha>), so every
assertion (resolved commit, bin links, cache folder name, directory listing)
is preserved. The file drops from ~58s to ~3s.
@coderabbitai

coderabbitai Bot commented Jul 22, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 8 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: d63de947-e657-434b-9560-49f32df84a78

📥 Commits

Reviewing files that changed from the base of the PR and between 47597ab and 9836c2a.

📒 Files selected for processing (2)
  • test/cli/install/bun-add.test.ts
  • test/cli/install/dummy.registry.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Jul 22, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: ready for review. Self-review found no surviving concerns.

test/cli/install/bun-add.test.ts passes on every lane in build 77900:

  • Linux bun bd → 54 pass / 0 fail
  • Windows x64/aarch64 → 54 pass / 0 fail (verified after 9836c2a, which gives each fixture subdirectory a file so Windows extraction creates it)
  • macOS, alpine, debian, ubuntu → all green for this file

Remaining red in that build is unrelated to this diff (test-only change under test/cli/install/):

  • test-net-connect-memleak.js / test-gc-http-client-connaborted.js on ubuntu 25.04 x64: GC/net tests, reported separately
  • :darwin: 26 aarch64 step failed outside runner: "The number of VMs exceeds the system limit" (CI infra)
  • proxy-stress-errors.test.ts / spawn.test.ts: passed on retry

@robobun

robobun commented Jul 22, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 10:42 AM PT - Jul 22nd, 2026

❌ @robobun, your commit 9836c2a has 3 failures in Build #77900 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 35149

That installs a local version of the PR into your bun-35149 executable, so you can run:

bun-35149 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — test-only change that makes seven GitHub-tarball bun add tests hermetic without weakening any assertions.

What was reviewed:

  • alloc_github_url in src/install/PackageManager/runTasks.rs — confirmed the constructed path (/repos/{owner}/{repo}/tarball/{ref}, including empty-ref) matches the fixture keys and route.
  • requested counter — the new /repos/… route returns before requested++, so expect(requested).toBe(0) in the GitHub tests still holds.
  • Hook ordering — fixture registration only touches a module-level Map; githubEnv() reads root_url at spawn time, after dummyBeforeAll has set it.
  • Checked the route prefix is narrow enough not to collide with other dummy.registry.ts consumers, and that no existing assertion was loosened.
Extended reasoning...

Overview

This PR removes live api.github.com dependencies from seven tests in test/cli/install/bun-add.test.ts by serving equivalent tarball fixtures from the existing dummy.registry.ts server via the GITHUB_API_URL override. It adds two exported helpers (makeGithubTarball, setGithubTarball) and a /repos/:owner/:repo/tarball/:ref route to the shared dummy server. No src/** files are touched.

Security risks

None. Test-infrastructure only; no production code paths, no auth/crypto/permissions changes.

Level of scrutiny

Low-to-medium. The changes are confined to test harness code and follow an established pattern already used by symlink-path-traversal.test.ts (pointing GITHUB_API_URL at a local server). The important thing to verify is that test coverage is preserved rather than weakened — and it is: every existing expect (directory listings, cache-folder names, resolved-commit sha in stdout, bin linking) is left byte-for-byte unchanged, with the fixtures constructed to satisfy them.

Other factors

  • Verified against alloc_github_url that the URL shape (including trailing-slash + empty committish for the liz3/empty-bun-repo case) matches the fixture map keys exactly.
  • The new route intentionally returns before incrementing requested, preserving expect(requested).toBe(0) in the GitHub tests (which previously never touched the local server).
  • The ustar writer is minimal but valid; a similar hand-rolled writer already exists in symlink-path-traversal.test.ts. Placing the new one in dummy.registry.ts makes it reusable for the follow-ups the description names.
  • 54/54 pass on both debug and release per the PR body, and the release-build wall time drops from ~58s to ~3s — a meaningful CI-cost win on top of the flake fix.
  • The fixup commit addressed Windows extraction by giving each asserted subdirectory a real file entry; the remaining {} fixture (install-test-3 v1.0.0) has no subdirectory assertions and only the root dir, so it is unaffected.

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

#39115 adds a githubTarball(rootDir, files) export to test/harness.ts (same shape as makeGithubTarball here, built with Bun.Archive); once it lands this PR can import it instead of carrying its own builder.

@robobun

robobun commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator Author

Closing in favor of #42800. It converts the same seven tests in bun-add.test.ts and leaves their assertions unchanged. Its fixtures come from the githubTarball() helper in test/harness.ts. One /repos/... route in dummy.registry.ts serves them, and bun-install.test.ts uses the same route.

@robobun robobun closed this Sep 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants