Add focused coverage for production readiness test gaps#688
Add focused coverage for production readiness test gaps#688ChristianPavilonis wants to merge 2 commits into
Conversation
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
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
ForbiddenandAllowlistViolationuser_message untested — Both variants fall into the_catch-all inuser_messageand return"An internal error occurred"despite being 4xx responses. The existingserver_errors_return_generic_messagetest 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) — Inputhttps://cdn.example.com/assets/origin.example.com/image.pngwill be rewritten because/is not ais_host_char. This may be intentional for Next.js__next_fpayloads 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 doesconcatenated_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
| 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 { |
There was a problem hiding this comment.
🤔 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 { .. } => (),
};| | TrustedServerError::Ec { .. } => (), | ||
| }; | ||
|
|
||
| let cases = vec![ |
There was a problem hiding this comment.
⛏ nitpick — let cases = vec![...] here; existing tests in this module use array literals (let cases = [...]). Minor inconsistency.
| ); | ||
| } | ||
|
|
||
| fn assert_no_rewrite(input: &str) { |
There was a problem hiding this comment.
⛏ 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.
| } | ||
|
|
||
| #[test] | ||
| fn rewrites_origin_host_with_port_when_origin_includes_port() { |
There was a problem hiding this comment.
👍 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.
| } | ||
|
|
||
| #[test] | ||
| fn tsjs_deferred_script_src_uses_empty_hash_for_unknown_module() { |
There was a problem hiding this comment.
👍 praise — Documenting the ?v= empty-hash fallback as spec (rather than asserting a computed value) is a clean use of tests as specification.
| } | ||
| } | ||
|
|
||
| fn make_bid(slot_id: &str, bidder: &str, price: Option<f64>) -> Bid { |
There was a problem hiding this comment.
🏕 camp site — make_bid always sets creative: Some(...) and adomain: Some(...), leaving two paths uncovered:
creative: None— theelsebranch inconvert_to_openrtb_responseemitslog::warn!and setsadm: "". No test verifies the response still succeeds.adomain: None— exercisesunwrap_or_default(), which should serialize as"adomain": [].
| } | ||
| } | ||
|
|
||
| fn make_result(bid: Bid) -> OrchestrationResult { |
There was a problem hiding this comment.
🏕 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.
| } | ||
|
|
||
| #[test] | ||
| fn convert_tsjs_to_auction_request_maps_banner_units_bidders_context_and_request_metadata() { |
There was a problem hiding this comment.
♻️ 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.
2 participants
Summary
Changes
crates/trusted-server-core/src/error.rsTrustedServerErrorvariant, including a compile-time exhaustive match guard for new variants.crates/trusted-server-core/src/host_rewrite.rscrates/trusted-server-core/src/tsjs.rscrates/trusted-server-core/src/auction/formats.rsCloses
Closes #448
Closes #449
Closes #450
Closes #453
Note: #455 references the old synthetic-template path, which no longer exists on current
mainafter the EC ID migration. It should be triaged separately rather than auto-closed by this PR.Test plan
cargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest runcd crates/js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute servecargo 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 -- --nocaptureChecklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)