diff --git a/src/executor/memory/executor.rs b/src/executor/memory/executor.rs index c1677533..72ba4a05 100644 --- a/src/executor/memory/executor.rs +++ b/src/executor/memory/executor.rs @@ -1,4 +1,5 @@ use crate::binary_installer::ensure_binary_installed; +use crate::executor::ExecutorName; use crate::executor::helpers::command::CommandBuilder; use crate::executor::helpers::env::get_base_injected_env; use crate::executor::helpers::get_bench_command::get_bench_command; @@ -7,7 +8,6 @@ use crate::executor::helpers::run_with_env::wrap_with_env; use crate::executor::helpers::run_with_sudo::wrap_with_sudo; use crate::executor::shared::fifo::RunnerFifo; use crate::executor::{ExecutionContext, Executor}; -use crate::executor::{ExecutorName, ExecutorTeardownResult}; use crate::instruments::mongo_tracer::MongoTracer; use crate::prelude::*; use crate::run::check_system::SystemInfo; @@ -118,10 +118,7 @@ impl Executor for MemoryExecutor { Ok(()) } - async fn teardown( - &self, - execution_context: &ExecutionContext, - ) -> Result { + async fn teardown(&self, execution_context: &ExecutionContext) -> Result<()> { let files: Vec<_> = std::fs::read_dir(execution_context.profile_folder.join("results"))? .filter_map(Result::ok) // Filter out non-memtrack files: @@ -134,22 +131,13 @@ impl Executor for MemoryExecutor { .flat_map(|f| std::fs::File::open(f.path())) .filter(|file| !MemtrackArtifact::is_empty(file)) .collect(); - if files.is_empty() { - if !execution_context.config.allow_empty { - bail!( - "No memtrack artifact files found. Does the integration support memory profiling?" - ); - } else { - warn!( - "No memtrack artifact files found. Does the integration support memory profiling?" - ); - } + bail!( + "No memtrack artifact files found. Does the integration support memory profiling?" + ); } - Ok(ExecutorTeardownResult { - has_results: !files.is_empty(), - }) + Ok(()) } } diff --git a/src/executor/mod.rs b/src/executor/mod.rs index 9410cdb2..1204bc9c 100644 --- a/src/executor/mod.rs +++ b/src/executor/mod.rs @@ -71,13 +71,6 @@ pub fn get_all_executors() -> Vec> { ] } -pub struct ExecutorTeardownResult { - /// Whether the executor has produced results - /// Normally, the executor should fail if it does not produce results - /// Unless `allow_empty` option is set - has_results: bool, -} - #[async_trait(?Send)] pub trait Executor { fn name(&self) -> ExecutorName; @@ -98,10 +91,7 @@ pub trait Executor { mongo_tracer: &Option, ) -> Result<()>; - async fn teardown( - &self, - execution_context: &ExecutionContext, - ) -> Result; + async fn teardown(&self, execution_context: &ExecutionContext) -> Result<()>; } /// Execute benchmarks with the given configuration @@ -129,8 +119,6 @@ where end_group!(); } - let mut has_results = false; - if !execution_context.config.skip_run { start_opened_group!("Running the benchmarks"); @@ -153,8 +141,7 @@ where } end_group!(); start_opened_group!("Tearing down environment"); - let teardown_result = executor.teardown(execution_context).await?; - has_results = teardown_result.has_results; + executor.teardown(execution_context).await?; execution_context .logger @@ -166,7 +153,7 @@ where }; // Handle upload and polling - if has_results && !execution_context.config.skip_upload { + if !execution_context.config.skip_upload { if !execution_context.is_local() { // If relevant, set the OIDC token for authentication // Note: OIDC tokens can expire quickly, so we set it just before the upload diff --git a/src/executor/valgrind/executor.rs b/src/executor/valgrind/executor.rs index 21efcc79..53d76034 100644 --- a/src/executor/valgrind/executor.rs +++ b/src/executor/valgrind/executor.rs @@ -1,8 +1,8 @@ use async_trait::async_trait; use std::path::Path; +use crate::executor::Executor; use crate::executor::{ExecutionContext, ExecutorName}; -use crate::executor::{Executor, ExecutorTeardownResult}; use crate::instruments::mongo_tracer::MongoTracer; use crate::prelude::*; use crate::run::check_system::SystemInfo; @@ -45,10 +45,7 @@ impl Executor for ValgrindExecutor { Ok(()) } - async fn teardown( - &self, - execution_context: &ExecutionContext, - ) -> Result { + async fn teardown(&self, execution_context: &ExecutionContext) -> Result<()> { harvest_perf_maps(&execution_context.profile_folder).await?; // No matter the command in input, at this point valgrind will have been run and have produced output files. @@ -58,6 +55,6 @@ impl Executor for ValgrindExecutor { // A comprehensive message will be sent to the user if no benchmarks are detected, // even if it's later in the process than technically possible. - Ok(ExecutorTeardownResult { has_results: true }) + Ok(()) } } diff --git a/src/executor/wall_time/executor.rs b/src/executor/wall_time/executor.rs index 59429a91..7bb4faac 100644 --- a/src/executor/wall_time/executor.rs +++ b/src/executor/wall_time/executor.rs @@ -2,7 +2,6 @@ use super::helpers::validate_walltime_results; use super::perf::PerfRunner; use crate::executor::Config; use crate::executor::Executor; -use crate::executor::ExecutorTeardownResult; use crate::executor::helpers::command::CommandBuilder; use crate::executor::helpers::env::{get_base_injected_env, is_codspeed_debug_enabled}; use crate::executor::helpers::get_bench_command::get_bench_command; @@ -196,10 +195,7 @@ impl Executor for WallTimeExecutor { Ok(()) } - async fn teardown( - &self, - execution_context: &ExecutionContext, - ) -> Result { + async fn teardown(&self, execution_context: &ExecutionContext) -> Result<()> { debug!("Copying files to the profile folder"); if let Some(perf) = &self.perf @@ -209,12 +205,12 @@ impl Executor for WallTimeExecutor { .await?; } - let has_results = validate_walltime_results( + validate_walltime_results( &execution_context.profile_folder, execution_context.config.allow_empty, )?; - Ok(ExecutorTeardownResult { has_results }) + Ok(()) } } diff --git a/src/executor/wall_time/helpers.rs b/src/executor/wall_time/helpers.rs index dc4102fb..23c15acc 100644 --- a/src/executor/wall_time/helpers.rs +++ b/src/executor/wall_time/helpers.rs @@ -12,15 +12,13 @@ fn add_empty_result_error_explanation(error_details: &str) -> String { /// Validates that walltime results exist and contain at least one benchmark. /// When `allow_empty` is true, empty benchmark results are allowed. -/// -/// Returns `Ok(true)` if valid results are found, `Ok(false)` if no results are found but `allow_empty` is true -pub fn validate_walltime_results(profile_folder: &Path, allow_empty: bool) -> Result { +pub fn validate_walltime_results(profile_folder: &Path, allow_empty: bool) -> Result<()> { let results_dir = profile_folder.join("results"); if !results_dir.exists() { if allow_empty { warn!("No walltime results found in profile folder: {results_dir:?}."); - return Ok(false); + return Ok(()); } bail!(add_empty_result_error_explanation(&format!( "No walltime results found in profile folder: {results_dir:?}." @@ -54,26 +52,26 @@ pub fn validate_walltime_results(profile_folder: &Path, allow_empty: bool) -> Re ))); } debug!("No benchmarks found in {path:?} (allowed)"); - } else { - found_benchmark_results = true; - debug!( - "Found {} benchmark(s) in {path:?}", - results.benchmarks.len() - ); } + + found_benchmark_results = true; + debug!( + "Found {} benchmark(s) in {path:?}", + results.benchmarks.len() + ); } if !found_benchmark_results { if allow_empty { warn!("No JSON result files found in: {results_dir:?}."); - return Ok(false); + return Ok(()); } bail!(add_empty_result_error_explanation(&format!( "No JSON result files found in: {results_dir:?}." ))); } - Ok(true) + Ok(()) } #[cfg(test)] @@ -190,7 +188,6 @@ mod tests { let result = validate_walltime_results(profile.path(), false); assert!(result.is_ok()); - assert!(result.unwrap()); } #[test] @@ -201,7 +198,6 @@ mod tests { let result = validate_walltime_results(profile.path(), false); assert!(result.is_ok()); - assert!(result.unwrap()); } #[test] @@ -213,7 +209,6 @@ mod tests { let result = validate_walltime_results(profile.path(), false); assert!(result.is_ok()); - assert!(result.unwrap()); } // Failure cases @@ -295,7 +290,6 @@ mod tests { let result = validate_walltime_results(profile.path(), true); assert!(result.is_ok()); - assert!(!result.unwrap()); } #[test] @@ -305,7 +299,6 @@ mod tests { let result = validate_walltime_results(profile.path(), true); assert!(result.is_ok()); - assert!(!result.unwrap()); } #[test] @@ -315,6 +308,5 @@ mod tests { let result = validate_walltime_results(profile.path(), true); assert!(result.is_ok()); - assert!(!result.unwrap()); } }