From cbdc131a327db13c24a2fc5536280e7982eb24ae Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Mon, 6 Oct 2025 11:07:03 -0500 Subject: [PATCH 1/3] =?UTF-8?q?=F0=9F=A4=96=20Add=20diagnostic=20logging?= =?UTF-8?q?=20to=20tool=20policy=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of timing out silently, the tests now: - Wait for either stream-end OR stream-error - Log all collected events for debugging - Log the actual error message and error type if stream fails - Fail with a clear message pointing to the logs This will help us understand what's actually failing when the tests are flaky in CI (e.g., is the AI trying to use disabled tools? Is there an API error? etc.) _Generated with `cmux`_ --- tests/ipcMain/sendMessage.test.ts | 60 +++++++++++++++++++++++++++++-- 1 file changed, 58 insertions(+), 2 deletions(-) diff --git a/tests/ipcMain/sendMessage.test.ts b/tests/ipcMain/sendMessage.test.ts index 4a4495e6b7..7262cd8abd 100644 --- a/tests/ipcMain/sendMessage.test.ts +++ b/tests/ipcMain/sendMessage.test.ts @@ -833,7 +833,35 @@ describeIntegration("IpcMain sendMessage integration tests", () => { // Wait for stream to complete (longer timeout for tool policy tests) const collector = createEventCollector(env.sentEvents, workspaceId); - await collector.waitForEvent("stream-end", 30000); + + // Wait for either stream-end or stream-error to get diagnostic info + await Promise.race([ + collector.waitForEvent("stream-end", 30000), + collector.waitForEvent("stream-error", 30000), + ]); + + // Log all events for debugging + const allEvents = collector.getEvents(); + console.log( + `[${provider}] Collected events:`, + allEvents.map((e) => ("type" in e ? e.type : "unknown")) + ); + + // If there was an error, log it for debugging + if (collector.hasError()) { + const errorEvent = allEvents.find((e) => "type" in e && e.type === "stream-error"); + if (errorEvent && "error" in errorEvent) { + console.error(`[${provider}] Stream error:`, errorEvent.error); + if ("errorType" in errorEvent) { + console.error(`[${provider}] Error type:`, errorEvent.errorType); + } + } + // Fail the test with the actual error message + throw new Error( + `Stream ended with error instead of completing successfully. Check logs above for details.` + ); + } + assertStreamSuccess(collector); // Verify file still exists (bash tool was disabled, so deletion shouldn't have happened) @@ -884,7 +912,35 @@ describeIntegration("IpcMain sendMessage integration tests", () => { // Wait for stream to complete (longer timeout for tool policy tests) const collector = createEventCollector(env.sentEvents, workspaceId); - await collector.waitForEvent("stream-end", 30000); + + // Wait for either stream-end or stream-error to get diagnostic info + await Promise.race([ + collector.waitForEvent("stream-end", 30000), + collector.waitForEvent("stream-error", 30000), + ]); + + // Log all events for debugging + const allEvents = collector.getEvents(); + console.log( + `[${provider}] Collected events:`, + allEvents.map((e) => ("type" in e ? e.type : "unknown")) + ); + + // If there was an error, log it for debugging + if (collector.hasError()) { + const errorEvent = allEvents.find((e) => "type" in e && e.type === "stream-error"); + if (errorEvent && "error" in errorEvent) { + console.error(`[${provider}] Stream error:`, errorEvent.error); + if ("errorType" in errorEvent) { + console.error(`[${provider}] Error type:`, errorEvent.errorType); + } + } + // Fail the test with the actual error message + throw new Error( + `Stream ended with error instead of completing successfully. Check logs above for details.` + ); + } + assertStreamSuccess(collector); // Verify file content unchanged (file_edit tools and bash were disabled) From 518cfffba1dc1fb742fd5570b9ea5466cf1404e1 Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Mon, 6 Oct 2025 11:12:21 -0500 Subject: [PATCH 2/3] =?UTF-8?q?=F0=9F=A4=96=20Improve=20test=20helper=20di?= =?UTF-8?q?agnostics=20for=20stream=20failures?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Enhanced and to provide better error messages when tests fail: **waitForEvent improvements:** - When timing out, now automatically logs stream-error details if present - Shows error message and error type for debugging **assertStreamSuccess improvements:** - Replaced generic expect() calls with descriptive Error messages - Shows all collected events when any assertion fails - Distinguishes between different failure modes: - Stream didn't complete (no stream-end) - Stream errored (has stream-error) - Stream completed but missing final message - Includes the actual error message in the failure output This makes test failures in CI much easier to diagnose without needing to add test-specific logging everywhere. _Generated with `cmux`_ --- tests/ipcMain/helpers.ts | 46 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 43 insertions(+), 3 deletions(-) diff --git a/tests/ipcMain/helpers.ts b/tests/ipcMain/helpers.ts index b47469cf6d..da97ff652d 100644 --- a/tests/ipcMain/helpers.ts +++ b/tests/ipcMain/helpers.ts @@ -142,6 +142,15 @@ export class EventCollector { `waitForEvent timeout: Expected "${eventType}" but got events: [${eventTypes.join(", ")}]` ); + // If there was a stream-error, log the error details + const errorEvent = this.events.find((e) => "type" in e && e.type === "stream-error"); + if (errorEvent && "error" in errorEvent) { + console.error("Stream error details:", errorEvent.error); + if ("errorType" in errorEvent) { + console.error("Stream error type:", errorEvent.errorType); + } + } + return null; } @@ -186,12 +195,43 @@ export function createEventCollector( /** * Assert that a stream completed successfully + * Provides helpful error messages when assertions fail */ export function assertStreamSuccess(collector: EventCollector): void { - expect(collector.hasStreamEnd()).toBe(true); - expect(collector.hasError()).toBe(false); + const allEvents = collector.getEvents(); + const eventTypes = allEvents.filter((e) => "type" in e).map((e) => (e as { type: string }).type); + + // Check for stream-end + if (!collector.hasStreamEnd()) { + const errorEvent = allEvents.find((e) => "type" in e && e.type === "stream-error"); + if (errorEvent && "error" in errorEvent) { + throw new Error( + `Stream did not complete successfully. Got stream-error: ${errorEvent.error}\n` + + `All events: [${eventTypes.join(", ")}]` + ); + } + throw new Error( + `Stream did not emit stream-end event.\n` + `All events: [${eventTypes.join(", ")}]` + ); + } + + // Check for errors + if (collector.hasError()) { + const errorEvent = allEvents.find((e) => "type" in e && e.type === "stream-error"); + const errorMsg = errorEvent && "error" in errorEvent ? errorEvent.error : "unknown"; + throw new Error( + `Stream completed but also has error event: ${errorMsg}\n` + + `All events: [${eventTypes.join(", ")}]` + ); + } + + // Check for final message const finalMessage = collector.getFinalMessage(); - expect(finalMessage).toBeDefined(); + if (!finalMessage) { + throw new Error( + `Stream completed but final message is missing.\n` + `All events: [${eventTypes.join(", ")}]` + ); + } } /** From d343a40abdee404bbf5180a11093e503ac77faea Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Mon, 6 Oct 2025 11:13:02 -0500 Subject: [PATCH 3/3] =?UTF-8?q?=F0=9F=A4=96=20Simplify=20tool=20policy=20t?= =?UTF-8?q?ests=20using=20improved=20helpers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now that waitForEvent and assertStreamSuccess provide detailed diagnostics, we can remove the test-specific logging code. The tests now: - Wait for either stream-end or stream-error (prevents timeout) - Rely on helper functions for error diagnostics - Are much more readable and maintainable All the diagnostic logging happens automatically in the helpers, making it a general fix for all integration tests, not just these two. _Generated with `cmux`_ --- tests/ipcMain/sendMessage.test.ts | 52 ++++--------------------------- 1 file changed, 6 insertions(+), 46 deletions(-) diff --git a/tests/ipcMain/sendMessage.test.ts b/tests/ipcMain/sendMessage.test.ts index 7262cd8abd..7f5d4de92a 100644 --- a/tests/ipcMain/sendMessage.test.ts +++ b/tests/ipcMain/sendMessage.test.ts @@ -834,34 +834,14 @@ describeIntegration("IpcMain sendMessage integration tests", () => { // Wait for stream to complete (longer timeout for tool policy tests) const collector = createEventCollector(env.sentEvents, workspaceId); - // Wait for either stream-end or stream-error to get diagnostic info + // Wait for either stream-end or stream-error + // (helpers will log diagnostic info on failure) await Promise.race([ collector.waitForEvent("stream-end", 30000), collector.waitForEvent("stream-error", 30000), ]); - // Log all events for debugging - const allEvents = collector.getEvents(); - console.log( - `[${provider}] Collected events:`, - allEvents.map((e) => ("type" in e ? e.type : "unknown")) - ); - - // If there was an error, log it for debugging - if (collector.hasError()) { - const errorEvent = allEvents.find((e) => "type" in e && e.type === "stream-error"); - if (errorEvent && "error" in errorEvent) { - console.error(`[${provider}] Stream error:`, errorEvent.error); - if ("errorType" in errorEvent) { - console.error(`[${provider}] Error type:`, errorEvent.errorType); - } - } - // Fail the test with the actual error message - throw new Error( - `Stream ended with error instead of completing successfully. Check logs above for details.` - ); - } - + // This will throw with detailed error info if stream didn't complete successfully assertStreamSuccess(collector); // Verify file still exists (bash tool was disabled, so deletion shouldn't have happened) @@ -913,34 +893,14 @@ describeIntegration("IpcMain sendMessage integration tests", () => { // Wait for stream to complete (longer timeout for tool policy tests) const collector = createEventCollector(env.sentEvents, workspaceId); - // Wait for either stream-end or stream-error to get diagnostic info + // Wait for either stream-end or stream-error + // (helpers will log diagnostic info on failure) await Promise.race([ collector.waitForEvent("stream-end", 30000), collector.waitForEvent("stream-error", 30000), ]); - // Log all events for debugging - const allEvents = collector.getEvents(); - console.log( - `[${provider}] Collected events:`, - allEvents.map((e) => ("type" in e ? e.type : "unknown")) - ); - - // If there was an error, log it for debugging - if (collector.hasError()) { - const errorEvent = allEvents.find((e) => "type" in e && e.type === "stream-error"); - if (errorEvent && "error" in errorEvent) { - console.error(`[${provider}] Stream error:`, errorEvent.error); - if ("errorType" in errorEvent) { - console.error(`[${provider}] Error type:`, errorEvent.errorType); - } - } - // Fail the test with the actual error message - throw new Error( - `Stream ended with error instead of completing successfully. Check logs above for details.` - ); - } - + // This will throw with detailed error info if stream didn't complete successfully assertStreamSuccess(collector); // Verify file content unchanged (file_edit tools and bash were disabled)