Add focused coverage for production readiness test gaps by ChristianPavilonis · Pull Request #688 · IABTechLab/trusted-server

ChristianPavilonis · 2026-05-13T14:03:05Z

Summary

Add focused unit coverage for production-readiness backlog test gaps under Production readiness #396.
Cover error status mapping, bare-host rewrite boundaries, TSJS script tag/source generation, and auction format conversion behavior.
Document the current banner-only auction request conversion behavior without changing runtime behavior.
Supersedes Add test coverage for error mapping and tsjs helpers #648 for the overlapping Add tests for TrustedServerError HTTP status mapping #448/Add unit tests for tsjs script source and tag generation #450 coverage.

Changes

File	Change
`crates/trusted-server-core/src/error.rs`	Added direct status-code mapping coverage for every current `TrustedServerError` variant, including a compile-time exhaustive match guard for new variants.
`crates/trusted-server-core/src/host_rewrite.rs`	Added direct boundary tests for exact hosts, paths, punctuation, multiple occurrences, subdomains, suffixes, empty inputs, and ports.
`crates/trusted-server-core/src/tsjs.rs`	Added tests for unified/deferred script source formatting, hash shape, script tag attributes, empty deferred output, and deferred order preservation.
`crates/trusted-server-core/src/auction/formats.rs`	Added request/response conversion tests for banner formats, bidder params, context allowlisting, malformed sizes, unsupported media skip behavior, OpenRTB response fields, mediation strategy, missing prices, and out-of-range dimensions.

Closes

Closes #448
Closes #449
Closes #450
Closes #453

Note: #455 references the old synthetic-template path, which no longer exists on current main after the EC ID migration. It should be triaged separately rather than auto-closed by this PR.

Test plan

cargo test --workspace
cargo clippy --workspace --all-targets --all-features -- -D warnings
cargo fmt --all -- --check
JS tests: cd crates/js/lib && npx vitest run
JS format: cd crates/js/lib && npm run format
Docs format: cd docs && npm run format
WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
Manual testing via fastly compute serve
Other: cargo test -p trusted-server-core status_code_maps_each_error_variant_to_expected_http_response -- --nocapture; cargo test -p trusted-server-core tsjs_ -- --nocapture; cargo test -p trusted-server-core rewrite -- --nocapture; cargo test -p trusted-server-core convert_tsjs_to_auction_request -- --nocapture; cargo test -p trusted-server-core convert_to_openrtb_response -- --nocapture

Checklist

Changes follow CLAUDE.md conventions
No unwrap() in production code — use expect("should ...")
Uses log macros (not println!)
New code has tests
No secrets or credentials committed

prk-Jr

Summary

Pure test additions across four files — error.rs, host_rewrite.rs, tsjs.rs, and auction/formats.rs. No production code changed. All CI checks pass. Test quality is high overall; a few missing edge cases and one misleading helper message noted inline.

Non-blocking

🏕 camp site

Forbidden and AllowlistViolation user_message untested — Both variants fall into the _ catch-all in user_message and return "An internal error occurred" despite being 4xx responses. The existing server_errors_return_generic_message test does not include them. Worth adding both to explicitly document (and lock in) the intended behavior — or, if a more descriptive message is preferred for 403s, this is the place to catch it.

🌱 seedling

Host-in-path-segment behavior unspecified (host_rewrite.rs) — Input https://cdn.example.com/assets/origin.example.com/image.png will be rewritten because / is not a is_host_char. This may be intentional for Next.js __next_f payloads but is undocumented and untested. A test explicitly asserting current behavior would prevent silent regressions.
Empty module list edge case (tsjs.rs) — tsjs_script_src(&[]) behavior is unspecified. What does concatenated_hash(&[]) return? Low risk but a documentation test would lock in the contract.

CI Status

cargo fmt: PASS
cargo clippy: PASS
cargo test: PASS
vitest: PASS
integration tests: PASS
browser integration tests: PASS

prk-Jr · 2026-05-14T15:04:13Z

