refactor: split eval runner execution helpers

Separate fixture preparation and result assembly from the main eval runner so execution changes stay easier to isolate and verify.

Made-with: Cursor

Author: haasonsaas

src/commands/eval/runner/execute.rs

@@ -1,117 +1,40 @@
+#[path = "execute/loading.rs"]
+mod loading;
+#[path = "execute/result.rs"]
+mod result;
+
 use anyhow::Result;
-use std::path::{Path, PathBuf};
 
 use crate::config;
-use crate::core::eval_benchmarks::FixtureResult as BenchmarkFixtureResult;
 use crate::review::review_diff_content_raw;
 
+use self::loading::prepare_fixture_execution;
+use self::result::{append_total_comment_failures, build_benchmark_metrics, build_fixture_result};
 use super::super::{EvalFixtureResult, LoadedEvalFixture};
 use super::matching::evaluate_fixture_expectations;
 
 pub(in super::super) async fn run_eval_fixture(
     config: &config::Config,
     loaded_fixture: LoadedEvalFixture,
 ) -> Result<EvalFixtureResult> {
-    let LoadedEvalFixture {
-        fixture_path,
-        fixture,
-        suite_name,
-        suite_thresholds,
-        difficulty,
-    } = loaded_fixture;
-    let fixture_name = fixture.name.clone().unwrap_or_else(|| {
-        fixture_path
-            .file_name()
-            .and_then(|value| value.to_str())
-            .unwrap_or("fixture")
-            .to_string()
-    });
-    let fixture_dir = fixture_path
-        .parent()
-        .map(Path::to_path_buf)
-        .unwrap_or_else(|| PathBuf::from("."));
-
-    let diff_content = match (fixture.diff.clone(), fixture.diff_file.clone()) {
-        (Some(diff), _) => diff,
-        (None, Some(diff_file)) => {
-            let path = if diff_file.is_absolute() {
-                diff_file
-            } else {
-                fixture_dir.join(diff_file)
-            };
-            std::fs::read_to_string(path)?
-        }
-        (None, None) => anyhow::bail!(
-            "Fixture '{}' must define either diff or diff_file",
-            fixture_name
-        ),
-    };
-
-    let repo_path = fixture
-        .repo_path
-        .clone()
-        .map(|path| {
-            if path.is_absolute() {
-                path
-            } else {
-                fixture_dir.join(path)
-            }
-        })
-        .unwrap_or_else(|| PathBuf::from("."));
-
-    let review_result = review_diff_content_raw(&diff_content, config.clone(), &repo_path).await?;
+    let prepared = prepare_fixture_execution(loaded_fixture)?;
+    let review_result =
+        review_diff_content_raw(&prepared.diff_content, config.clone(), &prepared.repo_path)
+            .await?;
     let comments = review_result.comments;
     let total_comments = comments.len();
-    let match_summary = evaluate_fixture_expectations(&fixture.expect, &comments);
-    let mut failures = match_summary.failures;
-
-    if let Some(min_total) = fixture.expect.min_total {
-        if total_comments < min_total {
-            failures.push(format!(
-                "Expected at least {} comments, got {}",
-                min_total, total_comments
-            ));
-        }
-    }
-    if let Some(max_total) = fixture.expect.max_total {
-        if total_comments > max_total {
-            failures.push(format!(
-                "Expected at most {} comments, got {}",
-                max_total, total_comments
-            ));
-        }
-    }
+    let match_summary = evaluate_fixture_expectations(&prepared.fixture.expect, &comments);
+    let mut failures = match_summary.failures.clone();
 
-    let benchmark_metrics = suite_name.as_ref().map(|_| {
-        let accounted_for = match_summary
-            .used_comment_indices
-            .union(&match_summary.unexpected_comment_indices)
-            .count();
-        let extra_findings = total_comments.saturating_sub(accounted_for);
-        let mut result = BenchmarkFixtureResult::compute(
-            &fixture_name,
-            fixture.expect.must_find.len(),
-            fixture.expect.must_not_find.len(),
-            match_summary.required_matches,
-            match_summary.unexpected_comment_indices.len(),
-            extra_findings,
-        );
-        result.details = failures.clone();
-        result
-    });
+    append_total_comment_failures(&mut failures, total_comments, &prepared.fixture.expect);
+    let benchmark_metrics =
+        build_benchmark_metrics(&prepared, total_comments, &match_summary, &failures);
 
-    Ok(EvalFixtureResult {
-        fixture: fixture_name,
-        suite: suite_name,
-        passed: failures.is_empty(),
+    Ok(build_fixture_result(
+        prepared,
         total_comments,
-        required_matches: match_summary.required_matches,
-        required_total: match_summary.required_total,
+        match_summary,
         benchmark_metrics,
-        suite_thresholds,
-        difficulty,
-        rule_metrics: match_summary.rule_metrics,
-        rule_summary: match_summary.rule_summary,
         failures,
-    })
+    ))
 }

src/commands/eval/runner/execute/loading.rs

