BlakeOrth commented on code in PR #18085:
URL: https://github.com/apache/datafusion/pull/18085#discussion_r2434144273
##########
datafusion-cli/src/object_storage/instrumented.rs:
##########
@@ -537,26 +662,14 @@ mod tests {
extra_display: None,
});
- let summaries = RequestSummary::summarize_by_operation(&requests);
- assert_eq!(summaries.len(), 1);
-
- let summary = summaries.get(&Operation::Get).unwrap();
- assert_eq!(summary.count, 1);
- assert_eq!(
- summary.duration_stats.as_ref().unwrap().min,
- Duration::from_secs(5)
- );
- assert_eq!(
- summary.duration_stats.as_ref().unwrap().max,
- Duration::from_secs(5)
- );
- assert_eq!(
- summary.duration_stats.as_ref().unwrap().sum,
- Duration::from_secs(5)
- );
- assert_eq!(summary.size_stats.as_ref().unwrap().min, 100);
- assert_eq!(summary.size_stats.as_ref().unwrap().max, 100);
- assert_eq!(summary.size_stats.as_ref().unwrap().sum, 100);
+ assert_snapshot!(RequestSummaries::new(&requests), @r"
+
+-----------+----------+-----------+-----------+-----------+-----------+
+ | Operation | Metric | min | max | avg | sum
|
+
+-----------+----------+-----------+-----------+-----------+-----------+
+ | Get | duration | 5.000000s | 5.000000s | 5.000000s | 5.000000s
|
+ | Get | size | 100 B | 100 B | 100 B | 100 B
|
+
+-----------+----------+-----------+-----------+-----------+-----------+
+ ");
Review Comment:
These test changes are the only thing that's really caused me to pause and
think some. I really like how it's reduced the overall size of the test and
improved readability. However, I think it comes with the tradeoff that _if_
there was a regression of some sort that caused the test to fail it's going to
be a bit harder to hunt down exactly what's happening since it's now behind a
display method implementation. I don't think I have a strong opinion on the
"right" path forward here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
