From c7410eafa90c35c63ef0d097203592e1f1336901 Mon Sep 17 00:00:00 2001 From: Dylan Frankland <2488926+dfrankland@users.noreply.github.com> Date: Thu, 29 Oct 2026 18:10:39 +0000 Subject: [PATCH 1/2] feat(cli): print the summary on stdout and remove EndOutput run() returns a Report, and main sends each part of it to its own stream: the summary goes to stdout with superconsole's emit_aux, while notices (deprecation warnings, BEP parse errors), the error of a run that fails outright, the test command header and the summary file message go to stderr. DisplayMessage::Final and the EndOutput trait are replaced by Report and Notice variants that carry the rendered lines. Scripts that capture only stderr will lose the summary. Co-Authored-By: Claude Opus 5.5 --- cli/src/main.rs | 136 +++++++++++++++++---------------- cli/src/summary_output/text.rs | 66 +++++----------- cli/src/test_command.rs | 32 +++----- cli/tests/summary/validate.rs | 41 ++++++---- cli/tests/test.rs | 26 ++++++- cli/tests/upload.rs | 73 ++++++++++++------ cli/tests/validate.rs | 54 ++++++++----- display/src/end_output.rs | 37 --------- display/src/lib.rs | 1 - display/src/message.rs | 49 ++++++++++-- 10 files changed, 277 insertions(+), 238 deletions(-) delete mode 100644 display/src/end_output.rs diff --git a/cli/src/main.rs b/cli/src/main.rs index 9e3cc0de..7040519f 100644 --- a/cli/src/main.rs +++ b/cli/src/main.rs @@ -3,7 +3,7 @@ mod shutdown; use std::{ env, path::{Path, PathBuf}, - sync::{Arc, mpsc::Sender}, + sync::mpsc::Sender, thread::JoinHandle, }; @@ -14,19 +14,19 @@ use display::{ render::spin_up_renderer, }; use sentry::ClientInitGuard; +use superconsole::Line; use tracing_subscriber::{filter::FilterFn, prelude::*}; use trunk_analytics_cli::{ context::gather_debug_props, error_report::ErrorReport, exit_code::ExitCode, summary_output::{ - ErrorSummary, UploadCommand, UploadReport, ValidateReport, github, - json::{self, Report}, - text::{SummaryFileText, TextOptions, UploadText}, + ErrorSummary, UploadCommand, UploadReport, ValidateReport, github, json, + text::{self, SummaryFileText, TextOptions}, }, test_command::{TestArgs, run_test}, - upload_command::{RunUploadOptions, UploadArgs, UploadRunResult, run_upload}, - validate_command::{ValidateArgs, ValidateRunResult, run_validate}, + upload_command::{RunUploadOptions, UploadArgs, run_upload}, + validate_command::{ValidateArgs, run_validate}, }; #[derive(Debug, Parser)] @@ -195,75 +195,57 @@ fn main() -> anyhow::Result<()> { ); close_out_and_exit(exit_code, guard, render_sender, render_handle) } - MainOutcome::Finished(Ok(RunResult::Upload(run_result))) => { - let text = Arc::new(UploadText { - report: UploadReport::new(&run_result, UploadCommand::Upload), - options: TextOptions::from(&run_result), - }); - send_message( - DisplayMessage::Final(text.clone(), String::from("upload display")), + MainOutcome::Finished(Ok(Report::Upload(report, options))) => { + show( + DisplayMessage::Notice, + text::render_notices(&report.notices), + "notices", &render_sender, ); - write_summary_file( - summary_output_file.as_deref(), - Report::Upload(&text.report), - &render_sender, - ); - close_out_and_exit( - text.report.cli_exit_code(), - guard, - render_sender, - render_handle, - ) - } - MainOutcome::Finished(Ok(RunResult::Test(run_result))) => { - let text = Arc::new(UploadText { - report: UploadReport::new(&run_result, UploadCommand::Test), - options: TextOptions::from(&run_result), - }); - send_message( - DisplayMessage::Final(text.clone(), String::from("test display")), + show( + DisplayMessage::Report, + text::render_upload(&report, &options), + "upload summary", &render_sender, ); write_summary_file( summary_output_file.as_deref(), - Report::Upload(&text.report), + json::Report::Upload(&report), &render_sender, ); - close_out_and_exit( - text.report.cli_exit_code(), - guard, - render_sender, - render_handle, - ) + close_out_and_exit(report.cli_exit_code(), guard, render_sender, render_handle) } - MainOutcome::Finished(Ok(RunResult::Validate(run_result))) => { - let report = ValidateReport::new(&run_result); - let exit_code = report.exit_code; - send_message( - DisplayMessage::Final(Arc::new(report), String::from("validate display")), + MainOutcome::Finished(Ok(Report::Validate(report))) => { + show( + DisplayMessage::Notice, + text::render_notices(&report.notices), + "notices", &render_sender, ); - close_out_and_exit(exit_code, guard, render_sender, render_handle) + show( + DisplayMessage::Report, + text::render_validate(&report), + "validate summary", + &render_sender, + ); + close_out_and_exit(report.exit_code, guard, render_sender, render_handle) } - MainOutcome::Finished(Ok(RunResult::SummaryOutputSchema)) => { + MainOutcome::Finished(Ok(Report::SummaryOutputSchema)) => { println!("{}", serde_json::to_string_pretty(&json::Report::schema())?); close_out_and_exit(ExitCode::Success, guard, render_sender, render_handle) } MainOutcome::Finished(Err(error)) => { - let error = Arc::new(ErrorSummary::new(&ErrorReport::new( - error, - org_url_slug, - None, - ))); + let error = ErrorSummary::new(&ErrorReport::new(error, org_url_slug, None)); let exit_code = error.exit_code.unwrap_or(ExitCode::Success); - send_message( - DisplayMessage::Final(error.clone(), String::from("error display")), + show( + DisplayMessage::Notice, + text::render_error(&error), + "error", &render_sender, ); write_summary_file( summary_output_file.as_deref(), - Report::Error(&error), + json::Report::Error(&error), &render_sender, ); close_out_and_exit(exit_code, guard, render_sender, render_handle) @@ -272,13 +254,29 @@ fn main() -> anyhow::Result<()> { }) } -fn write_summary_file(path: Option<&Path>, report: Report, render_sender: &Sender) { +fn show( + message: fn(Vec, String) -> DisplayMessage, + lines: anyhow::Result>, + description: &str, + render_sender: &Sender, +) { + match lines { + Ok(lines) => send_message(message(lines, String::from(description)), render_sender), + Err(error) => tracing::error!("Failed to render {description}: {error:?}"), + } +} + +fn write_summary_file( + path: Option<&Path>, + report: json::Report, + render_sender: &Sender, +) { let Some(path) = path else { return; }; let text = SummaryFileText::new(path, report.write_file(path)); send_message( - DisplayMessage::Final(Arc::new(text), String::from("summary file display")), + DisplayMessage::Notice(text.render(), String::from("summary file display")), render_sender, ); } @@ -286,19 +284,19 @@ fn write_summary_file(path: Option<&Path>, report: Report, render_sender: &Sende /// Outcome of racing the command against termination signals in `main`. enum MainOutcome { /// The command ran to completion (successfully or with an error). - Finished(anyhow::Result), + Finished(anyhow::Result), /// A termination signal was received first; the payload is the exit code to use. Interrupted(ExitCode), } -enum RunResult { - Upload(UploadRunResult), - Test(UploadRunResult), - Validate(ValidateRunResult), +/// What a command found, to show as its summary and take the exit code from. +enum Report { + Upload(Box, TextOptions), + Validate(ValidateReport), SummaryOutputSchema, } -async fn run(cli: Cli, render_sender: Sender) -> anyhow::Result { +async fn run(cli: Cli, render_sender: Sender) -> anyhow::Result { tracing::info!( "Starting trunk-analytics-cli {} (git={}) rustc={}", env!("CARGO_PKG_VERSION"), @@ -315,17 +313,23 @@ async fn run(cli: Cli, render_sender: Sender) -> anyhow::Result< }, ) .await?; - Ok(RunResult::Upload(result)) + Ok(Report::Upload( + Box::new(UploadReport::new(&result, UploadCommand::Upload)), + TextOptions::from(&result), + )) } Commands::Test(test_args) => { let result = run_test(test_args, render_sender).await?; - Ok(RunResult::Test(result)) + Ok(Report::Upload( + Box::new(UploadReport::new(&result, UploadCommand::Test)), + TextOptions::from(&result), + )) } Commands::Validate(validate_args) => { let result = run_validate(validate_args).await?; - Ok(RunResult::Validate(result)) + Ok(Report::Validate(ValidateReport::new(&result))) } - Commands::SummaryOutputSchema => Ok(RunResult::SummaryOutputSchema), + Commands::SummaryOutputSchema => Ok(Report::SummaryOutputSchema), Commands::GithubAnnotations(..) => { anyhow::bail!("github-annotations runs before the async runtime starts") } diff --git a/cli/src/summary_output/text.rs b/cli/src/summary_output/text.rs index b3795556..1fabb3f6 100644 --- a/cli/src/summary_output/text.rs +++ b/cli/src/summary_output/text.rs @@ -1,6 +1,5 @@ use std::path::{Path, PathBuf}; -use display::end_output::EndOutput; use pluralizer::pluralize; use superconsole::{ Hyperlink, Line, Lines, Span, SuperConsole, @@ -8,7 +7,7 @@ use superconsole::{ }; use super::{ - ErrorSummary, FailureGroup, FileValidation, GroupKey, NoticeKind, Outcome, ParseError, + ErrorSummary, FailureGroup, FileValidation, GroupKey, Notice, NoticeKind, Outcome, ParseError, Quarantining, TestCounts, UploadReport, ValidateReport, ValidationCounts, }; use crate::{ @@ -16,7 +15,6 @@ use crate::{ exit_code::ExitCode, report_limiting::ValidationReport, upload_command::{UploadRunResult, get_bundle_upload_id_message}, - validate_command::ValidateRunResult, }; /// Flags that change how much of the summary to show, not what it says. @@ -38,23 +36,6 @@ impl From<&UploadRunResult> for TextOptions { } } -pub struct UploadText { - pub report: UploadReport, - pub options: TextOptions, -} - -impl EndOutput for UploadText { - fn output(&self) -> anyhow::Result> { - render_upload(&self.report, &self.options) - } -} - -impl EndOutput for ErrorSummary { - fn output(&self) -> anyhow::Result> { - render_error(self) - } -} - pub struct SummaryFileText { path: PathBuf, error: Option, @@ -83,14 +64,12 @@ impl SummaryFileText { Some(error) => format!("⚠️ Failed to write the summary to {path}: {error}"), } } -} -impl EndOutput for SummaryFileText { - fn output(&self) -> anyhow::Result> { - Ok(vec![ + pub fn render(&self) -> Vec { + vec![ Line::default(), Line::from_iter([Span::new_unstyled_lossy(self.message())]), - ]) + ] } } @@ -567,35 +546,28 @@ fn url_span(url: String, attribute: Attribute, clickable: bool) -> anyhow::Resul }) } -impl EndOutput for ValidateReport { - fn output(&self) -> anyhow::Result> { - render_validate(self) +/// Shown on stderr, apart from the summary. +pub fn render_notices(notices: &[Notice]) -> anyhow::Result> { + let mut output: Vec = Vec::new(); + for notice in notices { + let prefix = match notice.kind { + NoticeKind::Deprecation => "⚠️ ", + NoticeKind::BepParseError => "⚠️ BEP file had a parse error: ", + }; + output.push(Line::from_iter([ + Span::new_unstyled(prefix)?, + Span::new_unstyled_lossy(¬ice.message), + ])); } -} - -impl EndOutput for ValidateRunResult { - fn output(&self) -> anyhow::Result> { - render_validate(&ValidateReport::new(self)) + if !output.is_empty() { + output.push(Line::default()); } + Ok(output) } pub fn render_validate(report: &ValidateReport) -> anyhow::Result> { let mut output: Vec = Vec::new(); - let bep_parse_errors: Vec<&String> = report - .notices - .iter() - .filter(|notice| notice.kind == NoticeKind::BepParseError) - .map(|notice| ¬ice.message) - .collect(); - if !bep_parse_errors.is_empty() { - output.push(Line::from_iter([ - Span::new_unstyled("⚠️ BEP file had parse errors: ")?, - Span::new_unstyled_lossy(format!("{:?}", bep_parse_errors)), - ])); - output.push(Line::default()); - } - let num_files: usize = report .matched_files .iter() diff --git a/cli/src/test_command.rs b/cli/src/test_command.rs index 9339e09d..7d7d8ef3 100644 --- a/cli/src/test_command.rs +++ b/cli/src/test_command.rs @@ -2,15 +2,12 @@ use std::{ env, path::PathBuf, process::{Command, Stdio}, - sync::{Arc, mpsc::Sender}, + sync::mpsc::Sender, time::SystemTime, }; use clap::Args; -use display::{ - end_output::EndOutput, - message::{DisplayMessage, send_message}, -}; +use display::message::{DisplayMessage, send_message}; use superconsole::{ Line, Span, style::{Attribute, Stylize}, @@ -23,22 +20,6 @@ use crate::{ upload_command::{RunUploadOptions, UploadArgs, UploadRunResult, run_upload}, }; -enum RunOutput { - Title, -} -impl EndOutput for RunOutput { - fn output(&self) -> anyhow::Result> { - match self { - RunOutput::Title => Ok(vec![ - Line::from_iter([Span::new_styled( - String::from("📒 Test command outputs").attribute(Attribute::Bold), - )?]), - Line::default(), - ]), - } - } -} - #[derive(Args, Clone, Debug)] pub struct TestArgs { #[command(flatten)] @@ -128,9 +109,14 @@ pub async fn run_test_command>( render_sender: Sender, ) -> anyhow::Result { let exec_start = SystemTime::now(); - let title_ptr = Arc::new(RunOutput::Title); + let title = vec![ + Line::from_iter([Span::new_styled( + String::from("📒 Test command outputs").attribute(Attribute::Bold), + )?]), + Line::default(), + ]; send_message( - DisplayMessage::Final(title_ptr, String::from("test command title")), + DisplayMessage::Notice(title, String::from("test command title")), &render_sender, ); let child = Command::new(command.first().map(|s| s.as_ref()).unwrap_or_default()) diff --git a/cli/tests/summary/validate.rs b/cli/tests/summary/validate.rs index f5ff3cbb..aab17ee3 100644 --- a/cli/tests/summary/validate.rs +++ b/cli/tests/summary/validate.rs @@ -1,8 +1,10 @@ use clap::Parser; -use display::end_output::EndOutput; use pretty_assertions::assert_eq; use superconsole::Line; -use trunk_analytics_cli::validate_command::{ValidateArgs, run_validate}; +use trunk_analytics_cli::{ + summary_output::{ValidateReport, text}, + validate_command::{ValidateArgs, run_validate}, +}; #[derive(Parser)] struct Validate { @@ -10,12 +12,13 @@ struct Validate { args: ValidateArgs, } -async fn validate_summary(args: &[&str]) -> Vec { +async fn validate_report(args: &[&str]) -> ValidateReport { let Validate { args } = Validate::parse_from(["validate"].iter().chain(args)); - run_validate(args) - .await - .unwrap() - .output() + ValidateReport::new(&run_validate(args).await.unwrap()) +} + +fn unstyled(lines: anyhow::Result>) -> Vec { + lines .unwrap() .iter() .map(Line::to_unstyled) @@ -24,6 +27,10 @@ async fn validate_summary(args: &[&str]) -> Vec { .collect() } +async fn validate_summary(args: &[&str]) -> Vec { + unstyled(text::render_validate(&validate_report(args).await)) +} + #[tokio::test] async fn prints_validation_errors_warnings_and_codeowners() { assert_eq!( @@ -79,16 +86,22 @@ async fn prints_validation_errors_warnings_and_codeowners() { } #[tokio::test] -async fn prints_bep_parse_errors_without_a_codeowners_file() { +async fn prints_bep_parse_errors_as_notices_without_a_codeowners_file() { + let report = validate_report(&[ + "--bazel-bep-path", + "test_fixtures/summary/bep/bep_with_parse_error", + ]) + .await; assert_eq!( - validate_summary(&[ - "--bazel-bep-path", - "test_fixtures/summary/bep/bep_with_parse_error" - ]) - .await, + unstyled(text::render_notices(&report.notices)), [ - "⚠️ BEP file had parse errors: [\"Error parsing build event: EOF while parsing a value at line 3 column 0\"]", + "⚠️ BEP file had a parse error: Error parsing build event: EOF while parsing a value at line 3 column 0", "", + ] + ); + assert_eq!( + unstyled(text::render_validate(&report)), + [ "📚 Validating 1 files:", " Files matching: test_fixtures/summary/bep/test.xml:", " test_fixtures/summary/bep/test.xml", diff --git a/cli/tests/test.rs b/cli/tests/test.rs index 0a02628e..5f01847b 100644 --- a/cli/tests/test.rs +++ b/cli/tests/test.rs @@ -153,10 +153,10 @@ async fn test_command_fails_with_no_junit_files_no_quarantine_successful_upload( .assert() .failure() .code(128) - .stderr(predicate::str::contains( + .stdout(predicate::str::contains( "No tests were found in the provided test results, but non zero exit code provided: ", )) - .stderr(predicate::str::contains( + .stdout(predicate::str::contains( "The test step appears to have failed without running tests", )); @@ -614,3 +614,25 @@ async fn test_command_fails_when_the_command_cannot_be_started() { assert!(state.requests.lock().unwrap().is_empty()); } + +#[tokio::test(flavor = "multi_thread")] +async fn test_command_writes_summary_to_stdout_and_its_header_to_stderr() { + let temp_dir = tempdir().unwrap(); + generate_mock_git_repo(&temp_dir); + generate_mock_valid_junit_xmls(&temp_dir); + + let state = MockServerBuilder::new().spawn_mock_server().await; + + CommandBuilder::test( + temp_dir.path(), + state.host.clone(), + vec![String::from("true")], + ) + .command() + .assert() + .success() + .stdout(predicate::str::contains("📚 Test Report")) + .stderr(predicate::str::contains("📚 Test Report").not()) + .stderr(predicate::str::contains("📒 Test command outputs")) + .stdout(predicate::str::contains("📒 Test command outputs").not()); +} diff --git a/cli/tests/upload.rs b/cli/tests/upload.rs index 15c9b83c..4523858a 100644 --- a/cli/tests/upload.rs +++ b/cli/tests/upload.rs @@ -310,7 +310,7 @@ async fn upload_bundle() { println!("{assert}"); // A collection id was passed, so the upload surfaces as a link rather than the bare id // (the bare-id form is covered by the links-hidden test below). - assert.stderr(predicate::str::contains( + assert.stdout(predicate::str::contains( "/test-org/flaky-tests/collections/tc_123/uploads/eyJpZCI6IjgyYzZhNmU1LWY4ZWEtNGQ5My05YTI2LWI4YWI2ZmY4ZjZiYyIsImNyZWF0ZWRBdCI6MTc3ODQxNjQ5NjAwMH0", )); } @@ -332,11 +332,11 @@ async fn upload_bundle_prints_test_collection_links() { .failure(); assert - .stderr(predicate::str::contains( + .stdout(predicate::str::contains( "/test-org/flaky-tests/collections/tc_123/tests/", )) - .stderr(predicate::str::contains("/collections/tc_123/t/").not()) - .stderr(predicate::str::contains("?repo=trunk-io%2Fanalytics-cli").not()); + .stdout(predicate::str::contains("/collections/tc_123/t/").not()) + .stdout(predicate::str::contains("?repo=trunk-io%2Fanalytics-cli").not()); } // NOTE: must be multi threaded to start a mock server @@ -376,11 +376,11 @@ async fn upload_bundle_falls_back_to_short_links_without_collection_ids() { .failure(); assert - .stderr(predicate::str::contains( + .stdout(predicate::str::contains( "/test-org/flaky-tests/collections/tc_123/t/", )) - .stderr(predicate::str::contains("?repo=trunk-io%2Fanalytics-cli")) - .stderr(predicate::str::contains("/collections/tc_123/tests/").not()); + .stdout(predicate::str::contains("?repo=trunk-io%2Fanalytics-cli")) + .stdout(predicate::str::contains("/collections/tc_123/tests/").not()); } // NOTE: must be multi threaded to start a mock server @@ -402,10 +402,10 @@ async fn upload_bundle_prints_test_collection_upload_link() { // The canonical form: a base64url `{id, createdAt}` key from both halves the server // returned, not the bundle upload id. Needs no ?repo= — the short id scopes the upload. assert - .stderr(predicate::str::contains( + .stdout(predicate::str::contains( "/test-org/flaky-tests/collections/tc_123/uploads/eyJpZCI6IjgyYzZhNmU1LWY4ZWEtNGQ5My05YTI2LWI4YWI2ZmY4ZjZiYyIsImNyZWF0ZWRBdCI6MTc3ODQxNjQ5NjAwMH0", )) - .stderr(predicate::str::contains("Bundle Upload ID:").not()); + .stdout(predicate::str::contains("Bundle Upload ID:").not()); } // NOTE: must be multi threaded to start a mock server @@ -426,10 +426,10 @@ async fn upload_bundle_prints_bare_upload_id_when_links_hidden() { .failure(); assert - .stderr(predicate::str::contains( + .stdout(predicate::str::contains( get_bundle_upload_id_message("test-bundle-upload-id").as_str(), )) - .stderr(predicate::str::contains("/flaky-tests/collections/tc_123/uploads/").not()); + .stdout(predicate::str::contains("/flaky-tests/collections/tc_123/uploads/").not()); } // NOTE: must be multi threaded to start a mock server @@ -450,8 +450,8 @@ async fn upload_bundle_hides_test_collection_links_when_env_set() { .failure(); assert - .stderr(predicate::str::contains("/test-org/flaky-tests/test/")) - .stderr(predicate::str::contains("/flaky-tests/collections/").not()); + .stdout(predicate::str::contains("/test-org/flaky-tests/test/")) + .stdout(predicate::str::contains("/flaky-tests/collections/").not()); } // NOTE: must be multi threaded to start a mock server @@ -472,8 +472,8 @@ async fn upload_bundle_without_test_collection_id_prints_repo_links() { .failure(); assert - .stderr(predicate::str::contains("/test-org/flaky-tests/test/")) - .stderr(predicate::str::contains("/flaky-tests/collections/").not()); + .stdout(predicate::str::contains("/test-org/flaky-tests/test/")) + .stdout(predicate::str::contains("/flaky-tests/collections/").not()); } // NOTE: must be multi threaded to start a mock server @@ -876,7 +876,7 @@ async fn upload_bundle_using_xcresult(#[case] extra_args: &[&str]) { .command() .assert() .success() - .stderr(predicate::str::contains("1 file found, 1 with issues").not()); + .stdout(predicate::str::contains("1 file found, 1 with issues").not()); let requests = state.requests.lock().unwrap().clone(); assert_eq!(requests.len(), 3); @@ -1523,9 +1523,12 @@ async fn upload_bundle_invalid_repo_root() { .command() .assert() .failure() + .stderr(predicate::str::contains("Error Encountered")) .stderr(predicate::str::contains( "\"../\" does not appear to be a git repository", - )); + )) + // A run that fails outright has no summary + .stdout(predicate::str::is_empty()); let requests = state.requests.lock().unwrap().clone(); assert_eq!(requests.len(), 0); @@ -1758,10 +1761,10 @@ async fn upload_bundle_with_no_junit_files_no_quarantine_successful_upload() { .assert() .code(0) .success() - .stderr(predicate::str::contains( + .stdout(predicate::str::contains( "No tests were found in the provided test results", )) - .stderr(predicate::str::contains("without running tests").not()); + .stdout(predicate::str::contains("without running tests").not()); // HINT: View CLI output with `cargo test -- --nocapture` println!("{assert}"); @@ -1976,7 +1979,7 @@ async fn is_not_ok_on_bad_request() { command .assert() .failure() - .stderr(predicate::str::contains("error")); + .stdout(predicate::str::contains("error")); } #[tokio::test(flavor = "multi_thread")] @@ -2765,7 +2768,7 @@ async fn presents_error_message_if_you_dont_have_permission_for_s3_upload() { .command() .assert() .failure() - .stderr(predicate::str::contains("Error Encountered")); + .stdout(predicate::str::contains("Error Encountered")); // HINT: View CLI output with `cargo test -- --nocapture` println!("{assert}"); @@ -2807,7 +2810,7 @@ async fn presents_error_message_if_you_dont_have_permission_for_upload_endpoint( .command() .assert() .failure() - .stderr(predicate::str::contains("Error Encountered")); + .stdout(predicate::str::contains("Error Encountered")); // HINT: View CLI output with `cargo test -- --nocapture` println!("{assert}"); @@ -2895,7 +2898,7 @@ async fn does_not_print_exit_code_with_validation_reports_none() { .command() .assert() .failure() - .stderr(predicate::str::contains("File Validation").not()); + .stdout(predicate::str::contains("File Validation").not()); println!("{assert}"); } @@ -2919,7 +2922,7 @@ async fn reports_failures_even_if_cannot_get_quarantine_context() { let mut command = CommandBuilder::upload(temp_dir.path(), state.host.clone()).command(); - let assert = command.assert().failure().stderr(predicate::str::contains( + let assert = command.assert().failure().stdout(predicate::str::contains( "We were unable to determine the quarantine status for tests.", )); @@ -2963,7 +2966,7 @@ async fn reports_failing_tests_but_succeeds_when_quarantine_disabled() { let assert = command .assert() .success() - .stderr(predicate::str::contains("Fail: 0").not()); + .stdout(predicate::str::contains("Fail: 0").not()); println!("{assert}"); } @@ -3915,3 +3918,23 @@ async fn upload_quietest_verbosity_logs_nothing() { .stdout(predicate::str::contains(DEBUG_LOG_MARKER).not()) .stderr(predicate::str::contains(DEBUG_LOG_MARKER).not()); } + +#[tokio::test(flavor = "multi_thread")] +async fn upload_writes_summary_to_stdout_and_notices_to_stderr() { + let temp_dir = tempdir().unwrap(); + generate_mock_git_repo(&temp_dir); + generate_mock_valid_junit_xmls(&temp_dir); + + let state = MockServerBuilder::new().spawn_mock_server().await; + + CommandBuilder::upload(temp_dir.path(), state.host.clone()) + .extra_args(&["--hide-banner"]) + .command() + .assert() + .stdout(predicate::str::contains("📚 Test Report")) + .stderr(predicate::str::contains("📚 Test Report").not()) + .stderr(predicate::str::contains( + "The --hide-banner flag is deprecated", + )) + .stdout(predicate::str::contains("deprecated").not()); +} diff --git a/cli/tests/validate.rs b/cli/tests/validate.rs index 82f9f2e3..8527c991 100644 --- a/cli/tests/validate.rs +++ b/cli/tests/validate.rs @@ -25,14 +25,14 @@ fn validate_success() { .command() .assert() .success() - .stderr(predicate::str::contains( + .stdout(predicate::str::contains( Line::from_iter([ Span::new_styled(style(String::from("0")).with(Color::Red)).unwrap(), Span::new_unstyled(String::from(" errors")).unwrap(), ]) .render(), )) - .stderr(predicate::str::contains( + .stdout(predicate::str::contains( Line::from_iter([ Span::new_styled(style(String::from("0")).with(Color::Green)).unwrap(), Span::new_unstyled(String::from(" valid files, ")).unwrap(), @@ -43,8 +43,8 @@ fn validate_success() { ]) .render(), )) - .stderr(predicate::str::contains("Checking for codeowners file...")) - .stderr(predicate::str::contains("Found codeowners path:")); + .stdout(predicate::str::contains("Checking for codeowners file...")) + .stdout(predicate::str::contains("Found codeowners path:")); println!("{assert}"); } @@ -105,7 +105,7 @@ fn validate_invalid_junits_no_codeowners() { .command() .assert() .failure() - .stderr(predicate::str::contains( + .stdout(predicate::str::contains( Line::from_iter([ Span::new_styled(style(String::from("0")).with(Color::Yellow)).unwrap(), Span::new_unstyled(String::from(" warnings, and ")).unwrap(), @@ -114,9 +114,9 @@ fn validate_invalid_junits_no_codeowners() { ]) .render(), )) - .stderr(predicate::str::contains("test suite names are missing")) - .stderr(predicate::str::contains("Checking for codeowners file...")) - .stderr(predicate::str::contains("No codeowners file found")); + .stdout(predicate::str::contains("test suite names are missing")) + .stdout(predicate::str::contains("Checking for codeowners file...")) + .stdout(predicate::str::contains("No codeowners file found")); println!("{assert}"); } @@ -131,7 +131,7 @@ fn validate_empty_xml() { .command() .assert() .success() - .stderr(predicate::str::contains( + .stdout(predicate::str::contains( Line::from_iter([ Span::new_styled(style(String::from("1")).with(Color::Yellow)).unwrap(), Span::new_unstyled(String::from(" warning, and ")).unwrap(), @@ -140,7 +140,7 @@ fn validate_empty_xml() { ]) .render(), )) - .stderr(predicate::str::contains("no reports found")); + .stdout(predicate::str::contains("no reports found")); println!("{assert}"); } @@ -155,7 +155,7 @@ fn validate_invalid_xml() { .command() .assert() .failure() - .stderr(predicate::str::contains( + .stdout(predicate::str::contains( Line::from_iter([ Span::new_styled(style(String::from("0")).with(Color::Yellow)).unwrap(), Span::new_unstyled(String::from(" warnings, and ")).unwrap(), @@ -164,7 +164,7 @@ fn validate_invalid_xml() { ]) .render(), )) - .stderr(predicate::str::contains("syntax error: tag not closed")); + .stdout(predicate::str::contains("syntax error: tag not closed")); println!("{assert}"); } @@ -178,7 +178,7 @@ fn validate_suboptimal_junits() { .command() .assert() .success() - .stderr(predicate::str::contains( + .stdout(predicate::str::contains( Line::from_iter([ Span::new_styled(style(String::from("1")).with(Color::Yellow)).unwrap(), Span::new_unstyled(String::from(" warning, and ")).unwrap(), @@ -187,7 +187,7 @@ fn validate_suboptimal_junits() { ]) .render(), )) - .stderr(predicate::str::contains( + .stdout(predicate::str::contains( "report has stale (> 1 hour(s)) timestamps", )); @@ -204,7 +204,7 @@ fn validate_missing_filepath_suboptimal_junits() { .command() .assert() .success() - .stderr(predicate::str::contains( + .stdout(predicate::str::contains( Line::from_iter([ Span::new_styled(style(String::from("2")).with(Color::Yellow)).unwrap(), Span::new_unstyled(String::from(" warnings, and ")).unwrap(), @@ -212,12 +212,32 @@ fn validate_missing_filepath_suboptimal_junits() { Span::new_unstyled(String::from(" errors")).unwrap(), ]).render(), )) - .stderr(predicate::str::contains( + .stdout(predicate::str::contains( "report has test cases with missing file or filepath", )) - .stderr(predicate::str::contains( + .stdout(predicate::str::contains( "CODEOWNERS found but test cases are missing filepaths. We will not be able to correlate flaky tests with owners.", )); println!("{assert}"); } + +#[test] +fn validate_writes_summary_to_stdout_and_notices_to_stderr() { + let temp_dir = tempdir().unwrap(); + generate_mock_valid_junit_xmls(&temp_dir); + + let assert = CommandBuilder::validate(temp_dir.path()) + .extra_args(&["--hide-banner"]) + .command() + .assert() + .success() + .stdout(predicate::str::contains("Checking for codeowners file...")) + .stderr(predicate::str::contains("Checking for codeowners file...").not()) + .stderr(predicate::str::contains( + "The --hide-banner flag is deprecated", + )) + .stdout(predicate::str::contains("deprecated").not()); + + println!("{assert}"); +} diff --git a/display/src/end_output.rs b/display/src/end_output.rs deleted file mode 100644 index cedcbe1e..00000000 --- a/display/src/end_output.rs +++ /dev/null @@ -1,37 +0,0 @@ -use std::convert::Infallible; - -use anyhow::Result; -use superconsole::{Component, Dimensions, DrawMode, Line, Lines, SuperConsole}; - -pub trait EndOutput { - fn output(&self) -> Result>; -} - -enum EmptyComponent { - Base, -} -impl Component for EmptyComponent { - type Error = Infallible; - fn draw_unchecked( - &self, - _dimensions: Dimensions, - _mode: DrawMode, - ) -> Result { - Ok(Lines(vec![])) - } -} - -// Clears out any renders, and outputs the entire output of result, scrolling the terminal and wrapping long lines (you know, normal terminal output) -pub fn display_end( - superconsole: &mut SuperConsole, - result: &(impl EndOutput + ?Sized), -) -> Result<()> { - let lines = result.output()?; - // Superconsole truncates line emission if you emit more lines than the console has rows in a single render loop. - // We could batch these based on tty rows, but the performance is fine without doing so, and we're not exactly running - // a reactive ui here. - for line in lines { - superconsole.emit_now(Lines(vec![line]), &EmptyComponent::Base)?; - } - Ok(()) -} diff --git a/display/src/lib.rs b/display/src/lib.rs index cf9c93ae..d4a7f6fd 100644 --- a/display/src/lib.rs +++ b/display/src/lib.rs @@ -1,3 +1,2 @@ -pub mod end_output; pub mod message; pub mod render; diff --git a/display/src/message.rs b/display/src/message.rs index 7a05a929..a6176f3e 100644 --- a/display/src/message.rs +++ b/display/src/message.rs @@ -1,13 +1,17 @@ -use std::sync::{Arc, mpsc::Sender}; +use std::{ + convert::Infallible, + sync::{Arc, mpsc::Sender}, +}; use anyhow::Result; use superconsole::{Component, Dimensions, DrawMode, Line, Lines, Span, SpanError, SuperConsole}; -use crate::end_output::{EndOutput, display_end}; - pub enum DisplayMessage { Progress(Arc, String), - Final(Arc, String), + /// The command's summary, on stdout so it can be captured apart from logs and progress. + Report(Vec, String), + /// Anything else to show the user, such as warnings or why a run failed, on stderr. + Notice(Vec, String), } impl DisplayMessage { @@ -16,7 +20,10 @@ impl DisplayMessage { DisplayMessage::Progress(progress_message, _) => superconsole .render(&**progress_message) .map_err(anyhow::Error::new), - DisplayMessage::Final(final_message, _) => display_end(superconsole, &**final_message), + DisplayMessage::Report(lines, _) => { + emit_lines(superconsole, lines, SuperConsole::emit_aux) + } + DisplayMessage::Notice(lines, _) => emit_lines(superconsole, lines, SuperConsole::emit), }; if let Err(e) = render_result { tracing::error!("Failed to render {}: {:?}", self.description(), e); @@ -26,11 +33,41 @@ impl DisplayMessage { fn description(&self) -> String { match self { DisplayMessage::Progress(_, description) => description.to_string(), - DisplayMessage::Final(_, description) => description.to_string(), + DisplayMessage::Report(_, description) => description.to_string(), + DisplayMessage::Notice(_, description) => description.to_string(), } } } +struct EmptyComponent; + +impl Component for EmptyComponent { + type Error = Infallible; + fn draw_unchecked( + &self, + _dimensions: Dimensions, + _mode: DrawMode, + ) -> Result { + Ok(Lines(vec![])) + } +} + +// Clears the progress line, and writes the lines as normal terminal output that scrolls and wraps. +fn emit_lines( + superconsole: &mut SuperConsole, + lines: &[Line], + emit: fn(&mut SuperConsole, Lines), +) -> Result<()> { + // Superconsole truncates line emission if you emit more lines than the console has rows in a single render loop. + // We could batch these based on tty rows, but the performance is fine without doing so, and we're not exactly running + // a reactive ui here. + for line in lines { + emit(superconsole, Lines(vec![line.clone()])); + superconsole.render(&EmptyComponent)?; + } + Ok(()) +} + pub fn send_message(message: DisplayMessage, sender: &Sender) { let send_result = sender.send(message); if let Err(e) = send_result { From 37da64437d1150062d3a9a596f49c6dd447e4c7a Mon Sep 17 00:00:00 2001 From: Dylan Frankland <2488926+dfrankland@users.noreply.github.com> Date: Wed, 4 Nov 2026 19:53:27 +0000 Subject: [PATCH 2/2] fix(cli): show UploadSkipped notices with the others on stderr main's #1287 printed Deprecation and UploadSkipped notices inside the upload summary. This branch already prints every notice on stderr through render_notices, so render UploadSkipped there and drop the copy in the summary, which would have printed each notice twice. Co-Authored-By: Claude Opus 5.5 --- cli/src/summary_output/text.rs | 15 +-------------- cli/tests/summary/upload.rs | 19 ++++++++++++++++--- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/cli/src/summary_output/text.rs b/cli/src/summary_output/text.rs index 1fabb3f6..6763f439 100644 --- a/cli/src/summary_output/text.rs +++ b/cli/src/summary_output/text.rs @@ -160,19 +160,6 @@ pub fn render_upload(report: &UploadReport, options: &TextOptions) -> anyhow::Re output.push(Line::default()); } - // The console shows tracing only when verbose, so these are shown here or not at all. - for notice in &report.notices { - if matches!( - notice.kind, - NoticeKind::Deprecation | NoticeKind::UploadSkipped - ) { - output.push(Line::from_iter([Span::new_styled_lossy( - style(format!("⚠️ {}", notice.message)).with(Color::Yellow), - )])); - output.push(Line::default()); - } - } - let bold = |text: String| Span::new_styled(style(text).attribute(Attribute::Bold)); let upload_line = match (&report.upload_url, &report.bundle_upload_id) { (Some(url), _) => Some(Line::from_iter([ @@ -551,7 +538,7 @@ pub fn render_notices(notices: &[Notice]) -> anyhow::Result> { let mut output: Vec = Vec::new(); for notice in notices { let prefix = match notice.kind { - NoticeKind::Deprecation => "⚠️ ", + NoticeKind::Deprecation | NoticeKind::UploadSkipped => "⚠️ ", NoticeKind::BepParseError => "⚠️ BEP file had a parse error: ", }; output.push(Line::from_iter([ diff --git a/cli/tests/summary/upload.rs b/cli/tests/summary/upload.rs index 0e4b35cf..8a84a1d2 100644 --- a/cli/tests/summary/upload.rs +++ b/cli/tests/summary/upload.rs @@ -227,20 +227,33 @@ fn prints_a_passing_run() { } #[test] -fn prints_deprecation_notices_first() { +fn prints_notices_apart_from_the_summary() { let mut result = upload_result(5); result.notices = vec![ Notice::deprecation( "Uploading to test collection zQ8nP2mW, inferred from the repository. Pass --test-collection-id zQ8nP2mW: analytics-cli 1.0.0 requires it.", ), - Notice::bep_parse_error("not shown here"), + Notice::upload_skipped("Test results were not uploaded."), + Notice::bep_parse_error("unexpected EOF"), ]; + let report = UploadReport::new(&result, UploadCommand::Upload); assert_eq!( - upload_summary(&result), + text::render_notices(&report.notices) + .unwrap() + .iter() + .map(Line::to_unstyled) + .collect::>(), [ "⚠️ Uploading to test collection zQ8nP2mW, inferred from the repository. Pass --test-collection-id zQ8nP2mW: analytics-cli 1.0.0 requires it.", + "⚠️ Test results were not uploaded.", + "⚠️ BEP file had a parse error: unexpected EOF", "", + ] + ); + assert_eq!( + upload_summary(&result), + [ "🏷️ Bundle Upload ID: 4b1d2c3e-5f60-4a7b-8c9d-0e1f2a3b4c5d", "", "📚 Test Report",