fix(McpHub): resolve persistently flaky McpHub.spec.ts tests after Vitest 4 upgrade

[edelauna]

Related GitHub Issue

Closes: #489

Description

This PR fixes 35 persistently failing tests in services/mcp/__tests__/McpHub.spec.ts that regressed after the Vitest 4 upgrade. Three compounding root causes were identified and fixed:

1. import fs vs import * as fs mock instance mismatch

McpHub.ts uses import * as fs from "fs/promises" (namespace import). The test used import fs from "fs/promises" (default import). The vi.mock factory created separate vi.fn() instances for the default and named exports, so vi.mocked(fs.readFile).mockResolvedValue(...) in tests was controlling a dead mock — the SUT called the named-export binding which still returned the factory default "{}". Fix: align to import * as fs and share all vi.fn() instances via a closure: const shared = {...}; return { default: shared, ...shared }.

2. Duplicate vi.mock("fs/promises") silently overriding the factory

A bare vi.mock("fs/promises") (no factory) at line ~96 was silently overriding the factory mock above it with Vitest's auto-mock, replacing all carefully constructed vi.fn() instances with new stubs. Removed.

3. Non-async beforeEach racing against async initialization

The outer beforeEach was synchronous, so test bodies started running while initializationPromise (from Promise.all([initializeGlobalMcpServers(), initializeProjectMcpServers()])) was still in flight. Fix: make beforeEach async and await mcpHub.waitUntilReady().

Additional fixes:

• Replaced 15× await new Promise(r => setTimeout(r, 100)) timing hacks with await mcpHub.waitUntilReady() (deterministic)
• Restructured 5× Windows command-wrapping tests: set fs.readFile mock before new McpHub() and removed direct calls to the private initializeGlobalMcpServers() method
• Replaced 3× await Promise.resolve(); await Promise.resolve() microtask tick-counting with vi.waitFor() / vi.advanceTimersByTimeAsync(0) to decouple tests from the SUT's internal async structure
• Added expect(firstEntry.unsubscribe).toHaveBeenCalledOnce() to the OAuth watcher-replacement test, closing a mutation gap (required changing mockReturnValue(vi.fn()) → mockImplementation(() => vi.fn()) so each onDidChange call gets its own spy)

Test Procedure

pnpm vitest run services/mcp/__tests__/McpHub.spec.ts

All 62 tests pass. Verified across 3 consecutive runs before and after the fix.

Pre-Submission Checklist

• Issue Linked: This PR is linked to an approved GitHub Issue (see "Related GitHub Issue" above).
• Scope: My changes are focused on the linked issue (one major feature/fix per PR).
• Self-Review: I have performed a thorough self-review of my code.
• Testing: New and/or updated tests have been added to cover my changes (if applicable).
• Documentation Impact: No documentation updates required (test-only change).
• Contribution Guidelines: I have read and agree to the Contributor Guidelines.

Summary by CodeRabbit

• Tests
  • Refactored module mocking in test suite to ensure consistent mock instances across different import styles.
  • Replaced ad-hoc timing delays with deterministic initialization waits across multiple test cases for improved reliability.
  • Reordered test setup to mock configuration reads before component construction.
  • Tightened synchronization in OAuth-flow tests for more predictable test behavior.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 61aa1691-3230-4e96-891a-a6b2f6ee16ca

📥 Commits

Reviewing files that changed from the base of the PR and between 1b5830c and 7312199.

📒 Files selected for processing (1)

• src/services/mcp/__tests__/McpHub.spec.ts

---

📝 Walkthrough

Walkthrough

The McpHub.spec.ts test file is refactored to improve determinism: the fs/promises mock is restructured so both import styles share the same function instances, all setTimeout-based initialization waits are replaced with await mcpHub.waitUntilReady(), Windows command-wrapping tests reorder config mocking before construction, and OAuth/watcher tests use vi.waitFor and vi.advanceTimersByTimeAsync(0) for precise synchronization.

Changes

McpHub test synchronization refactor

Layer / File(s)	Summary
fs/promises mock consolidation src/services/mcp/__tests__/McpHub.spec.ts	Switches import to namespace form (import * as fs) and rewires the vi.mock factory to return a shared object as both default and spread named exports, removing a redundant standalone mock call.
Replace setTimeout with waitUntilReady() across tests src/services/mcp/__tests__/McpHub.spec.ts	Makes beforeEach async and replaces all fixed-delay setTimeout waits with await mcpHub.waitUntilReady() across watcher, disable-reason, null-safety, disabled-server protection, and MCP global enable/disable test cases.
Windows command-wrapping test setup reorder src/services/mcp/__tests__/McpHub.spec.ts	Moves fs.readFile config mock setup before McpHub construction and replaces manual internal initialization invocations with await mcpHub.waitUntilReady() in all five Windows/platform command-wrapping scenarios.
OAuth and watcher test synchronization fixes src/services/mcp/__tests__/McpHub.spec.ts	Switches onDidChange mock to mockImplementation, replaces Promise.resolve tick-chains with vi.waitFor for capturedCancellationToken population, uses vi.advanceTimersByTimeAsync(0) to flush watcher replacement, and adds assertions for first watcher unsubscribe and map size.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 No more counting sheep with setTimeout delays,
The hub now declares when it's truly ready to play!
Shared mock instances hop through import styles with grace,
OAuth tokens await in their proper time and place.
Each watcher replaced, each unsubscribe confirmed—
Deterministic tests, by this bunny, affirmed! ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main fix: resolving flaky McpHub.spec.ts tests after a Vitest 4 upgrade.
Description check	✅ Passed	The description is complete, covering the related issue, detailed root cause analysis, implementation details, test procedures, and all pre-submission checklist items.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/flakey-vitest-tests

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!