Add route_request dispatch coverage#689
Conversation
prk-Jr
left a comment
There was a problem hiding this comment.
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
aram356
left a comment
There was a problem hiding this comment.
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 two —
route_tests.rs:128(inline)
🤔 thinking
.invalid502 is consistent withProxybut not exclusive to it —route_tests.rs:322(inline)
⛏ nitpick
!is_empty()is satisfied by any non-empty body —route_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
aram356
left a comment
There was a problem hiding this comment.
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_requestbranches —/verify-signature(POST), the/first-party/*routes, integration-proxy dispatch viahas_route, and theenforce_basic_authErrarm 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
aram356
left a comment
There was a problem hiding this comment.
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, andIntegration Testslast ran on commit3f7d3333(2026-05-15). The two commits after it —055ce1f9and73887d8d(post-approval polish) — only carry CodeQL'sAnalyze (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 on73887d8dso the green checks reflect the merged commit. Verified locally on head:cargo fmt --all -- --checkclean,cargo clippy --workspace --all-targets --all-features -- -D warningsclean, all 8 route tests pass.
Non-blocking
⛏ nitpick
- Inert auction body (route_tests.rs:301): the
503is from the consent gate, not the emptyadUnits; a clarifying comment would help. See inline.
📝 note
- Expected stderr DNS error (route_tests.rs:313-320): the
.invalidorigin prints ahyperconnect error on every run; harmless and intentional. See inline.
📌 out of scope
- Misplaced
JWKS_CONFIG_STORE_NAMEassertion (route_tests.rs:168-171, pre-existing onmain— not touched by this PR): theassert_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::getignores_store_nameand matches onkey, 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)
| #[test] | ||
| fn auction_route_enforces_consent_store_when_configured() { | ||
| let settings = create_test_settings(); | ||
| let req = Request::post("https://test.com/auction").with_body(r#"{"adUnits":[]}"#); |
There was a problem hiding this comment.
⛏ nitpick — Inert request body reads as if it tests empty ad units
The {"adUnits":[]} body looks like it exercises empty-ad-unit handling, but the 503 comes purely from the consent gate failing in runtime_services_for_consent_route before the auction body is ever parsed (main.rs:236). A one-line comment prevents a future reader from over-reading it:
// Body is irrelevant: the consent gate (runtime_services_for_consent_route)
// fails before the auction handler parses the request.
let req = Request::post("https://test.com/auction").with_body(r#"{"adUnits":[]}"#);|
|
||
| #[test] | ||
| fn unknown_route_falls_back_to_publisher_proxy_path() { | ||
| let settings = create_test_settings_with(None, "https://publisher-origin.invalid"); |
There was a problem hiding this comment.
📝 note — Expected stderr noise, no action needed
This test intentionally prints hyper::Error(Connect, ConnectError("dns error", ...)) to stderr on every run because the .invalid origin never resolves. It's harmless and the inline comment below already explains the intent — just flagging that the scary-looking error line in cargo test output is expected, not a failure.
Summary
route_requestdispatch coverage.Closes #451
Tests
cargo test -p trusted-server-adapter-fastly route_tests -- --nocapturecargo fmt --all -- --checkcargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warnings