Skip to content

Add test coverage for error mapping and tsjs helpers#648

Open
ChristianPavilonis wants to merge 2 commits intomainfrom
tests/improve-test-covereage
Open

Add test coverage for error mapping and tsjs helpers#648
ChristianPavilonis wants to merge 2 commits intomainfrom
tests/improve-test-covereage

Conversation

@ChristianPavilonis
Copy link
Copy Markdown
Collaborator

Summary

  • add exhaustive unit coverage for TrustedServerError::status_code() so each public variant is pinned to its expected HTTP response
  • add direct tsjs formatting tests for unified and deferred script URL/tag helpers using computed hashes
  • cover invalid UTF-8 cookie-header handling and simplify duplicate callback serde rename tests

Changes

File Change
crates/trusted-server-core/src/error.rs Added a table-driven test covering HTTP status code mapping for every TrustedServerError variant.
crates/trusted-server-core/src/tsjs.rs Added direct unit tests for unified and deferred TS/JS script URL and tag helpers.
crates/trusted-server-core/src/cookies.rs Added a unit test for invalid UTF-8 Cookie header handling in handle_request_cookies().
crates/trusted-server-core/src/models.rs Consolidated duplicate callback serde rename tests into one focused test.

Closes

Closes #448
Closes #450
Closes #454
Closes #456

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:

Checklist

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

Copy link
Copy Markdown
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 timeerror.rs:144-247. Hand-maintained cases array means a new variant added to TrustedServerError won't fail this test. A non-exhaustive match guard would force exhaustive coverage. See inline comment.
  • tsjs hash assertions are tautologicaltsjs.rs:75-116. expected_hash is derived by calling the same concatenated_hash the production code calls. If concatenated_hash regressed, 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 moduletsjs.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_module locks in silent failuretsjs.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 invisibletsjs.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 add log::warn! for unregistered module IDs.

📝 note

  • Issue #454 wording vs implementationcookies.rs:353-372. Issue says "returning None" but production returns Err. Test correctly verifies current implementation; issue text is stale.

⛏ nitpick

  • ["core", "creative"] redundantly includes coretsjs.rs:76, 88. concatenate_modules always prepends core; could be simplified to ["creative"].
  • HeaderValue::from_bytes expect message is slightly misleadingcookies.rs:358. Reads like a property of handle_request_cookies but is about HeaderValue::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

Comment thread crates/trusted-server-core/src/error.rs
Comment thread crates/trusted-server-core/src/tsjs.rs
Comment thread crates/trusted-server-core/src/tsjs.rs Outdated
Comment thread crates/trusted-server-core/src/tsjs.rs
Comment thread crates/trusted-server-core/src/cookies.rs
Comment thread crates/trusted-server-core/src/tsjs.rs Outdated
Comment thread crates/trusted-server-core/src/cookies.rs Outdated
@ChristianPavilonis ChristianPavilonis requested a review from aram356 May 5, 2026 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants