Add test coverage for error mapping and tsjs helpers#648
Add test coverage for error mapping and tsjs helpers#648ChristianPavilonis wants to merge 3 commits intomainfrom
Conversation
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
aram356
left a comment
There was a problem hiding this comment.
Summary
Pure test additions — no production code changes. CI is fully green and 809 trusted-server-core tests pass locally on wasm32-wasip1. The exhaustiveness guard added in 02ac32f for status_code() mapping is a strong improvement. However, two of the four "addressed" items still contain the same self-referential pattern the prior review flagged — the fix landed for concatenated_hash but the same shape was preserved (or freshly introduced) for single_module_hash and the deferred-tags collection helper. Plus one test whose name advertises a contract its body does not verify. Requesting changes on those three; the rest is non-blocking.
Blocking
🔧 wrench
tsjs_deferred_helpers_format_single_module_urls_and_tagsis tautological —expected_hashis derived by calling the samesingle_module_hashthe production code calls (tsjs.rs:130-145). Same critique the prior review raised againstconcatenated_hash, shifted helper. See inline comment.tsjs_deferred_script_tags_concatenates_tags_in_input_orderis byte-identical to production — test body literally re-executesmodule_ids.iter().map(|id| tsjs_deferred_script_tag(id)).collect::<String>()and asserts equality with the production function (tsjs.rs:156-169). Any symmetric regression passes. See inline comment.tsjs_unified_helpers_use_all_module_idsdoes not verify the contract its name advertises — never asserts thatall_module_ids()is consulted or that every module is in the hash (tsjs.rs:113-127). Would still pass if production silently degraded totsjs_script_src(&[]). See inline comment.
Non-blocking
🤔 thinking
- Status code
casesarray still hand-maintained — error.rs:165-258. The_exhaustiveclosure (lines 147-163) forces compile-fail on a new variant — good — but does not force adding the new variant tocases. A developer could update the closure to silence the compile error and forget the case. Stronger alternative: derivecasesfrom the same exhaustive arm pattern, orassert_eq!(cases.len(), <const tied to closure>). Not blocking — closure catches the common slip — but worth a follow-up.
🌱 seedling
- Follow-up: log unregistered deferred module IDs — tsjs.rs:44.
single_module_hash(module_id).unwrap_or_default()silently produces a 404-bound URL with no logging when the module isn't registered. The PR now documents this as expected behavior at tsjs.rs:148-154; tracking issue would be useful so the test's "documented fallback" framing has a paired plan to remove it.
⛏ nitpick
- Inconsistent test naming convention within this PR — new tests in
error.rs/tsjs.rsuse the descriptive style withouttest_prefix; the new test incookies.rsusestest_prefix matching its file-local neighbors. The codebase has both styles, so this is fine — just a note for future consistency.
📝 note
- Issue #454 wording vs implementation — already noted in the prior review; cookies.rs test correctly verifies current
Err(InvalidHeaderValue)behavior despite the issue text saying "returnsNone". No PR action needed. - Branch is 4 commits behind
origin/main— no conflicting paths, but a rebase before merge is standard.
CI Status
- fmt: PASS
- clippy: PASS
- cargo test: PASS (verified locally on
wasm32-wasip1, 809 trusted-server-core tests) - vitest: PASS
- format-typescript / format-docs: PASS
- browser & integration tests: PASS
- CodeQL: PASS
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!)