[codex] Add Playwright E2E coverage - #39
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (25)
📝 WalkthroughWalkthroughThis PR introduces a comprehensive Docker-based E2E testing infrastructure with multi-stage builds, a reusable base image, and Playwright browser tests. It adds suite-based E2E execution (browser/shell/install-uninstall), panel bootstrap scripts, UI test instrumentation via data-testid attributes, and a safety check requiring ChangesDocker E2E Testing Infrastructure with Playwright
Remove Command Safety
Supporting Configuration Changes
Sequence Diagram(s)sequenceDiagram
participant GHA as GitHub Actions
participant GHCR as GHCR Registry
participant Docker as Docker Build
participant Compose as Docker Compose
participant App as App Container
participant TestRunner as Test Runner
participant DB as Database
participant Browser as Browser
GHA->>Docker: Build E2E base image (PHP/Node/Panel)
Docker->>GHCR: Push base image to GHCR
GHA->>Docker: Build app image (from base + repo)
Docker->>Compose: app image ready
GHA->>Docker: Build test-runner image (from base + repo)
Docker->>Compose: test-runner image ready
Compose->>DB: Start database
Compose->>App: Start app container
App->>DB: Connect to database
GHA->>App: Run setup-panel.sh (Notur install/migrations/bootstrap)
App->>DB: Create/migrate Notur tables
App->>App: Install extensions, seed data
Compose->>TestRunner: Start test-runner container
TestRunner->>App: Wait for healthcheck (HTTP 200 on /auth/login)
TestRunner->>Browser: Launch Playwright
Browser->>App: POST /sanctum/csrf-cookie
Browser->>App: POST /login (with XSRF header)
Browser->>App: GET /admin/extensions (authenticated)
Browser->>DB: Verify extension settings persist
Browser->>App: Exercise UI flows (install/enable/disable/remove)
GHA->>Compose: Reset containers between test suites
GHA->>TestRunner: Run install-uninstall suite
TestRunner->>App: Assert Notur routes/state before uninstall
TestRunner->>App: Run notur:uninstall --force
TestRunner->>App: Assert all Notur state removed
GHA->>GHA: Collect logs (.env, extensions.json, container logs)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes This PR contains substantial, heterogeneous changes across Docker infrastructure, test automation, UI instrumentation, and command safety. The Docker refactoring involves multi-stage build logic, environment variable parameterization, and orchestration changes. The Playwright test suite is comprehensive with 600+ lines covering multiple admin workflows and database assertions. UI testid additions are repetitive but span three view files. The remove command safety feature touches controller, command, and two separate test files. Review requires understanding Docker build/compose mechanics, E2E test patterns, Playwright specifics, and tracing control flow changes across the extension removal feature. Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.54)PHPStan was skipped because the sandbox runner could not parse its output. 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. Comment |
🤖 Augment PR SummarySummary: This PR adds Docker-backed Playwright E2E coverage for Notur’s admin/runtime flows and wires it into CI. Changes:
Technical Notes: CI prefers a pre-published GHCR base image for speed, but can fall back to building the base layer with GHA cache when unavailable. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| "sort-packages": true | ||
| }, | ||
| "minimum-stability": "stable" | ||
| "minimum-stability": "dev", |
There was a problem hiding this comment.
composer.json:56 sets minimum-stability to dev, which changes dependency resolution for all consumers of this package (not just E2E) and can pull dev versions unexpectedly. This could make installs less deterministic even with prefer-stable enabled.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| ok "Panel migrations complete" | ||
| echo "" | ||
| if [ "$SUITE" = "install-uninstall" ]; then | ||
| info "Checking panel availability before Notur installation..." |
There was a problem hiding this comment.
docker/e2e/run-e2e.sh:186 checks panel availability for the install-uninstall suite before setup-panel.sh runs migrations/bootstrap. On a fresh DB this will likely never return a 2xx response (curl -f), causing the install/uninstall suite to time out.
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| local retries=0 | ||
|
|
||
| while [ "$retries" -lt "$max_retries" ]; do | ||
| if curl -s -o /dev/null --max-time 5 "$url" 2>/dev/null; then |
There was a problem hiding this comment.
tests/E2E/run-tests.sh:13 treats any HTTP response as "ready" because curl doesn't use -f, so 500/404 responses will still pass the readiness gate. This can start Playwright while the panel is still erroring, leading to flaky failures.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| local value="$2" | ||
|
|
||
| if grep -q "^${key}=" .env; then | ||
| sed -i "s|^${key}=.*|${key}=${value}|" .env |
There was a problem hiding this comment.
docker/e2e/setup-panel.sh:21 writes values into .env via sed without escaping replacement metacharacters (notably & and backslashes). If any of the env values contain these characters (e.g., a DB password), the .env line can be corrupted and subsequent artisan/composer steps may fail.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
1 participant
Summary
Validation
git diff --cached --checknpx playwright test --config=playwright.config.ts --listbash docker/e2e/run-e2e.sh --suite browser- 19 passedNotes
Generated local artifacts were intentionally left out of the commit: root
checksums.json,test.txt.rej,examples/hello-world/package-lock.json, andexamples/hello-world/node_modules/.Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests
Chores