refactor(testing): add Docker Chrome helpers for reliable CI testing#85
refactor(testing): add Docker Chrome helpers for reliable CI testing#85
Conversation
- Add SetupDockerChrome() helper to simplify Docker Chrome setup in tests - Add WaitForServer() helper to replace arbitrary time.Sleep() calls - Refactor loading indicator and focus preservation tests to use: - Dynamic port allocation via GetFreePort() - Docker Chrome for consistent CI execution - Proper server readiness checks - GetChromeTestURL() for Docker-to-host networking This improves test reliability by eliminating hardcoded ports and race conditions from sleep-based waiting. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds reusable helpers to make browser E2E tests more reliable in CI by standardizing Docker-based Chrome setup and replacing arbitrary sleeps with a server-readiness poll, then refactors selected tests to use these helpers and dynamic ports.
Changes:
- Added
SetupDockerChrome()and aDockerChromeContexthelper to simplify Docker Chrome usage in tests that run their own HTTP server. - Added
WaitForServer()helper to poll for server readiness instead of usingtime.Sleep. - Refactored 4 E2E tests to use dynamic ports + Docker Chrome + readiness polling.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| testing/testing.go | Introduces DockerChromeContext and SetupDockerChrome() helper for Docker-based chromedp contexts. |
| testing/chrome.go | Adds WaitForServer() helper for server readiness polling. |
| e2e/livetemplate_core_test.go | Updates loading/focus E2E tests to use dynamic ports, Docker Chrome, and server readiness checks. |
Comments suppressed due to low confidence (1)
e2e/livetemplate_core_test.go:1216
urlis nowGetChromeTestURL(port)(host.docker.internal), but thishttp.Get(url)runs on the host test process, not inside the Docker Chrome container. On Linux CI,host.docker.internaltypically won’t resolve from the host, causing this check to fail. Usehttp://localhost:<port>for the host-side fetch, and keepGetChromeTestURLonly for chromedp navigation inside Docker.
// Use host.docker.internal for Docker Chrome to access host server
url := e2etest.GetChromeTestURL(port)
resp, err := http.Get(url)
if err != nil {
t.Fatalf("Failed to fetch page: %v", err)
}
testing/testing.go
Outdated
| // | ||
| // // Use GetChromeTestURL for Docker Chrome to access host | ||
| // url := e2etest.GetChromeTestURL(port) | ||
| // chromedp.Run(chromeCtx, chromedp.Navigate(url), ...) |
There was a problem hiding this comment.
In the usage comment, SetupDockerChrome returns a *DockerChromeContext, but the example calls chromedp.Run(chromeCtx, ...). That won’t compile and may mislead future test authors; the example should use chromeCtx.Context (or return a context.Context directly).
| // chromedp.Run(chromeCtx, chromedp.Navigate(url), ...) | |
| // chromedp.Run(chromeCtx.Context, chromedp.Navigate(url), ...) |
| 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, | ||
| } |
There was a problem hiding this comment.
DockerChromeContext.Cancel is set to timeoutCancel, which only cancels the timeout context and does not cancel the chromedp context/allocator or stop the Docker container. Since this is an exported field, it’s easy to misuse and leak containers/resources; consider removing the Cancel field or making it cancel the full Chrome session (or expose only the cleanup function).
| 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) |
There was a problem hiding this comment.
WaitForServer uses http.Get with the default client, which has no request/response timeout. If the connection hangs (e.g., SYN stalls or server accepts but never responds), this function can block well past the provided timeout. Use an http.Client with a small per-request timeout (or NewRequestWithContext) so the overall deadline is respected.
Code Review: PR #85 - Docker Chrome Test HelpersSummaryThis PR refactors E2E testing infrastructure by introducing reusable Docker Chrome helpers ( ✅ Strengths1. Excellent Design & Abstraction
2. Code Quality
3. Cleanup & Resource Management
4. Testing Best Practices
🔍 Issues & Concerns1. Missing Test Coverage
|
- Fix docstring example to use chromeCtx.Context (Copilot) - Remove Cancel field from DockerChromeContext to prevent misuse (Copilot) Users should use the cleanup function returned by SetupDockerChrome - Add per-request HTTP timeout to WaitForServer (Copilot) Prevents indefinite blocking if connection hangs Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
PR Review: Refactor Testing Helpers for Docker ChromeOverviewThis PR refactors E2E tests to use new helper functions for Docker Chrome setup and server readiness checks. The changes improve test reliability by eliminating hardcoded ports and race conditions. ✅ Strengths1. Excellent Problem-Solution Fit
2. Well-Designed Abstractions
3. Good Documentation
🔍 Code Quality Observationstesting/testing.go:268-301 ✅The
testing/chrome.go:351-370 ✅
e2e/livetemplate_core_test.go ✅Refactored tests follow consistent patterns:
🐛 Potential Issues1. Missing Error Check (Minor)Location: The tests check chromeCtx, cleanup := e2etest.SetupDockerChrome(t, 30*time.Second)
defer cleanup()
ctx := chromeCtx.Context // Used immediately without checking for setup errorsImpact: Low - Recommendation: This is acceptable given 2. Cleanup Order with Deferred FunctionsLocation: All refactored tests in The tests have two deferred cleanup functions: chromeCtx, cleanup := e2etest.SetupDockerChrome(t, 30*time.Second)
defer cleanup() // Chrome cleanup (second to execute)
// ... server setup ...
defer func() { // Server cleanup (first to execute)
shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
if err := server.Shutdown(shutdownCtx); err != nil {
t.Logf("Server shutdown warning: %v", err)
}
}()Impact: Low - The cleanup order (server first, then Chrome) is actually correct for this use case, since Chrome depends on the server. Observation: The order is correct but worth noting for future reviewers. 🚀 Performance & ReliabilityPositive Impact
ConcernsNone identified. The polling intervals (100ms) are reasonable for test scenarios. 🔒 Security ConsiderationsNo security concerns. These are test-only utilities. 🧪 Test CoverageCoverage Impact
RecommendationConsider adding unit tests for
This could be a follow-up PR rather than blocking this one. 📝 Documentation & MaintainabilityStrengths
Minor Suggestions
✅ Final VerdictLGTM - Approve with minor observations This is a well-executed refactoring that improves test reliability without introducing regressions. The new abstractions are clean, well-documented, and properly tested through usage. The minor issues noted are cosmetic and don't affect functionality. Checklist
Recommendations for Future PRs
Great work! This refactoring significantly improves the test infrastructure. 🎉 |
On Linux CI, host.docker.internal only resolves from inside Docker containers, not from the host machine. This fixes the test failure: Failed to fetch page: Get "http://host.docker.internal:38295": dial tcp: lookup host.docker.internal: no such host Now uses: - localhost for host-side http.Get() calls (runs on test host) - GetChromeTestURL() only for chromedp navigation (runs inside Docker) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Code Review - PR #85Overall, this is a well-structured refactoring that significantly improves test reliability. The changes follow good practices and the implementation is solid. Below are my detailed findings: ✅ Strengths
🔍 Code Quality Observationstesting/testing.go:268-302 (SetupDockerChrome)Good:
Minor Suggestion: cleanup := func() {
// Cancel in reverse order of creation
timeoutCancel() // Cancel timeout first
ctxCancel() // Then context
allocCancel() // Then allocator
StopDockerChrome(t, chromePort) // Finally stop container
}testing/chrome.go:348-369 (WaitForServer)Good:
Potential Issue: resp, err := client.Get(serverURL)
if err == nil {
resp.Body.Close()
if resp.StatusCode < 500 { // Accept any non-server-error status
t.Logf("✅ Server ready at %s", serverURL)
return
}
}However, this might be intentional if you want to detect the server is listening regardless of its health state. e2e/livetemplate_core_test.go (Test Refactoring)Good:
Observation: localhostURL := fmt.Sprintf("http://localhost:%d", port)
chromeURL := e2etest.GetChromeTestURL(port)
resp, err := http.Get(localhostURL) // Correct - host-side fetchThis is excellent and prevents a common mistake. 🔒 Security ConsiderationsNo security concerns identified. The changes:
⚡ Performance ConsiderationsPositive:
Minor Note: 🧪 Test CoverageObservation:
Suggestion:
This would increase confidence that the helpers work correctly in edge cases. 📋 DocumentationGood:
Suggestion: 🐛 Potential Issues
✅ RecommendationsMust Fix: None Should Consider:
Nice to Have:
📊 SummaryThis is a high-quality refactoring that achieves its stated goals:
The code is well-written, properly documented, and ready for merge. The suggestions above are minor improvements and edge case considerations rather than blocking issues. Verdict: Approved with minor suggestions for future improvements. Review generated with assistance from Claude Code |
1 participant
Summary
SetupDockerChrome()helper to simplify Docker Chrome setup in tests that manage their own serverWaitForServer()helper to replace arbitrarytime.Sleep()calls with proper readiness checksChanges
testing/testing.goDockerChromeContextstruct andSetupDockerChrome()helpertesting/chrome.goWaitForServer()helper functione2e/livetemplate_core_test.goBenefits
GetFreePort()instead of hardcoded 9001-9004WaitForServer()replacestime.Sleep(100ms)with proper pollingTest plan
🤖 Generated with Claude Code