From 057575baa2140256bc8df198232abf61305e6bb1 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 8 Apr 2026 03:40:48 -0400 Subject: [PATCH] test(cli): move timeout-prone run coverage into ITs Replace bin-scoped localhost HTTP tests with command-facing integration coverage so they run under the intended IT timeout budget without changing nextest overrides. --- .../fabro-cli/src/commands/run/output.rs | 55 --------- .../fabro-cli/src/commands/run/runner.rs | 83 +------------- lib/crates/fabro-cli/src/server_client.rs | 108 ------------------ lib/crates/fabro-cli/tests/it/cmd/run.rs | 94 ++++++++++++++- lib/crates/fabro-cli/tests/it/cmd/runner.rs | 75 +++++++++++- 5 files changed, 164 insertions(+), 251 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/run/output.rs b/lib/crates/fabro-cli/src/commands/run/output.rs index 7c6666421..1372d38ac 100644 --- a/lib/crates/fabro-cli/src/commands/run/output.rs +++ b/lib/crates/fabro-cli/src/commands/run/output.rs @@ -364,58 +364,3 @@ async fn print_assets_with_client( } Ok(()) } - -#[cfg(test)] -mod tests { - use super::*; - use httpmock::MockServer; - - #[tokio::test] - async fn local_artifact_display_paths_come_from_server_artifact_list() { - let server = MockServer::start(); - let run_id: RunId = "01JT56VE4Z5NZ814GZN2JZD65A".parse().unwrap(); - let run_dir = tempfile::tempdir().unwrap(); - - server.mock(|when, then| { - when.method("GET") - .path(format!("/api/v1/runs/{run_id}/artifacts")); - then.status(200) - .header("Content-Type", "application/json") - .body( - serde_json::json!({ - "data": [ - { - "stage_id": "test@2", - "node_slug": "test", - "retry": 2, - "relative_path": "reports/junit.xml", - "size": 123 - } - ] - }) - .to_string(), - ); - }); - - let client = crate::server_client::connect_server_target_direct(&format!( - "{}/api/v1", - server.base_url() - )) - .await - .unwrap(); - - let paths = - resolve_local_artifact_display_paths_with_client(&client, &run_id, run_dir.path()) - .await - .unwrap(); - - assert_eq!( - paths, - vec![ - RunScratch::new(run_dir.path()) - .artifact_stage_dir("test", 2) - .join("reports/junit.xml") - ] - ); - } -} diff --git a/lib/crates/fabro-cli/src/commands/run/runner.rs b/lib/crates/fabro-cli/src/commands/run/runner.rs index 5321062ad..1082f3f89 100644 --- a/lib/crates/fabro-cli/src/commands/run/runner.rs +++ b/lib/crates/fabro-cli/src/commands/run/runner.rs @@ -471,13 +471,10 @@ fn install_signal_handlers( mod tests { use std::sync::Arc; - use httpmock::MockServer; - use serde_json::json; - use super::{ MissingArtifactUploadTokenUploader, WorkerTitlePhase, apply_worker_control_line, - build_artifact_uploader, execute, initial_worker_title_phase, read_worker_control_stream, - worker_title, worker_title_phase_for_event, + initial_worker_title_phase, read_worker_control_stream, worker_title, + worker_title_phase_for_event, }; use crate::args::RunWorkerMode; use fabro_interview::{AnswerValue, ControlInterviewer, Interviewer, Question, QuestionType}; @@ -581,24 +578,6 @@ mod tests { ); } - #[tokio::test] - async fn build_artifact_uploader_does_not_depend_on_run_capability_flag() { - let server = MockServer::start_async().await; - - let uploader = build_artifact_uploader( - fixtures::RUN_1, - crate::server_client::connect_server_target_direct(&format!( - "{}/api/v1", - server.base_url() - )) - .await - .unwrap(), - Some("token".to_string()), - ); - - assert!(uploader.is_some()); - } - #[tokio::test] async fn missing_artifact_upload_token_error_does_not_mention_removed_storage_mode() { let uploader = MissingArtifactUploadTokenUploader { @@ -619,64 +598,6 @@ mod tests { assert!(!error.to_string().contains("object-backed artifacts")); } - #[tokio::test] - async fn worker_bootstrap_loads_run_state_without_prefetching_run_events() { - let server = MockServer::start_async().await; - let run_id = fixtures::RUN_1; - - let state_mock = server - .mock_async(|when, then| { - when.method("GET") - .path(format!("/api/v1/runs/{run_id}/state")); - then.status(200) - .header("Content-Type", "application/json") - .body( - json!({ - "run": null, - "graph_source": null, - "start": null, - "status": null, - "checkpoint": null, - "checkpoints": [], - "conclusion": null, - "retro": null, - "retro_prompt": null, - "retro_response": null, - "sandbox": null, - "final_patch": null, - "pull_request": null, - "nodes": {} - }) - .to_string(), - ); - }) - .await; - let events_mock = server - .mock_async(|when, then| { - when.method("GET") - .path(format!("/api/v1/runs/{run_id}/events")); - then.status(200) - .header("Content-Type", "application/json") - .body(json!({ "data": [], "meta": { "has_more": false } }).to_string()); - }) - .await; - - let run_dir = tempfile::tempdir().unwrap(); - let error = execute( - run_id, - format!("{}/api/v1", server.base_url()), - None, - run_dir.path().to_path_buf(), - RunWorkerMode::Start, - ) - .await - .unwrap_err(); - - assert!(error.to_string().contains("has no run record")); - state_mock.assert_async().await; - assert_eq!(events_mock.calls_async().await, 0); - } - #[tokio::test] async fn worker_control_line_routes_answer_by_question_id() { let interviewer = Arc::new(ControlInterviewer::new()); diff --git a/lib/crates/fabro-cli/src/server_client.rs b/lib/crates/fabro-cli/src/server_client.rs index 1fe3f6549..4e8234dbd 100644 --- a/lib/crates/fabro-cli/src/server_client.rs +++ b/lib/crates/fabro-cli/src/server_client.rs @@ -871,111 +871,3 @@ fn non_zero_u64_from_u32(value: u32) -> Option { fn non_zero_u64_from_usize(value: usize) -> Option { u64::try_from(value).ok().and_then(NonZeroU64::new) } - -#[cfg(test)] -mod tests { - use httpmock::MockServer; - - use super::*; - - fn test_event(run_id: &str, seq: u32, event: &str) -> serde_json::Value { - serde_json::json!({ - "seq": seq, - "payload": { - "event": event, - "id": format!("evt-{seq}"), - "run_id": run_id, - "ts": "2026-04-05T12:00:00Z", - "properties": {} - } - }) - } - - fn test_client(base_url: &str) -> ServerStoreClient { - connect_remote_api_client_bundle(base_url, None).unwrap() - } - - #[tokio::test] - async fn list_run_events_follows_pagination_when_limit_unspecified() { - let server = MockServer::start_async().await; - let run_id: RunId = "01ARZ3NDEKTSV4RRFFQ69G5FAV".parse().unwrap(); - let run_id_str = run_id.to_string(); - - let first_page = server - .mock_async(|when, then| { - when.method("GET") - .path(format!("/api/v1/runs/{run_id_str}/events")) - .query_param_missing("since_seq") - .query_param_missing("limit"); - then.status(200) - .header("Content-Type", "application/json") - .body( - serde_json::json!({ - "data": [test_event(&run_id_str, 1, "run.running")], - "meta": { "has_more": true } - }) - .to_string(), - ); - }) - .await; - - let second_page = server - .mock_async(|when, then| { - when.method("GET") - .path(format!("/api/v1/runs/{run_id_str}/events")) - .query_param("since_seq", "2") - .query_param_missing("limit"); - then.status(200) - .header("Content-Type", "application/json") - .body( - serde_json::json!({ - "data": [test_event(&run_id_str, 2, "run.completed")], - "meta": { "has_more": false } - }) - .to_string(), - ); - }) - .await; - - let client = test_client(&server.url("/api/v1")); - let events = client.list_run_events(&run_id, None, None).await.unwrap(); - - first_page.assert_async().await; - second_page.assert_async().await; - assert_eq!(events.len(), 2); - assert_eq!(events[0].seq, 1); - assert_eq!(events[1].seq, 2); - } - - #[tokio::test] - async fn list_run_events_with_explicit_limit_keeps_single_page_behavior() { - let server = MockServer::start_async().await; - let run_id: RunId = "01ARZ3NDEKTSV4RRFFQ69G5FAV".parse().unwrap(); - let run_id_str = run_id.to_string(); - - let first_page = server - .mock_async(|when, then| { - when.method("GET") - .path(format!("/api/v1/runs/{run_id_str}/events")) - .query_param_missing("since_seq") - .query_param("limit", "1"); - then.status(200) - .header("Content-Type", "application/json") - .body( - serde_json::json!({ - "data": [test_event(&run_id_str, 1, "run.running")], - "meta": { "has_more": true } - }) - .to_string(), - ); - }) - .await; - - let client = test_client(&server.url("/api/v1")); - let events = client.list_run_events(&run_id, None, Some(1)).await.unwrap(); - - first_page.assert_async().await; - assert_eq!(events.len(), 1); - assert_eq!(events[0].seq, 1); - } -} diff --git a/lib/crates/fabro-cli/tests/it/cmd/run.rs b/lib/crates/fabro-cli/tests/it/cmd/run.rs index d250b5d6a..9da7096ec 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/run.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/run.rs @@ -92,6 +92,19 @@ fn run_completed_event(run_id: &str) -> serde_json::Value { }) } +fn run_running_event(run_id: &str, seq: u32) -> serde_json::Value { + serde_json::json!({ + "seq": seq, + "payload": { + "event": "run.running", + "id": format!("evt-run-running-{seq}"), + "run_id": run_id, + "ts": "2026-04-05T12:00:00Z", + "properties": {} + } + }) +} + #[test] fn help() { let context = test_context!(); @@ -318,7 +331,7 @@ fn detach_cli_server_target_overrides_configured_server_target() { } #[test] -fn remote_foreground_run_prints_server_backed_summary_without_local_run_dir() { +fn remote_foreground_run_consumes_paginated_events_and_prints_server_backed_summary() { let context = test_context!(); let server = MockServer::start(); let run_id = unique_run_id(); @@ -341,7 +354,7 @@ fn remote_foreground_run_prints_server_backed_summary_without_local_run_dir() { .header("Content-Type", "application/json") .body(run_status_response(run_id.as_str(), "queued").to_string()); }); - server.mock(|when, then| { + let first_page = server.mock(|when, then| { when.method("GET") .path(format!("/api/v1/runs/{run_id}/events")) .query_param_missing("since_seq"); @@ -349,13 +362,13 @@ fn remote_foreground_run_prints_server_backed_summary_without_local_run_dir() { .header("Content-Type", "application/json") .body( serde_json::json!({ - "data": [run_completed_event(run_id.as_str())], - "meta": { "has_more": false } + "data": [run_running_event(run_id.as_str(), 1)], + "meta": { "has_more": true } }) .to_string(), ); }); - server.mock(|when, then| { + let second_page = server.mock(|when, then| { when.method("GET") .path(format!("/api/v1/runs/{run_id}/events")) .query_param("since_seq", "2"); @@ -363,7 +376,7 @@ fn remote_foreground_run_prints_server_backed_summary_without_local_run_dir() { .header("Content-Type", "application/json") .body( serde_json::json!({ - "data": [], + "data": [run_completed_event(run_id.as_str())], "meta": { "has_more": false } }) .to_string(), @@ -410,6 +423,8 @@ fn remote_foreground_run_prints_server_backed_summary_without_local_run_dir() { String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr) ); + first_page.assert(); + second_page.assert(); let stderr = output_stderr(&output); assert!(stderr.contains("=== Run Result ==="), "{stderr}"); @@ -425,6 +440,73 @@ fn remote_foreground_run_prints_server_backed_summary_without_local_run_dir() { assert!(!stderr.contains("=== Artifacts ==="), "{stderr}"); } +#[test] +fn local_foreground_run_prints_artifact_paths_from_server_artifact_list() { + let context = test_context!(); + let run_id = unique_run_id(); + let workspace_dir = context.temp_dir.join("artifact-summary"); + context.write_temp( + "artifact-summary/workflow.fabro", + r#"digraph ArtifactSummary { + graph [goal="Show stored artifacts"] + start [shape=Mdiamond] + exit [shape=Msquare] + create_assets [shape=parallelogram, script="mkdir -p assets/shared && printf one > assets/shared/report.txt"] + start -> create_assets -> exit +} +"#, + ); + context.write_temp( + "artifact-summary/run.toml", + r#"version = 1 +graph = "workflow.fabro" +goal = "Show stored artifacts" + +[sandbox] +provider = "local" +preserve = true + +[sandbox.local] +worktree_mode = "never" + +[artifacts] +include = ["assets/**"] +"#, + ); + + let output = context + .run_cmd() + .current_dir(&workspace_dir) + .env("OPENAI_API_KEY", "test") + .args([ + "--run-id", + run_id.as_str(), + "--auto-approve", + "--no-retro", + "--sandbox", + "local", + "--provider", + "openai", + "run.toml", + ]) + .output() + .expect("command should execute"); + + assert!( + output.status.success(), + "command failed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + + let stderr = output_stderr(&output); + let expected_path = context + .find_run_dir(&run_id) + .join("cache/artifacts/files/create_assets/retry_1/assets/shared/report.txt"); + assert!(stderr.contains("=== Artifacts ==="), "{stderr}"); + assert!(stderr.contains(expected_path.to_string_lossy().as_ref()), "{stderr}"); +} + #[test] fn dry_run_simple() { let context = test_context!(); diff --git a/lib/crates/fabro-cli/tests/it/cmd/runner.rs b/lib/crates/fabro-cli/tests/it/cmd/runner.rs index 4763ea671..894ee01b1 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/runner.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/runner.rs @@ -1,8 +1,9 @@ use fabro_store::EventEnvelope; use fabro_test::{fabro_snapshot, test_context}; use fabro_types::{EventBody, RunEvent}; +use httpmock::MockServer; -use super::support::{run_events, run_state, server_target}; +use super::support::{output_stderr, run_events, run_state, server_target}; use crate::support::{fabro_json_snapshot, unique_run_id}; const SHARED_DAEMON_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(30); @@ -397,6 +398,78 @@ digraph Test { assert_eq!(after_summary, before_summary); } +#[test] +fn runner_reports_missing_run_record_without_prefetching_events() { + let context = test_context!(); + let server = MockServer::start(); + let run_id = unique_run_id(); + let run_dir = tempfile::tempdir().expect("temp run dir should exist"); + + let state_mock = server.mock(|when, then| { + when.method("GET") + .path(format!("/api/v1/runs/{run_id}/state")); + then.status(200) + .header("Content-Type", "application/json") + .body( + serde_json::json!({ + "run": null, + "graph_source": null, + "start": null, + "status": null, + "checkpoint": null, + "checkpoints": [], + "conclusion": null, + "retro": null, + "retro_prompt": null, + "retro_response": null, + "sandbox": null, + "final_patch": null, + "pull_request": null, + "nodes": {} + }) + .to_string(), + ); + }); + let events_mock = server.mock(|when, then| { + when.method("GET") + .path(format!("/api/v1/runs/{run_id}/events")); + then.status(200) + .header("Content-Type", "application/json") + .body(r#"{"data":[],"meta":{"has_more":false}}"#); + }); + + let output = context + .command() + .args([ + "__run-worker", + "--server", + &format!("{}/api/v1", server.base_url()), + "--run-dir", + run_dir.path().to_str().expect("run dir should be UTF-8"), + "--run-id", + &run_id, + "--mode", + "start", + ]) + .timeout(SHARED_DAEMON_TIMEOUT) + .output() + .expect("worker should execute"); + + assert!( + !output.status.success(), + "worker should fail when run record is missing:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + state_mock.assert(); + events_mock.assert_calls(0); + assert!( + output_stderr(&output).contains("has no run record in store"), + "{}", + output_stderr(&output) + ); +} + #[test] fn detached_run_answers_pending_question_without_interview_scratch_files() { let context = test_context!();