Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

- (snapshots) Add `snapshots diff` command for locally comparing directories of PNG snapshot images using odiff ([#3306](https://github.com/getsentry/sentry-cli/pull/3306))
- (snapshots) Add `snapshots download` command for downloading baseline snapshot images from Sentry ([#3310](https://github.com/getsentry/sentry-cli/pull/3310))
- (snapshots) Add `--all-image-file-names` and `--all-image-file-names-file` flags to `snapshots upload` for detecting image removals and renames in selective builds ([#3312](https://github.com/getsentry/sentry-cli/pull/3312))

### Fixes

Expand Down
35 changes: 35 additions & 0 deletions src/api/data_types/snapshots.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ pub struct SnapshotsManifest<'a> {
/// Removals and renames will not be detected on PRs.
#[serde(skip_serializing_if = "std::ops::Not::not")]
pub selective: bool,
#[serde(skip_serializing_if = "Option::is_none")]
pub all_image_file_names: Option<Vec<String>>,
#[serde(flatten)]
pub vcs_info: VcsInfo<'a>,
}
Expand Down Expand Up @@ -83,6 +85,7 @@ mod tests {
images: HashMap::new(),
diff_threshold: None,
selective: false,
all_image_file_names: None,
vcs_info: empty_vcs_info(),
};
let json = serde_json::to_value(&manifest).unwrap();
Expand All @@ -96,12 +99,44 @@ mod tests {
images: HashMap::new(),
diff_threshold: None,
selective: true,
all_image_file_names: None,
vcs_info: empty_vcs_info(),
};
let json = serde_json::to_value(&manifest).unwrap();
assert_eq!(json["selective"], json!(true));
}

#[test]
fn manifest_omits_all_image_file_names_when_none() {
let manifest = SnapshotsManifest {
app_id: "app".into(),
images: HashMap::new(),
diff_threshold: None,
selective: true,
all_image_file_names: None,
vcs_info: empty_vcs_info(),
};
let json = serde_json::to_value(&manifest).unwrap();
assert!(!json
.as_object()
.unwrap()
.contains_key("all_image_file_names"));
}

#[test]
fn manifest_includes_all_image_file_names_when_some() {
let manifest = SnapshotsManifest {
app_id: "app".into(),
images: HashMap::new(),
diff_threshold: None,
selective: true,
all_image_file_names: Some(vec!["a.png".into(), "b.png".into()]),
vcs_info: empty_vcs_info(),
};
let json = serde_json::to_value(&manifest).unwrap();
assert_eq!(json["all_image_file_names"], json!(["a.png", "b.png"]));
}

#[test]
fn cli_managed_fields_override_sidecar_fields() {
let extra = serde_json::from_value(json!({
Expand Down
123 changes: 121 additions & 2 deletions src/commands/snapshots/upload.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,30 @@ pub fn make_command(command: Command) -> Command {
Removals and renames cannot be detected on PRs.",
),
)
.arg(
Arg::new("all_image_file_names")
.long("all-image-file-names")
.value_name("NAMES")
.conflicts_with("all_image_file_names_file")
.help(
"Comma-separated list of all image names (including subdirectory paths) \
in the full test suite. \
Used with selective uploads to detect image removals and renames. \
Implicitly enables --selective.",
),
)
.arg(
Arg::new("all_image_file_names_file")
.long("all-image-file-names-file")
.value_name("PATH")
.conflicts_with("all_image_file_names")
.help(
"Path to a file containing all image names (including subdirectory paths), \
one per line. \
Used with selective uploads to detect image removals and renames. \
Implicitly enables --selective.",
),
)
.git_metadata_args()
}

Expand Down Expand Up @@ -146,6 +170,26 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {

validate_image_sizes(&images)?;

let all_image_file_names = parse_all_image_file_names(matches)?;

let selective = matches.get_flag("selective") || all_image_file_names.is_some();

if let Some(ref all_names) = all_image_file_names {
let all_names_set: HashSet<&str> = all_names.iter().map(|s| s.as_str()).collect();
let mut unknown: Vec<String> = images
.iter()
.map(|img| crate::utils::fs::path_as_url(&img.relative_path))
.filter(|k| !all_names_set.contains(k.as_str()))
.collect();
if !unknown.is_empty() {
Comment thread
NicoHinderling marked this conversation as resolved.
unknown.sort();
anyhow::bail!(
"The following uploaded images are not in --all-image-file-names: {}",
unknown.join(", ")
);
Comment thread
sentry[bot] marked this conversation as resolved.
}
}

println!(
"{} Processing {} image {}",
style(">").dim(),
Expand All @@ -158,13 +202,12 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
// Build manifest from discovered images
let diff_threshold = matches.get_one::<f64>("diff_threshold").copied();

let selective = matches.get_flag("selective");

let manifest = SnapshotsManifest {
app_id: app_id.clone(),
images: manifest_entries,
diff_threshold,
selective,
all_image_file_names,
vcs_info,
};

Expand Down Expand Up @@ -199,6 +242,45 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
Ok(())
}

fn split_and_trim(input: &str, separator: char) -> Vec<String> {
input
.split(separator)
.map(|s| s.trim().to_owned())
.filter(|s| !s.is_empty())
.collect()
}

fn normalize_image_names(names: Vec<String>) -> Vec<String> {
names
.into_iter()
.map(|s| s.strip_prefix("./").unwrap_or(&s).replace('\\', "/"))
.collect()
}
Comment thread
sentry[bot] marked this conversation as resolved.

fn parse_all_image_file_names(matches: &ArgMatches) -> Result<Option<Vec<String>>> {
if let Some(names_str) = matches.get_one::<String>("all_image_file_names") {
let names = normalize_image_names(split_and_trim(names_str, ','));
if names.is_empty() {
anyhow::bail!("--all-image-file-names must not be empty");
}
return Ok(Some(names));
}

if let Some(file_path) = matches.get_one::<String>("all_image_file_names_file") {
let content = std::fs::read_to_string(file_path)
.with_context(|| format!("Failed to read --all-image-file-names-file: {file_path}"))?;
let names = normalize_image_names(split_and_trim(&content, '\n'));
if names.is_empty() {
anyhow::bail!(
"--all-image-file-names-file is empty or contains only blank lines: {file_path}"
);
}
return Ok(Some(names));
}

Ok(None)
}

fn collect_images(dir: &Path) -> Vec<ImageInfo> {
WalkDir::new(dir)
.follow_links(true)
Expand Down Expand Up @@ -505,4 +587,41 @@ mod tests {
let err = validate_image_sizes(&[make_image(8001, 5000)]).unwrap_err();
assert!(err.to_string().contains("exceed the maximum pixel limit"));
}

#[test]
fn test_split_and_trim_comma_separated() {
assert_eq!(
split_and_trim("a.png, b.png, c.png", ','),
vec!["a.png", "b.png", "c.png"]
);
}

#[test]
fn test_split_and_trim_whitespace_and_empty() {
assert_eq!(
split_and_trim(" a.png , b.png , ", ','),
vec!["a.png", "b.png"]
);
}

#[test]
fn test_split_and_trim_newline_separated() {
assert_eq!(
split_and_trim("a.png\nb.png\n\nc.png\n", '\n'),
vec!["a.png", "b.png", "c.png"]
);
}

#[test]
fn test_normalize_image_names_strips_dot_slash() {
let input = vec![
"./img/a.png".to_owned(),
"./img/b.png".to_owned(),
"img/c.png".to_owned(),
];
assert_eq!(
normalize_image_names(input),
vec!["img/a.png", "img/b.png", "img/c.png"]
);
}
}
10 changes: 10 additions & 0 deletions tests/integration/_cases/snapshots/snapshots-upload-help.trycmd
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,16 @@ Options:
Indicates this upload contains only a subset of images. Removals and renames cannot be
detected on PRs.

--all-image-file-names <NAMES>
Comma-separated list of all image names (including subdirectory paths) in the full test
suite. Used with selective uploads to detect image removals and renames. Implicitly
enables --selective.

--all-image-file-names-file <PATH>
Path to a file containing all image names (including subdirectory paths), one per line.
Used with selective uploads to detect image removals and renames. Implicitly enables
--selective.

--head-sha <head_sha>
The VCS commit sha to use for the upload. If not provided, the current commit sha will be
used.
Expand Down
Loading