Skip to content

fix(core/test): split connection.test.ts from DB-test batch to avoid mock pollution#1269

Merged
Wirasm merged 1 commit intodevfrom
fix/connection-test-mock-pollution
Apr 17, 2026
Merged

fix(core/test): split connection.test.ts from DB-test batch to avoid mock pollution#1269
Wirasm merged 1 commit intodevfrom
fix/connection-test-mock-pollution

Conversation

@Wirasm
Copy link
Copy Markdown
Collaborator

@Wirasm Wirasm commented Apr 17, 2026

Summary

packages/core/src/db/messages.test.ts:9 uses mock.module('./connection', ...) at module-load time. Per CLAUDE.md test-isolation rules and oven-sh/bun#7823, mock.module() is process-global and irreversible — mock.restore() does NOT undo it.

When Bun pre-loads all test files in a single batch invocation, the mock replaces the real ./connection module before connection.test.ts even runs. So getDatabaseType() returns the mocked 'postgresql' regardless of DATABASE_URL, and three tests fail:

  • connection > getDatabaseType > should return sqlite when DATABASE_URL is not set
  • connection > getDatabaseType > should return sqlite when DATABASE_URL is empty string
  • connection > getDatabaseType > should not initialize database connection

These are pre-existing on dev — I hit them while working on an unrelated branch and traced the root cause.

Fix

One-line package.json edit: move src/db/connection.test.ts into its own bun test invocation, immediately after postgres.test.ts, before the shared DB/utils/config/state batch that contains messages.test.ts.

Follows the same isolation pattern already applied to command-handler.test.ts, clone.test.ts, postgres.test.ts, and path-validation.test.ts.

Test plan

  • bun --filter @archon/core test — all 355 tests pass, 0 fail (previously 3 fail)
  • bun run validate — fully green end-to-end

Summary by CodeRabbit

  • Chores
    • Reorganized test execution order in the build configuration.

…mock pollution

messages.test.ts uses mock.module('./connection', ...) at module-load time.
Per CLAUDE.md:131 (Bun issue oven-sh/bun#7823), mock.module() is process-
global and irreversible. When Bun pre-loads all test files in a batch, the
mock shadows the real connection module before connection.test.ts runs,
causing getDatabaseType() to always return the mocked value regardless of
DATABASE_URL.

Move connection.test.ts into its own `bun test` invocation immediately
after postgres.test.ts (which runs alone) and before the big DB/utils/
config/state batch that contains messages.test.ts. This follows the same
isolation pattern already used for command-handler, clone, postgres, and
path-validation tests.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 17, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c6318f0e-2c38-4e3a-aae7-e21757e53371

📥 Commits

Reviewing files that changed from the base of the PR and between bed36ca and 889ffca.

📒 Files selected for processing (1)
  • packages/core/package.json

📝 Walkthrough

Walkthrough

The test npm script in packages/core/package.json was reordered to segment the database test execution differently. The change reorganizes how bun test commands are chained together, separating src/db/adapters/sqlite.test.ts from subsequent database tests to prevent test isolation issues.

Changes

Cohort / File(s) Summary
Test Script Configuration
packages/core/package.json
Reordered test command chain to segment database test execution separately, addressing test state pollution between bun test runs.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Possibly related issues

Poem

🐰 A hop, a skip, a script so neat,
Tests now dance in separate beat,
No more tangled mock.module mess,
Bun runs clean—a fuzzy success! ✨

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/connection-test-mock-pollution

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@Wirasm Wirasm merged commit 301a139 into dev Apr 17, 2026
3 of 4 checks passed
ztech-gthb pushed a commit to ztech-gthb/Archon that referenced this pull request Apr 18, 2026
…mock pollution (coleam00#1269)

messages.test.ts uses mock.module('./connection', ...) at module-load time.
Per CLAUDE.md:131 (Bun issue oven-sh/bun#7823), mock.module() is process-
global and irreversible. When Bun pre-loads all test files in a batch, the
mock shadows the real connection module before connection.test.ts runs,
causing getDatabaseType() to always return the mocked value regardless of
DATABASE_URL.

Move connection.test.ts into its own `bun test` invocation immediately
after postgres.test.ts (which runs alone) and before the big DB/utils/
config/state batch that contains messages.test.ts. This follows the same
isolation pattern already used for command-handler, clone, postgres, and
path-validation tests.
joaobmonteiro pushed a commit to joaobmonteiro/Archon that referenced this pull request Apr 26, 2026
…mock pollution (coleam00#1269)

messages.test.ts uses mock.module('./connection', ...) at module-load time.
Per CLAUDE.md:131 (Bun issue oven-sh/bun#7823), mock.module() is process-
global and irreversible. When Bun pre-loads all test files in a batch, the
mock shadows the real connection module before connection.test.ts runs,
causing getDatabaseType() to always return the mocked value regardless of
DATABASE_URL.

Move connection.test.ts into its own `bun test` invocation immediately
after postgres.test.ts (which runs alone) and before the big DB/utils/
config/state batch that contains messages.test.ts. This follows the same
isolation pattern already used for command-handler, clone, postgres, and
path-validation tests.
@Wirasm Wirasm deleted the fix/connection-test-mock-pollution branch April 27, 2026 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant