-
Notifications
You must be signed in to change notification settings - Fork 35
Update tests #91
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
Update tests #91
Conversation
Mesa DescriptionThis PR overhauls the CI workflows for the Chromium images to improve reliability and add multi-platform support. Key changes include:
Note Refactors CI to build and run headful/headless Chromium images on amd64/arm64 with concurrency and service readiness checks, builds images inline in server tests, and adds DETACH support to run scripts.
Written by Cursor Bugbot for commit 5185d51. This will update automatically on new commits. Configure here. Description generated by Mesa. Update settings |
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.
Performed full review of b51da91...da55596
Analysis
-
Container name hardcoding in trap cleanup creates brittle coupling rather than using the defined NAME environment variable, making future naming changes difficult.
-
Using docker run with --rm flag in detached mode auto-removes failed containers, making CI failure debugging nearly impossible since logs are lost.
-
Complex shell-based health checking logic (using awk, grep, sed, tr) is difficult to maintain and error-prone compared to more robust alternatives like a Python script or supervisorctl's XML-RPC API.
-
While container startup is tested, there's no functional testing to verify Chromium can actually launch and render pages, making this merely a smoke test.
-
The PR represents a significant architectural shift from build-and-push to build-and-test locally, but lacks documentation explaining this strategic change and its implications.
Tip
Help
Slash Commands:
/review- Request a full code review/review latest- Review only changes since the last review/describe- Generate PR description. This will update the PR body or issue comment depending on your configuration/help- Get help with Mesa commands and configuration options
4 files reviewed | 0 comments | Edit Agent Settings • Read Docs
| if [[ "${DETACH:-false}" == "true" ]]; then | ||
| docker run -d --rm "${RUN_ARGS[@]}" "$IMAGE" | ||
| else | ||
| docker run -it "${RUN_ARGS[@]}" "$IMAGE" |
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.
Bug: Bug
Inconsistent container cleanup behavior: interactive mode (line 88) is missing the --rm flag while detached mode (line 86) has it. This causes containers to persist after exit when run interactively but be auto-removed when run detached. The chromium-headless script consistently uses --rm in both modes (lines 31 and 33), which is the correct pattern. This inconsistency will lead to container accumulation when running in interactive mode.
TODO
Note
Refactors CI to locally build/run multi-arch Chromium images with Docker layer caching and concurrency, updates server tests to use these images, and adds cache-aware build/run scripts with optional detachment.
.github/workflows/chromium-headful-image.yaml,chromium-headless-image.yaml):push/pull_request; addconcurrency.${CACHE_DIR}; build images viabuild-docker.sh.pulseaudioexception)..github/workflows/server-test.yaml):concurrency,timeout-minutes, Docker layer cache restore.E2E_*_IMAGEenv to test tags.shared/start-buildkit.sh: exportCACHE_ARGSwhenCACHE_DIRis set (local cache-from/to).images/chromium-*/build-docker.sh: pass"${CACHE_ARGS[@]}"todocker build.images/chromium-*/run-docker.sh: supportDETACH=trueto run containers in background..gitignore: ignore.buildx-cache-*.package.json.Written by Cursor Bugbot for commit 38ffdfe. This will update automatically on new commits. Configure here.