test: collapse test/isolated/ into test/lib and test/commands#840
Merged
Conversation
Follow-up to #839. With `bun test --isolate` giving each file a fresh module graph, the test/isolated/ split that existed to work around `mock.module()` pollution (oven-sh/cli#258) is no longer needed. ## Merged into existing files Per-feature tests moved into their natural counterparts: - isolated/brew-upgrade.test.ts → test/lib/upgrade.test.ts - isolated/log-view-prompt.test.ts → test/commands/log/view.test.ts - isolated/login-reauth.test.ts → test/commands/auth/login.test.ts - isolated/project-delete-confirm.test.ts → test/commands/project/delete.test.ts - isolated/set-commits-auto.test.ts → test/lib/api/releases.test.ts - isolated/set-commits-auto-no-remote.test.ts → test/lib/api/releases.test.ts (merged as a single setCommitsAuto describe; getRepositoryName is now a controllable `mock()`) - isolated/dsn/project-root.test.ts → test/lib/dsn/project-root.test.ts Pattern: `mock.module()` at the top of the merged file, followed by a dynamic `await import()` for the code under test so the mocked bindings win. The rest of the file's static imports stay as-is. Also unblocks a previously-deferred stub in `test/lib/api/releases.test.ts` that said "setCommitsAuto tests live in test/isolated/ because they require mock.module() for git helpers". ## Moved as sibling files Two isolated files tested the same subjects as existing test files but with incompatible mocking strategies. Under `--isolate` they're safe to keep as siblings since module state doesn't leak: - isolated/resolve-target.test.ts → test/lib/resolve-target.mocked.test.ts - isolated/delta-upgrade.test.ts → test/lib/delta-upgrade.mocked.test.ts Both existing `resolve-target.test.ts` and `delta-upgrade.test.ts` use real DB + mocked fetch; the `.mocked.` siblings stub out every collaborator via `mock.module()`. The two styles can coexist in the same directory now that module graphs are per-file. ## New files (no prior counterpart) Three isolated DSN helpers had no target file in test/lib/dsn/: - isolated/dsn/errors.test.ts → test/lib/dsn/errors.test.ts - isolated/dsn/fs-utils.test.ts → test/lib/dsn/fs-utils.test.ts - isolated/dsn/resolver.test.ts → test/lib/dsn/resolver.test.ts ## Removed `scopedFetchMock` helper `test/lib/sentry-client.test.ts` previously wrapped all its fetch mocks with `scopedFetchMock(marker, handler)` to filter out foreign fetch calls that leaked from other test files (CI run 24835339085). Under `bun test --isolate` each file gets a fresh `globalThis.fetch`, so cross-file leakage is eliminated. The 6 call sites now use `mockFetch()` directly; the helper and its `urlOf()` dependency are gone (~30 lines). ## Other cleanups - Removed `test/isolated/` directory - Updated `package.json` `test:unit` to drop `test/isolated` from the argument list (files now live under `test/lib/` and `test/commands/`) - `test/lib/dsn/project-root.test.ts`: `noopSpan` stub now includes `setAttributes` (production code calls both `setAttribute` and `setAttributes`) - Replaced several `() => {}` test stubs with named `noop` functions to satisfy `lint/suspicious/noEmptyBlockStatements` without per-line biome-ignore comments ## Verification - `bun run test:unit` — 5920 pass / 0 fail across 253 files (was 260 — merges consolidated 12 files into 5 new + 7 existing) - 3 consecutive stress runs clean - `bun run typecheck`, `bun run lint` - `bun run lint:fix` applied one formatting tweak
Contributor
Codecov Results 📊✅ 5920 passed | Total: 5920 | Pass Rate: 100% | Execution Time: 0ms 📊 Comparison with Base Branch
✨ No test changes detected All tests are passing successfully. ✅ Patch coverage is 100.00%. Project has 13025 uncovered lines. Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 75.10% 75.16% +0.06%
==========================================
Files 284 284 —
Lines 52482 52437 -45
Branches 0 0 —
==========================================
+ Hits 39412 39412 —
- Misses 13070 13025 -45
- Partials 0 0 —Generated by Codecov Action |
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.
Follow-up to #839. With
bun test --isolategiving each file a fresh module graph, thetest/isolated/split that existed to work aroundmock.module()registry pollution (oven-sh/cli#258) is no longer needed.Merged into existing files
Per-feature tests now live with their natural counterparts:
isolated/brew-upgrade.test.tstest/lib/upgrade.test.tsisolated/log-view-prompt.test.tstest/commands/log/view.test.tsisolated/login-reauth.test.tstest/commands/auth/login.test.tsisolated/project-delete-confirm.test.tstest/commands/project/delete.test.tsisolated/set-commits-auto.test.tstest/lib/api/releases.test.tsisolated/set-commits-auto-no-remote.test.tstest/lib/api/releases.test.ts(merged together —getRepositoryNameis now a controllablemock())isolated/dsn/project-root.test.tstest/lib/dsn/project-root.test.tsPattern:
mock.module()at the top of the merged file, followed by a dynamicawait import()for the code under test so the mocked bindings win. Other static imports stay as-is.Also unblocks the previously-deferred stub in
test/lib/api/releases.test.tsthat said "setCommitsAuto tests live in test/isolated/ because they require mock.module() for git helpers".Moved as sibling files
Two isolated files tested the same subjects as existing files but with incompatible mocking strategies. Under
--isolatethey coexist safely as siblings:isolated/resolve-target.test.tstest/lib/resolve-target.mocked.test.tsisolated/delta-upgrade.test.tstest/lib/delta-upgrade.mocked.test.tsThe existing
resolve-target.test.tsanddelta-upgrade.test.tsuse real DB + mocked fetch; the.mocked.siblings stub every collaborator viamock.module().New files (no prior counterpart)
Three isolated DSN helpers had no target file in
test/lib/dsn/:isolated/dsn/errors.test.ts→test/lib/dsn/errors.test.tsisolated/dsn/fs-utils.test.ts→test/lib/dsn/fs-utils.test.tsisolated/dsn/resolver.test.ts→test/lib/dsn/resolver.test.tsRemoved
scopedFetchMockhelpertest/lib/sentry-client.test.tspreviously wrapped all its fetch mocks withscopedFetchMock(marker, handler)to filter out foreign fetch calls that leaked from other test files (CI run 24835339085). Underbun test --isolateeach file gets a freshglobalThis.fetch, so cross-file leakage is eliminated. The 6 call sites now usemockFetch()directly; the helper and itsurlOf()dependency are gone (~30 lines).Other cleanups
test/isolated/directorypackage.jsontest:unitto droptest/isolatedfrom the argument listtest/lib/dsn/project-root.test.ts:noopSpanstub now includessetAttributes(production code calls bothsetAttributeandsetAttributes)() => {}test stubs with namednoopfunctions to satisfylint/suspicious/noEmptyBlockStatementswithout per-line biome-ignore commentsVerification
bun run test:unit— 5920 pass / 0 fail across 253 files (was 260 — merges consolidated 12 files into 5 new + 7 existing)bun run typecheck,bun run lintNet diff: +1219 / −3830 (most "deletions" are file moves; real code reduction is the
scopedFetchMockremoval plus deduplication from merging).