@@ -0,0 +1,87 @@
+use anyhow::Result;
+use std::path::{Path, PathBuf};
+
+use crate::core::eval_benchmarks::{BenchmarkThresholds, Difficulty};
+
+use super::super::super::{EvalFixture, LoadedEvalFixture};
+
+pub(super) struct PreparedFixtureExecution {
+    pub(super) fixture_name: String,
+    pub(super) fixture: EvalFixture,
+    pub(super) suite_name: Option<String>,
+    pub(super) suite_thresholds: Option<BenchmarkThresholds>,
+    pub(super) difficulty: Option<Difficulty>,
+    pub(super) diff_content: String,
+    pub(super) repo_path: PathBuf,
+}
+
+pub(super) fn prepare_fixture_execution(
+    loaded_fixture: LoadedEvalFixture,
+) -> Result<PreparedFixtureExecution> {
+    let LoadedEvalFixture {
+        fixture_path,
+        fixture,
+        suite_name,
+        suite_thresholds,
+        difficulty,
+    } = loaded_fixture;
+    let fixture_name = fixture.name.clone().unwrap_or_else(|| {
+        fixture_path
+            .file_name()
+            .and_then(|value| value.to_str())
+            .unwrap_or("fixture")
+            .to_string()
+    });
+    let fixture_dir = fixture_path
+        .parent()
+        .map(Path::to_path_buf)
+        .unwrap_or_else(|| PathBuf::from("."));
+    let diff_content = load_diff_content(&fixture_name, &fixture_dir, &fixture)?;
+    let repo_path = resolve_repo_path(&fixture_dir, &fixture);
+
+    Ok(PreparedFixtureExecution {
+        fixture_name,
+        fixture,
+        suite_name,
+        suite_thresholds,
+        difficulty,
+        diff_content,
+        repo_path,
+    })
+}
+
+fn load_diff_content(
+    fixture_name: &str,
+    fixture_dir: &Path,
+    fixture: &EvalFixture,
+) -> Result<String> {
+    match (fixture.diff.clone(), fixture.diff_file.clone()) {
+        (Some(diff), _) => Ok(diff),
+        (None, Some(diff_file)) => {
+            let path = if diff_file.is_absolute() {
+                diff_file
+            } else {
+                fixture_dir.join(diff_file)
+            };
+            Ok(std::fs::read_to_string(path)?)
+        }
+        (None, None) => anyhow::bail!(
+            "Fixture '{}' must define either diff or diff_file",
+            fixture_name
+        ),
+    }
+}
+
+fn resolve_repo_path(fixture_dir: &Path, fixture: &EvalFixture) -> PathBuf {
+    fixture
+        .repo_path
+        .clone()
+        .map(|path| {
+            if path.is_absolute() {
+                path
+            } else {
+                fixture_dir.join(path)
+            }
+        })
+        .unwrap_or_else(|| PathBuf::from("."))
+}

src/commands/eval/runner/execute/result.rs

@@ -0,0 +1,76 @@
+use crate::core::eval_benchmarks::FixtureResult as BenchmarkFixtureResult;
+
+use super::super::super::{EvalExpectations, EvalFixtureResult};
+use super::super::matching::FixtureMatchSummary;
+use super::loading::PreparedFixtureExecution;
+
+pub(super) fn append_total_comment_failures(
+    failures: &mut Vec<String>,
+    total_comments: usize,
+    expectations: &EvalExpectations,
+) {
+    if let Some(min_total) = expectations.min_total {
+        if total_comments < min_total {
+            failures.push(format!(
+                "Expected at least {} comments, got {}",
+                min_total, total_comments
+            ));
+        }
+    }
+    if let Some(max_total) = expectations.max_total {
+        if total_comments > max_total {
+            failures.push(format!(
+                "Expected at most {} comments, got {}",
+                max_total, total_comments
+            ));
+        }
+    }
+}
+
+pub(super) fn build_benchmark_metrics(
+    prepared: &PreparedFixtureExecution,
+    total_comments: usize,
+    match_summary: &FixtureMatchSummary,
+    failures: &[String],
+) -> Option<BenchmarkFixtureResult> {
+    prepared.suite_name.as_ref().map(|_| {
+        let accounted_for = match_summary
+            .used_comment_indices
+            .union(&match_summary.unexpected_comment_indices)
+            .count();
+        let extra_findings = total_comments.saturating_sub(accounted_for);
+        let mut result = BenchmarkFixtureResult::compute(
+            &prepared.fixture_name,
+            prepared.fixture.expect.must_find.len(),
+            prepared.fixture.expect.must_not_find.len(),
+            match_summary.required_matches,
+            match_summary.unexpected_comment_indices.len(),
+            extra_findings,
+        );
+        result.details = failures.to_vec();
+        result
+    })
+}
+
+pub(super) fn build_fixture_result(
+    prepared: PreparedFixtureExecution,
+    total_comments: usize,
+    match_summary: FixtureMatchSummary,
+    benchmark_metrics: Option<BenchmarkFixtureResult>,
+    failures: Vec<String>,
+) -> EvalFixtureResult {
+    EvalFixtureResult {
+        fixture: prepared.fixture_name,
+        suite: prepared.suite_name,
+        passed: failures.is_empty(),
+        total_comments,
+        required_matches: match_summary.required_matches,
+        required_total: match_summary.required_total,
+        benchmark_metrics,
+        suite_thresholds: prepared.suite_thresholds,
+        difficulty: prepared.difficulty,
+        rule_metrics: match_summary.rule_metrics,
+        rule_summary: match_summary.rule_summary,
+        failures,
+    }
+}