Skip to content

Add route_request dispatch coverage#689

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

Add route_request dispatch coverage#689
ChristianPavilonis wants to merge 4 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
Comment thread crates/trusted-server-adapter-fastly/src/route_tests.rs Outdated
Comment thread crates/trusted-server-adapter-fastly/src/route_tests.rs
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)

#[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":[]}"#);

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.

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");

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.

📝 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.

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