From 6640ed2ce3a442c011834c851f25bbb2d56f0c10 Mon Sep 17 00:00:00 2001 From: rgarcia <72655+rgarcia@users.noreply.github.com> Date: Sun, 21 Jun 2026 12:39:37 +0000 Subject: [PATCH] Fix flaky TestNetworkEvents by checkpointing before triggering events waitForNew snapshotted the event count after the messages that trigger the awaited event were already sent. Events are published asynchronously (readLoop dispatch and the async response-body goroutine), so the event could be appended before the snapshot, get treated as old, and the wait would block until its deadline. This caused intermittent ~2s/~3s timeouts in the loading_failed and binary_resource_skips_body subtests. Replace the internal snapshot with an explicit checkpoint() taken before the trigger, and have waitForNew wait for events at or after it. Co-Authored-By: Claude Opus 4.7 --- server/lib/cdpmonitor/cdp_test.go | 22 +++++++++++++++------- server/lib/cdpmonitor/handlers_test.go | 18 +++++++++++------- 2 files changed, 26 insertions(+), 14 deletions(-) diff --git a/server/lib/cdpmonitor/cdp_test.go b/server/lib/cdpmonitor/cdp_test.go index 50994e47..f5a43c17 100644 --- a/server/lib/cdpmonitor/cdp_test.go +++ b/server/lib/cdpmonitor/cdp_test.go @@ -203,18 +203,26 @@ func (c *eventCollector) waitFor(t *testing.T, eventType string, timeout time.Du } } -// waitForNew blocks until a NEW event of the given type is published after this -// call, ignoring any events already in the collector. -func (c *eventCollector) waitForNew(t *testing.T, eventType string, timeout time.Duration) events.Event { - t.Helper() +// checkpoint records the current event count. Pass it to waitForNew to wait for +// an event published after the checkpoint. Take the checkpoint before sending +// the messages that trigger the event: events are published asynchronously +// (readLoop dispatch and the async body-fetch goroutine), so the event can land +// before waitForNew runs. Snapshotting inside waitForNew would then skip it and +// the wait would hang until timeout. +func (c *eventCollector) checkpoint() int { c.mu.Lock() - skip := len(c.events) - c.mu.Unlock() + defer c.mu.Unlock() + return len(c.events) +} +// waitForNew blocks until an event of the given type is published at or after +// the given checkpoint index, ignoring earlier events. +func (c *eventCollector) waitForNew(t *testing.T, eventType string, since int, timeout time.Duration) events.Event { + t.Helper() deadline := time.After(timeout) for { c.mu.Lock() - for i := skip; i < len(c.events); i++ { + for i := since; i < len(c.events); i++ { if c.events[i].Type == eventType { ev := c.events[i] c.mu.Unlock() diff --git a/server/lib/cdpmonitor/handlers_test.go b/server/lib/cdpmonitor/handlers_test.go index cb7fe6e4..3af2afe6 100644 --- a/server/lib/cdpmonitor/handlers_test.go +++ b/server/lib/cdpmonitor/handlers_test.go @@ -59,6 +59,7 @@ func TestConsoleEvents(t *testing.T) { }) t.Run("non_string_args", func(t *testing.T) { + cp := ec.checkpoint() srv.sendToMonitor(t, map[string]any{ "method": "Runtime.consoleAPICalled", "params": map[string]any{ @@ -70,7 +71,7 @@ func TestConsoleEvents(t *testing.T) { }, }, }) - ev := ec.waitForNew(t, "console_log", 2*time.Second) + ev := ec.waitForNew(t, "console_log", cp, 2*time.Second) var data map[string]any require.NoError(t, json.Unmarshal(ev.Data, &data)) args := data["args"].([]any) @@ -146,6 +147,7 @@ func TestNetworkEvents(t *testing.T) { }) t.Run("loading_failed", func(t *testing.T) { + cp := ec.checkpoint() srv.sendToMonitor(t, map[string]any{ "method": "Network.requestWillBeSent", "params": map[string]any{ @@ -153,7 +155,7 @@ func TestNetworkEvents(t *testing.T) { "request": map[string]any{"method": "GET", "url": "https://fail.example.com/"}, }, }) - ec.waitForNew(t, "network_request", 2*time.Second) + ec.waitForNew(t, "network_request", cp, 2*time.Second) srv.sendToMonitor(t, map[string]any{ "method": "Network.loadingFailed", @@ -172,6 +174,7 @@ func TestNetworkEvents(t *testing.T) { t.Run("binary_resource_skips_body", func(t *testing.T) { getBodyCalled.Store(false) + cp := ec.checkpoint() // Use PDL wire key "type" (not "resourceType") — Chrome emits ResourceType // under "type" for Network.requestWillBeSent. srv.sendToMonitor(t, map[string]any{ @@ -194,7 +197,7 @@ func TestNetworkEvents(t *testing.T) { "params": map[string]any{"requestId": "img-001"}, }) - ev := ec.waitForNew(t, "network_response", 3*time.Second) + ev := ec.waitForNew(t, "network_response", cp, 3*time.Second) var data map[string]any require.NoError(t, json.Unmarshal(ev.Data, &data)) assert.Nil(t, data["body"], "binary resource should not have body field") @@ -483,11 +486,12 @@ func TestPerTargetStateMachines(t *testing.T) { }) // Wait past sess-b's 500 ms debounce so its network_idle fires before we - // set our checkpoint. The next new network_idle will then come from sess-a. + // take the checkpoint. The next network_idle will then come from sess-a. time.Sleep(700 * time.Millisecond) - // Finish sess-a's request; waitForNew captures the current event count so - // sess-b's already-fired network_idle is excluded from the result. + // Checkpoint after sess-b's network_idle has fired so it is excluded, then + // finish sess-a's request to drive sess-a's own network_idle. + cp := ec.checkpoint() srv.sendToMonitor(t, map[string]any{ "method": "Network.responseReceived", "sessionId": "sess-a", "params": map[string]any{ @@ -500,7 +504,7 @@ func TestPerTargetStateMachines(t *testing.T) { "params": map[string]any{"requestId": "req-a"}, }) - ev := ec.waitForNew(t, "network_idle", 2*time.Second) + ev := ec.waitForNew(t, "network_idle", cp, 2*time.Second) var data map[string]any require.NoError(t, json.Unmarshal(ev.Data, &data)) assert.Equal(t, "sess-a", data["session_id"], "network_idle must be attributed to sess-a")