Add test coverage for error mapping and tsjs helpers#648
Add test coverage for error mapping and tsjs helpers#648ChristianPavilonis wants to merge 1 commit intomainfrom
Conversation
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Pure test additions/consolidation — no production code changes. Tests pass on wasm32-wasip1, clippy is clean, and CI is fully green. The substantive concerns below are about test design — what these tests would catch on regression — rather than correctness of what was added. Requesting changes primarily on the exhaustiveness and tautology issues; the rest are non-blocking.
Blocking
♻️ refactor
- Status code mapping test is not exhaustive at compile time —
error.rs:144-247. Hand-maintainedcasesarray means a new variant added toTrustedServerErrorwon't fail this test. A non-exhaustivematchguard would force exhaustive coverage. See inline comment. - tsjs hash assertions are tautological —
tsjs.rs:75-116.expected_hashis derived by calling the sameconcatenated_hashthe production code calls. Ifconcatenated_hashregressed, both sides would change in lockstep. Suggest asserting hash structure (64 hex chars) and that different module sets yield different URLs. See inline comment. - Redundant import in tsjs test module —
tsjs.rs:70. Duplicates the top-of-file import;use super::*at line 72 is sufficient (verified locally).
Non-blocking
🤔 thinking
tsjs_deferred_script_src_uses_empty_hash_for_unknown_modulelocks in silent failure —tsjs.rs:137-143. Empty hash produces a 404-bound URL with no logging. Capturing this as "expected" makes future improvements look like regressions.
🌱 seedling
- Production
unwrap_or_default()for unknown deferred modules is invisible —tsjs.rs:44(unchanged in this PR, so noted in the body).single_module_hash(module_id).unwrap_or_default()silently produces a 404-bound URL with no logging when the module isn't registered. Worth a follow-up issue to addlog::warn!for unregistered module IDs.
📝 note
- Issue #454 wording vs implementation —
cookies.rs:353-372. Issue says "returningNone" but production returnsErr. Test correctly verifies current implementation; issue text is stale.
⛏ nitpick
["core", "creative"]redundantly includescore—tsjs.rs:76, 88.concatenate_modulesalways prepends core; could be simplified to["creative"].HeaderValue::from_bytesexpect message is slightly misleading —cookies.rs:358. Reads like a property ofhandle_request_cookiesbut is aboutHeaderValue::from_bytes.
CI Status
- fmt: PASS
- clippy: PASS
- rust tests: PASS (verified locally on
wasm32-wasip1) - vitest: PASS
- format-typescript / format-docs: PASS
- browser & integration tests: PASS
| "should map {error:?} to the expected HTTP status", | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
♻️ refactor — The table-driven cases array is hand-maintained. If a new TrustedServerError variant is added and status_code() is updated but this test isn't, the test silently passes despite the new variant being uncovered. Since the whole point of this test is to lock in the variant→status mapping, a compile-time exhaustiveness guard would be much stronger.
Fix — add a non-exhaustive match somewhere in the test that fails to compile if a variant is added:
// Compile-time guard: adding a TrustedServerError variant without
// updating this test will fail to compile.
let _exhaustive = |e: &TrustedServerError| match e {
TrustedServerError::BadRequest { .. }
| TrustedServerError::Configuration { .. }
| TrustedServerError::Auction { .. }
| TrustedServerError::Gam { .. }
| TrustedServerError::GdprConsent { .. }
| TrustedServerError::InvalidUtf8 { .. }
| TrustedServerError::InvalidHeaderValue { .. }
| TrustedServerError::KvStore { .. }
| TrustedServerError::Prebid { .. }
| TrustedServerError::Integration { .. }
| TrustedServerError::Proxy { .. }
| TrustedServerError::Forbidden { .. }
| TrustedServerError::AllowlistViolation { .. }
| TrustedServerError::Settings { .. }
| TrustedServerError::Ec { .. } => (),
};| format!("/static/tsjs=tsjs-unified.min.js?v={expected_hash}"), | ||
| "should include the unified bundle path and cache-busting hash", | ||
| ); | ||
| } |
There was a problem hiding this comment.
♻️ refactor — expected_hash is derived by calling concatenated_hash — the same function the production code calls. The test verifies "function composition matches itself" instead of cache-busting behavior. If concatenated_hash silently regressed (e.g. always returned "") both production and test would change in lockstep and the test would still pass.
Fix — assert hash structure (64 hex chars for SHA-256) and that different module sets yield different URLs:
let a = tsjs_script_src(&["creative"]);
let b = tsjs_script_src(&["creative", "prebid"]);
assert_ne!(a, b, "different module sets must yield different cache-busting URLs");
let hash = a.rsplit_once("?v=").expect("should have v= param").1;
assert_eq!(hash.len(), 64, "should be SHA-256 hex");
assert!(hash.chars().all(|c| c.is_ascii_hexdigit()));The same concern applies to tsjs_unified_helpers_use_all_module_ids below.
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use trusted_server_js::{all_module_ids, concatenated_hash, single_module_hash}; |
There was a problem hiding this comment.
♻️ refactor — Duplicate of the import at the top of the file. Submodules can see private items of ancestors, so use super::*; at line 72 is sufficient. Verified locally: removing this line keeps both cargo build and cargo test green.
Fix — delete this line.
| "/static/tsjs=tsjs-unknown-module.min.js?v=", | ||
| "should fall back to an empty hash for unknown deferred modules", | ||
| ); | ||
| } |
There was a problem hiding this comment.
🤔 thinking — This test asserts that calling tsjs_deferred_script_src("unknown-module") produces "/static/tsjs=tsjs-unknown-module.min.js?v=" — an empty hash that points at a non-existent file → silent 404 in the browser. Capturing this as "what we expect" makes future improvements to the production unwrap_or_default() (e.g. logging or returning Option<String>) look like regressions when they're actually fixes.
Suggestion — either re-frame the test so its name describes a documented contract (e.g. "caller is responsible for not generating tags for unregistered modules") and add a log::warn! in production for unknown IDs, or mark this as a known-bad behavior with a follow-up issue.
| ), | ||
| "should return InvalidHeaderValue for invalid UTF-8 cookie headers", | ||
| ); | ||
| } |
There was a problem hiding this comment.
📝 note — Issue #454 says "returning None" but handle_request_cookies returns Err(InvalidHeaderValue) for non-UTF-8 cookie headers. The test correctly verifies the current implementation. Just flagging that the issue text is stale (likely written when the function had a different signature) — the issue can still be closed; no action on this PR.
|
|
||
| #[test] | ||
| fn tsjs_script_src_formats_unified_bundle_url_with_hash() { | ||
| let module_ids = ["core", "creative"]; |
There was a problem hiding this comment.
⛏ nitpick — bundle.rs::concatenate_modules always prepends "core" and skips it when present in ids (bundle.rs:31-39). Passing ["core", "creative"] produces the same hash as ["creative"]. Could be simplified to ["creative"] to avoid implying caller-side intent that doesn't matter. Same applies to line 88.
| req.set_header( | ||
| header::COOKIE, | ||
| HeaderValue::from_bytes(b"session=\xFF") | ||
| .expect("should allow non-UTF-8 cookie header bytes"), |
There was a problem hiding this comment.
⛏ nitpick — The expect message reads like a property of handle_request_cookies, but it's actually about HeaderValue::from_bytes accepting any opaque bytes. Consider "should construct HeaderValue from non-UTF-8 bytes" for clarity.
2 participants
Summary
TrustedServerError::status_code()so each public variant is pinned to its expected HTTP responsetsjsformatting tests for unified and deferred script URL/tag helpers using computed hashesChanges
crates/trusted-server-core/src/error.rsTrustedServerErrorvariant.crates/trusted-server-core/src/tsjs.rscrates/trusted-server-core/src/cookies.rsCookieheader handling inhandle_request_cookies().crates/trusted-server-core/src/models.rsCloses
Closes #448
Closes #450
Closes #454
Closes #456
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 serveChecklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)