From 501612309fef897c5f295207cea48e00b02d75c1 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 9 Sep 2026 14:01:10 +0200 Subject: [PATCH 1/3] Pin both e2e stacks to the gateway release the app is built on The Playwright stack pinned a July build of the gateway, which reports 0.6.0, while package.json takes the generated client at ^0.7.0. The suite was running against an older schema than the app is built on. Both stacks now pin gateway 0.7.0. The pin stays a digest rather than a tag, because tags on this registry are mutable and a re-run of the publishing workflow on the same commit moves one. It is the multi-arch manifest list for the release, so an arm64 host gets a native image instead of running the gateway under emulation. The rosbag stack defaulted to the mutable :latest tag, because the recording-id contract its specs assert on was newer than any published release. It ships in 0.7.0, so that default becomes the same digest, still overridable so a locally built gateway can be dropped in ahead of a release. --- e2e/docker-compose.rosbag.yml | 12 ++++++------ e2e/docker-compose.yml | 16 ++++++++++------ 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/e2e/docker-compose.rosbag.yml b/e2e/docker-compose.rosbag.yml index b3961ca..60d76b5 100644 --- a/e2e/docker-compose.rosbag.yml +++ b/e2e/docker-compose.rosbag.yml @@ -11,11 +11,11 @@ name: e2e-rosbag services: gateway: - # Overridable because the recording-id contract these specs assert on - # (ros2_medkit#620) is newer than any published tag: point this at a - # locally built image to run them before that lands. Once it is - # published, pin a digest here the way docker-compose.yml does. - image: ${E2E_ROSBAG_GATEWAY_IMAGE:-ghcr.io/selfpatch/ros2_medkit-jazzy:latest} + # The same gateway 0.7.0 digest docker-compose.yml pins, for the same + # reason: registry tags are mutable, so a tag would let an unrelated + # gateway change decide what these specs run against. Overridable so a + # locally built gateway can be dropped in ahead of a release. + image: ${E2E_ROSBAG_GATEWAY_IMAGE:-ghcr.io/selfpatch/ros2_medkit-jazzy@sha256:c706a4c16a4f4f2963690f776e93a70ac4afebbc52a0774a4edb984369a0dbd8} ports: # Loopback only, and on its own port so this stack can run alongside # the scripts one without either stealing the other's. @@ -69,7 +69,7 @@ services: # so the fault manager could not write a bag into it. Same one-shot chown # the scripts stack does for its upload volume. init-bags: - image: ${E2E_ROSBAG_GATEWAY_IMAGE:-ghcr.io/selfpatch/ros2_medkit-jazzy:latest} + image: ${E2E_ROSBAG_GATEWAY_IMAGE:-ghcr.io/selfpatch/ros2_medkit-jazzy@sha256:c706a4c16a4f4f2963690f776e93a70ac4afebbc52a0774a4edb984369a0dbd8} user: root volumes: - e2e-bags:/e2e-bags diff --git a/e2e/docker-compose.yml b/e2e/docker-compose.yml index 650b2f7..13e6992 100644 --- a/e2e/docker-compose.yml +++ b/e2e/docker-compose.yml @@ -5,8 +5,8 @@ services: # chowns the volume before the gateway starts; it reuses the pinned # gateway image (which already has chown) instead of pulling another one. init-uploads: - # ghcr.io/selfpatch/ros2_medkit-jazzy:sha-7939c94 - image: ghcr.io/selfpatch/ros2_medkit-jazzy@sha256:565db07e1e972b31684bf864fbaad7e8a70aacabf2ef0cd4510fdbd8e3281831 + # ghcr.io/selfpatch/ros2_medkit-jazzy, gateway 0.7.0 + image: ghcr.io/selfpatch/ros2_medkit-jazzy@sha256:c706a4c16a4f4f2963690f776e93a70ac4afebbc52a0774a4edb984369a0dbd8 user: root volumes: - e2e-uploads:/e2e-uploads @@ -14,10 +14,14 @@ services: gateway: # Pinned on purpose: :latest is overwritten on every push to the gateway # main branch, which would let unrelated changes turn this repo CI red. - # Pinned by digest, not by the sha-7939c94 tag alone: tags on this - # registry are mutable and a re-run of the publishing workflow on the - # same commit would move one. ghcr.io/selfpatch/ros2_medkit-jazzy:sha-7939c94 - image: ghcr.io/selfpatch/ros2_medkit-jazzy@sha256:565db07e1e972b31684bf864fbaad7e8a70aacabf2ef0cd4510fdbd8e3281831 + # Pinned by digest rather than by a tag: tags on this registry are + # mutable and a re-run of the publishing workflow on the same commit + # would move one. This digest is gateway 0.7.0, the release the app is + # built against: package.json takes the generated client at ^0.7.0, so + # the suite runs against the schema that client was generated from. It + # is the manifest-list digest, not a per-arch one, so an arm64 host gets + # a native image instead of running the gateway under emulation. + image: ghcr.io/selfpatch/ros2_medkit-jazzy@sha256:c706a4c16a4f4f2963690f776e93a70ac4afebbc52a0774a4edb984369a0dbd8 ports: # Bound to loopback only, on purpose: this gateway has uploads enabled # and executes uploaded shell scripts without authentication. From 9a84d80844907d57853f82011041c37592ed028e Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 9 Sep 2026 14:01:36 +0200 Subject: [PATCH 2/3] Let the rosbag stack's shutdown trap reach bash The trap never ran. Compose substitutes $FM, $SEED and $GW out of the command string before the container sees it, so the container installed a trap that runs `kill` with no operands, and every run logged three "variable is not set" warnings saying so. The children were left to the container teardown, which is the fault manager killed mid-write that the trap exists to avoid. Doubling the dollars keeps them for bash. The volume comment claimed a reused volume makes the suite see three recordings instead of two, as if that were a failure. The specs assert against the count they read back from the gateway, so three would pass; a clean volume is what keeps a local run comparable with CI, not what makes it correct. --- e2e/docker-compose.rosbag.yml | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/e2e/docker-compose.rosbag.yml b/e2e/docker-compose.rosbag.yml index 60d76b5..58ee51c 100644 --- a/e2e/docker-compose.rosbag.yml +++ b/e2e/docker-compose.rosbag.yml @@ -47,21 +47,25 @@ services: # with its exit code instead of leaving a healthy-looking stack whose # specs skip. The trap makes SIGTERM stop the children before the shell # exits. + # + # The `$$` are not a typo: Compose interpolates `$VAR` in this string + # before the container ever sees it, so a single `$` would hand bash an + # empty word and leave `kill` with no operands. command: - > source /opt/ros/jazzy/setup.bash; source /home/medkit/ws/install/setup.bash; ros2 run ros2_medkit_fault_manager fault_manager_node --ros-args --params-file /e2e/params.yaml & - FM=$!; + FM=$$!; python3 /e2e/seed_recordings.py & - SEED=$!; + SEED=$$!; ros2 run ros2_medkit_gateway gateway_node --ros-args --params-file /e2e/params.yaml & - GW=$!; - trap 'kill $FM $SEED $GW 2>/dev/null' TERM INT; - wait -n $FM $SEED $GW; - exit $? + GW=$$!; + trap 'kill $$FM $$SEED $$GW 2>/dev/null' TERM INT; + wait -n $$FM $$SEED $$GW; + exit $$? depends_on: init-bags: condition: service_completed_successfully @@ -78,6 +82,8 @@ services: volumes: # Holds the bags AND faults.db, and it outlives `docker compose down`. # Re-seed from a clean slate with `down -v` first: on a reused volume the - # fault is already CONFIRMED, the first confirm captures nothing, and the - # suite sees three recordings instead of two. + # fault is already CONFIRMED and the first confirm captures nothing, so the + # recording count drifts from what a fresh seed produces. The specs assert + # against the count they read back, so this is hygiene rather than a + # correctness gate, but it keeps a run comparable with CI. e2e-bags: From e35f19246e23e65d8571a473fba83d42640ba09e Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 9 Sep 2026 14:01:50 +0200 Subject: [PATCH 3/3] Run the rosbag specs in CI instead of letting them skip The e2e job started only the scripts stack. The three rosbag specs need a fault manager, which that stack does not run, so they skipped themselves on every run. A skip is green, so the job reported success whether or not they ever executed. The job now starts the rosbag stack too, on its own port, and waits for the fixture before running the suite. The wait judges the same app the specs judge: they take the first app whose fault list carries the code and never look further, so scanning on to a healthier one would let the wait pass while they skip. Anything that is not a plain count means the fixture is not ready rather than an app to step over, because the specs parse those same bodies and throw on a bad one. The deadline is checked again before success is declared, since the two requests before it can take five seconds each. The rosbag stack is torn down with -v because its volume holds faults.db as well as the bags, so a reused one starts with the fault already confirmed and seeds a different number of recordings than a clean run. CONTRIBUTING.md and README.md described a one-stack local run, which left the rosbag specs skipping on a developer machine even after CI stopped letting them, and offered port 8081 as the workaround for a busy 8080 - the rosbag stack's own default. --- .github/workflows/ci.yml | 23 +++++- CONTRIBUTING.md | 35 +++++++-- README.md | 5 +- e2e/wait-for-rosbag-fixture.sh | 138 +++++++++++++++++++++++++++++++++ 4 files changed, 194 insertions(+), 7 deletions(-) create mode 100755 e2e/wait-for-rosbag-fixture.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c4dd126..13abaed 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -40,7 +40,7 @@ jobs: e2e: runs-on: ubuntu-latest - timeout-minutes: 20 + timeout-minutes: 30 steps: - name: Checkout repository @@ -58,9 +58,19 @@ jobs: - name: Start gateway run: docker compose -f e2e/docker-compose.yml up -d + # A second stack, on its own port: the rosbag specs need a fault + # manager to own the recordings, which the scripts stack does not run. + - name: Start rosbag gateway + run: docker compose -f e2e/docker-compose.rosbag.yml up -d + - name: Install Playwright browsers run: npx playwright install --with-deps chromium + # The rosbag specs skip themselves when the fixture is not there, so + # without this the job stays green whether or not they ever ran. + - name: Wait for the rosbag fixture + run: ./e2e/wait-for-rosbag-fixture.sh + - name: Run E2E tests run: npm run test:e2e @@ -77,10 +87,21 @@ jobs: if: failure() run: docker compose -f e2e/docker-compose.yml logs + - name: Dump rosbag gateway logs on failure + if: failure() + run: docker compose -f e2e/docker-compose.rosbag.yml logs + - name: Stop gateway if: always() run: docker compose -f e2e/docker-compose.yml down -v + # -v matters here: the volume holds faults.db as well as the bags, so + # a reused one starts with the fault already confirmed and seeds a + # different number of recordings than a clean run. + - name: Stop rosbag gateway + if: always() + run: docker compose -f e2e/docker-compose.rosbag.yml down -v + docker-build: runs-on: ubuntu-latest timeout-minutes: 15 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8f44930..f97302a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -93,10 +93,20 @@ Before opening or updating a Pull Request, you **must**: The Playwright suite in `e2e/` runs the real UI against a containerised gateway instead of mocks, so it needs Docker. -1. Start the gateway: +1. Start both gateways. The scripts stack serves most of the suite; the rosbag + specs need the second one, which also runs a fault manager so a fault can own + black-box recordings. Skip it and those three specs skip themselves: ```bash docker compose -f e2e/docker-compose.yml up -d + docker compose -f e2e/docker-compose.rosbag.yml up -d + ``` + + The rosbag stack seeds its fixture in the background. Wait for it before + running the suite, the way CI does: + + ```bash + ./e2e/wait-for-rosbag-fixture.sh ``` 2. Run the suite: @@ -107,10 +117,14 @@ The Playwright suite in `e2e/` runs the real UI against a containerised gateway Use `npm run test:e2e:ui` instead to step through the tests with the Playwright UI. -3. Stop the gateway once you are done, dropping the uploads volume along with it: +3. Stop both gateways once you are done, dropping their volumes along with them. + The rosbag volume holds `faults.db` as well as the bags, so a reused one + starts with the fault already confirmed and seeds a different number of + recordings than a clean run: ```bash docker compose -f e2e/docker-compose.yml down -v + docker compose -f e2e/docker-compose.rosbag.yml down -v ``` `e2e/scripts.spec.ts` uploads, runs and deletes scripts against the shared gateway container, mutating its state as it goes, so it and the other specs that touch the live gateway are pinned to a single Playwright worker (see `playwright.config.ts`). Do not attempt to parallelize these specs or run them against a gateway instance you care about keeping in a known state. @@ -118,11 +132,22 @@ The Playwright suite in `e2e/` runs the real UI against a containerised gateway If port 8080 or 5173 is already taken on your machine, override the gateway port and/or the dev server URL before starting the stack: ```bash -E2E_GATEWAY_PORT=8081 docker compose -f e2e/docker-compose.yml up -d -E2E_GATEWAY_PORT=8081 npm run test:e2e +E2E_GATEWAY_PORT=8090 docker compose -f e2e/docker-compose.yml up -d +E2E_GATEWAY_PORT=8090 npm run test:e2e ``` -`E2E_GATEWAY_PORT` is the only variable you need for the gateway side: `e2e/global-setup.ts` derives the full gateway URL from it, and the gateway's CORS configuration allows any origin so an overridden dev server port is never rejected. Set `E2E_APP_URL` instead (e.g. `E2E_APP_URL=http://localhost:5174`) if the dev server port needs to change; `playwright.config.ts` derives the dev server's port from it. The gateway container stays bound to `127.0.0.1` regardless of the port chosen. +Do not reach for 8081 here: that is the rosbag stack's own default, so the two +gateways would fight over it, and with the rosbag stack down the rosbag specs +would point at the scripts gateway, find no seeded fault and skip for a reason +that has nothing to do with the code. + +`E2E_GATEWAY_PORT` is the only variable the scripts stack needs: `e2e/global-setup.ts` derives the full gateway URL from it, and the gateway's CORS configuration allows any origin so an overridden dev server port is never rejected. Set `E2E_APP_URL` instead (e.g. `E2E_APP_URL=http://localhost:5174`) if the dev server port needs to change; `playwright.config.ts` derives the dev server's port from it. The gateway container stays bound to `127.0.0.1` regardless of the port chosen. + +The rosbag stack has its own set, all optional: `E2E_ROSBAG_GATEWAY_PORT` +(default 8081), `E2E_ROSBAG_GATEWAY_URL`, `E2E_ROSBAG_FAULT_CODE`, and +`E2E_ROSBAG_FIXTURE_TIMEOUT` for the wait script. `E2E_ROSBAG_GATEWAY_IMAGE` +points the stack at a locally built gateway instead of the pinned release, which +is how you run these specs against a gateway change before it ships. ### Pull Request Checklist diff --git a/README.md b/README.md index ef7259a..a132aa1 100644 --- a/README.md +++ b/README.md @@ -87,8 +87,11 @@ npm run test:ui # Run tests with coverage npm run test:coverage -# Run the end-to-end suite against a containerised gateway +# Run the end-to-end suite against containerised gateways +# (the second stack carries the rosbag specs; see CONTRIBUTING.md) docker compose -f e2e/docker-compose.yml up -d +docker compose -f e2e/docker-compose.rosbag.yml up -d +./e2e/wait-for-rosbag-fixture.sh npm run test:e2e # Run the end-to-end suite with the Playwright UI diff --git a/e2e/wait-for-rosbag-fixture.sh b/e2e/wait-for-rosbag-fixture.sh new file mode 100755 index 0000000..6b72c3e --- /dev/null +++ b/e2e/wait-for-rosbag-fixture.sh @@ -0,0 +1,138 @@ +#!/usr/bin/env bash +# +# Copyright 2026 bburda +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +# Blocks until the rosbag stack holds the fixture rosbag-recordings.spec.ts +# needs, and exits non-zero if it never does. +# +# The specs skip themselves when the fixture is missing, which is right for a +# developer who did not bring the second stack up, but in CI a skip is +# indistinguishable from a pass. This script is what makes the difference +# visible. +# +# It judges the SAME app the specs will judge: appHoldingTheFault() takes the +# first app whose fault list carries the code and never looks further, so this +# stops there too. Scanning on to a later app that happens to hold enough bags +# would let this pass while the specs skip on the first one. +# +# Seeding is not instant. The fault has to confirm, be acknowledged and confirm +# again before a second bag exists, so the wait is a real one, not a formality. + +set -euo pipefail + +GATEWAY_PORT="${E2E_ROSBAG_GATEWAY_PORT:-8081}" +GATEWAY_URL="${E2E_ROSBAG_GATEWAY_URL:-http://localhost:${GATEWAY_PORT}/api/v1}" +FAULT_CODE="${E2E_ROSBAG_FAULT_CODE:-E2E_FLAPPING_SENSOR}" +TIMEOUT_SEC="${E2E_ROSBAG_FIXTURE_TIMEOUT:-240}" + +# Two, because the whole point of the specs is a fault that kept more than the +# newest recording. One bag means the gateway is up but the fixture is half +# seeded, which would skip the specs just as surely as no bag at all. +REQUIRED_RECORDINGS=2 + +deadline=$((SECONDS + TIMEOUT_SEC)) +last_state="nothing answered on ${GATEWAY_URL}" + +# Anything that is not a plain count means the gateway is not serving what the +# specs will parse. They call .json() on these same bodies and throw on a bad +# one, so a malformed answer is a fixture that is not ready yet - never an app +# to step over on the way to a healthier one. +is_count() { + [[ $1 =~ ^[0-9]+$ ]] +} + +expired() { + ((SECONDS >= deadline)) +} + +while ! expired; do + if ! apps=$(curl -fsS --max-time 5 "${GATEWAY_URL}/apps" 2>/dev/null); then + sleep 2 + continue + fi + + if ! app_ids=$(jq -r '.items[]?.id' <<<"${apps}" 2>/dev/null); then + last_state="${GATEWAY_URL} answered /apps with something jq could not read" + sleep 2 + continue + fi + + # The likeliest failure is a gateway that answers perfectly while the seeder + # never confirms the fault, so the no-fault case gets its own message rather + # than falling back to the "nothing answered" one. + saw_fault=0 + unusable=0 + + # Read line by line: an id is one line, and word splitting would break an id + # containing whitespace and glob-expand one containing * or ?. + while IFS= read -r app; do + [[ -n ${app} ]] || continue + if expired; then + unusable=1 + last_state="${GATEWAY_URL} was still being scanned when the deadline passed" + break + fi + + if ! faults=$(curl -fsS --max-time 5 "${GATEWAY_URL}/apps/${app}/faults" 2>/dev/null); then + unusable=1 + last_state="${GATEWAY_URL} did not serve faults for ${app}" + break + fi + + hits=$(jq --arg fc "${FAULT_CODE}" '[.items[]? | select(.fault_code == $fc)] | length' <<<"${faults}" 2>/dev/null) || hits='' + if ! is_count "${hits}"; then + unusable=1 + last_state="${GATEWAY_URL} answered faults for ${app} with something the specs cannot parse" + break + fi + ((hits > 0)) || continue + saw_fault=1 + + if ! bags=$(curl -fsS --max-time 5 "${GATEWAY_URL}/apps/${app}/bulk-data/rosbags" 2>/dev/null); then + unusable=1 + last_state="${GATEWAY_URL} did not serve rosbags for ${app}" + break + fi + + count=$(jq --arg fc "${FAULT_CODE}" \ + '[.items[]? | select((."x-medkit".fault_codes // []) | index($fc))] | length' <<<"${bags}" 2>/dev/null) || count='' + if ! is_count "${count}"; then + unusable=1 + last_state="${GATEWAY_URL} answered rosbags for ${app} with something the specs cannot parse" + break + fi + + # Checked again here, not only on entry: the two requests above can each + # take up to five seconds, so a scan that began in time can finish out of + # it, and a wait that reports success past its own deadline is not one. + if ((count >= REQUIRED_RECORDINGS)) && ! expired; then + echo "${app} holds ${count} recordings for ${FAULT_CODE}" + exit 0 + fi + + last_state="${app} holds ${count} recording(s) for ${FAULT_CODE}, need ${REQUIRED_RECORDINGS}" + # The specs stop at the first app whose faults carry the code, so this + # stops there too. Scanning on to a healthier app would let this pass + # while they skip on this one. + break + done <<<"${app_ids}" + + ((saw_fault || unusable)) || last_state="${GATEWAY_URL} answered, but no app reports ${FAULT_CODE} yet" + + sleep 2 +done + +echo "Timed out after ${TIMEOUT_SEC}s waiting for the rosbag fixture: ${last_state}" >&2 +exit 1