Skip to content

Add test coverage for error mapping and tsjs helpers #648

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ChristianPavilonis wants to merge 2 commits into
base: main
Choose a base branch
from
+258 −15
Open

Add test coverage for error mapping and tsjs helpers #648

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f65a391
Add test coverage for error mapping and tsjs helpers
ChristianPavilonis Apr 20, 2026
02ac32f
Address PR review feedback on test coverage
ChristianPavilonis May 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions crates/trusted-server-core/src/cookies.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,8 @@ pub fn expire_ec_cookie(settings: &Settings, response: &mut fastly::Response) {

#[cfg(test)]
mod tests {
use fastly::http::HeaderValue;

use crate::test_support::tests::create_test_settings;

use super::*;
Expand Down Expand Up @@ -347,6 +349,28 @@ mod tests {
assert!(jar.iter().count() == 0);
}

#[test]
fn test_handle_request_cookies_invalid_utf8_header_value() {
let mut req = Request::get("http://example.com");
req.set_header(
header::COOKIE,
HeaderValue::from_bytes(b"session=\xFF")
.expect("should construct HeaderValue from non-UTF-8 bytes"),
);

let error = handle_request_cookies(&req).expect_err("should reject invalid UTF-8");
let trusted_server_error = error.current_context();

assert!(
matches!(
trusted_server_error,
TrustedServerError::InvalidHeaderValue { message }
if message == "Cookie header contains invalid UTF-8"
),
"should return InvalidHeaderValue for invalid UTF-8 cookie headers",
);
}
Comment thread
ChristianPavilonis marked this conversation as resolved.

#[test]
fn test_set_ec_cookie() {
let settings = create_test_settings();
Expand Down
128 changes: 128 additions & 0 deletions crates/trusted-server-core/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,136 @@ impl IntoHttpResponse for TrustedServerError {

#[cfg(test)]
mod tests {
use http::StatusCode;

use super::*;

#[test]
fn status_code_maps_each_error_variant_to_expected_http_response() {
// Compile-time guard: adding a TrustedServerError variant without
// updating this test will fail to compile.
let _exhaustive = |error: &TrustedServerError| match error {
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 { .. } => (),
};

let cases = [
(
TrustedServerError::BadRequest {
message: "missing field".into(),
},
StatusCode::BAD_REQUEST,
),
(
TrustedServerError::Configuration {
message: "invalid config".into(),
},
StatusCode::INTERNAL_SERVER_ERROR,
),
(
TrustedServerError::Auction {
message: "bid timeout".into(),
},
StatusCode::BAD_GATEWAY,
),
(
TrustedServerError::Gam {
message: "upstream error".into(),
},
StatusCode::BAD_GATEWAY,
),
(
TrustedServerError::GdprConsent {
message: "missing consent string".into(),
},
StatusCode::BAD_REQUEST,
),
(
TrustedServerError::InvalidUtf8 {
message: "byte 0xff".into(),
},
StatusCode::INTERNAL_SERVER_ERROR,
),
(
TrustedServerError::InvalidHeaderValue {
message: "non-ascii".into(),
},
StatusCode::BAD_REQUEST,
),
(
TrustedServerError::KvStore {
store_name: "users".into(),
message: "timeout".into(),
},
StatusCode::SERVICE_UNAVAILABLE,
),
(
TrustedServerError::Prebid {
message: "adapter error".into(),
},
StatusCode::BAD_GATEWAY,
),
(
TrustedServerError::Integration {
integration: "foo".into(),
message: "connection refused".into(),
},
StatusCode::BAD_GATEWAY,
),
(
TrustedServerError::Proxy {
message: "upstream refused".into(),
},
StatusCode::BAD_GATEWAY,
),
(
TrustedServerError::Forbidden {
message: "missing credentials".into(),
},
StatusCode::FORBIDDEN,
),
(
TrustedServerError::AllowlistViolation {
host: "blocked.example.com".into(),
},
StatusCode::FORBIDDEN,
),
(
TrustedServerError::Settings {
message: "parse failed".into(),
},
StatusCode::INTERNAL_SERVER_ERROR,
),
(
TrustedServerError::Ec {
message: "seed missing".into(),
},
StatusCode::INTERNAL_SERVER_ERROR,
),
];

for (error, expected_status) in cases {
assert_eq!(
error.status_code(),
expected_status,
"should map {error:?} to the expected HTTP status",
);
}
}
Comment thread
ChristianPavilonis marked this conversation as resolved.

#[test]
fn server_errors_return_generic_message() {
let cases = [
Expand Down
17 changes: 2 additions & 15 deletions crates/trusted-server-core/src/models.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,32 +55,19 @@ mod tests {
use serde_json::json;

#[test]
fn test_callback_deserialization() {
fn test_callback_deserialization_renames_type_field() {
let json_data = json!({
"type": "impression",
"url": "https://example.com/track/impression"
});

let callback: Callback =
serde_json::from_value(json_data).expect("should deserialize callback");

assert_eq!(callback.callback_type, "impression");
assert_eq!(callback.url, "https://example.com/track/impression");
}

#[test]
fn test_callback_type_field_rename() {
// Test that "type" is correctly renamed to callback_type
let json_str = r#"{
"type": "click",
"url": "https://example.com/track/click"
}"#;

let callback: Callback =
serde_json::from_str(json_str).expect("should deserialize callback from str");
assert_eq!(callback.callback_type, "click");
assert_eq!(callback.url, "https://example.com/track/click");
}

#[test]
fn test_ad_response_full_deserialization() {
let json_data = json!({
Expand Down
104 changes: 104 additions & 0 deletions crates/trusted-server-core/src/tsjs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,107 @@ pub fn tsjs_deferred_script_tags(module_ids: &[&str]) -> String {
.map(|id| tsjs_deferred_script_tag(id))
.collect::<String>()
}

#[cfg(test)]
mod tests {
use super::*;

fn assert_sha256_hex_hash(src: &str) {
let hash = src
.rsplit_once("?v=")
.expect("should have cache-busting query parameter")
.1;

assert_eq!(hash.len(), 64, "should use a SHA-256 hex hash");
assert!(
hash.chars().all(|character| character.is_ascii_hexdigit()),
"should use only ASCII hex digits",
);
}

#[test]
fn tsjs_script_src_formats_unified_bundle_url_with_hash() {
let creative_src = tsjs_script_src(&["creative"]);
let creative_prebid_src = tsjs_script_src(&["creative", "prebid"]);

assert!(
creative_src.starts_with("/static/tsjs=tsjs-unified.min.js?v="),
"should include the unified bundle path and cache-busting parameter",
);
assert_sha256_hex_hash(&creative_src);
assert_ne!(
creative_src, creative_prebid_src,
"should generate different cache-busting URLs for different module sets",
);
}
Comment thread
ChristianPavilonis marked this conversation as resolved.

#[test]
fn tsjs_script_tag_wraps_source_in_a_single_tag() {
let module_ids = ["creative"];
let expected_src = tsjs_script_src(&module_ids);

assert_eq!(
tsjs_script_tag(&module_ids),
format!("<script src=\"{expected_src}\" id=\"trustedserver-js\"></script>"),
"should render the injected trustedserver script tag",
);
}

#[test]
fn tsjs_unified_helpers_use_all_module_ids() {
Copy link
Copy Markdown
Collaborator

@aram356 aram356 May 6, 2026

Choose a reason for hiding this comment

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

🔧 wrench — Test name advertises "use all module IDs" but the body never verifies that contract. It only checks URL prefix, SHA-256 hex hash structure, and that tsjs_unified_script_tag wraps tsjs_unified_script_src. If tsjs_unified_script_src() were silently changed to tsjs_script_src(&[]), the test would still pass — the unified bundle path would still match, the empty hash would still be 64 hex chars (well, it would actually be the hash of just core, which is still a valid SHA-256 hex), and the tag wrap assertion would still hold.

Fix — make the contract executable by asserting equivalence with the manual all-modules call:

let unified = tsjs_unified_script_src();
let manual = tsjs_script_src(&all_module_ids());
assert_eq!(
    unified, manual,
    "unified helpers must hash every registered module",
);
assert!(
    unified.starts_with("/static/tsjs=tsjs-unified.min.js?v="),
    "should include the unified bundle path and cache-busting parameter",
);
assert_sha256_hex_hash(&unified);
assert_eq!(
    tsjs_unified_script_tag(),
    format!("<script src=\"{unified}\" id=\"trustedserver-js\"></script>"),
    "should wrap the unified bundle source in the standard script tag",
);

This closes the gap: any divergence between tsjs_unified_script_src and the all-modules contract is caught directly.

let src = tsjs_unified_script_src();

assert!(
src.starts_with("/static/tsjs=tsjs-unified.min.js?v="),
"should include the unified bundle path and cache-busting parameter",
);
assert_sha256_hex_hash(&src);
assert_eq!(
tsjs_unified_script_tag(),
format!("<script src=\"{src}\" id=\"trustedserver-js\"></script>"),
"should wrap the unified bundle source in the standard script tag",
);
}

#[test]
fn tsjs_deferred_helpers_format_single_module_urls_and_tags() {
let module_id = "prebid";
let expected_hash = single_module_hash(module_id).expect("should hash known module");
Copy link
Copy Markdown
Collaborator

@aram356 aram356 May 6, 2026

Choose a reason for hiding this comment

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

🔧 wrench — This re-introduces the same tautology pattern the prior review flagged for concatenated_hash, just shifted to single_module_hash. expected_hash is computed by calling the same hash function the production code calls — if single_module_hash regressed to return String::new(), both tsjs_deferred_script_src(module_id) and expected_hash would change in lockstep and this test would still pass (equivalent to the documented unregistered-module case at line 148-154).

Fix — mirror the unified-bundle approach: assert URL prefix and SHA-256 hex hash structure, not equality with another call to the same hash function. For the tag wrap test, derive expected_src from tsjs_deferred_script_src itself — that's verifying the wrapping format, not re-computing the hash.

let src = tsjs_deferred_script_src("prebid");
assert!(
    src.starts_with("/static/tsjs=tsjs-prebid.min.js?v="),
    "should include the deferred module path and cache-busting parameter",
);
assert_sha256_hex_hash(&src);

assert_eq!(
    tsjs_deferred_script_tag("prebid"),
    format!("<script src=\"{src}\" defer></script>"),
    "should render a deferred script tag for the module",
);

let expected_src = format!("/static/tsjs=tsjs-{module_id}.min.js?v={expected_hash}");

assert_eq!(
tsjs_deferred_script_src(module_id),
expected_src,
"should include the deferred module path and hash",
);
assert_eq!(
tsjs_deferred_script_tag(module_id),
format!("<script src=\"{expected_src}\" defer></script>"),
"should render a deferred script tag for the module",
);
}

#[test]
fn tsjs_deferred_script_src_documents_unregistered_module_fallback() {
assert_eq!(
tsjs_deferred_script_src("unknown-module"),
"/static/tsjs=tsjs-unknown-module.min.js?v=",
"should preserve the current fallback until callers validate registered modules",
);
}
Comment thread
ChristianPavilonis marked this conversation as resolved.

#[test]
fn tsjs_deferred_script_tags_concatenates_tags_in_input_order() {
let module_ids = ["prebid", "creative"];
let expected = module_ids
Copy link
Copy Markdown
Collaborator

@aram356 aram356 May 6, 2026

Choose a reason for hiding this comment

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

🔧 wrench — Test body is byte-identical to the production function body (tsjs.rs:62-65):

// production
module_ids.iter().map(|id| tsjs_deferred_script_tag(id)).collect::<String>()

// test
let expected = module_ids.iter().map(|id| tsjs_deferred_script_tag(id)).collect::<String>();
assert_eq!(tsjs_deferred_script_tags(&module_ids), expected, ...);

Any symmetric regression in either side passes the test. The test name claims to verify input-order concatenation, but the test never actually checks that the modules appear in input order — it just checks that re-running the same expression yields the same result.

Fix — assert the property (order preservation, count of tags), not equality with a re-execution of the same code:

let result = tsjs_deferred_script_tags(&["prebid", "creative"]);
let prebid_pos = result.find("tsjs-prebid").expect("should contain prebid script");
let creative_pos = result.find("tsjs-creative").expect("should contain creative script");
assert!(prebid_pos < creative_pos, "should preserve input order");
assert_eq!(
    result.matches("<script ").count(),
    2,
    "should emit one tag per module",
);

.iter()
.map(|id| tsjs_deferred_script_tag(id))
.collect::<String>();

assert_eq!(
tsjs_deferred_script_tags(&module_ids),
expected,
"should concatenate one deferred script tag per module in order",
);
}
}
Loading