fix: migrate E2E tests to Docker Chrome for CI reliability#123
Conversation
Replace local Chrome (chromedp.NewExecAllocator) with Docker Chrome (chromedp/headless-shell) across all E2E tests to fix flaky test failures in CI environments. Changes: - Add e2e_helpers_test.go and e2e_helpers_external_test.go with Docker Chrome setup helpers (SetupDockerChrome, ConvertURLForDockerChrome) - Migrate 20+ E2E test files to use Docker Chrome pattern - Refactor setupMarkdownTestInternal helper to return structured context - Tests now skip gracefully when Docker is unavailable This fixes flaky tests like TestMermaidDiagramsRendering (websocket url timeout) and TestLoadingIndicator (context deadline exceeded) that were caused by local Chrome timing issues. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
PR Review: Migrate E2E Tests to Docker Chrome for CI ReliabilitySummaryThis PR successfully addresses flaky E2E tests in CI by migrating from local Chrome to Docker-based Chrome ( Critical Issues1. Code Duplication Between Helper FilesLocation: These two files contain nearly identical code (440 lines of duplication). The only differences are:
Impact: Maintenance burden - any bug fix or improvement needs to be applied twice. Recommendation: Consider one of these approaches:
2. Inconsistent
|
There was a problem hiding this comment.
Pull request overview
This PR migrates E2E tests from local Chrome to Docker Chrome (chromedp/headless-shell) to improve CI reliability. The migration addresses flaky tests that were experiencing "websocket url timeout" and "context deadline exceeded" errors by introducing containerized Chrome with proper health checks.
Changes:
- Added two helper files (
e2e_helpers_test.goande2e_helpers_external_test.go) with shared functions for Docker Chrome setup and URL conversion - Migrated 20+ E2E test files to use Docker Chrome instead of local Chrome execution allocators
- Replaced hardcoded port numbers with dynamic port allocation to avoid conflicts
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| e2e_helpers_test.go | New helper for package tinkerdown with Docker Chrome setup and utilities |
| e2e_helpers_external_test.go | New helper for package tinkerdown_test with Docker Chrome setup and utilities |
| ux_enhancements_e2e_test.go | Migrated to Docker Chrome; removed context imports |
| tutorial_nav_bug_test.go | Migrated to Docker Chrome; retained context for HTTP server |
| search_e2e_test.go | Migrated to Docker Chrome with URL conversion |
| presentation_mode_e2e_test.go | Migrated to Docker Chrome; added dynamic ports; fixed typo |
| partials_e2e_test.go | Migrated to Docker Chrome with URL conversion |
| mermaid_diagrams_e2e_test.go | Migrated to Docker Chrome; improved wait logic for SVG rendering |
| lvtsource_sqlite_e2e_test.go | Migrated to Docker Chrome with URL conversion |
| lvtsource_rest_e2e_test.go | Migrated to Docker Chrome with URL conversion |
| lvtsource_pg_e2e_test.go | Migrated to Docker Chrome; updated to use any instead of interface{} |
| lvtsource_markdown_e2e_test.go | Migrated to Docker Chrome; refactored test context structure |
| lvtsource_graphql_e2e_test.go | Migrated to Docker Chrome with URL conversion |
| lvtsource_file_e2e_test.go | Migrated to Docker Chrome with URL conversion |
| lvtsource_e2e_test.go | Migrated to Docker Chrome with URL conversion |
| frontmatter_config_e2e_test.go | Migrated to Docker Chrome with URL conversion |
| execargs_e2e_test.go | Migrated to Docker Chrome with URL conversion |
| exec_toolbar_e2e_test.go | Migrated to Docker Chrome with URL conversion |
| computed_expressions_e2e_test.go | Migrated to Docker Chrome with URL conversion |
| component_library_e2e_test.go | Migrated to Docker Chrome with URL conversion |
| build_command_e2e_test.go | Migrated to Docker Chrome; retained context for exec operations |
| action_buttons_e2e_test.go | Migrated to Docker Chrome with URL conversion |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Set viewport via emulation | ||
| chromedp.EmulateViewport(int64(vp.width), int64(vp.height)), | ||
| chromedp.Navigate(url), |
There was a problem hiding this comment.
The viewport configuration from the test case is no longer being applied. Previously, the viewport was set through chromedp.WindowSize() in the allocator options, but now you're calling chromedp.EmulateViewport() after the Chrome context is already created with default settings. However, the viewport dimensions from the test case (vp.width, vp.height) are not being used - the test appears to be testing different viewport sizes but is now using the same viewport for all test cases.
| //go:build !ci | ||
|
|
||
| package tinkerdown | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "net" | ||
| "net/http" | ||
| "os/exec" | ||
| "strings" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/chromedp/chromedp" | ||
| ) | ||
|
|
||
| const ( | ||
| dockerImage = "chromedp/headless-shell:latest" | ||
| chromeContainerPrefix = "chrome-e2e-tinkerdown-" | ||
| ) | ||
|
|
||
| // DockerChromeContext provides a Docker Chrome context for E2E tests. | ||
| type DockerChromeContext struct { | ||
| Context context.Context | ||
| Cancel context.CancelFunc | ||
| ChromePort int | ||
| t *testing.T | ||
| } | ||
|
|
||
| // SetupDockerChrome starts a Docker Chrome container and returns a chromedp context. | ||
| // Call cleanup() when done to stop the container and cancel the context. | ||
| func SetupDockerChrome(t *testing.T, timeout time.Duration) (*DockerChromeContext, func()) { | ||
| t.Helper() | ||
|
|
||
| chromePort, err := getFreePort() | ||
| if err != nil { | ||
| t.Fatalf("Failed to allocate Chrome port: %v", err) | ||
| } | ||
|
|
||
| if err := startDockerChrome(t, chromePort); err != nil { | ||
| t.Fatalf("Failed to start Docker Chrome: %v", err) | ||
| } | ||
|
|
||
| chromeURL := fmt.Sprintf("http://localhost:%d", chromePort) | ||
| allocCtx, allocCancel := chromedp.NewRemoteAllocator(context.Background(), chromeURL) | ||
|
|
||
| ctx, ctxCancel := chromedp.NewContext(allocCtx, chromedp.WithLogf(t.Logf)) | ||
|
|
||
| // Apply timeout | ||
| ctx, timeoutCancel := context.WithTimeout(ctx, timeout) | ||
|
|
||
| dcc := &DockerChromeContext{ | ||
| Context: ctx, | ||
| Cancel: timeoutCancel, | ||
| ChromePort: chromePort, | ||
| t: t, | ||
| } | ||
|
|
||
| cleanup := func() { | ||
| timeoutCancel() | ||
| ctxCancel() | ||
| allocCancel() | ||
| stopDockerChrome(t, chromePort) | ||
| } | ||
|
|
||
| return dcc, cleanup | ||
| } | ||
|
|
||
| // GetChromeTestURL returns the URL for Chrome (in Docker) to access the test server. | ||
| // Chrome container uses host.docker.internal to reach the host on all platforms. | ||
| func GetChromeTestURL(port int) string { | ||
| return fmt.Sprintf("http://host.docker.internal:%d", port) | ||
| } | ||
|
|
||
| // getFreePort asks the kernel for a free open port that is ready to use. | ||
| func getFreePort() (port int, err error) { | ||
| var a *net.TCPAddr | ||
| if a, err = net.ResolveTCPAddr("tcp", "localhost:0"); err == nil { | ||
| var l *net.TCPListener | ||
| if l, err = net.ListenTCP("tcp", a); err == nil { | ||
| defer l.Close() | ||
| return l.Addr().(*net.TCPAddr).Port, nil | ||
| } | ||
| } | ||
| return | ||
| } | ||
|
|
||
| // startDockerChrome starts the chromedp headless-shell Docker container. | ||
| func startDockerChrome(t *testing.T, debugPort int) error { | ||
| t.Helper() | ||
|
|
||
| // Check if Docker is available | ||
| if _, err := exec.Command("docker", "version").CombinedOutput(); err != nil { | ||
| t.Skip("Docker not available, skipping E2E test") | ||
| } | ||
|
|
||
| containerName := fmt.Sprintf("%s%d", chromeContainerPrefix, debugPort) | ||
| cleanupContainerByName(t, containerName) | ||
|
|
||
| // Check if image exists, if not try to pull it (with timeout) | ||
| checkCmd := exec.Command("docker", "image", "inspect", dockerImage) | ||
| if _, err := checkCmd.CombinedOutput(); err != nil { | ||
| // Image doesn't exist, try to pull with timeout | ||
| t.Log("Pulling chromedp/headless-shell Docker image...") | ||
|
|
||
| pullCtx, pullCancel := context.WithTimeout(context.Background(), 60*time.Second) | ||
| defer pullCancel() | ||
|
|
||
| pullCmd := exec.CommandContext(pullCtx, "docker", "pull", dockerImage) | ||
| if output, err := pullCmd.CombinedOutput(); err != nil { | ||
| if pullCtx.Err() == context.DeadlineExceeded { | ||
| t.Fatal("Docker pull timed out after 60 seconds") | ||
| } | ||
| t.Fatalf("Failed to pull Docker image: %v\nOutput: %s", err, output) | ||
| } | ||
| t.Log("Docker image pulled successfully") | ||
| } | ||
|
|
||
| // Start the container | ||
| t.Log("Starting Chrome headless Docker container...") | ||
| portMapping := fmt.Sprintf("%d:9222", debugPort) | ||
|
|
||
| cmd := exec.Command("docker", "run", "-d", | ||
| "--rm", | ||
| "--memory", "512m", | ||
| "--cpus", "0.5", | ||
| "-p", portMapping, | ||
| "--name", containerName, | ||
| "--add-host", "host.docker.internal:host-gateway", | ||
| dockerImage, | ||
| ) | ||
|
|
||
| if _, err := cmd.Output(); err != nil { | ||
| return fmt.Errorf("failed to start Chrome Docker container: %w", err) | ||
| } | ||
|
|
||
| // Wait for Chrome to be ready | ||
| t.Log("Waiting for Chrome to be ready...") | ||
| chromeURL := fmt.Sprintf("http://localhost:%d/json/version", debugPort) | ||
| ready := false | ||
| var lastErr error | ||
| for i := 0; i < 120; i++ { // 60 seconds | ||
| resp, err := http.Get(chromeURL) | ||
| if err == nil { | ||
| resp.Body.Close() | ||
| ready = true | ||
| t.Logf("Chrome ready after %d attempts (%.1fs)", i+1, float64(i+1)*0.5) | ||
| break | ||
| } | ||
| lastErr = err | ||
| time.Sleep(500 * time.Millisecond) | ||
| } | ||
|
|
||
| if !ready { | ||
| t.Logf("Chrome failed to start within 60 seconds. Last error: %v", lastErr) | ||
|
|
||
| // Try to get container logs for debugging | ||
| logsCmd := exec.Command("docker", "logs", "--tail", "50", containerName) | ||
| if output, err := logsCmd.CombinedOutput(); err == nil && len(output) > 0 { | ||
| t.Logf("Chrome container logs:\n%s", string(output)) | ||
| } | ||
|
|
||
| // Clean up the container | ||
| _, _ = exec.Command("docker", "rm", "-f", containerName).CombinedOutput() | ||
| return fmt.Errorf("Chrome failed to start within 60 seconds: %w", lastErr) | ||
| } | ||
|
|
||
| t.Log("Chrome headless Docker container ready") | ||
| return nil | ||
| } | ||
|
|
||
| // stopDockerChrome stops and removes the Chrome Docker container. | ||
| func stopDockerChrome(t *testing.T, debugPort int) { | ||
| t.Helper() | ||
| t.Log("Stopping Chrome Docker container...") | ||
|
|
||
| containerName := fmt.Sprintf("%s%d", chromeContainerPrefix, debugPort) | ||
|
|
||
| rmCmd := exec.Command("docker", "rm", "-f", containerName) | ||
| if output, err := rmCmd.CombinedOutput(); err != nil { | ||
| errMsg := string(output) | ||
| if !strings.Contains(errMsg, "No such container") && !strings.Contains(err.Error(), "No such container") { | ||
| t.Logf("Warning: Failed to remove Docker container: %v (output: %s)", err, errMsg) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // cleanupContainerByName removes any existing container with the given name. | ||
| func cleanupContainerByName(t *testing.T, name string) { | ||
| rmCmd := exec.Command("docker", "rm", "-f", name) | ||
| rmCmd.CombinedOutput() // Ignore errors - container may not exist | ||
| } | ||
|
|
||
| // WaitForServer polls an HTTP server until it responds or timeout is reached. | ||
| func WaitForServer(t *testing.T, serverURL string, timeout time.Duration) { | ||
| t.Helper() | ||
|
|
||
| deadline := time.Now().Add(timeout) | ||
| for time.Now().Before(deadline) { | ||
| resp, err := http.Get(serverURL) | ||
| if err == nil { | ||
| resp.Body.Close() | ||
| t.Logf("Server ready at %s", serverURL) | ||
| return | ||
| } | ||
| time.Sleep(100 * time.Millisecond) | ||
| } | ||
| t.Fatalf("Server at %s failed to become ready within %v", serverURL, timeout) | ||
| } | ||
|
|
||
| // ConvertURLForDockerChrome converts an httptest URL (like http://127.0.0.1:12345) | ||
| // to a URL accessible from Docker Chrome (http://host.docker.internal:12345). | ||
| func ConvertURLForDockerChrome(httptestURL string) string { | ||
| // httptest URLs are like "http://127.0.0.1:12345" or "http://[::1]:12345" | ||
| // We need to replace the host with host.docker.internal | ||
| url := strings.Replace(httptestURL, "127.0.0.1", "host.docker.internal", 1) | ||
| url = strings.Replace(url, "[::1]", "host.docker.internal", 1) | ||
| return url | ||
| } |
There was a problem hiding this comment.
These two helper files (e2e_helpers_test.go and e2e_helpers_external_test.go) contain nearly identical code (220 lines each), differing only in the package name and container name prefix. This violates the DRY principle and creates a maintenance burden. Consider consolidating this duplicated code into a single shared implementation or using build tags more effectively to share the common functionality while allowing package-specific variations only where necessary.
| // ConvertURLForDockerChrome converts an httptest URL (like http://127.0.0.1:12345) | ||
| // to a URL accessible from Docker Chrome (http://host.docker.internal:12345). | ||
| func ConvertURLForDockerChrome(httptestURL string) string { | ||
| // httptest URLs are like "http://127.0.0.1:12345" or "http://[::1]:12345" | ||
| // We need to replace the host with host.docker.internal | ||
| url := strings.Replace(httptestURL, "127.0.0.1", "host.docker.internal", 1) | ||
| url = strings.Replace(url, "[::1]", "host.docker.internal", 1) | ||
| return url | ||
| } |
There was a problem hiding this comment.
The URL conversion logic only handles IPv4 (127.0.0.1) and IPv6 ([::1]) localhost addresses. However, httptest.NewServer() may use other address formats depending on the system configuration. Consider using a more robust URL parsing approach with the net/url package to extract and replace just the host portion, which would handle edge cases more reliably.
e2e_helpers_test.go
Outdated
| // startDockerChrome starts the chromedp headless-shell Docker container. | ||
| func startDockerChrome(t *testing.T, debugPort int) error { | ||
| t.Helper() | ||
|
|
||
| // Check if Docker is available | ||
| if _, err := exec.Command("docker", "version").CombinedOutput(); err != nil { | ||
| t.Skip("Docker not available, skipping E2E test") | ||
| } | ||
|
|
||
| containerName := fmt.Sprintf("%s%d", chromeContainerPrefix, debugPort) | ||
| cleanupContainerByName(t, containerName) |
There was a problem hiding this comment.
There's a potential race condition when running tests in parallel. If multiple tests use the same Chrome port due to getFreePort() being called before the container actually starts listening, or if container cleanup from a previous test hasn't completed when a new test starts, tests could interfere with each other. The container name includes the port to help with this, but consider adding additional safeguards such as retrying container startup if the port is already in use, or using a mutex to serialize Chrome container creation.
| //go:build !ci | ||
|
|
There was a problem hiding this comment.
The build tag //go:build !ci prevents these tests from running in CI environments, which contradicts the PR's stated goal of "migrating E2E tests to Docker Chrome for reliable CI execution". If the intent is to make tests runnable in CI, this build tag should either be removed or changed to a different condition. The PR description mentions tests should "skip gracefully when Docker is unavailable" which is already handled by the Docker check in startDockerChrome, so the build tag seems unnecessary.
| //go:build !ci |
e2e_helpers_test.go
Outdated
| } | ||
|
|
||
| // cleanupContainerByName removes any existing container with the given name. | ||
| func cleanupContainerByName(t *testing.T, name string) { |
There was a problem hiding this comment.
The cleanupContainerByName function signature is inconsistent between the two helper files. In e2e_helpers_test.go it takes a *testing.T parameter (line 190), while in e2e_helpers_external_test.go it does not (line 200). This inconsistency could lead to confusion and makes the code harder to maintain. The function should have the same signature in both files.
| func cleanupContainerByName(t *testing.T, name string) { | |
| func cleanupContainerByName(name string) { |
| "--memory", "512m", | ||
| "--cpus", "0.5", |
There was a problem hiding this comment.
The Docker container is configured with very conservative resource limits (512MB memory, 0.5 CPUs). While this is good for preventing resource exhaustion, these limits might be too restrictive for Chrome to run reliably, especially when running multiple tests in parallel or when pages have complex rendering requirements. Consider making these limits configurable or increasing them based on empirical testing to ensure Chrome doesn't crash due to resource constraints.
| // Wait for Chrome to be ready | ||
| t.Log("Waiting for Chrome to be ready...") | ||
| chromeURL := fmt.Sprintf("http://localhost:%d/json/version", debugPort) | ||
| ready := false | ||
| var lastErr error | ||
| for i := 0; i < 120; i++ { // 60 seconds | ||
| resp, err := http.Get(chromeURL) | ||
| if err == nil { | ||
| resp.Body.Close() | ||
| ready = true | ||
| t.Logf("Chrome ready after %d attempts (%.1fs)", i+1, float64(i+1)*0.5) | ||
| break | ||
| } | ||
| lastErr = err | ||
| time.Sleep(500 * time.Millisecond) | ||
| } |
There was a problem hiding this comment.
The Chrome readiness check polls for up to 60 seconds (120 iterations × 0.5s), but closes the HTTP response body immediately after a successful GET without reading it. While this is technically correct for checking readiness, it would be more robust to also verify the response status code is 200 and optionally parse the JSON to ensure Chrome is actually ready to accept connections, not just responding to HTTP requests.
| tempDir, cleanup := createTempMarkdownExample(t) | ||
| defer cleanup() | ||
|
|
||
| ts, ctx, cancel, consoleLogs := setupMarkdownTest(t, tempDir) | ||
| defer cancel() | ||
| testCtx, cleanup := setupMarkdownTest(t, tempDir) | ||
| defer cleanup() |
There was a problem hiding this comment.
Variable shadowing issue: The cleanup variable is declared twice in the same scope - once from createTempMarkdownExample(t) and again from setupMarkdownTest(t, tempDir). This shadows the first cleanup function, which means the temporary directory cleanup from createTempMarkdownExample will never be called, potentially leaving temporary files on disk after the test completes. You should use different variable names (e.g., tempCleanup and testCleanup) as was done in other tests in this file.
- Pin Docker image to specific version (131.0.6778.264) for reproducible builds - Add HTTP client timeout (2s) to avoid hanging during Chrome readiness check - Remove unused `t` parameter from cleanupContainerByName - Add explanatory comment about intentional code duplication between e2e_helpers_test.go and e2e_helpers_external_test.go (required by Go package boundaries - cannot share code between internal and external tests) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Code Review SummaryThis PR successfully migrates E2E tests from local Chrome to Docker Chrome for improved CI reliability. The changes are well-structured and address real flakiness issues. Overall, this is a solid improvement with good attention to detail. ✅ Strengths
🔍 Potential Issues & Concerns1. Code Duplication (e2e_helpers_test.go vs e2e_helpers_external_test.go)The 230+ lines are duplicated between internal and external test packages. While the comment explains the Go package boundary limitation, consider:
2. Missing GetChromeTestURL UsageBoth helper files define GetChromeTestURL(port int) but it's never used in e2e_helpers_test.go:82 and e2e_helpers_external_test.go:81. All tests use ConvertURLForDockerChrome(httptestURL) instead. This suggests:
3. Resource Cleanup ConcernsIssue in cleanupContainerByName (e2e_helpers_test.go:202, e2e_helpers_external_test.go:207): Recommendation: At minimum, log errors that aren't "No such container" (similar to how stopDockerChrome handles it properly in e2e_helpers_test.go:189-195). 4. Context Hierarchy in DockerChromeContextThe struct holds both Context and Cancel, but the context is created with THREE cancels (timeout, context, allocator). Only the timeout cancel is exposed. This means:
Suggestion: Either remove the Cancel field (since cleanup handles everything) or rename it to TimeoutCancel for clarity. 5. Test Timeout ConsistencyDifferent tests use different timeouts:
Question: Are these timeouts based on empirical data or arbitrary? Consider documenting why certain tests need longer timeouts. 6. Docker Image Pull TimeoutThe 60-second pull timeout (e2e_helpers_test.go:115) might be too short on slow connections. Consider making this configurable via environment variable for CI environments with slow network. 🔐 Security Considerations
⚡ Performance Considerations
📝 Code Quality
🧪 Test CoverageThe PR description checklist shows:
Critical: The last item is unchecked. Combined with the !ci build tag concern, this suggests the Docker Chrome migration may not actually be running in CI yet. 📌 RecommendationsHigh Priority:
Medium Priority: Low Priority: ✨ ConclusionThis is a well-executed migration that addresses real CI reliability issues. The code is clean, follows good practices, and the comprehensive migration across all test files shows attention to detail. The main concern is the !ci build tag which may prevent the Docker Chrome setup from being used in CI environments - please verify this isn't blocking the intended functionality. Once the build tag issue is clarified and the unused function is addressed, this PR will be ready to merge. Recommendation: Approve with requested changes to build tags and unused code removal. |
1 participant
Summary
SetupDockerChrome,ConvertURLForDockerChrome)Problem
Tests like
TestMermaidDiagramsRenderingandTestLoadingIndicatorwere flaky in CI due to:Solution
Use Docker Chrome (
chromedp/headless-shell) with health polling - waits up to 60 seconds for Chrome to be ready before running tests.Test plan
go vetpasses🤖 Generated with Claude Code