🤖 refactor: eliminate TEST_INTEGRATION env var and per-file boilerplate#2392
🤖 refactor: eliminate TEST_INTEGRATION env var and per-file boilerplate#2392ammar-agent wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6d66bbfdd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const hasAnthropicKey = Boolean(process.env.ANTHROPIC_API_KEY); | ||
| const describeWithApi = hasAnthropicKey ? describe : describe.skip; |
There was a problem hiding this comment.
Require explicit opt-in for real-API StreamManager test
This change gates the real Anthropic test on ANTHROPIC_API_KEY alone, so it now runs during bun test src whenever that env var is present. Because make test/make test-unit executes bun test src (Makefile lines 340-343), developers or CI environments with a key exported will unexpectedly hit live API calls in what is supposed to be a unit-test path, causing slow/flaky/costly runs and potential failures from transient API issues.
Useful? React with 👍 / 👎.
Remove shouldRunIntegrationTests() gating from 60+ test files across
tests/ipc/, tests/ui/, tests/run/, and src/. Phase 3 of integration
test restructuring.
Infrastructure changes:
- tests/testUtils.ts: Remove shouldRunIntegrationTests(). Simplify
validateApiKeys() and getApiKey() to not depend on env var.
- tests/ipc/setup.ts: createTestEnvironment() now accepts
{ seedDummyKeys?: boolean } option instead of checking env var.
Always seeds dummy keys by default (tests using real API keys call
setupProviders() which overwrites them).
- tests/setup.ts: Remove conditional preload block. Tests needing
AI SDK preloading call preloadTestModules() in their own beforeAll.
- jest.config.js: Replace TEST_INTEGRATION ternary with fixed 120s
timeout. AI tests set jest.setTimeout(600_000) individually.
Test file changes:
- Remove shouldRunIntegrationTests import + describeIntegration
pattern from all 22 UI test files and 38 IPC test files
- Replace conditional describe wrappers with bare describe()
- Add jest.setTimeout(600_000) to 26 AI-calling test files
- Files with compound conditions (anthropicCacheStrategy, ollama,
system1BashCompaction) now check only key/service availability
- src/node/services/streamManager.test.ts: Replace env var check
with key-availability gate for its API integration section
- Remove stale compiled testUtils.js/.js.map artifacts
Net: -323 lines of boilerplate removed across 69 files.
e6d66bb to
4f59470
Compare
Summary
Eliminate the
TEST_INTEGRATIONenv var andshouldRunIntegrationTests()per-file boilerplate from 72 files. Phase 3 of the integration test restructuring (Phase 1: #2370, Phase 2: #2389).Background
Every integration test file repeated the same pattern:
This conflated "integration test" with "needs real AI API keys" — two different concerns. Tests in
tests/are integration tests by definition; whether they need API keys should be expressed differently.Implementation
Infrastructure (4 files)
tests/testUtils.tsshouldRunIntegrationTests(). SimplifiedvalidateApiKeys()/getApiKey()to not depend on env var.tests/ipc/setup.tscreateTestEnvironment({ seedDummyKeys?: boolean })— explicit parameter replaces env var check. Defaulttrueseeds dummy keys for UI tests;setupProviders()overwrites for real-key tests.tests/setup.tspreloadTestModules()in their ownbeforeAll.jest.config.jsTEST_INTEGRATIONternary). AI tests setjest.setTimeout(600_000)individually.Test files (65 files)
shouldRunIntegrationTestsimport +describeIntegrationwrapper → baredescribe()jest.setTimeout(600_000).anthropicCacheStrategy,ollama,system1BashCompactionnow check only key/service availability (notTEST_INTEGRATION)src/node/services/streamManager.test.ts: Replaced env var check with key-availability gate (describeWithApi)tests/run/smoke.integration.test.ts: Same pattern removal +jest.setTimeouttests/testUtils.js+.js.mapartifacts// Skip all tests if TEST_INTEGRATION is not setcomments and Docker skip messagesDeveloper documentation & CI (3 files)
.mux/skills/tests/SKILL.mdTEST_INTEGRATION=1/shouldRunIntegrationTests()guidance. Added guidance onjest.setTimeout(600_000)andvalidateApiKeys()for AI tests. Updated example test paths..github/workflows/pr.ymlTEST_INTEGRATION=1prefixes from CI jest invocations (4 occurrences) andTEST_INTEGRATIONenv var from Windows smoke test job.MakefileTEST_INTEGRATION=1fromtest-integrationtarget.Net impact: -379 lines across 72 files
Validation
make typecheckcleanmake lintcleanmake fmt-checkcleanTEST_INTEGRATION=1validateApiKeys()fails fast when keys missingRisks
jest.config.jstimeout raised from 10s → 120s for all tests. This is safe because unit tests insrc/run underbun test(not jest), and integration tests intests/universally need >10s for service container setup.tests/runtime/files retain their own localshouldRunIntegrationTests()andTEST_INTEGRATIONcheck — these are self-contained, Docker-gated tests with separate infrastructure. Out of scope for this change.src/files that checkTEST_INTEGRATIONfor telemetry suppression or network-calling unit test gating are unrelated concerns and left untouched.Generated with
mux• Model:anthropic:claude-opus-4-6• Thinking:xhigh• Cost:$25.47