+    fn status_code_maps_each_error_variant_to_expected_http_response() {
+        // Compile-time guard: adding a TrustedServerError variant without
+        // updating this test will fail to compile.
+        let _exhaustive = |error: &TrustedServerError| match error {


🤔 thinking — _exhaustive is never called; it works because Rust type-checks closure bodies at compile time even for dead closures, but the intent is non-obvious to a reader. A comment above it explaining the compile-time guard purpose would help, or use the more explicit form:

let _guard: fn(&TrustedServerError) = |e| match e { TrustedServerError::BadRequest { .. } | ... | TrustedServerError::Ec { .. } => (), };

prk-Jr · 2026-05-14T15:04:13Z

+            | TrustedServerError::Ec { .. } => (),
+        };
+
+        let cases = vec![


⛏ nitpick — let cases = vec![...] here; existing tests in this module use array literals (let cases = [...]). Minor inconsistency.

prk-Jr · 2026-05-14T15:04:13Z

+        );
+    }
+
+    fn assert_no_rewrite(input: &str) {


⛏ nitpick — The assertion message "should not rewrite host embedded in a larger host token" is inaccurate when called from returns_none_when_origin_host_is_absent (line 89) — there the host is simply absent, not embedded. Consider accepting a message parameter or a separate assert_absent helper.

prk-Jr · 2026-05-14T15:04:14Z

+    }
+
+    #[test]
+    fn rewrites_origin_host_with_port_when_origin_includes_port() {


👍 praise — The port inclusion/exclusion distinction (origin.example.com:8443 as origin vs. origin.example.com alone) is a subtle and important boundary. Good coverage.

prk-Jr · 2026-05-14T15:04:14Z

+    }
+
+    #[test]
+    fn tsjs_deferred_script_src_uses_empty_hash_for_unknown_module() {


👍 praise — Documenting the ?v= empty-hash fallback as spec (rather than asserting a computed value) is a clean use of tests as specification.

prk-Jr · 2026-05-14T15:04:14Z

+        }
+    }
+
+    fn make_bid(slot_id: &str, bidder: &str, price: Option<f64>) -> Bid {


🏕 camp site — make_bid always sets creative: Some(...) and adomain: Some(...), leaving two paths uncovered:

creative: None — the else branch in convert_to_openrtb_response emits log::warn! and sets adm: "". No test verifies the response still succeeds.

adomain: None — exercises unwrap_or_default(), which should serialize as "adomain": [].

prk-Jr · 2026-05-14T15:04:14Z

+        }
+    }
+
+    fn make_result(bid: Bid) -> OrchestrationResult {


🏕 camp site — make_result always produces a single winning bid. No test exercises convert_to_openrtb_response with multiple slots. Adding a two-slot result would verify seatbid ordering and per-slot impid values are preserved.

prk-Jr · 2026-05-14T15:04:14Z

+    }
+
+    #[test]
+    fn convert_tsjs_to_auction_request_maps_banner_units_bidders_context_and_request_metadata() {


♻️ refactor — This 130-line test exercises ~9 behaviors in one function: slot mapping, format sizes, bidder params, publisher domain, site metadata, EC ID, fresh ID, device user-agent, and context filtering. When it fails the full setup must be re-read to isolate which invariant broke. Consider splitting into focused tests — e.g. maps_banner_sizes_to_formats, copies_user_agent_to_device_info, filters_disallowed_context_keys.

aram356

Summary

Follow-up to the prior review on the same head commit — only new findings missed last pass. Still no blocking items; verdict is COMMENT.

3 🌱 seedling — coverage gaps: hash order/dedup invariants, empty winning_bids response, height out-of-range symmetry.
2 ⛏ nitpick — redundant use in the test module; mixed *.example / example.com test-fixture TLDs.
2 👍 praise — compile-time exhaustiveness guard pattern; banner-only behavior pinned by a test.

The prior review's findings remain open (no commits since 2a3edb77) — they're tracked there, not re-posted here.

CI Status

cargo fmt: PASS
cargo clippy: PASS
cargo test: PASS
vitest: PASS
integration tests: PASS
browser integration tests: PASS

aram356 · 2026-05-28T05:16:26Z

+            creative_src, creative_prebid_src,
+            "should include requested modules in cache-busting hash"
+        );
+    }


🌱 seedling — tsjs_script_src_hash_changes_with_module_set only locks the weakest invariant: "adding a module changes the hash." Two stronger contracts of concatenate_modules are still unverified:

Order matters — concatenated_hash(&["a","b"]) != concatenated_hash(&["b","a"]) (parts are pushed in iteration order).

