Skip to content

fix(test): isolate mockCurrentUser in announcement-admin-route tests - #6991

Merged
garvitkaushik-123 merged 3 commits into
adcontextprotocol:mainfrom
garvitkaushik-123:fix/announcement-test-mock-isolation
Aug 29, 2026
Merged

fix(test): isolate mockCurrentUser in announcement-admin-route tests#6991
garvitkaushik-123 merged 3 commits into
adcontextprotocol:mainfrom
garvitkaushik-123:fix/announcement-test-mock-isolation

Conversation

@garvitkaushik-123

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

Copy link
Copy Markdown
Collaborator

Summary

Eliminate thread-safety issues in test files that run under the root vitest config (pool: 'threads') by isolating shared mutable state. This addresses root cause (3) from the #6740 triage and hardens all other process.env mutation sites found in the tests/ tree against cross-thread bleed.

Root causes (1) and (2) were fixed in #6800.

Changes

tests/announcement/announcement-admin-route.test.ts — shared mutable mock object

  • Replace mockCurrentUser shared object with mockGetCurrentUser vi.fn() factory
  • Auth middleware mocks call mockGetCurrentUser() per invocation instead of sharing a reference
  • 403 test uses mockReturnValue() instead of property mutation

tests/announcement/announcement-channel-resolver.test.ts — env var leak

  • Replace manual process.env delete/restore with vi.stubEnv + vi.unstubAllEnvs
  • Remove afterEach and ORIGINAL_ENV manual save/restore pattern
  • SLACK_EDITORIAL_REVIEW_CHANNEL mutations no longer leak to concurrent workers

tests/addie/admin-tools.test.ts — env var leak

  • Replace unconditional process.env.ADMIN_API_KEY / process.env.BASE_URL assignments with vi.stubEnv
  • Add vi.unstubAllEnvs() in beforeEach for automatic restoration

tests/billing/webhook-security.test.ts — env var leak

  • Replace direct process.env.STRIPE_WEBHOOK_SECRET / STRIPE_SECRET_KEY assignments with vi.stubEnv
  • Replace manual delete/restore in "webhook secret must be configured" test
  • Add vi.unstubAllEnvs() in beforeEach for automatic restoration

Verification

  • npx vitest run tests/announcement/ — 175/175 passed
  • npx vitest run tests/addie/admin-tools.test.ts — 30/30 passed
  • npx vitest run tests/billing/webhook-security.test.ts — 7/7 passed
  • Full parallel run (12 files, 212 tests) — all passed under threads pool
  • Swept all tests/ files for remaining non-idempotent process.env mutations — zero remaining

Refs #6740

Replace the shared mutable mockCurrentUser object with a vi.fn() factory
(mockGetCurrentUser) that returns a fresh user object per middleware
invocation. The 403 test now uses mockReturnValue instead of mutating the
shared reference.

Under the root vitest config (pool: 'threads'), the shared mutable object
could race with other test files' workers when modules were cached across
threads, producing intermittent 403→404 failures where mock state was
clobbered between mutation and request.

Refs adcontextprotocol#6740
aao-secretariat[bot]
aao-secretariat Bot previously approved these changes Aug 29, 2026

@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 — test-only change with no blocking or medium findings.

This PR touches a single test file (tests/announcement/announcement-admin-route.test.ts, +7/−6), replacing a shared mutable mockCurrentUser object with a mockGetCurrentUser vi.fn() and setting return values via mockReturnValue instead of mutating a shared reference across sequential requests. The fix eliminates in-place mutation of shared state and is self-consistent.

No protocol surface (static/schemas/source/**), docs, changeset, or released-artifact scope is touched — schema↔docs coherence, changeset, oneOf, and immutability gates do not apply. No no-auto-approve team match, gated_paths is false, and high_risk is false.

Decision table: rows 1–8 do not fire (no critical/high, no medium, no gated paths, no deletions, no team gate). Falls through to row 9 → approve. Note review_decision is REVIEW_REQUIRED, but with gated_paths: false and no other trigger, that does not block approval.

…ests

announcement-channel-resolver: replace manual process.env delete/restore
with vi.stubEnv + vi.unstubAllEnvs. The old pattern leaked
SLACK_EDITORIAL_REVIEW_CHANNEL mutations to concurrent thread workers
under the root vitest threads pool.

addie/admin-tools: replace unconditional process.env assignments
(ADMIN_API_KEY, BASE_URL) with vi.stubEnv so values are automatically
restored after each test, preventing cross-thread env bleed.

Refs adcontextprotocol#6740
Replace direct process.env assignments and manual delete/restore with
vi.stubEnv + vi.unstubAllEnvs in beforeEach. The old pattern leaked
STRIPE_WEBHOOK_SECRET and STRIPE_SECRET_KEY mutations to concurrent
thread workers under the root vitest threads pool.

Refs adcontextprotocol#6740

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

Re-approval: no reviewable delta since the prior approval; re-submitting after stale-review dismissal.

@garvitkaushik-123
garvitkaushik-123 merged commit cfa288b into adcontextprotocol:main Aug 29, 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.

1 participant