Skip to content

Add route_request dispatch coverage#689

Open
ChristianPavilonis wants to merge 6 commits into
mainfrom
test-route-request-dispatch-451
Open

Add route_request dispatch coverage#689
ChristianPavilonis wants to merge 6 commits into
mainfrom
test-route-request-dispatch-451

Conversation

@ChristianPavilonis

Copy link
Copy Markdown
Collaborator

Summary

  • Add focused Fastly adapter route_request dispatch coverage.
  • Cover static TSJS success and unknown bundle handling.
  • Cover public discovery and unauthenticated admin dispatch.
  • Cover auction dispatch through consent-dependent setup.
  • Cover unknown-route publisher fallback dispatch without configured consent persistence.

Closes #451

Tests

  • cargo test -p trusted-server-adapter-fastly route_tests -- --nocapture
  • cargo fmt --all -- --check
  • cargo test --workspace
  • cargo clippy --workspace --all-targets --all-features -- -D warnings

@ChristianPavilonis ChristianPavilonis marked this pull request as ready for review May 13, 2026 19:06

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

This PR adds focused Fastly adapter coverage for route_request. The overall direction is useful, but one assertion currently does not prove the route branch it is intended to cover because another branch can produce the same failure status.

CI Status

  • cargo fmt: PASS
  • cargo test: PASS
  • vitest: PASS
  • integration tests: PASS
  • browser integration tests: PASS
  • CodeQL: PASS
  • analysis jobs: PASS

Comment thread crates/trusted-server-adapter-fastly/src/route_tests.rs
Comment thread crates/trusted-server-adapter-fastly/src/route_tests.rs Outdated
Comment thread crates/trusted-server-adapter-fastly/src/route_tests.rs

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Decomposing the previous monolithic test into focused per-route dispatch tests is a clean improvement, and the prior blocking concerns from @prk-Jr (auction passing through fallback / ambient DNS dependency) are fully addressed in 3f7d3333 — the "Invalid banner size" body assertion and the .invalid origin URL both make the intended failure modes unambiguous. CI is green and the new tests pass locally.

Requesting changes on one point only: switching auction_route_dispatches_to_auction_handler to consent_store = None proves handler dispatch but drops the regression assertion that runtime_services_for_consent_route actually gates the /auction route. Only the publisher-fallback gate is now under test. A small companion test restores parity with the original coverage — see the inline 🌱.

Non-blocking

🌱 seedling

  • Lost regression coverage for "auction returns 503 when consent_store is misconfigured"route_tests.rs:296 (inline)

♻️ refactor

  • Collapse the four-layer settings helper into tworoute_tests.rs:128 (inline)

🤔 thinking

  • .invalid 502 is consistent with Proxy but not exclusive to itroute_tests.rs:322 (inline)

⛏ nitpick

  • !is_empty() is satisfied by any non-empty bodyroute_tests.rs:243 (inline)

CI Status

  • cargo fmt: PASS
  • cargo clippy: PASS (verified locally on trusted-server-adapter-fastly)
  • cargo test: PASS (verified locally — 7/7 new tests pass)
  • vitest: PASS
  • format-docs / format-typescript: PASS
  • browser integration tests / integration tests: PASS
  • CodeQL: PASS

Comment thread crates/trusted-server-adapter-fastly/src/route_tests.rs Outdated
Comment thread crates/trusted-server-adapter-fastly/src/route_tests.rs Outdated
Comment thread crates/trusted-server-adapter-fastly/src/route_tests.rs Outdated
Comment thread crates/trusted-server-adapter-fastly/src/route_tests.rs Outdated

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Re-review of 055ce1f9. All prior feedback from both earlier rounds is resolved: auction dispatch now proves the route via the invalid-banner 400, the .invalid origin makes the publisher-fallback 502 intentional (with an intent comment), consent-store gating is now verified at both call sites, and the settings helpers are collapsed to a single parameterized factory. I read the full test file and the route_request path, verified every assertion against the production status-code mappings, and ran the suite — 8/8 pass (the hyper DNS error logged by unknown_route_falls_back_to_publisher_proxy_path is the intended .invalid-origin failure that yields the 502).

Approving. Remaining items are all non-blocking polish/coverage — see inline comments plus the out-of-scope note below.

Non-blocking

