Skip to content

Refactor overlapping test coverage for streaming and creative rewrites #661

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 1 commit into
base: main
Choose a base branch
from
+226 −194
Open

Refactor overlapping test coverage for streaming and creative rewrites #661

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
260 changes: 129 additions & 131 deletions crates/trusted-server-core/src/creative.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -775,6 +775,49 @@ mod tests {
rewrite_creative_html, rewrite_srcset, rewrite_style_urls, sanitize_creative_html, to_abs,
};

fn rewrite_srcset_attr(attr_name: &str, attr_value: &str) -> String {
let settings = crate::test_support::tests::create_test_settings();
let html = format!(r#"<img {}="{}">"#, attr_name, attr_value);
rewrite_creative_html(&settings, &html)
Copy link
Copy Markdown
Collaborator

@aram356 aram356 Apr 30, 2026

Choose a reason for hiding this comment

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

🤔 thinking — The new helper wraps both srcset and imagesrcset in <img>. The old imagesrcset tests used <div>. Both are HTML-spec-illegal placements — imagesrcset belongs on <link rel="preload" as="image"> — but both rely on the element-agnostic [imagesrcset] selector at creative.rs:660, so runtime behavior is identical.

Coverage gap (pre-existing, not introduced here): the realistic <link rel="preload" as="image" imagesrcset="…"> path also flows through the link[href] handler at creative.rs:556-561. Neither the old nor new tests exercise that path. Worth a follow-up issue rather than expanding this PR.

}

fn assert_rewritten_srcset_attr_case(
case_name: &str,
attr_name: &str,
attr_value: &str,
expected_relative: &str,
expected_descriptors: &[&str],
) {
let out = rewrite_srcset_attr(attr_name, attr_value);

assert!(
out.matches("/first-party/proxy?tsurl=").count() >= 2,
"case `{}` expected two rewritten {} candidates: {}",
case_name,
attr_name,
out
Copy link
Copy Markdown
Collaborator

@aram356 aram356 Apr 30, 2026

Choose a reason for hiding this comment

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

🔧 wrench — Assertion message and check are inconsistent.

out.matches("/first-party/proxy?tsurl=").count() >= 2,
"case `{}` expected two rewritten {} candidates: {}",

The message says expected two but the predicate accepts ≥ 2. All current cases produce exactly two rewritten candidates, so tightening the check is the correct fix and would catch any future regression that quietly rewrites a third (e.g., relative-candidate) entry.

Fix:

assert_eq!(
    out.matches("/first-party/proxy?tsurl=").count(),
    2,
    "case `{}` expected exactly two rewritten {} candidates: {}",
    case_name,
    attr_name,
    out
);

);
assert!(
out.contains(expected_relative),
"case `{}` expected relative {} candidate `{}` to be preserved in {}",
case_name,
attr_name,
expected_relative,
out
);

for descriptor in expected_descriptors {
assert!(
out.contains(descriptor),
"case `{}` expected {} descriptor `{}` in {}",
case_name,
attr_name,
descriptor,
out
);
}
}

#[test]
fn rewrites_width_height_attrs() {
use crate::http_util::encode_url;
Expand Down Expand Up @@ -941,26 +984,50 @@ mod tests {
}

#[test]
fn rewrites_srcset_absolute_candidates_and_preserves_descriptors() {
// Absolute + protocol-relative get rewritten to /first-party/proxy?tsurl=..., relative remains
let settings = crate::test_support::tests::create_test_settings();
let html = r#"<img srcset="https://cdn.example/img-1x.png 1x, //cdn.example/img-2x.png 2x, /local/img.png 1x">"#;
let out = rewrite_creative_html(&settings, html);

// Should have at least two proxied candidates
let cnt = out.matches("/first-party/proxy?tsurl=").count();
assert!(
cnt >= 2,
"expected at least two rewritten candidates: {}",
out
);

// Descriptors preserved
assert!(out.contains(" 1x"));
assert!(out.contains(" 2x"));
fn rewrites_srcset_attribute_cases() {
struct SrcsetCase<'a> {
name: &'a str,
attr_value: &'a str,
expected_relative: &'a str,
expected_descriptors: &'a [&'a str],
}

// Relative left as-is
assert!(out.contains("/local/img.png 1x"));
let cases = [
SrcsetCase {
name: "absolute and protocol-relative candidates",
attr_value: "https://cdn.example/img-1x.png 1x, //cdn.example/img-2x.png 2x, /local/img.png 1x",
expected_relative: "/local/img.png 1x",
expected_descriptors: &[" 1x", " 2x"],
},
SrcsetCase {
name: "no-space commas with fractional density",
attr_value: "https://cdn.example/img-1x.png 1x,//cdn.example/img-1_5x.png 1.5x,/local/img.png 2x",
expected_relative: "/local/img.png 2x",
expected_descriptors: &[" 1x", " 1.5x"],
},
SrcsetCase {
name: "relative middle candidate without leading slash",
attr_value: "https://cdn.example/a.png 1x,local/b.png 2x,//cdn.example/c.png 3x",
expected_relative: "local/b.png 2x",
expected_descriptors: &[" 1x", " 2x", " 3x"],
},
SrcsetCase {
name: "extra spaces normalize but preserve semantics",
attr_value: " https://cdn.example/a.png 1x , //cdn.example/b.png 2x , /local/c.png 1x ",
expected_relative: "/local/c.png 1x",
expected_descriptors: &[" 1x", " 2x"],
},
];

for case in cases {
assert_rewritten_srcset_attr_case(
case.name,
"srcset",
case.attr_value,
case.expected_relative,
case.expected_descriptors,
);
}
}

#[test]
Expand All @@ -986,59 +1053,6 @@ mod tests {
assert!(out.contains("<img src=\"/fallback.jpg\""));
}

#[test]
fn rewrites_srcset_no_space_commas_preserves_descriptors() {
let settings = crate::test_support::tests::create_test_settings();
let html = r#"<img srcset="https://cdn.example/img-1x.png 1x,//cdn.example/img-1_5x.png 1.5x,/local/img.png 2x">"#;
let out = rewrite_creative_html(&settings, html);
// Absolute and protocol-relative candidates rewritten
assert!(
out.matches("/first-party/proxy?tsurl=").count() >= 2,
"{}",
out
);
// Descriptors preserved (including fractional)
assert!(out.contains(" 1x"));
assert!(out.contains(" 1.5x"));
// Relative left unchanged
assert!(out.contains("/local/img.png 2x"));
}

#[test]
fn rewrites_srcset_relative_no_space_middle() {
let settings = crate::test_support::tests::create_test_settings();
// Relative candidate (no leading slash) in the middle, no space after commas
let html =
r#"<img srcset="https://cdn.example/a.png 1x,local/b.png 2x,//cdn.example/c.png 3x">"#;
let out = rewrite_creative_html(&settings, html);
// Two absolute/protocol-relative rewritten
assert!(
out.matches("/first-party/proxy?tsurl=").count() >= 2,
"{}",
out
);
// Relative preserved as-is
assert!(out.contains("local/b.png 2x"));
}

#[test]
fn rewrites_srcset_with_extra_spaces() {
let settings = crate::test_support::tests::create_test_settings();
let html = r#"<img srcset=" https://cdn.example/a.png 1x , //cdn.example/b.png 2x , /local/c.png 1x ">"#;
let out = rewrite_creative_html(&settings, html);
// Two absolute/protocol-relative rewritten
assert!(
out.matches("/first-party/proxy?tsurl=").count() >= 2,
"{}",
out
);
// Relative preserved
assert!(out.contains("/local/c.png 1x"));
// Normalized spacing: single space before descriptor is acceptable
assert!(out.contains(" 1x"));
assert!(out.contains(" 2x"));
}

#[test]
fn rewrites_script_src_and_leaves_relative() {
let settings = crate::test_support::tests::create_test_settings();
Expand Down Expand Up @@ -1077,66 +1091,50 @@ mod tests {
}

#[test]
fn rewrites_imagesrcset_absolute_candidates_and_preserves_descriptors() {
let settings = crate::test_support::tests::create_test_settings();
// Use a valid quoted attribute; the previous string had malformed escapes
let html = r#"<div imagesrcset="https://cdn.example/img-1x.png 1x, //cdn.example/img-2x.png 2x, /local/img.png 1x"></div>"#;
let out = rewrite_creative_html(&settings, html);
let cnt = out.matches("/first-party/proxy?tsurl=").count();
assert!(
cnt >= 1,
"expected at least one rewritten imagesrcset candidate: {}",
out
);
assert!(out.contains("/local/img.png 1x"));
}

#[test]
fn rewrites_imagesrcset_no_space_commas() {
let settings = crate::test_support::tests::create_test_settings();
let html = r#"<div imagesrcset="https://cdn.example/a.png 1x,//cdn.example/b.png 2x,/local/c.png 1x"></div>"#;
let out = rewrite_creative_html(&settings, html);
// At least two rewritten
assert!(
out.matches("/first-party/proxy?tsurl=").count() >= 2,
"{}",
out
);
// Relative preserved
assert!(out.contains("/local/c.png 1x"));
}

#[test]
fn rewrites_imagesrcset_relative_no_space_middle() {
let settings = crate::test_support::tests::create_test_settings();
let html = r#"<div imagesrcset="https://cdn.example/a.png 1x,local/b.png 2x,//cdn.example/c.png 3x"></div>"#;
let out = rewrite_creative_html(&settings, html);
// Two absolute/protocol-relative rewritten
assert!(
out.matches("/first-party/proxy?tsurl=").count() >= 2,
"{}",
out
);
// Relative preserved
assert!(out.contains("local/b.png 2x"));
}
fn rewrites_imagesrcset_attribute_cases() {
struct SrcsetCase<'a> {
name: &'a str,
attr_value: &'a str,
expected_relative: &'a str,
expected_descriptors: &'a [&'a str],
}

#[test]
fn rewrites_imagesrcset_with_extra_spaces() {
let settings = crate::test_support::tests::create_test_settings();
let html = r#"<div imagesrcset=" https://cdn.example/a.png 1x , //cdn.example/b.png 2x , /local/c.png 1x "></div>"#;
let out = rewrite_creative_html(&settings, html);
// Two absolute/protocol-relative rewritten
assert!(
out.matches("/first-party/proxy?tsurl=").count() >= 2,
"{}",
out
);
// Relative preserved
assert!(out.contains("/local/c.png 1x"));
// Normalized spacing present
assert!(out.contains(" 1x"));
assert!(out.contains(" 2x"));
let cases = [
SrcsetCase {
name: "absolute and protocol-relative candidates",
attr_value: "https://cdn.example/img-1x.png 1x, //cdn.example/img-2x.png 2x, /local/img.png 1x",
expected_relative: "/local/img.png 1x",
expected_descriptors: &[" 1x", " 2x"],
},
SrcsetCase {
name: "no-space commas",
attr_value: "https://cdn.example/a.png 1x,//cdn.example/b.png 2x,/local/c.png 1x",
expected_relative: "/local/c.png 1x",
expected_descriptors: &[" 1x", " 2x"],
},
SrcsetCase {
name: "relative middle candidate without leading slash",
attr_value: "https://cdn.example/a.png 1x,local/b.png 2x,//cdn.example/c.png 3x",
expected_relative: "local/b.png 2x",
expected_descriptors: &[" 1x", " 2x", " 3x"],
},
SrcsetCase {
name: "extra spaces normalize but preserve semantics",
attr_value: " https://cdn.example/a.png 1x , //cdn.example/b.png 2x , /local/c.png 1x ",
expected_relative: "/local/c.png 1x",
expected_descriptors: &[" 1x", " 2x"],
},
];

for case in cases {
assert_rewritten_srcset_attr_case(
case.name,
"imagesrcset",
case.attr_value,
case.expected_relative,
case.expected_descriptors,
);
}
}

#[test]
Expand Down
Loading
Loading