core is deduped — concatenated_hash(&["core","prebid"]) == concatenated_hash(&["prebid"]) (the if *id == "core" { continue; } branch in bundle.rs).

Locking both prevents silent cache-bust regressions if concatenate_modules is later refactored. Pairs naturally with the empty-list seedling already noted in the prior review.

aram356 · 2026-05-28T05:16:26Z

+    fn status_code_maps_each_error_variant_to_expected_http_response() {
+        // Compile-time guard: adding a TrustedServerError variant without
+        // updating this test will fail to compile.
+        let _exhaustive = |error: &TrustedServerError| match error {


👍 praise — Even with the readability concern flagged separately, the underlying pattern — using a closure body to force the compiler into checking exhaustiveness against the match — is a clean way to make "add a new variant ⇒ failing build" the default. Worth keeping (with a one-line // Compile-time guard header).

aram356 · 2026-05-28T05:16:26Z

+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::auction::context::ContextValue;


⛏ nitpick — use crate::auction::context::ContextValue; at the file top (line 15) is already in scope here via use super::*;. This inner use is redundant.

aram356 · 2026-05-28T05:16:26Z

+    use std::collections::{HashMap, HashSet};
+
+    fn make_request() -> Request {
+        let mut req = Request::new("POST", "https://publisher.example/auction");


⛏ nitpick — publisher.example (here) and advertiser.example (line 378) use the bare reserved .example TLD, while line 358 uses test-publisher.com. CLAUDE.md says "for urls use: example.com domains only" — picking one keeps the fixtures consistent. (Cheapest fix: publisher.example.com, advertiser.example.com.)

aram356 · 2026-05-28T05:16:26Z

+        assert!(
+            auction_request.slots.is_empty(),
+            "should only create slots for banner media"
+        );


👍 praise — Pinning the silent-skip behavior for non-banner media is high-value. When video / native eventually land, that change has to either preserve this contract or knowingly break it — exactly the kind of decision a future reviewer wants surfaced by a failing test.

aram356 · 2026-05-28T05:16:26Z

+            format!("{err:?}").contains("has no decoded price"),
+            "should explain missing decoded price"
+        );
+    }


🌱 seedling — While you're testing the error path, the empty winning_bids path is also unverified. In real auctions, bidder timeouts produce an OrchestrationResult with no winners. The expected response is StatusCode::OK with an empty seatbid array and total_bids == 0 — a trivial third test alongside the happy and error cases here would lock that contract.

aram356 · 2026-05-28T05:16:26Z

+        let settings = create_test_settings();
+        let auction_request = make_auction_request();
+        let mut bid = make_bid("div-gpt-top", "appnexus", Some(2.75));
+        bid.width = u32::MAX;


🌱 seedling — Only bid.width is mutated. The symmetric bid.height = u32::MAX exercises the same to_openrtb_i32 helper at line 234 — covering both call sites costs one extra line and guards against accidentally diverging width/height handling in the future.

bid.width = u32::MAX; bid.height = u32::MAX; // ... assert!(bid.get("w").is_none(), "should omit out-of-range width"); assert!(bid.get("h").is_none(), "should omit out-of-range height");

aram356

Requesting changes to address the 5 non-praise findings (3 🌱 + 2 ⛏) in my prior review — and the 9 still-open items from the earlier review that haven't been addressed yet.

Add focused coverage for production readiness test gaps

e95fd91

ChristianPavilonis requested review from aram356 and prk-Jr May 13, 2026 14:37

Refine production readiness coverage PR consolidation

2a3edb7

This was referenced May 13, 2026

Add test coverage for error mapping and tsjs helpers #648

Closed

Add tests for synthetic ID template error paths #455

Open

prk-Jr reviewed May 14, 2026

View reviewed changes

aram356 assigned ChristianPavilonis May 18, 2026

aram356 reviewed May 28, 2026

View reviewed changes

aram356 requested changes May 28, 2026

View reviewed changes

Conversation

ChristianPavilonis commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Closes

Test plan

Checklist

Uh oh!

prk-Jr left a comment

Choose a reason for hiding this comment

Summary

Non-blocking

🏕 camp site

🌱 seedling

CI Status

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aram356 left a comment

Choose a reason for hiding this comment

Summary

CI Status

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aram356 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ChristianPavilonis commented May 13, 2026 •

edited

Loading