feat: enable parallel e2e test execution with dynamic port allocation #137
Conversation
Introduce TestContainer to manage Docker containers with dynamically allocated ports, eliminating port conflicts and enabling parallel test execution. This significantly speeds up CI runs on high-resource machines. Changes: - Add container.go with TestContainer struct for dynamic port management - Migrate all e2e tests to use TestContainer instead of global setup - Add t.Parallel() to test functions for concurrent execution - Remove -p 1 from Makefile now that port conflicts are resolved - Replace slog logging with t.Log/t.Logf for cleaner test output
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 5 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| t.Logf("[diagnostics] supervisor status: error (%v)", err) | ||
| } else { | ||
| logger.Info("[diagnostics]", "supervisor_status", supervisorOutput) | ||
| t.Logf("[diagnostics] supervisor status: %s", supervisorOutput) |
There was a problem hiding this comment.
Diagnostics function uses hardcoded port instead of container's port
Medium Severity
The logCDPDiagnostics function calls execCombinedOutput, which internally uses apiClient() with the hardcoded global apiBaseURL = "http://127.0.0.1:10001". With the new dynamic port allocation where each test container gets its own unique port, this function will send diagnostic commands to whatever container happens to be on port 10001 (or fail if none exists), rather than the test's actual container. This makes diagnostic output unreliable or misleading when debugging parallel test failures. The function needs to accept a *TestContainer parameter and use execCombinedOutputWithClient like other migrated functions.
| return combined, &RemoteExecError{Command: command, Args: args, ExitCode: exitCode, Output: combined} | ||
| } | ||
| return combined, nil | ||
| } |
There was a problem hiding this comment.
Duplicated remote exec logic across two functions
Low Severity
The new execCombinedOutputWithClient function is a near-complete copy of execCombinedOutput in e2e_chromium_test.go. The only difference is how the API client is obtained (c.APIClient() vs apiClient()). The entire body—request building, API call, base64 decoding, exit code handling, and error wrapping—is duplicated across 40 lines. This could be refactored to share the common logic, either by extracting a helper that accepts a client parameter or by making this a method on TestContainer.
| // ExitCh returns a channel that receives an error when the container exits. | ||
| func (c *TestContainer) ExitCh() <-chan error { | ||
| return c.exitCh | ||
| } |
There was a problem hiding this comment.
Unused ExitCh method never called
Low Severity
The ExitCh() method on TestContainer is defined but never called anywhere in the codebase. A grep for .ExitCh() returns no matches. This appears to be dead code that was added for potential future use but is currently unused.
| // TestContainer manages a Docker container with dynamically allocated ports. | ||
| // This enables parallel test execution by giving each test its own ports. | ||
| type TestContainer struct { | ||
| tb testing.TB // supports both *testing.T and *testing.B |
There was a problem hiding this comment.
Unused tb field stored but never read
Low Severity
The tb field in TestContainer is set during construction but never accessed afterward. A grep for c.tb or .tb in the container file returns no matches. The testing.TB interface is only used locally in NewTestContainer for calling tb.Helper(), tb.Fatalf(), and tb.Name(), but storing it in the struct serves no purpose since it's never read later.
Additional Locations (1)
| CDPPort int // dynamically allocated host port -> container 9222 | ||
| cmd *exec.Cmd | ||
| exitCh <-chan error | ||
| ctx context.Context |
There was a problem hiding this comment.
Unused ctx field stored but never read
Low Severity
The ctx field in TestContainer is assigned in the Start method but never read by any other method. A grep for c.ctx shows only the assignment at line 88. All methods that need a context receive it as a parameter rather than using the stored field.
Additional Locations (1)
| # (all e2e tests bind to the same ports: 10001, 9222) | ||
| go test -v -race -p 1 ./... | ||
| # E2E tests use dynamic ports via TestContainer, enabling parallel execution | ||
| go test -v -race ./... |
There was a problem hiding this comment.
let's scale this up slowly here in the public repo since it's running on a pretty weak github runner. maybe -p 2?
There was a problem hiding this comment.
actually nvm looks like it worked at full parallelism so disregard!
| // TestContainer manages a Docker container with dynamically allocated ports. | ||
| // This enables parallel test execution by giving each test its own ports. | ||
| type TestContainer struct { | ||
| tb testing.TB // supports both *testing.T and *testing.B |
There was a problem hiding this comment.
tb and ctx fields can be removed - they're stored but never read. tb is only used in the constructor, and ctx is stored in Start() but methods take ctx as parameters instead of using the stored field.
Introduce TestContainer to manage Docker containers with dynamically allocated ports, eliminating port conflicts and enabling parallel test execution. This significantly speeds up CI runs on high-resource machines.
Changes:
Checklist
Note
Medium Risk
Primarily test-only changes, but enabling parallel Docker-based e2e runs can introduce new CI flakiness (port allocation races, resource contention, shared Docker daemon limits) and change test timing/ordering assumptions.
Overview
E2E test infrastructure is refactored to use a new
TestContainerhelper (e2e/container.go) that starts per-test Docker containers with dynamically allocated host ports for the API (10001) and DevTools (9222), including readiness checks and a no-keepalive API client for post-restart calls.Most e2e tests/benchmarks are migrated to this helper, add
t.Parallel()to allow concurrent execution, and replacesloglogging witht.Log/t.Logfwhile updating CDP/WebSocket usage to use each container’s dynamic URLs.The
Makefiletest target drops-p 1sogo testcan run packages/tests in parallel now that port conflicts are avoided.Written by Cursor Bugbot for commit 06773db. This will update automatically on new commits. Configure here.