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)
aram356
left a comment
There was a problem hiding this comment.
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_pathdoesn't verify what it claims (route_tests.rs:695-708): the.invalidorigin is inert in the merged architecture — the502comes fromNoopBackend.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 aFixedBackend+RecordingHttpClientfix 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 head56d29cf4. 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)
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