Skip to content

Add error status code mapping tests - #755

Merged
ChristianPavilonis merged 4 commits into
mainfrom
issue-448-error-status-tests
Jun 16, 2026
Merged

Add error status code mapping tests#755
ChristianPavilonis merged 4 commits into
mainfrom
issue-448-error-status-tests

Conversation

@ChristianPavilonis

Copy link
Copy Markdown
Collaborator

Summary

  • Adds deterministic unit coverage for TrustedServerError::status_code().
  • Verifies every current error variant maps to the expected HTTP status code.

Changes

File Change
crates/trusted-server-core/src/error.rs Added a focused unit test covering status-code mappings for all TrustedServerError variants.

Closes

Closes #448

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: cargo test -p trusted-server-core error::tests; cargo test -p trusted-server-core

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

@aram356
aram356 requested review from aram356 and prk-Jr and removed request for aram356 June 15, 2026 15:47

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Clean, well-scoped test-only change. Adds status_code_returns_expected_http_status_for_each_variant, a table-driven test that locks the TrustedServerError::status_code() → HTTP-status contract. Verified all 17 enum variants are exercised and every expected status matches the implementation exactly. Closes #448.

Verdict: APPROVE.

Non-blocking

🌱 seedling

  • Exhaustiveness isn't compile-enforced — The new cases array is hand-maintained, so a future 18th variant would compile and pass without being covered here (it would simply go untested). The practical risk is low: status_code()'s match has no _ arm, so a new variant always forces an explicit mapping decision in production code — only its test coverage could be forgotten, not its handling. Flagging for awareness; no change requested. Driving the expected value through a second exhaustive match in the test would mostly duplicate the production match for little added assurance, so I'd lean against it.

👍 praise

  • Complete and accurate coverage — all 17 variants present, each mapped to the correct status; complements the existing user_message() tests without overlap. Follows conventions: "should ..." assertion messages, String::from(...), fictional example.com data only.

CI Status

  • cargo test: PASS (required)
  • cargo fmt: PASS (required)
  • format-typescript: PASS (required)
  • format-docs: PASS (required)
  • vitest: PASS
  • Analyze (rust): PASS
  • Analyze (javascript-typescript): PASS
  • Analyze (actions): PASS
  • CodeQL: PASS
  • integration tests: PASS
  • browser integration tests: PASS
  • prepare integration artifacts: PASS

Test passes locally in an isolated worktree at head c65c2a0 (cargo test -p trusted-server-core error::tests → 3 passed).

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Follow-up to my approval: one optional inline suggestion to make the variant coverage compile-enforced. The PR is already approved — this is a non-blocking ♻️ refactor you can one-click apply or skip.

Comment thread crates/trusted-server-core/src/error.rs
@ChristianPavilonis
ChristianPavilonis merged commit 531cac1 into main Jun 16, 2026
13 checks passed
@aram356
aram356 deleted the issue-448-error-status-tests branch June 22, 2026 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add tests for TrustedServerError HTTP status mapping

3 participants