Skip to content
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

Jump to bottom

Add set preset #139

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Add set preset #139

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
3 changes: 3 additions & 0 deletions moz-webgpu-cts/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,8 @@ enum UpdateExpectedPreset {
#[value(alias("same-build"))]
Merge,
ResetAll,
/// Sets only reported results
Set,
}

impl From<UpdateExpectedPreset> for process_reports::ReportProcessingPreset {
Expand All @@ -246,6 +248,7 @@ impl From<UpdateExpectedPreset> for process_reports::ReportProcessingPreset {
UpdateExpectedPreset::ResetContradictory => Self::ResetContradictoryOutcomes,
UpdateExpectedPreset::Merge => Self::MergeOutcomes,
UpdateExpectedPreset::ResetAll => Self::ResetAllOutcomes,
UpdateExpectedPreset::Set => Self::SetNewOutcomes,
}
}
}
Expand Down
7 changes: 7 additions & 0 deletions moz-webgpu-cts/src/process_reports.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ pub(crate) enum ReportProcessingPreset {
ResetContradictoryOutcomes,
MergeOutcomes,
ResetAllOutcomes,
SetNewOutcomes,
}

#[derive(Debug, Default)]
Expand Down Expand Up @@ -136,6 +137,10 @@ fn reconcile<Out>(
Some(rep) => meta | rep,
None => meta,
},
ReportProcessingPreset::SetNewOutcomes => |meta, rep| match rep {
Some(rep) => rep,
None => meta,
},
Comment on lines +140 to +143
Copy link
Collaborator

@ErichDonGubler ErichDonGubler Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

suggestion: At first glance, this appears to be different from ResetAll WRT fallback (i.e., Set falls back to meta, rather than ResetAll's Default::default()). But as of #131, ResetAll actually doesn't ever run into the fallback case, because missing entries are deleted and skip reconciliation entirely. Huh...I hadn't considered that that wasn't exercised before. 😅

We probably want to change ResetAll to match this function body instead. 🤔 If we do, I think that strengthens the argument for a separate option (instead of a new preset) at #139 (review).

};

ExpandedPropertyValue::from_query(|platform, build_profile| {
Expand Down Expand Up @@ -452,6 +457,7 @@ pub(crate) fn process_reports(
log::warn!("removing metadata after {msg}");
return None;
}
ReportProcessingPreset::SetNewOutcomes => {}
}
}

Expand Down Expand Up @@ -525,6 +531,7 @@ pub(crate) fn process_reports(
log::warn!("removing metadata after {msg}");
return None;
}
ReportProcessingPreset::SetNewOutcomes => {}
}
}

Expand Down
Loading