Replace docker compose script in yarn #8541
Conversation
📝 Walkthrough## Walkthrough
This change updates the automation scripts related to refreshing screenshots and end-to-end (E2E) snapshots in the project's `package.json`. The scripts now use a newly introduced `tools/docker-env.sh` shell script to manage Docker-based test environments. The `refresh-screenshots` script now retries the screenshot tests up to three times within a Puppeteer Docker container, while the `refresh-e2e-snapshots` script replaces Docker Compose commands with a direct `sbt` test invocation inside the Docker environment managed by `tools/docker-env.sh`. The new shell script handles Docker service setup, environment variable configuration, command execution, and cleanup. Additionally, the PostgreSQL port mapping was changed in the Docker Compose configuration, and the PostgreSQL port was updated in the CI workflow environment variables.
## Changes
| File(s) | Change Summary |
|------------------------------------------|---------------------------------------------------------------------------------------------------------------------|
| package.json | Updated `refresh-screenshots` to run `tools/refresh-screenshots.sh`; simplified `remove-e2e-snapshots`; updated `refresh-e2e-snapshots` to run `sbt` via `tools/docker-env.sh`. |
| tools/docker-env.sh | Added new shell script to set environment variables, start/stop Docker services, run passed commands, and clean up. |
| tools/refresh-screenshots.sh | Added new shell script to remove screenshots and run Puppeteer tests inside a Docker container with retries. |
| .github/docker-compose.yml | Changed PostgreSQL port mapping from `5432:5432` to `5434:5432`; switched PostgreSQL data volume to named Docker volume. |
| .github/workflows/build_test_deploy.yml | Updated `POSTGRES_URL` environment variable port from `5432` to `5434`. |
## Suggested labels
`CI`
## Poem
> 🐇
> In Docker’s warren, scripts now dwell,
> With bash and yarn, they cast their spell.
> Three hops for screenshots, E2E anew,
> Docker-env makes all things true!
> Snapshots refreshed, the tests all run—
> This rabbit’s work is never done!
> 🥕📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tools/docker-env.sh (1)
1-21: Solid approach for Docker environment management; consider signal trapping and error handling.This script effectively centralizes Docker environment setup and teardown for test workflows. It is readable and should work as intended for most CI and local development scenarios.
Suggestions for improvement:
- Signal trapping: If the script is interrupted (e.g., Ctrl+C), Docker containers may be left running. Consider trapping signals (SIGINT, SIGTERM, EXIT) to ensure cleanup.
- Docker daemon check: Add a check to ensure Docker is running before proceeding.
- Error handling: If
docker compose upfails, the script continues. Consider checking the exit status and aborting early if setup fails.- Working directory assumption: The script assumes it is run from the project root. Document this assumption or make the script robust to being called from other directories.
Example for signal trapping and error handling:
+cleanup() { + cd .github || exit + docker compose down -v + cd .. || exit +} +trap cleanup EXIT INT TERM + cd .github || exit docker compose down -v docker compose up -d postgres redis fossildb || exit 1 cd .. || exit
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.json(1 hunks)tools/docker-env.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
🔇 Additional comments (1)
package.json (1)
101-101: Good migration to centralized Docker environment; verify downstream dependencies and document retry rationale.
- The new scripts are cleaner and should improve reliability by centralizing Docker management.
- The retry loop for screenshots is a pragmatic solution for flaky tests; consider documenting the rationale for future maintainers.
- Please verify that all scripts, CI jobs, and developer workflows that previously used Docker Compose directly are now updated or accounted for, as the PR objectives mention possible breakage.
- Remember to update documentation and the changelog before merging.
Also applies to: 107-107
package.json
Outdated
| @@ -98,13 +98,13 @@ | |||
| "test-e2e": "tools/postgres/dbtool.js prepare-test-db && tools/test.sh test-e2e --timeout=60s --verbose", | |||
| "test-screenshot": "tools/test.sh test-screenshot --timeout=5m", | |||
| "test-wkorg-screenshot": "tools/test.sh test-wkorg-screenshot --timeout=5m", | |||
| "refresh-screenshots": "rm -rf frontend/javascripts/test/screenshots/** && docker compose up screenshot-tests", | |||
| "refresh-screenshots": "rm -rf frontend/javascripts/test/screenshots/** && ./tools/docker-env.sh bash -c 'for i in {1..3}; do yarn test-screenshot && break; done'", | |||
There was a problem hiding this comment.
I am getting a No matches found: "frontend/javascripts/test/screenshots/* error when trying to execute the script while the screenshots are already removed. There might be multiple possible reasons for this maybe zsh behaviour or so. This can be fixed by forcing execution in bash:
| "refresh-screenshots": "rm -rf frontend/javascripts/test/screenshots/** && ./tools/docker-env.sh bash -c 'for i in {1..3}; do yarn test-screenshot && break; done'", | |
| "refresh-screenshots": "bash -c 'rm -rf frontend/javascripts/test/screenshots/**' && ./tools/docker-env.sh bash -c 'for i in {1..3}; do yarn test-screenshot && break; done'", |
Could you please fix that? 🙏
There was a problem hiding this comment.
Moreover, the screenshot tests do not work for me :/. Don't know why but the dataset id cannot be retrieved successfully :/
dtype_datasets_rendering.screenshot › Dataset IDs were retrieved successfully public-test/test-bundle/test/puppeteer/dtype_datasets_rendering.screenshot.js:158 157: import_dataset_rendering_helpers.test.serial("Dataset IDs were retrieved successfully", (t) => { 158: (0, import_dataset_rendering_helpers.assertDatasetIds)(t, datasetNames, datasetNameToId); 159: }); Dataset ID not found for "dtype_test_uint8_color" Value is not truthy: undefined
@philippotto Do you maybe know more about this? I checked my browser stack credentials and wk auth token env var and they seemed to be alright.
package.json
Outdated
| @@ -98,13 +98,13 @@ | |||
| "test-e2e": "tools/postgres/dbtool.js prepare-test-db && tools/test.sh test-e2e --timeout=60s --verbose", | |||
| "test-screenshot": "tools/test.sh test-screenshot --timeout=5m", | |||
| "test-wkorg-screenshot": "tools/test.sh test-wkorg-screenshot --timeout=5m", | |||
| "refresh-screenshots": "rm -rf frontend/javascripts/test/screenshots/** && docker compose up screenshot-tests", | |||
| "refresh-screenshots": "rm -rf frontend/javascripts/test/screenshots/** && ./tools/docker-env.sh bash -c 'for i in {1..3}; do yarn test-screenshot && break; done'", | |||
There was a problem hiding this comment.
Moreover, the screenshot tests do not work for me :/. Don't know why but the dataset id cannot be retrieved successfully :/
dtype_datasets_rendering.screenshot › Dataset IDs were retrieved successfully public-test/test-bundle/test/puppeteer/dtype_datasets_rendering.screenshot.js:158 157: import_dataset_rendering_helpers.test.serial("Dataset IDs were retrieved successfully", (t) => { 158: (0, import_dataset_rendering_helpers.assertDatasetIds)(t, datasetNames, datasetNameToId); 159: }); Dataset ID not found for "dtype_test_uint8_color" Value is not truthy: undefined
@philippotto Do you maybe know more about this? I checked my browser stack credentials and wk auth token env var and they seemed to be alright.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tools/refresh-screenshots.sh (1)
7-13: Harden and pin the Docker run command
A few improvements to make the Docker invocation more reproducible and robust:
Pin the image instead of using
:master:-scalableminds/puppeteer:master +scalableminds/puppeteer:v1.2.3 # or use a digestDerive the repo root from the script’s location to avoid relative‐path pitfalls:
REPO_ROOT="$(cd "$(dirname "$0")/.." && pwd)"Then mount with
-v "${REPO_ROOT}:/home/pptruser/webknossos".Validate required env vars up front:
: "${URL:?Please set URL}" : "${WK_AUTH_TOKEN:?Please set WK_AUTH_TOKEN}"Optionally pre-pull the image to surface registry errors early:
docker pull scalableminds/puppeteer:v1.2.3These changes improve reproducibility, error reporting, and portability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/docker-compose.yml(2 hunks)package.json(1 hunks)tools/refresh-screenshots.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/docker-compose.yml
- package.json
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
|
|
||
| set -xe | ||
|
|
||
| rm -rf frontend/javascripts/test/screenshots/** |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix globbing to reliably clear screenshots
The pattern ** requires shopt -s globstar (not enabled by default), so it may not remove nested files. A simpler, reliable approach is:
-rm -rf frontend/javascripts/test/screenshots/**
+rm -rf frontend/javascripts/test/screenshots/*Or explicitly enable recursive globbing at the top:
shopt -s globstarEither ensures the directory is properly emptied.
| #!/usr/bin/env bash | ||
|
|
||
| set -xe |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Enhance shell strictness for safer execution
Currently the script uses set -xe, but adding stricter options can catch more classes of failures and undefined variables:
-set -xe
+set -euxo pipefail-u: error on unset variables-o pipefail: propagate failures in pipelines- Keeps
-x(trace) and-e(exit on error)
This hardens the script and surfaces issues early.
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' tools/refresh-screenshots.shLength of output: 398
Enhance shell strictness for safer execution in tools/refresh-screenshots.sh
In the shebang section, replace:
- set -xe
+ set -euxo pipefail-u: error on unset variables (e.g.$URL,$WK_AUTH_TOKEN)-o pipefail: catch failures in any pipeline- retains
-x(command tracing) and-e(exit on error)
This hardens the script and surfaces issues early.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #!/usr/bin/env bash | |
| set -xe | |
| #!/usr/bin/env bash | |
| set -euxo pipefail |
|
This will also need to be adapted now to the new snapshot locations due to the changes from #8543 |
|
I just tied to refresh the snapshots with this PR's code but was unable to do so. I think the reason is that the docker (compose) runs an outdated node version: And |
|
The node images require an update in our Dockerfiles: See scalableminds/dockerfiles#65 |
|
screenshots also work 🎉 |
Steps to test:
yarn refresh-screenshotsandyarn refresh-e2e-snapshotsTODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)