From 4f84938f72743e840751d7fbb2edd96a4ce79c35 Mon Sep 17 00:00:00 2001 From: "Karl M. Davis" Date: Tue, 23 Nov 2021 09:45:19 -0500 Subject: [PATCH] Tests should not return Result for clearer errors. It's kinda' sorta' explained here: . Basically, though, tests that return a `Result<_>` will have less helpful backtraces than tests that just use asserts and `unwrap()/expect(_)` all over the place. I recently ran into an undiagnosable test failure that this commit makes diagnosable, so this is now "the way things are done" for this project, going forwards. --- fhir-bench-orchestrator/src/sample_data.rs | 31 +++++++++++-------- .../src/servers/firely_spark.rs | 24 ++++++-------- .../src/servers/hapi_jpa.rs | 24 ++++++-------- .../src/servers/ibm_fhir.rs | 24 ++++++-------- .../src/test_framework/mod.rs | 20 +++++------- .../src/util/serde_histogram.rs | 23 +++++++------- 6 files changed, 64 insertions(+), 82 deletions(-) diff --git a/fhir-bench-orchestrator/src/sample_data.rs b/fhir-bench-orchestrator/src/sample_data.rs index 7bc9bb7..8de29bb 100644 --- a/fhir-bench-orchestrator/src/sample_data.rs +++ b/fhir-bench-orchestrator/src/sample_data.rs @@ -393,15 +393,15 @@ fn find_sample_data(data_dir: PathBuf) -> Result { #[cfg(test)] mod tests { use crate::sample_data::{SampleResource, SampleResourceMetadata}; - use eyre::Result; use std::collections::HashMap; /// Verifies that [crate::sample_data::generate_data] works as expected. #[tracing::instrument(level = "info")] #[test_env_log::test(tokio::test)] - async fn generate_data() -> Result<()> { - let benchmark_dir = crate::config::benchmark_dir()?; - let target_dir = tempfile::tempdir()?; + async fn generate_data() { + let benchmark_dir = + crate::config::benchmark_dir().expect("Unable to get benchmark directory."); + let target_dir = tempfile::tempdir().expect("Unable to create temp target directory."); let sample_data = super::generate_data( benchmark_dir.join("synthetic-data").as_path(), 10, @@ -414,23 +414,30 @@ mod tests { "Sample data generation failed: {:?}", sample_data ); - assert_ne!(0, sample_data?.patients.len(), "No patient files found"); - - Ok(()) + assert_ne!( + 0, + sample_data + .expect("Failed to generate sample data.") + .patients + .len(), + "No patient files found" + ); } /// Verifies that [crate::sample_data::SampleData::iter_orgs] works as expected. #[tracing::instrument(level = "info")] #[test_env_log::test(tokio::test)] - async fn iter_orgs() -> Result<()> { - let benchmark_dir = crate::config::benchmark_dir()?; - let target_dir = tempfile::tempdir()?; + async fn iter_orgs() { + let benchmark_dir = + crate::config::benchmark_dir().expect("Unable to get benchmark directory."); + let target_dir = tempfile::tempdir().expect("Unable to create temp target directory."); let sample_data = super::generate_data( benchmark_dir.join("synthetic-data").as_path(), 10, target_dir.path(), ) - .await?; + .await + .expect("Failed to generate sample data."); let orgs: Vec = sample_data.iter_orgs().collect(); // Synthea generates randomized output, but our default config should always produce at least this @@ -463,7 +470,5 @@ mod tests { orgs_with_id ); } - - Ok(()) } } diff --git a/fhir-bench-orchestrator/src/servers/firely_spark.rs b/fhir-bench-orchestrator/src/servers/firely_spark.rs index 85d66fd..e4527f9 100644 --- a/fhir-bench-orchestrator/src/servers/firely_spark.rs +++ b/fhir-bench-orchestrator/src/servers/firely_spark.rs @@ -215,12 +215,10 @@ impl ServerHandle for SparkFhirServerHandle { mod tests { use std::{ffi::OsStr, path::Path}; - use eyre::{eyre, Result}; - #[tracing::instrument(level = "info")] #[test_env_log::test(tokio::test)] #[serial_test::serial(sample_data)] - async fn verify_server_launch() -> Result<()> { + async fn verify_server_launch() { let log_target = std::env::temp_dir().join(format!( "{}_verify_server_launch.log", Path::new(file!()) @@ -244,18 +242,14 @@ mod tests { }; // Clean up the log if things went well, otherwise return an error with the path to it. - match launch_result { - Ok(_) => { - if log_target.exists() { - std::fs::remove_file(log_target)?; - } - Ok(()) - } - Err(err) => Err(eyre!( - "Server launch test failed due to error: {:?}. Log output: {:?}", - err, - log_target - )), + assert!( + launch_result.is_ok(), + "Server launch test failed due to error: {:?}. Log output: {:?}", + launch_result.unwrap_err(), + log_target + ); + if log_target.exists() { + std::fs::remove_file(log_target).expect("Unable to remove temp file."); } } } diff --git a/fhir-bench-orchestrator/src/servers/hapi_jpa.rs b/fhir-bench-orchestrator/src/servers/hapi_jpa.rs index e32d385..5ae21fe 100644 --- a/fhir-bench-orchestrator/src/servers/hapi_jpa.rs +++ b/fhir-bench-orchestrator/src/servers/hapi_jpa.rs @@ -218,12 +218,10 @@ impl ServerHandle for HapiJpaFhirServerHandle { mod tests { use std::{ffi::OsStr, path::Path}; - use eyre::{eyre, Result}; - #[tracing::instrument(level = "info")] #[test_env_log::test(tokio::test)] #[serial_test::serial(sample_data)] - async fn verify_server_launch() -> Result<()> { + async fn verify_server_launch() { let log_target = std::env::temp_dir().join(format!( "{}_verify_server_launch.log", Path::new(file!()) @@ -247,18 +245,14 @@ mod tests { }; // Clean up the log if things went well, otherwise return an error with the path to it. - match launch_result { - Ok(_) => { - if log_target.exists() { - std::fs::remove_file(log_target)?; - } - Ok(()) - } - Err(err) => Err(eyre!( - "Server launch test failed due to error: {:?}. Log output: {:?}", - err, - log_target - )), + assert!( + launch_result.is_ok(), + "Server launch test failed due to error: {:?}. Log output: {:?}", + launch_result.unwrap_err(), + log_target + ); + if log_target.exists() { + std::fs::remove_file(log_target).expect("Unable to remove temp file."); } } } diff --git a/fhir-bench-orchestrator/src/servers/ibm_fhir.rs b/fhir-bench-orchestrator/src/servers/ibm_fhir.rs index c000d31..d3f4df4 100644 --- a/fhir-bench-orchestrator/src/servers/ibm_fhir.rs +++ b/fhir-bench-orchestrator/src/servers/ibm_fhir.rs @@ -207,12 +207,10 @@ impl ServerHandle for IbmFhirServerHandle { mod tests { use std::{ffi::OsStr, path::Path}; - use eyre::{eyre, Result}; - #[tracing::instrument(level = "info")] #[test_env_log::test(tokio::test)] #[serial_test::serial(sample_data)] - async fn verify_server_launch() -> Result<()> { + async fn verify_server_launch() { let log_target = std::env::temp_dir().join(format!( "{}_verify_server_launch.log", Path::new(file!()) @@ -236,18 +234,14 @@ mod tests { }; // Clean up the log if things went well, otherwise return an error with the path to it. - match launch_result { - Ok(_) => { - if log_target.exists() { - std::fs::remove_file(log_target)?; - } - Ok(()) - } - Err(err) => Err(eyre!( - "Server launch test failed due to error: {:?}. Log output: {:?}", - err, - log_target - )), + assert!( + launch_result.is_ok(), + "Server launch test failed due to error: {:?}. Log output: {:?}", + launch_result.unwrap_err(), + log_target + ); + if log_target.exists() { + std::fs::remove_file(log_target).expect("Unable to remove temp file."); } } } diff --git a/fhir-bench-orchestrator/src/test_framework/mod.rs b/fhir-bench-orchestrator/src/test_framework/mod.rs index 3c0df4f..a001765 100644 --- a/fhir-bench-orchestrator/src/test_framework/mod.rs +++ b/fhir-bench-orchestrator/src/test_framework/mod.rs @@ -439,7 +439,6 @@ mod tests { use crate::{config::AppConfig, test_framework::FrameworkMetadata}; use chrono::prelude::*; use chrono::Duration; - use eyre::Result; use hdrhistogram::Histogram; use serde_json::json; @@ -470,7 +469,7 @@ mod tests { /// Verifies that `ServerOperationMetrics` serializes as expected. #[tracing::instrument(level = "info")] #[test_env_log::test(tokio::test)] - async fn serialize_server_operation_metrics() -> Result<()> { + async fn serialize_server_operation_metrics() { let expected = json!({ "throughput_per_second": 42.0, "latency_millis_mean": 1.0, @@ -491,13 +490,11 @@ mod tests { latency_millis_p99: 1, latency_millis_p999: 1, latency_millis_p100: 1, - latency_histogram: Histogram::::new(3)?, + latency_histogram: Histogram::::new(3).expect("Error creating histogram."), latency_histogram_hgrm_gzip: "foo".into(), }; let actual = serde_json::to_string(&actual).unwrap(); assert_eq!(expected, actual); - - Ok(()) } /// Verifies that `ServerOperationLog` serializes as expected. @@ -522,7 +519,7 @@ mod tests { /// Verifies that [ServerOperationMeasurement] serializes as expected. #[tracing::instrument(level = "info")] #[test_env_log::test(tokio::test)] - async fn serialize_server_operation_measurement() -> Result<()> { + async fn serialize_server_operation_measurement() { let expected = json!({ "concurrent_users": 10, "started": "2020-01-01T15:00:00Z", @@ -558,20 +555,18 @@ mod tests { latency_millis_p99: 1, latency_millis_p999: 1, latency_millis_p100: 1, - latency_histogram: Histogram::::new(3)?, + latency_histogram: Histogram::::new(3).expect("Error creating histogram."), latency_histogram_hgrm_gzip: "foo".into(), }, }; let actual = serde_json::to_string(&actual).unwrap(); assert_eq!(expected, actual); - - Ok(()) } /// Verifies that `FrameworkResults` serializes as expected. #[tracing::instrument(level = "info")] #[test_env_log::test(tokio::test)] - async fn serialize_framework_results() -> Result<()> { + async fn serialize_framework_results() { let expected = json!({ "started": "2020-01-01T12:00:00Z", "completed": "2020-01-01T19:00:00Z", @@ -679,7 +674,8 @@ mod tests { latency_millis_p99: 1, latency_millis_p999: 1, latency_millis_p100: 1, - latency_histogram: Histogram::::new(3)?, + latency_histogram: Histogram::::new(3) + .expect("Error creating histogram."), latency_histogram_hgrm_gzip: "foo".into(), }, }], @@ -693,7 +689,5 @@ mod tests { }; let actual = serde_json::to_string(&actual).unwrap(); assert_eq!(expected, actual); - - Ok(()) } } diff --git a/fhir-bench-orchestrator/src/util/serde_histogram.rs b/fhir-bench-orchestrator/src/util/serde_histogram.rs index f4bc849..047064f 100644 --- a/fhir-bench-orchestrator/src/util/serde_histogram.rs +++ b/fhir-bench-orchestrator/src/util/serde_histogram.rs @@ -58,7 +58,6 @@ where /// Unit tests for the [Duration] serializer & deserialzer. #[cfg(test)] mod tests { - use eyre::Result; use hdrhistogram::Histogram; use serde::{Deserialize, Serialize}; use serde_json::json; @@ -73,36 +72,38 @@ mod tests { /// Verifies that [Duration] values serialize as expected. #[tracing::instrument(level = "info")] #[test_env_log::test(tokio::test)] - async fn serialize() -> Result<()> { + async fn serialize() { let expected = json!({ "histogram": "HISTFAAAABx4nJNpmSzMwMDAxAABzFCaEUoz2X+AsQA/awKA", }); let expected = serde_json::to_string(&expected).unwrap(); let mut actual = DurationStruct { - histogram: Histogram::::new(3)?, + histogram: Histogram::::new(3).expect("Error creating histogram."), }; - actual.histogram.record(1)?; + actual + .histogram + .record(1) + .expect("Error recording into histogram."); let actual = serde_json::to_string(&actual).unwrap(); assert_eq!(expected, actual); - - Ok(()) } /// Verifies that [Duration] values deserialize as expected. #[tracing::instrument(level = "info")] #[test_env_log::test(tokio::test)] - async fn deserialize() -> Result<()> { + async fn deserialize() { let mut expected = DurationStruct { - histogram: Histogram::::new(3)?, + histogram: Histogram::::new(3).expect("Error creating histogram."), }; - expected.histogram.record(1)?; + expected + .histogram + .record(1) + .expect("Error recording into histogram."); let actual = json!({ "histogram": "HISTFAAAABx4nJNpmSzMwMDAxAABzFCaEUoz2X+AsQA/awKA", }); let actual = serde_json::to_string(&actual).unwrap(); let actual: DurationStruct = serde_json::from_str(&actual).unwrap(); assert_eq!(expected.histogram, actual.histogram); - - Ok(()) } }