📌 out of scope

  • Untested route_request branches/verify-signature (POST), the /first-party/* routes, integration-proxy dispatch via has_route, and the enforce_basic_auth Err arm are not exercised. Reasonable to defer given the focused scope of #451, but worth a follow-up so the gap stays visible.

CI Status

  • cargo fmt: PASS
  • cargo clippy: PASS
  • cargo test: PASS (verified locally — 8/8 route_tests pass on 055ce1f)
  • Analyze (javascript-typescript): PASS

Comment thread crates/trusted-server-adapter-fastly/src/route_tests.rs Outdated
Comment thread crates/trusted-server-adapter-fastly/src/route_tests.rs Outdated
Comment thread crates/trusted-server-adapter-fastly/src/route_tests.rs Outdated

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Re-review of the current head (73887d8d) after the post-approval polish commit. The test design is sound — every assertion traces to real dispatch behavior, and the route-specific assertions correctly discriminate dispatch from the publisher fallback. Requesting changes only to flag a CI gating gap before merge; the remaining items are non-blocking.

Blocking

🔧 wrench

  • Rust CI never ran on the current head: Run Tests, Run Format, and Integration Tests last ran on commit 3f7d3333 (2026-05-15). The two commits after it — 055ce1f9 and 73887d8d (post-approval polish) — only carry CodeQL's Analyze (javascript-typescript) check. For an entirely-Rust PR, the merge gate ("fmt/clippy/test pass") is not actually satisfied by CI for the code that would merge (likely the external-contributor "approve and run workflows" gate). Please re-trigger the Rust workflows on 73887d8d so the green checks reflect the merged commit. Verified locally on head: cargo fmt --all -- --check clean, cargo clippy --workspace --all-targets --all-features -- -D warnings clean, all 8 route tests pass.

Non-blocking

⛏ nitpick

  • Inert auction body (route_tests.rs:301): the 503 is from the consent gate, not the empty adUnits; a clarifying comment would help. See inline.

📝 note

  • Expected stderr DNS error (route_tests.rs:313-320): the .invalid origin prints a hyper connect error on every run; harmless and intentional. See inline.

📌 out of scope

  • Misplaced JWKS_CONFIG_STORE_NAME assertion (route_tests.rs:168-171, pre-existing on main — not touched by this PR): the assert_eq!(JWKS_CONFIG_STORE_NAME, "jwks_store", "...keep the stub discovery store aligned...") lives inside the settings factory, so it re-runs on every settings construction, and it doesn't actually couple to the stub — StubJwksConfigStore::get ignores _store_name and matches on key, so the discovery test would still pass if the constant changed. Worth a follow-up: move it to a dedicated #[test], or make the stub assert/match the store name so the check becomes load-bearing.

CI Status (verified locally on head 73887d8d)

  • fmt: PASS (local) — not run on head in CI
  • clippy: PASS (local) — not run on head in CI
  • rust tests: PASS (8/8 local) — not run on head in CI
  • js analyze (CodeQL): PASS (CI)

Comment thread crates/trusted-server-adapter-fastly/src/route_tests.rs Outdated
Comment thread crates/trusted-server-adapter-fastly/src/route_tests.rs Outdated

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Re-review after the main merge (56d29cf4). The branch now fully integrates current main (request-local consent, RouteResult/HandlerOutcome, platform-http-client proxying), and the net contribution over main is 3 tests + a route_with_settings helper. The merge correctly retired the obsolete consent-KV-gating tests. One test needs strengthening before merge; the previously-flagged CI gap is now resolved.

Blocking

🔧 wrench

  • unknown_route_falls_back_to_publisher_proxy_path doesn't verify what it claims (route_tests.rs:695-708): the .invalid origin is inert in the merged architecture — the 502 comes from NoopBackend.ensure() ("backend registration failed"), not from reaching the publisher origin (verified: no DNS/connect error under --nocapture, unlike the pre-merge version). The assertion can't distinguish a real publisher fallback from any all-Noop failure. See inline for a FixedBackend + RecordingHttpClient fix that proves dispatch.

Non-blocking

📝 note

  • Prior CI gap resolved: the merge re-triggered full Rust CI. cargo fmt, cargo test, Analyze (rust), vitest, and integration tests all pass on head 56d29cf4. Re-verified locally: 20/20 route tests pass, fmt clean.

CI Status (head 56d29cf4)

  • cargo fmt: PASS (CI + local)
  • cargo test: PASS (CI; 20/20 route tests local)
  • Analyze (rust): PASS (CI)
  • vitest: PASS (CI)
  • integration tests: PASS (CI)

Comment thread crates/trusted-server-adapter-fastly/src/route_tests.rs Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add integration coverage for route_request dispatch behavior

3 participants