Add integration testing with testcontainers and Playwright#442
Add integration testing with testcontainers and Playwright#442
Conversation
The integration-tests crate defines traits, error types, and helpers consumed by Docker-dependent tests. Without Docker the compiler sees them as unused and -D warnings promotes them to errors. A crate-level allow keeps CI green.
Proves that a next/script with strategy="afterInteractive" executes after a client-side navigation through the trusted-server proxy. The test SPA-navigates from / to /dashboard, waits for the script's global marker, asserts it ran exactly once, and confirms no document-level request fired (true SPA, not full reload).
prk-Jr
changed the title
…hub.com-prkjr:IABTechLab/trusted-server into feature/integration-testing-with-testcontainers
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Solid integration testing infrastructure. The RuntimeEnvironment + FrontendFramework matrix design is clean and extensible, the two-layer approach (Rust HTTP + Playwright browser) gives good coverage, and the assertion library having its own unit tests is a nice touch.
Cross-cutting notes
📝 Separate Cargo.lock — The 3400-line crates/integration-tests/Cargo.lock is disconnected from the workspace lock. If error-stack (or any shared dep) is updated in the workspace but not here, they silently diverge. Consider documenting the upgrade process or adding a CI check that validates shared dependency versions stay in sync.
📝 Viceroy config placeholders — viceroy-template.toml uses key = "placeholder" / data = "placeholder" for all KV stores. This means integration tests don't exercise KV store functionality, which is fine but worth a comment in the config noting this intentional scope limitation.
⛏ .gitignore missing trailing newline — The last line lacks a POSIX-compliant trailing newline.
What's done well
👍 Drop impl on ViceroyHandle ensures process cleanup even on panics — good defensive design.
👍 Assertion functions (assertions.rs) have thorough unit tests covering both passing and failing cases.
👍 Browser tests use hydration detection (data-hydrated attribute) before SPA navigation — avoids the classic "clicked before client router was ready" flake.
👍 Health check endpoint is namespaced (/__trusted-server/health) to minimize publisher route conflicts.
👍 Shell scripts detect native target dynamically via rustc -vV — no hardcoded platform assumptions.
👍 Smart use of PHP built-in server for WordPress fixture — avoids MySQL dependency while still testing real HTML processing behavior.
| branches: [main] | ||
| pull_request: | ||
| pull_request_review: | ||
| types: [submitted] |
There was a problem hiding this comment.
🌱 pull_request_review trigger means integration tests re-run on every review submission, even when no code changed. Combined with the pull_request trigger, an approval on an existing PR triggers a full redundant run. Consider removing this trigger or using workflow_dispatch for on-demand runs.
There was a problem hiding this comment.
One follow-up I still recommend:
.github/workflows/integration-tests.yml currently runs on pull_request_review approvals, which can trigger full integration + browser reruns without any code change.
Severity: P2 (efficiency/cost)
Suggestion: remove the pull_request_review trigger and keep pull_request + workflow_dispatch for canonical/manual runs.
Verdict: needs discussion (not a hard blocker).
| pull_request_review: | ||
| types: [submitted] |
There was a problem hiding this comment.
P2 (efficiency/cost): pull_request_review triggers will re-run the full integration + browser test suites on every review submission — even when there are no code changes. Consider removing this trigger and keeping just pull_request + workflow_dispatch for canonical/manual runs.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
All 12 CI checks pass. The architecture is solid — clean separation between runtime environments, frontend frameworks, and test scenarios; proper error-stack usage; good Drop-based cleanup; thoughtful extensibility patterns.
Three blocking issues need attention before merge, plus six non-blocking suggestions.
Blocking: 3 · Non-blocking: 6
| - name: Install Viceroy | ||
| if: steps.cache-viceroy.outputs.cache-hit != 'true' | ||
| shell: bash | ||
| run: cargo install --git https://github.com/fastly/Viceroy viceroy |
There was a problem hiding this comment.
🔧 Blocking — Unpinned Viceroy install
Both the cache key (line 41, git ls-remote HEAD) and this install use whatever is latest on Viceroy's default branch. A breaking upstream change will break CI with zero code changes in this repo.
Pin to a specific tag or --rev:
run: cargo install --git https://github.com/fastly/Viceroy --tag viceroy-v0.12.1 viceroyAnd update the cache key to match the pinned rev instead of HEAD.
|
|
||
| on: | ||
| push: | ||
| branches: [main] |
There was a problem hiding this comment.
🔧 Blocking — Workflow still triggers on push: main
Commit 5b00113 says "Trigger integration test on pull requests only" but this trigger is still here. Every merged PR runs the full Docker+WASM+Playwright suite twice (on the PR and on the push to main).
Remove the push trigger or make the duplication intentional with a comment explaining why.
| jobs: | ||
| integration-tests: | ||
| name: integration tests | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
🔧 Blocking — No timeout-minutes on CI jobs
Neither integration-tests nor browser-tests has a timeout-minutes. If Viceroy hangs or a container stalls, jobs run until GitHub's default 6-hour timeout, silently burning CI minutes.
jobs:
integration-tests:
runs-on: ubuntu-latest
timeout-minutes: 15|
|
||
| // If the response is a Flight payload, it must not contain injected scripts | ||
| if body.contains("/static/tsjs=") { | ||
| return Err(Report::new(TestError::ScriptTagNotFound) |
There was a problem hiding this comment.
🤔 Non-blocking — Wrong error variant semantics
TestError::ScriptTagNotFound fires when a script tag was found but shouldn't have been (RSC Flight response). The variant name says the opposite of the actual condition. A variant like UnexpectedScriptInjection would be semantically correct and avoid confusing future readers of error output.
| uses: actions/upload-artifact@v4 | ||
| if: failure() | ||
| with: | ||
| name: playwright-report |
There was a problem hiding this comment.
🤔 Non-blocking — Playwright report overwritten between framework runs
The Next.js run (line 68) writes to playwright-report/, then the WordPress run (line 77) overwrites it. This artifact upload only captures the WordPress report. If Next.js fails but WordPress passes, the failure report is lost.
Consider distinct artifact names per framework, or separate report output directories via PLAYWRIGHT_HTML_REPORT env var.
|
|
||
| test.beforeEach(async ({}, testInfo) => { | ||
| const state = readState(); | ||
| if (state.framework !== "nextjs") { |
There was a problem hiding this comment.
🤔 Non-blocking — Silent full-skip on framework typo
This pattern appears in 4+ spec files. If TEST_FRAMEWORK has a typo (e.g. nexjs), all gated tests silently skip → green CI with zero assertions run. Consider a guard in readState() or the beforeEach that fails on unrecognized framework values.
| // These modules are shared by ignored Docker-backed tests and unit tests | ||
| // inside the same integration test target, so some items are intentionally | ||
| // unused in a plain `cargo test` compile. | ||
| #[allow(dead_code, unused_imports)] |
There was a problem hiding this comment.
♻️ Non-blocking — Blanket lint suppression
This suppresses all dead_code and unused_imports warnings across three entire modules. It could mask real dead code that accumulates over time. Consider targeting #[allow] to specific items that are only used in the #[ignore]d tests.
| /** Kill a Viceroy process. */ | ||
| export function stopViceroy(pid: number): void { | ||
| try { | ||
| process.kill(pid, "SIGTERM"); |
There was a problem hiding this comment.
🌱 Non-blocking — stopViceroy sends SIGTERM without awaiting exit
process.kill(pid, "SIGTERM") returns immediately. If the suite runs frameworks back-to-back, the old Viceroy process could still hold its port when the next one tries to start. Consider awaiting the process exit or adding a short grace period.
| run: >- | ||
| cargo test | ||
| --manifest-path crates/integration-tests/Cargo.toml | ||
| --target x86_64-unknown-linux-gnu |
There was a problem hiding this comment.
📌 Non-blocking — Hardcoded target triple
x86_64-unknown-linux-gnu is hardcoded here, while the local script (integration-tests.sh) auto-detects via rustc -vV. Fine today on ubuntu-latest, but inconsistent with the local script approach. Consider aligning them.
3 participants
Summary
/healthendpoint and CI workflow for running both test suites separately from the main CIChanges
crates/fastly/src/main.rs/healthGET endpoint returning 200 with "ok" bodycrates/integration-tests/Cargo.tomlcrates/integration-tests/tests/common/assert_form_action_rewritten(9 unit tests)crates/integration-tests/tests/environments/crates/integration-tests/tests/frameworks/crates/integration-tests/tests/integration.rstest_all_combinations, per-framework tests)crates/integration-tests/fixtures/configs/crates/integration-tests/fixtures/frameworks/wordpress//wp-admin/stubcrates/integration-tests/fixtures/frameworks/nextjs/crates/integration-tests/browser/crates/integration-tests/browser/tests/shared/crates/integration-tests/browser/tests/nextjs/crates/integration-tests/browser/tests/wordpress/.github/workflows/integration-tests.ymlscripts/integration-tests.shscripts/integration-tests-browser.shcrates/integration-tests/README.mdINTEGRATION_TESTS_PLAN.mdCargo.tomlCargo.lockTest coverage
HTTP-level (Rust + testcontainers)
HtmlInjectionScriptServingAttributeRewritingScriptServingUnknownFile404WordPressAdminInjectionNextJsRscFlightNextJsServerActionsNextJsApiRouteNextJsFormActionBrowser-level (Playwright + Chromium)
script-injectionscript-bundlenavigation(4-page SPA chain + back button + deferred route script)api-passthroughform-rewritingadmin-injectionTotals: 12 Next.js + 5 WordPress browser tests, 0 flaky
Known gaps
AuctionRequestbut is not yet populated upstream. Requires implementation before it can be tested.testcontainerscrate; browser tests usedocker run/docker stopvia Playwright's globalSetup/globalTeardown (Node.js cannot use the Rust crate).Closes
Closes #398
Test plan
cargo test --workspace(excluding integration-tests, which requires native target)cargo clippy --all-targets --all-features -- -D warnings(excluding integration-tests)cargo fmt --all -- --checkcd crates/js/lib && npm run format-- passescd docs && npm run format-- passescargo test -p integration-tests --target aarch64-apple-darwin --no-run./scripts/integration-tests.sh-- all pass./scripts/integration-tests-browser.sh-- 17 pass, 0 failcargo clippy --manifest-path crates/integration-tests/Cargo.toml --tests-- cleanChecklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)