Skip to content

Commit 9f2b2cf

Browse files
committed
Address review comments.
1 parent 498f63b commit 9f2b2cf

File tree

1 file changed

+34
-16
lines changed

1 file changed

+34
-16
lines changed

site/src/comparison.rs

Lines changed: 34 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -447,32 +447,38 @@ impl ArtifactComparisonSummary {
447447
.filter(|c| c.is_regression())
448448
}
449449

450-
fn most_negative_improvement(&self) -> Option<&TestResultComparison> {
450+
// This is the most negative result.
451+
fn largest_improvement(&self) -> Option<&TestResultComparison> {
451452
self.relevant_comparisons
452453
.iter()
453454
.find(|s| s.is_improvement())
454455
}
455456

456-
fn least_negative_improvement(&self) -> Option<&TestResultComparison> {
457+
// This is the least negative result.
458+
fn smallest_improvement(&self) -> Option<&TestResultComparison> {
457459
self.relevant_comparisons
458460
.iter()
459461
.rfind(|s| s.is_improvement())
460462
}
461463

462-
fn most_positive_regression(&self) -> Option<&TestResultComparison> {
464+
// This is the most positive result.
465+
fn largest_regression(&self) -> Option<&TestResultComparison> {
463466
self.relevant_comparisons
464467
.iter()
465468
.rfind(|s| s.is_regression())
466469
}
467470

468-
fn least_positive_regression(&self) -> Option<&TestResultComparison> {
471+
// This is the least positive result.
472+
fn smallest_regression(&self) -> Option<&TestResultComparison> {
469473
self.relevant_comparisons.iter().find(|s| s.is_regression())
470474
}
471475

476+
// This may be an improvement or a regression.
472477
fn most_positive_change(&self) -> Option<&TestResultComparison> {
473478
self.relevant_comparisons.last()
474479
}
475480

481+
// This may be an improvement or a regression.
476482
fn most_negative_change(&self) -> Option<&TestResultComparison> {
477483
self.relevant_comparisons.first()
478484
}
@@ -487,7 +493,19 @@ impl ArtifactComparisonSummary {
487493
}
488494

489495
pub fn largest_change(&self) -> Option<&TestResultComparison> {
490-
self.relevant_comparisons.first()
496+
if self.num_changes() == 0 {
497+
None
498+
} else {
499+
let most_pos = self.most_positive_change().unwrap();
500+
let most_neg = self.most_negative_change().unwrap();
501+
let most_pos_abs = most_pos.relative_change().abs();
502+
let most_neg_abs = most_neg.relative_change().abs();
503+
if most_neg_abs.partial_cmp(&most_pos_abs) == Some(std::cmp::Ordering::Greater) {
504+
Some(most_neg)
505+
} else {
506+
Some(most_pos)
507+
}
508+
}
491509
}
492510
}
493511

@@ -605,36 +623,36 @@ pub fn write_summary_table(
605623
]);
606624

607625
// range
608-
let f = |r: Option<&TestResultComparison>| r.unwrap().relative_change() * 100.0;
626+
let rel_change = |r: Option<&TestResultComparison>| r.unwrap().relative_change() * 100.0;
609627
column_data.push(vec![
610628
render_range(primary.num_regressions, || {
611629
(
612-
f(primary.least_positive_regression()),
613-
f(primary.most_positive_regression()),
630+
rel_change(primary.smallest_regression()),
631+
rel_change(primary.largest_regression()),
614632
)
615633
}),
616634
render_range(secondary.num_regressions, || {
617635
(
618-
f(secondary.least_positive_regression()),
619-
f(secondary.most_positive_regression()),
636+
rel_change(secondary.smallest_regression()),
637+
rel_change(secondary.largest_regression()),
620638
)
621639
}),
622640
render_range(primary.num_improvements, || {
623641
(
624-
f(primary.most_negative_improvement()),
625-
f(primary.least_negative_improvement()),
642+
rel_change(primary.largest_improvement()),
643+
rel_change(primary.smallest_improvement()),
626644
)
627645
}),
628646
render_range(secondary.num_improvements, || {
629647
(
630-
f(secondary.most_negative_improvement()),
631-
f(secondary.least_negative_improvement()),
648+
rel_change(secondary.largest_improvement()),
649+
rel_change(secondary.smallest_improvement()),
632650
)
633651
}),
634652
render_range(primary.num_regressions + primary.num_improvements, || {
635653
(
636-
f(primary.most_negative_change()),
637-
f(primary.most_positive_change()),
654+
rel_change(primary.most_negative_change()),
655+
rel_change(primary.most_positive_change()),
638656
)
639657
}),
640658
]);

0 commit comments

Comments
 (0)