Skip to content

Commit 4dd030c

Browse files
nnethercotennethercote
committed
Fix printing of the table.
The table spacing was made nice in #1312 but then got messed up again in #1334. This commit does the following. - Fixes the table spacing. - Changes how test failures are printed, so the expected and actual outputs are printed nicely across multiple lines, instead of being stringified with into a single line. I should have done this some time ago, it makes adjusting the expected output *so* much easier. - Renames some variables, for clarity and consistency. - Imports `std::cmp` because it's used in multiple places. - Other minor tweaks.
1 parent 1022cfb commit 4dd030c

File tree

1 file changed

+127
-63
lines changed

1 file changed

+127
-63
lines changed

‎site/src/comparison.rs

Lines changed: 127 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,12 @@ use collector::Bound;
1313
use serde::{Deserialize, Serialize};
1414

1515
use database::CommitType;
16+
use std::cmp;
1617
use std::collections::{HashMap, HashSet};
1718
use std::error::Error;
1819
use std::fmt::Write;
1920
use std::hash::Hash;
21+
use std::iter;
2022
use std::sync::Arc;
2123

2224
type BoxedError = Box<dyn Error + Send + Sync>;
@@ -330,7 +332,7 @@ impl ArtifactComparisonSummary {
330332
let cmp = |b1: &TestResultComparison, b2: &TestResultComparison| {
331333
b1.relative_change()
332334
.partial_cmp(&b2.relative_change())
333-
.unwrap_or(std::cmp::Ordering::Equal)
335+
.unwrap_or(cmp::Ordering::Equal)
334336
};
335337
relevant_comparisons.sort_by(cmp);
336338

@@ -498,7 +500,7 @@ impl ArtifactComparisonSummary {
498500
let most_neg = self.most_negative_change().unwrap();
499501
let most_pos_abs = most_pos.relative_change().abs();
500502
let most_neg_abs = most_neg.relative_change().abs();
501-
if most_neg_abs.partial_cmp(&most_pos_abs) == Some(std::cmp::Ordering::Greater) {
503+
if most_neg_abs.partial_cmp(&most_pos_abs) == Some(cmp::Ordering::Greater) {
502504
Some(most_neg)
503505
} else {
504506
Some(most_pos)
@@ -568,7 +570,7 @@ pub fn write_summary_table(
568570
.map(|m| format!("({})", m.metric.as_str()))
569571
})
570572
.flatten()
571-
.unwrap_or_else(|| String::from(" "));
573+
.unwrap_or_else(|| " ".to_string());
572574

573575
fn render_stat<F: FnOnce() -> Option<f64>>(count: usize, calculate: F) -> String {
574576
let value = if count > 0 { calculate() } else { None };
@@ -587,10 +589,10 @@ pub fn write_summary_table(
587589
}
588590

589591
// (label, mean, max, count)
590-
let mut column_data = vec![];
592+
let mut columns = vec![];
591593

592594
// label
593-
column_data.push(vec![
595+
columns.push(vec![
594596
"Regressions ❌ <br /> (primary)".to_string(),
595597
"Regressions ❌ <br /> (secondary)".to_string(),
596598
"Improvements ✅ <br /> (primary)".to_string(),
@@ -599,7 +601,7 @@ pub fn write_summary_table(
599601
]);
600602

601603
// mean
602-
column_data.push(vec![
604+
columns.push(vec![
603605
render_stat(primary.num_regressions, || {
604606
Some(primary.arithmetic_mean_of_regressions())
605607
}),
@@ -621,7 +623,7 @@ pub fn write_summary_table(
621623

622624
// range
623625
let rel_change = |r: Option<&TestResultComparison>| r.unwrap().relative_change() * 100.0;
624-
column_data.push(vec![
626+
columns.push(vec![
625627
render_range(primary.num_regressions, || {
626628
(
627629
rel_change(primary.smallest_regression()),
@@ -655,7 +657,7 @@ pub fn write_summary_table(
655657
]);
656658

657659
// count
658-
column_data.push(vec![
660+
columns.push(vec![
659661
primary.num_regressions.to_string(),
660662
secondary.num_regressions.to_string(),
661663
primary.num_improvements.to_string(),
@@ -667,25 +669,65 @@ pub fn write_summary_table(
667669
// easy to read for anyone who is viewing the Markdown source.
668670
let column_labels = [
669671
metric,
670-
format!("mean"),
672+
"mean".to_string(),
671673
"range".to_string(),
672-
format!("count"),
674+
"count".to_string(),
673675
];
674-
let counts: Vec<usize> = column_labels.iter().map(|s| s.chars().count()).collect();
675-
for column in &column_labels {
676-
write!(result, "| {} ", column).unwrap();
676+
677+
// Calculate the console width of a string, allowing for double-width
678+
// chars. The `unicode_width` crate does this properly, but with only two
679+
// emoji in use we can do it easily ourselves.
680+
let width = |s: &str| {
681+
s.chars()
682+
.map(|c| if c == '❌' || c == '✅' { 2 } else { 1 })
683+
.sum()
684+
};
685+
686+
// Get the width of each column.
687+
let column_widths: Vec<usize> = column_labels
688+
.iter()
689+
.zip(columns.iter())
690+
.map(|(column_label, column)| {
691+
// Get the maximum width of the column data cells and the column label.
692+
column
693+
.iter()
694+
.chain(iter::once(column_label))
695+
.map(|cell| width(cell))
696+
.max()
697+
.unwrap()
698+
})
699+
.collect();
700+
701+
// Write column labels.
702+
for (column_label, &column_width) in column_labels.iter().zip(column_widths.iter()) {
703+
write!(
704+
result,
705+
"| {}{} ",
706+
column_label,
707+
" ".repeat(column_width - width(column_label))
708+
)
709+
.unwrap();
677710
}
678711
result.push_str("|\n");
679-
for &count in &counts {
680-
write!(result, "|:{}:", "-".repeat(count)).unwrap();
712+
713+
// Write lines under the column labels.
714+
for &column_width in &column_widths {
715+
write!(result, "|:{}:", "-".repeat(column_width)).unwrap();
681716
}
682717
result.push_str("|\n");
683718

684-
for row in 0..5 {
685-
let row_data = column_data.iter().map(|rows| rows[row].clone());
686-
debug_assert_eq!(row_data.len(), column_labels.len());
687-
for (column, &count) in row_data.zip(&counts) {
688-
write!(result, "| {:<1$} ", column, count).unwrap();
719+
// Write the column data.
720+
for row_idx in 0..5 {
721+
let row = columns.iter().map(|rows| rows[row_idx].clone());
722+
assert_eq!(row.len(), column_labels.len());
723+
for (cell, &column_width) in row.zip(column_widths.iter()) {
724+
write!(
725+
result,
726+
"| {}{} ",
727+
cell,
728+
" ".repeat(column_width - width(&cell))
729+
)
730+
.unwrap();
689731
}
690732
result.push_str("|\n");
691733
}
@@ -1040,7 +1082,7 @@ impl HistoricalData {
10401082
.zip(self.data.iter())
10411083
.map(|(d, &r)| d / r)
10421084
.collect::<Vec<_>>();
1043-
deltas.sort_by(|d1, d2| d1.partial_cmp(d2).unwrap_or(std::cmp::Ordering::Equal));
1085+
deltas.sort_by(|d1, d2| d1.partial_cmp(d2).unwrap_or(cmp::Ordering::Equal));
10441086
deltas
10451087
}
10461088

@@ -1242,15 +1284,15 @@ impl TestResultComparison {
12421284
}
12431285
}
12441286

1245-
impl std::cmp::PartialEq for TestResultComparison {
1287+
impl cmp::PartialEq for TestResultComparison {
12461288
fn eq(&self, other: &Self) -> bool {
12471289
self.benchmark == other.benchmark
12481290
&& self.profile == other.profile
12491291
&& self.scenario == other.scenario
12501292
}
12511293
}
12521294

1253-
impl std::cmp::Eq for TestResultComparison {}
1295+
impl cmp::Eq for TestResultComparison {}
12541296

12551297
impl std::hash::Hash for TestResultComparison {
12561298
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
@@ -1426,11 +1468,13 @@ mod tests {
14261468
(Category::Primary, 1.0, 3.0),
14271469
],
14281470
r#"
1429-
| Regressions ❌ <br /> (primary) | 146.7% | [100.0%, 200.0%] | 3 |
1430-
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1431-
| Improvements ✅ <br /> (primary) | - | - | 0 |
1432-
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1433-
| All ❌✅ (primary) | 146.7% | [100.0%, 200.0%] | 3 |
1471+
| (instructions:u) | mean | range | count |
1472+
|:----------------------------------:|:------:|:----------------:|:-----:|
1473+
| Regressions ❌ <br /> (primary) | 146.7% | [100.0%, 200.0%] | 3 |
1474+
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1475+
| Improvements ✅ <br /> (primary) | - | - | 0 |
1476+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1477+
| All ❌✅ (primary) | 146.7% | [100.0%, 200.0%] | 3 |
14341478
"#
14351479
.trim_start(),
14361480
);
@@ -1445,11 +1489,13 @@ mod tests {
14451489
(Category::Primary, 4.0, 1.0),
14461490
],
14471491
r#"
1448-
| Regressions ❌ <br /> (primary) | - | - | 0 |
1449-
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1450-
| Improvements ✅ <br /> (primary) | -71.7% | [-80.0%, -60.0%] | 3 |
1451-
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1452-
| All ❌✅ (primary) | -71.7% | [-80.0%, -60.0%] | 3 |
1492+
| (instructions:u) | mean | range | count |
1493+
|:----------------------------------:|:------:|:----------------:|:-----:|
1494+
| Regressions ❌ <br /> (primary) | - | - | 0 |
1495+
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1496+
| Improvements ✅ <br /> (primary) | -71.7% | [-80.0%, -60.0%] | 3 |
1497+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1498+
| All ❌✅ (primary) | -71.7% | [-80.0%, -60.0%] | 3 |
14531499
"#
14541500
.trim_start(),
14551501
);
@@ -1464,11 +1510,13 @@ mod tests {
14641510
(Category::Secondary, 4.0, 1.0),
14651511
],
14661512
r#"
1467-
| Regressions ❌ <br /> (primary) | - | - | 0 |
1468-
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1469-
| Improvements ✅ <br /> (primary) | - | - | 0 |
1513+
| (instructions:u) | mean | range | count |
1514+
|:----------------------------------:|:------:|:----------------:|:-----:|
1515+
| Regressions ❌ <br /> (primary) | - | - | 0 |
1516+
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1517+
| Improvements ✅ <br /> (primary) | - | - | 0 |
14701518
| Improvements ✅ <br /> (secondary) | -71.7% | [-80.0%, -60.0%] | 3 |
1471-
| All ❌✅ (primary) | - | - | 0 |
1519+
| All ❌✅ (primary) | - | - | 0 |
14721520
"#
14731521
.trim_start(),
14741522
);
@@ -1483,11 +1531,13 @@ mod tests {
14831531
(Category::Secondary, 1.0, 3.0),
14841532
],
14851533
r#"
1486-
| Regressions ❌ <br /> (primary) | - | - | 0 |
1487-
| Regressions ❌ <br /> (secondary) | 146.7% | [100.0%, 200.0%] | 3 |
1488-
| Improvements ✅ <br /> (primary) | - | - | 0 |
1489-
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1490-
| All ❌✅ (primary) | - | - | 0 |
1534+
| (instructions:u) | mean | range | count |
1535+
|:----------------------------------:|:------:|:----------------:|:-----:|
1536+
| Regressions ❌ <br /> (primary) | - | - | 0 |
1537+
| Regressions ❌ <br /> (secondary) | 146.7% | [100.0%, 200.0%] | 3 |
1538+
| Improvements ✅ <br /> (primary) | - | - | 0 |
1539+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1540+
| All ❌✅ (primary) | - | - | 0 |
14911541
"#
14921542
.trim_start(),
14931543
);
@@ -1503,11 +1553,13 @@ mod tests {
15031553
(Category::Primary, 4.0, 1.0),
15041554
],
15051555
r#"
1506-
| Regressions ❌ <br /> (primary) | 150.0% | [100.0%, 200.0%] | 2 |
1507-
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1508-
| Improvements ✅ <br /> (primary) | -62.5% | [-75.0%, -50.0%] | 2 |
1509-
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1510-
| All ❌✅ (primary) | 43.8% | [-75.0%, 200.0%] | 4 |
1556+
| (instructions:u) | mean | range | count |
1557+
|:----------------------------------:|:------:|:----------------:|:-----:|
1558+
| Regressions ❌ <br /> (primary) | 150.0% | [100.0%, 200.0%] | 2 |
1559+
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1560+
| Improvements ✅ <br /> (primary) | -62.5% | [-75.0%, -50.0%] | 2 |
1561+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1562+
| All ❌✅ (primary) | 43.8% | [-75.0%, 200.0%] | 4 |
15111563
"#
15121564
.trim_start(),
15131565
);
@@ -1525,11 +1577,13 @@ mod tests {
15251577
(Category::Primary, 4.0, 1.0),
15261578
],
15271579
r#"
1528-
| Regressions ❌ <br /> (primary) | 150.0% | [100.0%, 200.0%] | 2 |
1529-
| Regressions ❌ <br /> (secondary) | 100.0% | [100.0%, 100.0%] | 1 |
1530-
| Improvements ✅ <br /> (primary) | -62.5% | [-75.0%, -50.0%] | 2 |
1580+
| (instructions:u) | mean | range | count |
1581+
|:----------------------------------:|:------:|:----------------:|:-----:|
1582+
| Regressions ❌ <br /> (primary) | 150.0% | [100.0%, 200.0%] | 2 |
1583+
| Regressions ❌ <br /> (secondary) | 100.0% | [100.0%, 100.0%] | 1 |
1584+
| Improvements ✅ <br /> (primary) | -62.5% | [-75.0%, -50.0%] | 2 |
15311585
| Improvements ✅ <br /> (secondary) | -66.7% | [-66.7%, -66.7%] | 1 |
1532-
| All ❌✅ (primary) | 43.8% | [-75.0%, 200.0%] | 4 |
1586+
| All ❌✅ (primary) | 43.8% | [-75.0%, 200.0%] | 4 |
15331587
"#
15341588
.trim_start(),
15351589
);
@@ -1543,11 +1597,13 @@ mod tests {
15431597
(Category::Primary, 5.0, 6.0),
15441598
],
15451599
r#"
1546-
| Regressions ❌ <br /> (primary) | 20.0% | [20.0%, 20.0%] | 1 |
1547-
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1548-
| Improvements ✅ <br /> (primary) | -50.0% | [-50.0%, -50.0%] | 1 |
1549-
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1550-
| All ❌✅ (primary) | -15.0% | [-50.0%, 20.0%] | 2 |
1600+
| (instructions:u) | mean | range | count |
1601+
|:----------------------------------:|:------:|:----------------:|:-----:|
1602+
| Regressions ❌ <br /> (primary) | 20.0% | [20.0%, 20.0%] | 1 |
1603+
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1604+
| Improvements ✅ <br /> (primary) | -50.0% | [-50.0%, -50.0%] | 1 |
1605+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1606+
| All ❌✅ (primary) | -15.0% | [-50.0%, 20.0%] | 2 |
15511607
"#
15521608
.trim_start(),
15531609
);
@@ -1561,11 +1617,13 @@ mod tests {
15611617
(Category::Primary, 6.0, 5.0),
15621618
],
15631619
r#"
1564-
| Regressions ❌ <br /> (primary) | 100.0% | [100.0%, 100.0%] | 1 |
1565-
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1566-
| Improvements ✅ <br /> (primary) | -16.7% | [-16.7%, -16.7%] | 1 |
1567-
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1568-
| All ❌✅ (primary) | 41.7% | [-16.7%, 100.0%] | 2 |
1620+
| (instructions:u) | mean | range | count |
1621+
|:----------------------------------:|:------:|:----------------:|:-----:|
1622+
| Regressions ❌ <br /> (primary) | 100.0% | [100.0%, 100.0%] | 1 |
1623+
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1624+
| Improvements ✅ <br /> (primary) | -16.7% | [-16.7%, -16.7%] | 1 |
1625+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1626+
| All ❌✅ (primary) | 41.7% | [-16.7%, 100.0%] | 2 |
15691627
"#
15701628
.trim_start(),
15711629
);
@@ -1615,7 +1673,13 @@ mod tests {
16151673

16161674
let mut result = String::new();
16171675
write_summary_table(&primary, &secondary, true, &mut result);
1618-
let header = "| (instructions:u) | mean | range | count |\n|:----------------:|:----:|:-----:|:-----:|\n";
1619-
assert_eq!(result, format!("{header}{expected}"));
1676+
// We don't use `assert_eq!` here because it stringifies the arguments,
1677+
// making the tables hard to read when printed.
1678+
if result != expected {
1679+
panic!(
1680+
"output mismatch:\nexpected:\n{}actual:\n{}",
1681+
expected, result
1682+
);
1683+
}
16201684
}
16211685
}

0 commit comments

Comments
 (0)