-
Notifications
You must be signed in to change notification settings - Fork 37
feat: improve e2e test infrastructure #132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add ContainerOptions struct with HostAccess for tests needing host.docker.internal access - Add runContainerWithOptions() to support new container options - Add getContainerLogs() helper to retrieve container logs for debugging - Add waitHTTPOrExitWithLogs() that captures container logs on failure and periodically logs container status during wait - Auto-inject --no-sandbox flag for CI environments (Ubuntu 24.04 GA) - Fix tmpfs mode to 1777 for proper /dev/shm permissions - Run tests sequentially (-p 1) to avoid port conflicts - Add cookie and IndexedDB persistence tests These improvements were developed in kernel-images-private and contributed back to the public upstream.
a03b139 to
b67bc9a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
- Move defer stopContainer immediately after runContainerWithOptions to ensure container cleanup even if wait fails - Fail test if unzip exits with non-zero code instead of just logging - Check HTTP status code (200) in restartChromium, not just curl exit - Fail test with clear message if CDP doesn't become ready after 15s
The cookie and IndexedDB persistence tests require kernel-browser (patched Chromium with session cookie persistence and faster flush). These patches only exist in the private images, so these tests cannot run on the public kernel-images repo.
| go test -v -race ./... | ||
| # Run tests sequentially (-p 1) to avoid port conflicts in e2e tests | ||
| # (all e2e tests bind to the same ports: 10001, 9222) | ||
| go test -v -race -p 1 ./... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we randomly select available port in the testing suite?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have a plan ready to go to do this but just trying to fix web-bot-auth stuff before i open a follow up pr for parallelizing these tests and picking a random port
hiroTamada
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one question. But LGTM
Summary
This PR contributes improvements to the e2e test infrastructure:
Test Infrastructure Improvements
ContainerOptionsstruct - AddsHostAccessoption for tests that need--add-host=host.docker.internal:host-gatewayto reach services on the host machinerunContainerWithOptions()- New function supportingContainerOptionsfor flexible container configurationgetContainerLogs()- New helper to retrieve the last N lines of container logs for debugging failed testswaitHTTPOrExitWithLogs()- Enhanced wait function that:--no-sandboxauto-injection - Automatically adds--no-sandboxtoCHROMIUM_FLAGSfor CI environments where unprivileged user namespaces may be disabled (e.g., Ubuntu 24.04 GitHub Actions)--tmpfsmode fix - Changed from:size=2gto:size=2g,mode=1777for proper/dev/shmpermissions-p 1flag to run tests sequentially, avoiding port conflicts since all e2e tests bind to the same ports (10001, 9222)Test plan