Add integration tests

[mouryabalabhadra]

No description provided.

[mouryabalabhadra]

@copilot review the PR

[gemini-code-assist]

Code Review

This pull request significantly expands the test suite for the MCP server, introducing integration tests for the V2 API and comprehensive unit tests for session messaging, dashboard creation, and data source discovery. It also adds health check tests for the API server and connection validation tests for the ThoughtSpot service. Feedback was provided regarding the redundant instantiation of the MCPServer across multiple test blocks, suggesting the use of a shared instance to improve consistency and reduce boilerplate.

[gemini-code-assist]

There is redundant instantiation of the MCPServer within multiple it blocks (e.g., lines 1281, 1302, 1328, 1378, etc.). A fresh instance is already created and assigned to the server variable in the top-level beforeEach block (line 34). Using the existing server instance instead of creating a local testServer would reduce boilerplate and improve consistency. Additionally, the inconsistent use of server vs testServer across different tests makes the suite harder to maintain.

[Copilot]

@copilot review the PR

All 33 test files pass (643 tests, 5 skipped). Here's the full review:

✅ What's well done

• Coverage is comprehensive. Six test files gain new cases that cover happy paths, error paths, timer-driven polling behaviour, and the full streaming-parser-to-storage pipeline. The new mcp-server-v2.integration.spec.ts tests the HTTP contract between StorageServiceClient and ConversationStorageServer end-to-end without faking either side, which is exactly the right level of integration test.
• Timer mocks are correctly managed. Every test that calls vi.useFakeTimers() pairs with vi.useRealTimers() in afterEach, so fake timers don't leak between tests.
• Async/streaming edge cases are covered. The streaming parser integration tests (Group 3) verify that raw SSE bytes produce the correct Durable Object state, catching URL-encoding or JSON-shape mismatches invisible to unit tests.
• Import ordering is now consistent alphabetically across all modified files, matching the project's Biome config.

⚠️ Minor observations (not blocking)

1. makeRequest helper is duplicated. mcp-server-v2.integration.spec.ts defines a module-level makeRequest (line 156), and mcp-server.spec.ts re-defines a local copy inside the "V2 End-to-End Scenarios" describe block (line 1758). Since these live in separate files this is acceptable, but extracting it to a shared test/helpers.ts would remove the repetition.

2. Double init() in two "Create Dashboard" tests. The tests at lines 1600 and 1641 call await server.init() on the shared server fixture that beforeEach already initialised. This re-registers all request handlers (harmless because the tests still pass) but is slightly misleading. Either use a fresh testServer like the surrounding tests do, or drop the redundant init() call.

3. MixpanelTracker is not mocked in the integration test file. The other five test files all add a vi.mock("…/mixpanel") guard at the top. The integration test omits it, so BaseMCPServer.initializeService() creates a real MixpanelTracker during testServer.init(). Because the Mixpanel client is a thin wrapper that doesn't flush synchronously, this is safe in practice—but adding the same module mock would make the behaviour explicit and guard against future changes that introduce a sync flush.

None of these are blocking; the suite is solid and the coverage improvement is meaningful.

[mouryabalabhadra]

@copilot review the PR

[Copilot]

@copilot review the PR

All 643 tests pass, all 4 CI runs are green, and the three observations raised in the previous review have all been addressed in commits a5bcd95 and 8894064:

1. ✅ makeRequest deduplication — extracted to test/servers/helpers.ts and imported in both mcp-server.spec.ts and mcp-server-v2.integration.spec.ts.
2. ✅ Double init() in Create Dashboard tests — Create Dashboard describe block now has its own beforeEach(() => server.init()) and all per-test testServer instances have been replaced with the shared server fixture.
3. ✅ MixpanelTracker mock in integration test — vi.mock("…/mixpanel") guard added at the top of mcp-server-v2.integration.spec.ts.

The PR is ready to merge.