Skip to content

Add auction endpoint request handling coverage#690

Open
ChristianPavilonis wants to merge 3 commits into
mainfrom
test-auction-endpoint-452
Open

Add auction endpoint request handling coverage#690
ChristianPavilonis wants to merge 3 commits into
mainfrom
test-auction-endpoint-452

Conversation

@ChristianPavilonis
Copy link
Copy Markdown
Collaborator

@ChristianPavilonis ChristianPavilonis commented May 13, 2026

Summary

  • Add route-level coverage for /auction request handling
  • Return 400 Bad Request for malformed auction JSON payloads
  • Cover semantic validation failures, orchestration failures, and a stable no-network success path
  • Note /auction now returns 502 Bad Gateway when every configured provider is skipped, disabled, or fails to launch instead of returning 200 with an empty seatbid.

Closes #452

Testing

  • cargo test -p trusted-server-adapter-fastly route_tests -- --nocapture
  • cargo test -p trusted-server-core auction:: -- --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:14
Copy link
Copy Markdown
Collaborator

@prk-Jr prk-Jr left a comment

Choose a reason for hiding this comment

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

Summary

The route coverage is useful, but one new test now codifies a configuration typo as a request-time 502 instead of a startup/configuration failure.

Blocking

🔧 wrench

  • Missing auction provider should fail startup, not become request-time 502: auction.providers = ["missing-provider"] currently builds the route stack and only fails when /auction is called. Invalid enabled providers should fail during startup/registration so deployments do not start with a mistyped provider list.

CI Status

  • cargo fmt / clippy: PASS
  • rust tests: PASS
  • js tests: PASS
  • integration tests: PASS

Comment thread crates/trusted-server-adapter-fastly/src/route_tests.rs Outdated
Copy link
Copy Markdown
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

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

Summary

The endpoint change correctly fixes a real bug — malformed /auction JSON now returns 400 instead of 502 — and the three accompanying tests for that path are solid. The contentious piece (already flagged by @prk-Jr) is the fourth test, which codifies a configuration typo (providers = ["missing-provider"]) as a request-time 502. That conflicts with the project rule in CLAUDE.md and locks in disputed behavior with an asserting test ahead of the #669 follow-up. Inline notes cover the blocking item, two thinking notes, two refactor suggestions, and one seedling.

Blocking

🔧 wrench

  • Test codifies a config typo as request-time 502, conflicting with the project rule (crates/trusted-server-adapter-fastly/src/route_tests.rs:389-400). See inline comment for the two suggested resolutions (validate at startup, or drop just this test pending #669).

Non-blocking

🤔 thinking

  • Orchestrator guard behavior change is broader than the typo case (crates/trusted-server-core/src/auction/orchestrator.rs:365-369). Inline.
  • Test name "no_providers" is ambiguous (crates/trusted-server-adapter-fastly/src/route_tests.rs:377). Inline.

♻️ refactor

  • Error message lacks diagnostic info (crates/trusted-server-core/src/auction/orchestrator.rs:367). Inline alongside the thinking note.
  • TOML duplication between test settings helpers (crates/trusted-server-adapter-fastly/src/route_tests.rs:166-196). Inline.

🌱 seedling

  • Add a unit-level test for the new orchestrator guard. The existing TODO at orchestrator.rs:765-778 already notes the gap. A fake AuctionProvider whose request_bids returns Err would cover the "registered-but-failed-to-launch" path that the route-level test doesn't reach. Could land with the #669 follow-up.

CI Status

  • cargo fmt: PASS
  • cargo clippy: PASS
  • cargo test (Rust): PASS
  • vitest (JS): PASS
  • format-typescript / format-docs: PASS
  • integration tests + browser integration tests: PASS
  • CodeQL / Analyze: PASS

Comment thread crates/trusted-server-adapter-fastly/src/route_tests.rs Outdated
Comment thread crates/trusted-server-core/src/auction/orchestrator.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
Copy link
Copy Markdown
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

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

Summary

Adds route- and orchestrator-level coverage for /auction, reclassifies malformed-body parsing as 400, fails the auction with 502 when no provider request launches, and validates configured provider/mediator names at startup. The changes are correct, well-tested, and convention-aligned. Verified locally: cargo fmt --check, cargo clippy --all-targets --all-features -D warnings, and the affected auction:: / route_tests suites all pass. No blocking issues — approving with a few non-blocking observations.

Non-blocking

🤔 thinking

  • Whole-service blast radius for an auction config typobuild_orchestrator runs in the per-request entrypoint (main.rs:85) before routing, so the new validate_configured_provider_names makes an unregistered/typo'd provider return a Configuration error (500) for every route, not just /auction. This matches the pre-existing fail-closed posture of build_orchestrator (a bad server_url already does this) and aligns with the "don't silently disable invalid providers" convention — but it's the opposite philosophy from the adjacent consent-store handling, which deliberately scopes failures to consent routes (enshrined by configured_missing_consent_store_only_breaks_consent_routes). Worth a conscious maintainer thumbs-up that taking the whole service down on an [auction].providers typo is the intended tradeoff. (crates/trusted-server-core/src/auction/orchestrator.rs:56, crates/trusted-server-adapter-fastly/src/main.rs:85)
  • Empty-provider route test covers the old guard, not the new branch — see inline comment on route_tests.rs:383.

🌱 seedling / ⛏ nitpick

  • Validation message wording — see inline comment on orchestrator.rs:72.
  • Irrelevant bids in valid_banner_ad_unit_body — see inline comment on route_tests.rs:248.

CI Status

  • fmt: PASS (local)
  • clippy: PASS (local, core + adapter, -D warnings)
  • rust tests: PASS (local — auction:: 29 passed, route_tests 4 passed)
  • js tests: N/A (no JS changes)
  • CodeQL (GitHub): PASS


assert_eq!(
response.get_status(),
StatusCode::BAD_GATEWAY,
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.

🤔 thinking — This test uses providers = [], which hits the pre-existing "No providers configured" guard in orchestrator.rs (run_providers_parallel, ~line 284), not the new pending_requests.is_empty() branch (~line 391) this PR adds. Both return TrustedServerError::Auction → 502, so the route-level mapping is verified — but the new branch's end-to-end route mapping isn't covered here (only the LaunchFailingProvider orchestrator unit test exercises it).

Consider a route test with a registered-but-disabled provider so the request reaches the new branch end-to-end.

if !self.providers.contains_key(provider_name) {
return Err(Report::new(TrustedServerError::Configuration {
message: format!(
"Auction provider `{provider_name}` is configured but not registered"
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.

🌱 seedling — The message "configured but not registered" leans on internal vocabulary. The most common real cause is a disabled (or absent) integration, which "not registered" doesn't hint at. A more actionable phrasing, e.g.:

message: format!(
    "Auction provider `{provider_name}` is listed in [auction] but no enabled integration provides it"
),

would point an operator straight at the fix (enable the integration / fix the name).

"sizes": [[300, 250]]
}
},
"bids": [
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 — This bids array references bidder "missing-provider" that is never asserted on, and the only caller runs with providers = [], so it's irrelevant to the assertion. For a helper named valid_banner_ad_unit_body it's a bit confusing — dropping the bids block would read cleaner.

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 tests for auction endpoint request handling

3 participants