Skip to content

fix(test): isolate server unit tests from thread-pool env bleed - #6800

Merged
bokelley merged 2 commits into
adcontextprotocol:mainfrom
garvitkaushik-123:fix/flaky-server-unit-tests
Aug 23, 2026
Merged

fix(test): isolate server unit tests from thread-pool env bleed#6800
bokelley merged 2 commits into
adcontextprotocol:mainfrom
garvitkaushik-123:fix/flaky-server-unit-tests

Conversation

@garvitkaushik-123

@garvitkaushik-123 garvitkaushik-123 commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fix the flaky server-unit precommit path by ensuring individual changed test files run with the same isolated server Vitest configuration as the full suite.

Root cause: the precommit script's individual-file path (planServerUnitRunkind: 'files') invoked Vitest without --config, so it did not load the server setup and isolation settings. Full-suite runs were unaffected because npm run test:server-unit already passes the server config.

Changes

  1. scripts/precommit-server-unit.cjs — pass --config server/vitest.config.ts on the individual-file path.
  2. server/vitest.config.ts — explicitly declare pool: 'forks' instead of relying on the default.

Verification

  • npx vitest run tests/precommit-server-unit.test.ts — 14 passed
  • npm exec -- vitest run --config server/vitest.config.ts server/tests/unit/auth-api-key-format.test.ts — 5 passed

Refs #6740

garvitkaushik-123 and others added 2 commits August 23, 2026 22:45
The precommit individual-file path ran server tests through the root
vitest config (pool: threads), letting process.env mutations in
vi.hoisted() leak between concurrent workers. Full-suite runs used the
server config (forks, sequential) and were unaffected, creating an
intermittent-only failure signature that varied by test ordering.

Three fixes:

1. Pass --config server/vitest.config.ts on the individual-file
   precommit path so server tests always fork.

2. Declare pool: 'forks' explicitly in server/vitest.config.ts
   instead of relying on the implicit default.

3. Route tests/announcement/** and tests/billing/** to the forks
   pool in the root vitest config via poolMatchGlobs — these
   directories set WORKOS/Stripe secrets in vi.hoisted() and cannot
   share process.env with parallel thread workers.

Also add tests/setup/env-defaults.ts to pre-set common WORKOS env
defaults before any root test file runs.

Refs adcontextprotocol#6740
@bokelley

Copy link
Copy Markdown
Contributor

I pushed a maintainer follow-up that narrows this to the effective fix:

  • pass --config server/vitest.config.ts for changed-file precommit runs;
  • keep the server pool explicitly set to forks;
  • remove the protocol changeset (this is operational test infrastructure);
  • remove the root env defaults and poolMatchGlobs changes. poolMatchGlobs is not supported by the pinned Vitest 4.1.10, and global env defaults could mask test isolation bugs.

Verified with:

  • npx vitest run tests/precommit-server-unit.test.ts — 14 passed
  • npm exec -- vitest run --config server/vitest.config.ts server/tests/unit/auth-api-key-format.test.ts — 5 passed

@aao-secretariat aao-secretariat 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.

Ladon verdict: Approve

Approve — two-file test-infra fix with no blocking findings.

What I checked:

  • scripts/precommit-server-unit.cjs now passes --config server/vitest.config.ts on the individual-file path, so partial-commit server unit runs use the server config instead of falling through to the root threads pool. Correct and consistent with the full path (package.json:122 test:server-unit already used the server config).
  • server/vitest.config.ts makes pool: 'forks' explicit — sound.
  • No protocol surface touched (static/schemas/source/**, docs/reference/**, dist/**, task defs), so no changeset is required. Schema↔docs coherence, oneOf discriminator, artifact immutability, and 3.1.x eligibility gates are all N/A.

Medium findings: none.

Note (low, non-blocking): The PR description overclaims — items 3-4 (a root vitest.config.ts poolMatchGlobs edit, a new tests/setup/env-defaults.ts, and 1052 root tests) are referenced but not present in the diff (only 2 files changed). The actual change on disk is sound; the mismatch is a description accuracy issue, not a code defect.

No critical/high/medium findings. gated_paths is false, high_risk is false, no author-team gate. Falls through to row 9 → approve. (review_decision: REVIEW_REQUIRED only blocks approval when gated_paths is true, which it is not here.)

@bokelley
bokelley enabled auto-merge (squash) August 23, 2026 19:18
@bokelley
bokelley merged commit a29e3c8 into adcontextprotocol:main Aug 23, 2026
27 checks passed
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.

2 participants