Conversation
Rifdhan
commented
May 6, 2026
- Add coverage for new tools and related functions
- Increase min coverage level to 85% for all metrics
- Add coverage for new tools and related functions - Increase min coverage level to 85% for all metrics
There was a problem hiding this comment.
Code Review
This pull request significantly expands the test suite for the MCP server and ThoughtSpot service, introducing comprehensive unit tests for the send_session_message, get_session_updates, and create_dashboard tools. It also refactors the base server tests to support new tracking and storage functionalities and increases the branch coverage threshold in the Vitest configuration. The review feedback highlights opportunities to improve test performance and reliability, specifically by using Vitest's fake timers to avoid real delays and replacing brittle microtask flushing loops with the more robust vi.waitFor utility. Additionally, it was suggested to use stricter assertions for deterministic message arrays to ensure exact content and order matching.
| it("should return empty updates when not done and no messages", async () => { | ||
| mockStorageService.getNewMessages.mockResolvedValue({ | ||
| messages: [], | ||
| isDone: false, | ||
| }); | ||
|
|
||
| await server.init(); | ||
| const { callTool } = connect(server); | ||
|
|
||
| const result = await callTool("get_session_updates", { | ||
| analytical_session_id: "conv-abc-123", | ||
| }); | ||
|
|
||
| expect(result.isError).toBeUndefined(); | ||
| expect((result.structuredContent as any).is_done).toBe(false); | ||
| expect((result.structuredContent as any).session_updates).toEqual([]); | ||
| }, 60_000); |
There was a problem hiding this comment.
This test currently takes approximately 10 seconds to run because it polls 20 times with a 500ms delay using real setTimeout. This significantly slows down the test suite and can lead to flakiness in CI environments.
Consider using Vitest's fake timers to control time programmatically. You can enable them in a beforeEach block using vi.useFakeTimers() and advance time using vi.advanceTimersByTimeAsync() or vi.runAllTimersAsync(). This would allow the test to complete almost instantly.
| expect(result.isError).toBeUndefined(); | ||
| expect((result.structuredContent as any).is_done).toBe(true); | ||
| expect((result.structuredContent as any).session_updates).toEqual( | ||
| expect.arrayContaining([...firstMessages, ...secondMessages]), |
There was a problem hiding this comment.
Since the order of messages is deterministic and you are asserting on the complete set of expected messages, toEqual is more appropriate than expect.arrayContaining. toEqual provides a stricter check by ensuring the order and exact content match.
| expect.arrayContaining([...firstMessages, ...secondMessages]), | |
| [...firstMessages, ...secondMessages], |
| // Flush microtask queue enough times for the fire-and-forget async loop to complete | ||
| for (let i = 0; i < 10; i++) await Promise.resolve(); |
There was a problem hiding this comment.
Using a fixed loop of Promise.resolve() to flush microtasks is brittle as it depends on the internal number of await points in the streaming logic. A more robust and idiomatic way to test asynchronous side effects in Vitest is to use vi.waitFor to wait for the expected final call. This ensures the background promise is properly handled and prevents race conditions in the test.
// Wait for the fire-and-forget async loop to complete
await vi.waitFor(() => {
expect(appendStoredMessages).toHaveBeenCalledWith(
"conv-123",
[],
true,
);
});References
- When a method returns a Promise, ensure it is awaited to prevent race conditions and potential data loss, especially when subsequent operations depend on its completion.
- In SendConversationMessageSchema, the messages array can be empty as this is a valid state.
| appendStoredMessages, | ||
| ); | ||
|
|
||
| for (let i = 0; i < 10; i++) await Promise.resolve(); |
There was a problem hiding this comment.
Similar to the previous test case, using a fixed loop to flush microtasks is brittle. Using vi.waitFor to wait for the final call to appendStoredMessages is a more reliable approach to handle asynchronous completion and avoid race conditions.
await vi.waitFor(() => {
expect(appendStoredMessages).toHaveBeenLastCalledWith(
"conv-123",
[],
true,
);
});References
- When a method returns a Promise, ensure it is awaited to prevent race conditions and potential data loss, especially when subsequent operations depend on its completion.
- In SendConversationMessageSchema, the messages array can be empty as this is a valid state.
| appendStoredMessages, | ||
| ); | ||
|
|
||
| for (let i = 0; i < 10; i++) await Promise.resolve(); |
There was a problem hiding this comment.
Consider using vi.waitFor here instead of a fixed loop of Promise.resolve() to ensure the background streaming process has completed. This aligns with best practices for awaiting asynchronous operations to prevent race conditions.
await vi.waitFor(() => {
expect(appendStoredMessages).toHaveBeenCalledWith("conv-123", [
{ is_thinking: false, type: "text", text: "Done" },
]);
});References
- When a method returns a Promise, ensure it is awaited to prevent race conditions and potential data loss, especially when subsequent operations depend on its completion.
| appendStoredMessages, | ||
| ); | ||
|
|
||
| for (let i = 0; i < 10; i++) await Promise.resolve(); |
There was a problem hiding this comment.
Consider using vi.waitFor here instead of a fixed loop of Promise.resolve() to ensure the background streaming process has completed. This ensures the test reliably waits for the asynchronous side effect.
| for (let i = 0; i < 10; i++) await Promise.resolve(); | |
| await vi.waitFor(() => { | |
| expect(appendStoredMessages).toHaveBeenCalledWith("conv-123", [ | |
| { is_thinking: true, type: "text", text: "Thinking..." }, | |
| ]); | |
| }); |
References
- When a method returns a Promise, ensure it is awaited to prevent race conditions and potential data loss, especially when subsequent operations depend on its completion.