Add focused coverage for production readiness test gaps#688
Add focused coverage for production readiness test gaps#688ChristianPavilonis wants to merge 4 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
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
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_bidsresponse, height out-of-range symmetry. - 2 ⛏ nitpick — redundant
usein the test module; mixed*.example/example.comtest-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
left a comment
aram356
left a comment
There was a problem hiding this comment.
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.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
This test-coverage PR was branched from a stale main (merge-base f275aba5). Since then, #621 (Edge Cookie identity system) reshaped the auction APIs, and the PR no longer compiles against current main — which is why the cargo test CI job is failing. cargo fmt is also red on the merged tree. host_rewrite.rs and tsjs.rs are sound and land cleanly; auction/formats.rs and error.rs need a rebase and rework. Requesting changes on the three blocking items below.
Blocking
🔧 wrench
- Duplicate
mod tests+ stale auction API:mainalready has a test module atformats.rs:339; the new module collides (E0428) and is written against pre-#621 signatures (7-arg conversion,UserInfo.fresh_id, missingAdRequest.eids, removedHEADER_X_TS_EC/_FRESH). (auction/formats.rs:353) - Undisclosed runtime behavior change:
convert_to_openrtb_responsenow emits explicit emptyseatbid/adomainarrays via a second serialization pass, contradicting the PR's "without changing runtime behavior" claim. (auction/formats.rs:310) - Exhaustive error guard out of date: references removed
TrustedServerError::Ecand omits newEdgeCookie/PartnerNotFound/InsecureDefault. (error.rs:229)
Non-blocking
🤔 thinking
- Double serialization on response path: if the explicit-empty-array contract is required, customize serialization on the OpenRTB response types rather than re-walking the JSON tree (
to_value+to_vec) on every auction response. (auction/formats.rs:310)
♻️ refactor
- On rebase: fold both new test modules into the existing
mod tests; update toUserInfo { id: Some(...), .. }(nofresh_id),AdRequest { eids: None, .. }, the 5-argconvert_tsjs_to_auction_request, and assert the currentHEADER_X_TS_EC_CONSENT/HEADER_X_TS_EIDSheaders.
🌱 seedling
- Unknown-module hash:
tsjs_deferred_script_src_uses_empty_hash_for_unknown_modulecharacterizes "unknown module →?v=with empty hash." Worth a follow-up on whether that should instead be rejected. (tsjs.rs)
📝 note
tsjs.rstests are independent of the EC migration and should merge + compile unchanged.
CI Status
- fmt: FAIL
- clippy: PASS
- rust tests: FAIL (compile errors against merged
main— see blocking findings) - js tests (vitest): PASS
ChristianPavilonis
force-pushed
the
test-coverage-cleanup-396
branch
from
37b41f0 to
8d63e24
Compare
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Third-pass review of head 8d63e24 — all findings from the two prior review rounds are addressed. Only new findings below; nothing blocking. Verdict remains COMMENT.
- 2 ⛏ nitpick — redundant
HeaderValue::from_str, under-enumerateduser_messagedoc - 1 🏕 camp site — stale
# Errorsdoc onconvert_tsjs_to_auction_request(below) - 1 📌 out of scope — misleading generic message now pinned for 403/404 variants
- 3 🌱 seedling —
ec_id: Nonepropagation, empty bannersizes, host-rewrite case sensitivity - 1 👍 praise — order-insensitive multi-winner assertions
Non-blocking (body-level)
🏕 camp site
-
Stale
# Errorsdoc onconvert_tsjs_to_auction_request(crates/trusted-server-core/src/auction/formats.rs:82, outside this diff): the entry "Fresh EC ID generation fails" is stale — the function never generates EC IDs; the caller does, per the doc paragraph at lines 76–77, and the only error path is the invalid-banner-sizeensure!. Since this PR's purpose is documenting this function's behavior, removing the stale bullet fits here:/// # Errors /// /// Returns an error if the request contains invalid banner sizes /// (must be `[width, height]`).
CI Status
- fmt: PASS
- clippy (Analyze rust): PASS
- rust tests (cargo test): PASS
- js tests (vitest): PASS
- integration + browser integration: PASS
| let mut req = Request::new("POST", "https://publisher.example.com/auction"); | ||
| req.set_header( | ||
| header::USER_AGENT, | ||
| HeaderValue::from_str("Mozilla/5.0 test").expect("should create user-agent header"), |
There was a problem hiding this comment.
⛏ nitpick — HeaderValue::from_str(...).expect(...) is unnecessary: fastly's set_header takes impl ToHeaderValue, so a plain &str works (production code does exactly this at line 320 with set_header(HEADER_X_TS_EC_CONSENT, "ok")). This also drops the use fastly::http::HeaderValue; import at line 344.
fn make_request() -> Request {
let mut req = Request::new("POST", "https://publisher.example.com/auction");
req.set_header(header::USER_AGENT, "Mozilla/5.0 test");
req
}| /// Client errors (4xx) return a brief description; server/integration errors | ||
| /// return a generic message. Full error details are preserved in server logs. | ||
| /// Selected client errors return a brief description; forbidden and | ||
| /// server/integration errors return a generic message. Full error details |
There was a problem hiding this comment.
⛏ nitpick — The reworded doc says "forbidden and server/integration errors return a generic message", but PartnerNotFound (404) also returns the generic message, and it is neither forbidden nor a server/integration error. Enumerating categories here will keep drifting as variants are added. Simpler and future-proof:
/// Selected client errors return a brief description; all other errors
/// return a generic message. Full error details are preserved in server logs.
| } | ||
|
|
||
| #[test] | ||
| fn other_client_errors_return_generic_user_message() { |
There was a problem hiding this comment.
📌 out of scope — This test now pins "An internal error occurred" for Forbidden / AllowlistViolation (403) and PartnerNotFound (404). That message is misleading for client-fault statuses: a blocked redirect or unknown partner is not an internal error, and clients debugging a 403/404 get actively wrong guidance. Pinning current behavior is right for this PR — but a follow-up issue could introduce distinct generic messages (e.g. "Forbidden", "Not found") without leaking detail.
| } | ||
| } | ||
|
|
||
| fn convert_body_to_auction_request(body: &AdRequest, settings: &Settings) -> AuctionRequest { |
There was a problem hiding this comment.
🌱 seedling — Every conversion test routes through this helper with Some("existing-ec-id"), so the ec_id: None → user.id == None propagation (line 91 / 185 in production) is unpinned. One small test would cover it:
#[test]
fn convert_tsjs_to_auction_request_propagates_missing_ec_id() {
let settings = make_settings();
let req = make_request();
let auction_request = convert_tsjs_to_auction_request(
&make_banner_body(None),
&settings,
&req,
ConsentContext::default(),
None,
)
.expect("should convert request without EC ID");
assert!(
auction_request.user.id.is_none(),
"should leave user ID unset when caller provides no EC ID"
);
}| } | ||
|
|
||
| #[test] | ||
| fn convert_tsjs_to_auction_request_maps_banner_sizes_to_formats() { |
There was a problem hiding this comment.
🌱 seedling — A banner unit with empty sizes: vec![] passes the len() == 2 check vacuously and produces a slot with zero formats. Whether downstream providers handle an empty-formats slot gracefully is unverified. Since this PR pins malformed-size rejection and non-banner skipping, the zero-sizes boundary is the remaining gap — a documenting test (slot created with empty formats, or a decision to reject) would complete the set.
| mod tests { | ||
| use super::*; | ||
|
|
||
| const ORIGIN_HOST: &str = "origin.example.com"; |
There was a problem hiding this comment.
🌱 seedling — Case sensitivity is unpinned: str::find is case-sensitive, so ORIGIN.EXAMPLE.COM (or Origin.Example.com in markup) is never rewritten even though hostnames are case-insensitive per RFC 4343. That may be acceptable for __next_f/Flight payloads where hosts are machine-emitted lowercase, but a one-line documenting test would make the limitation explicit:
#[test]
fn does_not_rewrite_differently_cased_host() {
assert_no_rewrite(
"ORIGIN.EXAMPLE.COM/news",
"should not rewrite differently-cased host occurrences",
);
}| } | ||
|
|
||
| #[test] | ||
| fn convert_to_openrtb_response_serializes_multiple_winning_bids() { |
There was a problem hiding this comment.
👍 praise — Locating seatbids by impid lookup instead of array index makes these assertions immune to HashMap iteration order in winning_bids — exactly the kind of test that would otherwise flake intermittently.
3 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!)