From 3a907603bf1481fc56223f88aff620f1930e5056 Mon Sep 17 00:00:00 2001 From: Dylan Frankland Date: Tue, 27 Oct 2026 21:17:23 +0000 Subject: [PATCH 1/2] feat(cli): add a hidden github-annotations command `trunk-analytics-cli github-annotations --summary-file ` reads the summary that --summary-output-file wrote and prints GitHub Actions workflow commands saying why the run failed. The analytics-uploader action calls it when the CLI exits non-zero, so the CLI words the failure and the action never reads the summary's format. - Exits 0 only when it printed an annotation. A summary with nothing to say, or one it can't read, exits 1 so the action falls back to its own message. - Runs before Sentry, logging and the renderer start, so stdout carries only the workflow commands. - Follows the text summary: an error that blocks quarantining is the only annotation; any other upload error is a warning; then the outcome. Each failure that wasn't quarantined gets its own annotation with its file, up to 9, since GitHub shows 10 errors per step. - The wording is shared with the text summary through constants in report/text.rs, including the "failed without running tests" line. To read the file back, the report types derive Deserialize, ExitCode reads a number back as its named code (or PassedThrough), and json::from_str rejects a schemaVersion newer than it knows. Refs TRUNK-19074 Co-Authored-By: Claude Opus 5.5 --- cli/src/error_report.rs | 4 +- cli/src/exit_code.rs | 22 ++- cli/src/main.rs | 42 ++++- cli/src/summary_output/error.rs | 4 +- cli/src/summary_output/github.rs | 229 +++++++++++++++++++++++++++ cli/src/summary_output/json.rs | 36 ++++- cli/src/summary_output/mod.rs | 1 + cli/src/summary_output/notice.rs | 6 +- cli/src/summary_output/text.rs | 41 +++-- cli/src/summary_output/upload.rs | 22 +-- cli/tests/summary/github.rs | 257 +++++++++++++++++++++++++++++++ cli/tests/summary/main.rs | 1 + cli/tests/summary/upload.rs | 6 +- 13 files changed, 625 insertions(+), 46 deletions(-) create mode 100644 cli/src/summary_output/github.rs create mode 100644 cli/tests/summary/github.rs diff --git a/cli/src/error_report.rs b/cli/src/error_report.rs index 547a194d..130540b5 100644 --- a/cli/src/error_report.rs +++ b/cli/src/error_report.rs @@ -1,7 +1,7 @@ use api::client::ClientErrorExplanation; use http::StatusCode; use schemars::JsonSchema; -use serde::Serialize; +use serde::{Deserialize, Serialize}; use crate::exit_code::ExitCode; @@ -94,7 +94,7 @@ impl ErrorReport { } } -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, JsonSchema)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "SCREAMING_SNAKE_CASE")] pub enum ErrorKind { /// Couldn't connect to Trunk. diff --git a/cli/src/exit_code.rs b/cli/src/exit_code.rs index 96cbca11..a8c5f9ec 100644 --- a/cli/src/exit_code.rs +++ b/cli/src/exit_code.rs @@ -6,7 +6,7 @@ use std::{borrow::Cow, fmt}; use clap::builder::{StyledStr, styling::Styles}; use schemars::{JsonSchema, Schema, SchemaGenerator, json_schema}; -use serde::{Serialize, Serializer}; +use serde::{Deserialize, Deserializer, Serialize, Serializer}; use strum::{EnumIter, EnumMessage, IntoEnumIterator}; /// Two exit codes are equal when the process exits with the same number, so @@ -131,6 +131,18 @@ impl Serialize for ExitCode { } } +/// A number that matches a named code reads back as that code, and any other as `PassedThrough`. +impl<'de> Deserialize<'de> for ExitCode { + fn deserialize>(deserializer: D) -> Result { + let code = i32::deserialize(deserializer)?; + Ok(Self::iter() + .find(|exit_code| { + !matches!(exit_code, Self::PassedThrough(_)) && exit_code.code() == code + }) + .unwrap_or(Self::PassedThrough(code))) + } +} + impl JsonSchema for ExitCode { fn schema_name() -> Cow<'static, str> { "ExitCode".into() @@ -168,6 +180,14 @@ mod tests { assert!(ExitCode::PassedThrough(0).is_success()); } + #[test] + fn deserializes_from_its_number() { + let read = |json: &str| serde_json::from_str::(json).unwrap(); + assert!(matches!(read("70"), ExitCode::Error)); + assert!(matches!(read("0"), ExitCode::Success)); + assert!(matches!(read("3"), ExitCode::PassedThrough(3))); + } + #[test] fn serializes_as_its_number() { assert_eq!(serde_json::to_string(&ExitCode::Error).unwrap(), "70"); diff --git a/cli/src/main.rs b/cli/src/main.rs index b6f99699..425ef7dc 100644 --- a/cli/src/main.rs +++ b/cli/src/main.rs @@ -7,7 +7,7 @@ use std::{ thread::JoinHandle, }; -use clap::{Parser, Subcommand}; +use clap::{Args, Parser, Subcommand}; use clap_verbosity_flag::{InfoLevel, Verbosity, log::LevelFilter}; use display::{ message::{DisplayMessage, send_message}, @@ -20,7 +20,7 @@ use trunk_analytics_cli::{ error_report::ErrorReport, exit_code::ExitCode, summary_output::{ - ErrorSummary, UploadCommand, UploadReport, ValidateReport, + ErrorSummary, UploadCommand, UploadReport, ValidateReport, github, json::{self, Report}, text::{SummaryFileText, TextOptions, UploadText}, }, @@ -47,7 +47,9 @@ impl Cli { let token = match &self.command { Commands::Test(args) => args.token(), Commands::Upload(args) => args.token.clone(), - Commands::Validate(..) | Commands::SummaryOutputSchema => None, + Commands::Validate(..) + | Commands::SummaryOutputSchema + | Commands::GithubAnnotations(..) => None, }; token.map_or( @@ -62,6 +64,7 @@ impl Cli { Commands::Upload(..) => "upload", Commands::Validate(..) => "validate", Commands::SummaryOutputSchema => "summary-output-schema", + Commands::GithubAnnotations(..) => "github-annotations", } } @@ -74,7 +77,9 @@ impl Cli { match &self.command { Commands::Test(args) => args.org_url_slug(), Commands::Upload(args) => args.org_url_slug.clone(), - Commands::Validate(..) | Commands::SummaryOutputSchema => String::from("not used"), + Commands::Validate(..) + | Commands::SummaryOutputSchema + | Commands::GithubAnnotations(..) => String::from("not used"), } } @@ -82,7 +87,9 @@ impl Cli { match &self.command { Commands::Test(args) => args.summary_output_file(), Commands::Upload(args) => args.summary_output_file.clone(), - Commands::Validate(..) | Commands::SummaryOutputSchema => None, + Commands::Validate(..) + | Commands::SummaryOutputSchema + | Commands::GithubAnnotations(..) => None, } } @@ -90,7 +97,9 @@ impl Cli { let explicit_root = match &self.command { Commands::Test(args) => args.repo_root(), Commands::Upload(args) => args.repo_root.clone(), - Commands::Validate(..) | Commands::SummaryOutputSchema => None, + Commands::Validate(..) + | Commands::SummaryOutputSchema + | Commands::GithubAnnotations(..) => None, }; explicit_root .or(std::env::current_dir() @@ -114,6 +123,17 @@ enum Commands { Validate(ValidateArgs), /// Print the JSON Schema of the summary that `--summary-output-file` writes. SummaryOutputSchema, + /// Print GitHub Actions annotations for why a run failed, from its `--summary-output-file`. + /// Exits 0 only when it printed one. Used by the analytics-uploader action. + #[command(hide = true)] + GithubAnnotations(GithubAnnotationsArgs), +} + +#[derive(Debug, Args)] +struct GithubAnnotationsArgs { + /// The summary that `upload` or `test` wrote to `--summary-output-file`. + #[arg(long)] + summary_file: PathBuf, } fn close_out_and_exit( @@ -131,6 +151,12 @@ fn close_out_and_exit( // "the Sentry client must be initialized before starting an async runtime or spawning threads" // https://docs.sentry.io/platforms/rust/#async-main-function fn main() -> anyhow::Result<()> { + let cli = Cli::parse(); + // Before Sentry and logging start, so stdout carries only the workflow commands. + if let Commands::GithubAnnotations(args) = &cli.command { + std::process::exit(github::run(&args.summary_file).code()); + } + let release_name = format!("analytics-cli@{}", env!("CARGO_PKG_VERSION")); let guard = third_party::sentry::init(release_name.into(), None); @@ -138,7 +164,6 @@ fn main() -> anyhow::Result<()> { .enable_all() .build()? .block_on(async { - let cli = Cli::parse(); let org_url_slug = cli.org_url_slug(); let summary_output_file = cli.summary_output_file(); let log_level_filter = cli.verbose.log_level_filter(); @@ -301,6 +326,9 @@ async fn run(cli: Cli, render_sender: Sender) -> anyhow::Result< Ok(RunResult::Validate(result)) } Commands::SummaryOutputSchema => Ok(RunResult::SummaryOutputSchema), + Commands::GithubAnnotations(..) => { + anyhow::bail!("github-annotations runs before the async runtime starts") + } } } diff --git a/cli/src/summary_output/error.rs b/cli/src/summary_output/error.rs index 0985de3b..2b208e09 100644 --- a/cli/src/summary_output/error.rs +++ b/cli/src/summary_output/error.rs @@ -1,13 +1,13 @@ use api::client::get_api_host; use schemars::JsonSchema; -use serde::Serialize; +use serde::{Deserialize, Serialize}; use crate::{ error_report::{ErrorKind, ErrorReport, get_client_error_explanation, settings_url}, exit_code::ExitCode, }; -#[derive(Debug, Clone, Serialize, JsonSchema)] +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "camelCase")] pub struct ErrorSummary { pub kind: ErrorKind, diff --git a/cli/src/summary_output/github.rs b/cli/src/summary_output/github.rs new file mode 100644 index 00000000..99f96fe2 --- /dev/null +++ b/cli/src/summary_output/github.rs @@ -0,0 +1,229 @@ +//! The hidden `github-annotations` command. It turns a summary file into GitHub Actions workflow +//! commands, so the analytics-uploader action can say why a run failed without reading the +//! summary's format itself. + +use std::{fmt, path::Path}; + +use pluralizer::pluralize; + +use super::{ + ErrorSummary, FailedTest, Outcome, Quarantining, UploadReport, + json::{self, Summary}, + text::{ + FAILED_WITHOUT_RUNNING_TESTS, FAILURE_MESSAGE_MAX_LINES, NO_TEST_FAILURES, NO_TESTS_FOUND, + QUARANTINE_LOOKUP_FAILED, SOME_TESTS_MAY_NOT_HAVE_RUN, UNAUTHORIZED_HINT, + }, +}; +use crate::{ + error_report::{CONNECTION_REFUSED_CONTEXT, ErrorKind, UNAUTHORIZED_CONTEXT}, + exit_code::ExitCode, +}; + +/// GitHub shows at most 10 error annotations per step, and the summary takes one of them. +const MAX_TEST_ANNOTATIONS: usize = 9; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Level { + Error, + Warning, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Annotation { + pub level: Level, + pub file: Option, + pub title: Option, + pub message: String, +} + +/// Prints the summary's annotations on stdout. The action passes them through when this exits +/// 0, and otherwise falls back to its own message, so anything else exits non-zero: a summary +/// with nothing to say, and one that can't be read. +pub fn run(summary_file: &Path) -> ExitCode { + let summary = match json::read_file(summary_file) { + Ok(summary) => summary, + Err(error) => { + eprintln!("{error:#}"); + return ExitCode::Failure; + } + }; + let annotations = annotations(&summary); + if annotations.is_empty() { + return ExitCode::Failure; + } + for annotation in annotations { + println!("{annotation}"); + } + ExitCode::Success +} + +pub fn annotations(summary: &Summary) -> Vec { + match summary { + Summary::Upload(report) => upload_annotations(report), + Summary::Error(error) => vec![Annotation::error(error_message(error))], + } +} + +/// Follows `text::render_upload`: an error that blocks quarantining is all there is to say, +/// and any other error is a warning before the outcome. +fn upload_annotations(report: &UploadReport) -> Vec { + let mut annotations = Vec::new(); + if let Some(error) = &report.error { + if error.kind.blocks_quarantining() { + return vec![Annotation::error(error_message(error))]; + } + annotations.push(Annotation::warning(error_message(error))); + } + + let exit_code = report.cli_exit_code(); + match report.outcome { + Outcome::NoTestsFound if !exit_code.is_success() => { + annotations.push(Annotation::error(format!( + "{NO_TESTS_FOUND}, but non zero exit code provided: {exit_code}\n{FAILED_WITHOUT_RUNNING_TESTS}" + ))); + } + Outcome::NoFailuresButNonZeroExitCode => { + annotations.push(Annotation::error(format!( + "{NO_TEST_FAILURES}, but non zero exit code provided: {exit_code}\n{SOME_TESTS_MAY_NOT_HAVE_RUN}" + ))); + } + Outcome::FailuresNotQuarantined => { + let tests = report.failures.iter().flat_map(|group| &group.tests); + annotations.push(Annotation::error(failures_message(report))); + annotations.extend(tests.take(MAX_TEST_ANNOTATIONS).map(test_annotation)); + } + Outcome::NoTestsFound | Outcome::NoFailures | Outcome::AllFailuresQuarantined => {} + } + annotations +} + +fn failures_message(report: &UploadReport) -> String { + let failed = report.counts.failed; + let mut message = format!( + "{} failed and {} not quarantined, using exit code: {}", + pluralize("test", failed as isize, true), + pluralize("was", failed as isize, false), + report.cli_exit_code(), + ); + let reason = match report.quarantining { + Quarantining::LookupFailed => Some(QUARANTINE_LOOKUP_FAILED), + Quarantining::DisabledViaCli => { + Some("Quarantining was turned off with --disable-quarantining") + } + Quarantining::DisabledViaApi => Some("Quarantining is turned off for this repo in Trunk"), + Quarantining::Checked | Quarantining::NotNeeded => None, + }; + if let Some(reason) = reason { + message.push_str(&format!("\n{reason}")); + } + if failed > MAX_TEST_ANNOTATIONS { + message.push_str(&format!( + "\nThe first {MAX_TEST_ANNOTATIONS} are annotated; see the log for the rest." + )); + } + message +} + +fn test_annotation(test: &FailedTest) -> Annotation { + let separator = if test.suite.is_empty() { "" } else { "/" }; + let mut lines: Vec<&str> = test + .failure_text + .as_deref() + .or(test.failure_message.as_deref()) + .map(|message| message.lines().take(FAILURE_MESSAGE_MAX_LINES).collect()) + .unwrap_or_default(); + if lines.is_empty() { + lines.push("Failed"); + } + if let Some(link) = &test.link { + lines.push(link); + } + Annotation { + level: Level::Error, + file: test.file.clone(), + title: Some(format!("{}{separator}{}", test.suite, test.name)), + message: lines.join("\n"), + } +} + +/// The same lines as `text::render_error`, without its exit code line. +fn error_message(error: &ErrorSummary) -> String { + let mut lines = Vec::new(); + match error.kind { + ErrorKind::ConnectionRefused => { + lines.extend(error.message.clone()); + lines.push(CONNECTION_REFUSED_CONTEXT.to_string()); + } + ErrorKind::Unauthorized => { + lines.extend(error.message.clone()); + lines.push(UNAUTHORIZED_CONTEXT.to_string()); + lines.extend( + error + .settings_url + .as_ref() + .map(|url| format!("{UNAUTHORIZED_HINT} {url}")), + ); + } + ErrorKind::GitRepository | ErrorKind::Interrupted | ErrorKind::Other => { + match &error.message { + Some(message) => { + lines.push(message.clone()); + lines.extend(error.explanation.clone()); + } + None => lines.push(error.error.clone()), + } + } + } + lines.join("\n") +} + +impl Annotation { + fn error(message: String) -> Self { + Self { + level: Level::Error, + file: None, + title: None, + message, + } + } + + fn warning(message: String) -> Self { + Self { + level: Level::Warning, + ..Self::error(message) + } + } +} + +/// A workflow command, escaped per GitHub's rules: +/// https://github.com/actions/toolkit/blob/main/packages/core/src/command.ts +impl fmt::Display for Annotation { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let command = match self.level { + Level::Error => "error", + Level::Warning => "warning", + }; + let properties = [("file", &self.file), ("title", &self.title)] + .into_iter() + .filter_map(|(key, value)| Some(format!("{key}={}", escape_property(value.as_ref()?)))) + .collect::>(); + let separator = if properties.is_empty() { "" } else { " " }; + write!( + f, + "::{command}{separator}{}::{}", + properties.join(","), + escape_data(&self.message) + ) + } +} + +fn escape_data(value: &str) -> String { + value + .replace('%', "%25") + .replace('\r', "%0D") + .replace('\n', "%0A") +} + +fn escape_property(value: &str) -> String { + escape_data(value).replace(':', "%3A").replace(',', "%2C") +} diff --git a/cli/src/summary_output/json.rs b/cli/src/summary_output/json.rs index 04ee8b7b..c8f17510 100644 --- a/cli/src/summary_output/json.rs +++ b/cli/src/summary_output/json.rs @@ -1,7 +1,8 @@ use std::{fs, path::Path}; +use anyhow::Context; use schemars::{JsonSchema, Schema, generate::SchemaSettings}; -use serde::Serialize; +use serde::{Deserialize, Serialize}; use super::{ErrorSummary, UploadReport}; @@ -29,6 +30,20 @@ struct Versioned<'a> { report: Report<'a>, } +/// A summary read back from a file, owning its report. +#[derive(Debug, Clone, Deserialize)] +#[serde(tag = "type", rename_all = "SCREAMING_SNAKE_CASE")] +pub enum Summary { + Upload(Box), + Error(ErrorSummary), +} + +#[derive(Deserialize)] +#[serde(rename_all = "camelCase")] +struct SchemaVersion { + schema_version: u32, +} + pub fn schema() -> Schema { SchemaSettings::draft2020_12() .for_serialize() @@ -43,6 +58,25 @@ pub fn to_string(report: Report) -> serde_json::Result { }) } +/// Reads a summary that this or an older CLI wrote. A newer schema version is an error, since its +/// fields may have changed meaning. +pub fn from_str(json: &str) -> anyhow::Result { + let SchemaVersion { schema_version } = + serde_json::from_str(json).context("not a summary: no schemaVersion")?; + if schema_version > SCHEMA_VERSION { + anyhow::bail!( + "schemaVersion {schema_version} is newer than this CLI understands ({SCHEMA_VERSION})" + ); + } + Ok(serde_json::from_str(json)?) +} + +pub fn read_file(path: &Path) -> anyhow::Result { + let json = + fs::read_to_string(path).with_context(|| format!("couldn't read {}", path.display()))?; + from_str(&json).with_context(|| format!("couldn't read the summary in {}", path.display())) +} + /// Writes a sibling file and renames it into place, so a reader never sees half a summary. /// Not `tempfile::NamedTempFile`, which would leave the summary readable only by its owner. pub fn write_file(path: &Path, report: Report) -> anyhow::Result<()> { diff --git a/cli/src/summary_output/mod.rs b/cli/src/summary_output/mod.rs index 85e6b833..2270bec2 100644 --- a/cli/src/summary_output/mod.rs +++ b/cli/src/summary_output/mod.rs @@ -1,4 +1,5 @@ mod error; +pub mod github; pub mod json; mod notice; pub mod text; diff --git a/cli/src/summary_output/notice.rs b/cli/src/summary_output/notice.rs index 2da04e21..7a6ea973 100644 --- a/cli/src/summary_output/notice.rs +++ b/cli/src/summary_output/notice.rs @@ -1,14 +1,14 @@ use schemars::JsonSchema; -use serde::Serialize; +use serde::{Deserialize, Serialize}; -#[derive(Debug, Clone, PartialEq, Eq, Serialize, JsonSchema)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "camelCase")] pub struct Notice { pub kind: NoticeKind, pub message: String, } -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, JsonSchema)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "SCREAMING_SNAKE_CASE")] pub enum NoticeKind { /// A deprecated flag was passed. diff --git a/cli/src/summary_output/text.rs b/cli/src/summary_output/text.rs index 08d7596b..f58c71d1 100644 --- a/cli/src/summary_output/text.rs +++ b/cli/src/summary_output/text.rs @@ -94,6 +94,15 @@ impl EndOutput for SummaryFileText { } } +// Shared with the GitHub annotations, so both say the same thing. +pub(crate) const NO_TESTS_FOUND: &str = "No tests were found in the provided test results"; +pub(crate) const FAILED_WITHOUT_RUNNING_TESTS: &str = "The test step appears to have failed without running tests -- see above for the original failure."; +pub(crate) const NO_TEST_FAILURES: &str = "No test failures found"; +pub(crate) const SOME_TESTS_MAY_NOT_HAVE_RUN: &str = "This may indicate that some tests were not run or that there were other issues during the test run."; +pub(crate) const QUARANTINE_LOOKUP_FAILED: &str = "We were unable to determine the quarantine status for tests. Any failing tests will be reported as failures"; +pub(crate) const FAILURE_MESSAGE_MAX_LINES: usize = 20; +pub(crate) const UNAUTHORIZED_HINT: &str = "Hint: You can find it under the settings page at"; + pub fn render_error(error: &ErrorSummary) -> anyhow::Result> { let mut lines = Vec::new(); lines.push(Line::from_iter([Span::new_styled( @@ -112,7 +121,7 @@ pub fn render_error(error: &ErrorSummary) -> anyhow::Result> { } ErrorKind::Unauthorized => { let mut hint = Line::from_iter([Span::new_unstyled_lossy(format!( - "Hint: You can find it under the settings page at {}", + "{UNAUTHORIZED_HINT} {}", error.settings_url.as_deref().unwrap_or_default() ))]); hint.pad_left(2); @@ -216,7 +225,9 @@ pub fn render_upload(report: &UploadReport, options: &TextOptions) -> anyhow::Re ))?])); if report.quarantining == Quarantining::LookupFailed { output.push(Line::from_iter([Span::new_styled( - style(" We were unable to determine the quarantine status for tests. Any failing tests will be reported as failures".to_string()).with(Color::Yellow).attribute(Attribute::Bold), + style(format!(" {QUARANTINE_LOOKUP_FAILED}")) + .with(Color::Yellow) + .attribute(Attribute::Bold), )?])); } output.push(Line::default()); @@ -270,22 +281,20 @@ pub fn render_upload(report: &UploadReport, options: &TextOptions) -> anyhow::Re } match report.outcome { - Outcome::NoTestsFound if report.cli_exit_code().is_success() => { - output.push(Line::from_iter([Span::new_unstyled( - "⚠️ No tests were found in the provided test results", - )?])) - } + Outcome::NoTestsFound if report.cli_exit_code().is_success() => output.push( + Line::from_iter([Span::new_unstyled(format!("⚠️ {NO_TESTS_FOUND}"))?]), + ), Outcome::NoTestsFound => { output.push(Line::from_iter([ - Span::new_unstyled( - "⚠️ No tests were found in the provided test results, but non zero exit code provided: ", - )?, + Span::new_unstyled(format!( + "⚠️ {NO_TESTS_FOUND}, but non zero exit code provided: " + ))?, Span::new_styled( style(format!("{}", report.cli_exit_code())).attribute(Attribute::Bold), )?, ])); output.push(Line::from_iter([Span::new_unstyled( - "The test step appears to have failed without running tests -- see above for the original failure.", + FAILED_WITHOUT_RUNNING_TESTS, )?])); } Outcome::AllFailuresQuarantined => output.push(Line::from_iter([ @@ -299,15 +308,15 @@ pub fn render_upload(report: &UploadReport, options: &TextOptions) -> anyhow::Re )?])), Outcome::NoFailuresButNonZeroExitCode => { output.push(Line::from_iter([ - Span::new_unstyled( - "⚠️ No test failures found, but non zero exit code provided: ", - )?, + Span::new_unstyled(format!( + "⚠️ {NO_TEST_FAILURES}, but non zero exit code provided: " + ))?, Span::new_styled( style(format!("{}", report.cli_exit_code())).attribute(Attribute::Bold), )?, ])); output.push(Line::from_iter([Span::new_unstyled( - "This may indicate that some tests were not run or that there were other issues during the test run.", + SOME_TESTS_MAY_NOT_HAVE_RUN, )?])); } Outcome::FailuresNotQuarantined => output.push(Line::from_iter([ @@ -384,7 +393,7 @@ fn render_groups(groups: &[FailureGroup], options: &TextOptions) -> anyhow::Resu fn render_failure_message(failure_message: &str) -> anyhow::Result> { let mut output = Vec::new(); let lines: Vec<&str> = failure_message.split('\n').collect(); - let max_lines = 20; + let max_lines = FAILURE_MESSAGE_MAX_LINES; let mut failure_header = Line::from_iter([Span::new_styled( style("Failure: ".to_string()).with(Color::DarkGrey), )?]); diff --git a/cli/src/summary_output/upload.rs b/cli/src/summary_output/upload.rs index 9810b216..1569e31f 100644 --- a/cli/src/summary_output/upload.rs +++ b/cli/src/summary_output/upload.rs @@ -4,14 +4,14 @@ use api::urls::{url_for_test_case, url_for_upload}; use bundle::Test; use context::junit::validator::JunitValidationLevel; use schemars::JsonSchema; -use serde::Serialize; +use serde::{Deserialize, Serialize}; use super::{ErrorSummary, Notice}; use crate::{ exit_code::ExitCode, upload_command::UploadRunResult, validate_command::JunitReportValidations, }; -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, JsonSchema)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "SCREAMING_SNAKE_CASE")] pub enum UploadCommand { Upload, @@ -21,7 +21,7 @@ pub enum UploadCommand { RSpec, } -#[derive(Debug, Clone, Serialize, JsonSchema)] +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "camelCase")] pub struct UploadReport { /// Which command wrote this summary. @@ -49,7 +49,7 @@ pub struct UploadReport { pub notices: Vec, } -#[derive(Debug, Clone, Copy, PartialEq, Serialize, JsonSchema)] +#[derive(Debug, Clone, Copy, PartialEq, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "camelCase")] pub struct TestCounts { pub total: usize, @@ -63,7 +63,7 @@ pub struct TestCounts { pub pass_ratio: Option, } -#[derive(Debug, Clone, PartialEq, Eq, Serialize, JsonSchema)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "camelCase")] pub struct FailureGroup { pub key: GroupKey, @@ -71,7 +71,7 @@ pub struct FailureGroup { } /// Groups sort by file, then by suite, then the rest, in declaration order. -#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, JsonSchema)] +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize, JsonSchema)] #[serde(tag = "type", content = "name", rename_all = "SCREAMING_SNAKE_CASE")] pub enum GroupKey { /// Failed tests from this file. @@ -82,7 +82,7 @@ pub enum GroupKey { Other, } -#[derive(Debug, Clone, PartialEq, Eq, Serialize, JsonSchema)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "camelCase")] pub struct FailedTest { pub name: String, @@ -102,7 +102,7 @@ pub struct FailedTest { pub link: Option, } -#[derive(Debug, Clone, PartialEq, Eq, Serialize, JsonSchema)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "camelCase")] pub struct FileValidation { pub file: String, @@ -116,7 +116,7 @@ pub struct FileValidation { pub test_case_count: Option, } -#[derive(Debug, Clone, PartialEq, Eq, Serialize, JsonSchema)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "camelCase")] pub struct ParseError { pub error: String, @@ -125,7 +125,7 @@ pub struct ParseError { } /// Whether the failed tests could be checked for quarantining, and if not, why. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, JsonSchema)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "SCREAMING_SNAKE_CASE")] pub enum Quarantining { /// Trunk said which failed tests are quarantined. @@ -140,7 +140,7 @@ pub enum Quarantining { LookupFailed, } -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, JsonSchema)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "SCREAMING_SNAKE_CASE")] pub enum Outcome { /// The test reports had no tests. diff --git a/cli/tests/summary/github.rs b/cli/tests/summary/github.rs new file mode 100644 index 00000000..79f3ae06 --- /dev/null +++ b/cli/tests/summary/github.rs @@ -0,0 +1,257 @@ +use std::fs; + +use api::client::ClientErrorExplanation; +use assert_cmd::{Command, cargo}; +use pretty_assertions::assert_eq; +use trunk_analytics_cli::{ + context_quarantine::QuarantineFetchStatus, + error_report::{ErrorReport, InterruptingError}, + exit_code::ExitCode, + summary_output::{ + ErrorSummary, UploadCommand, UploadReport, + github::annotations, + json::{self, Report}, + }, + upload_command::UploadRunResult, +}; + +use super::{ + unauthorized_error, + upload::{UPLOAD_ERROR_MESSAGE, failure, upload_result}, +}; + +/// Writes the summary as `upload` would, then reads it back the way the command does. +fn upload_annotations(result: &UploadRunResult) -> Vec { + let report = UploadReport::new(result, UploadCommand::Upload); + let summary = json::from_str(&json::to_string(Report::Upload(&report)).unwrap()).unwrap(); + annotations(&summary) + .iter() + .map(ToString::to_string) + .collect() +} + +fn error_annotations(report: &ErrorReport) -> Vec { + let error = ErrorSummary::new(report); + let summary = json::from_str(&json::to_string(Report::Error(&error)).unwrap()).unwrap(); + annotations(&summary) + .iter() + .map(ToString::to_string) + .collect() +} + +#[test] +fn reads_back_the_summary_it_wrote() { + let mut result = upload_result(5); + result.quarantine_context.exit_code = ExitCode::Failure; + result.quarantine_context.failures = vec![failure("adds", "MathTests", Some("src/math.rs"))]; + let report = UploadReport::new(&result, UploadCommand::Upload); + let written = json::to_string(Report::Upload(&report)).unwrap(); + + let json::Summary::Upload(read) = json::from_str(&written).unwrap() else { + panic!("expected an upload summary"); + }; + assert_eq!(json::to_string(Report::Upload(&read)).unwrap(), written); +} + +#[test] +fn rejects_a_newer_schema_version() { + let error = json::from_str(r#"{"schemaVersion": 2, "type": "ERROR"}"#).unwrap_err(); + assert_eq!( + error.to_string(), + "schemaVersion 2 is newer than this CLI understands (1)" + ); + assert!(json::from_str("{}").is_err()); +} + +#[test] +fn says_nothing_about_a_passing_run() { + assert_eq!(upload_annotations(&upload_result(5)), Vec::::new()); +} + +#[test] +fn says_nothing_when_no_tests_were_found_with_exit_code_0() { + assert_eq!(upload_annotations(&upload_result(0)), Vec::::new()); +} + +#[test] +fn says_the_test_step_failed_without_running_tests() { + let mut result = upload_result(0); + result.quarantine_context.exit_code = ExitCode::Failure; + + assert_eq!( + upload_annotations(&result), + [ + "::error::No tests were found in the provided test results, but non zero exit code provided: 1%0AThe test step appears to have failed without running tests -- see above for the original failure." + ] + ); +} + +#[test] +fn says_some_tests_may_not_have_run() { + let mut result = upload_result(5); + result.quarantine_context.exit_code = ExitCode::PassedThrough(2); + + assert_eq!( + upload_annotations(&result), + [ + "::error::No test failures found, but non zero exit code provided: 2%0AThis may indicate that some tests were not run or that there were other issues during the test run." + ] + ); +} + +#[test] +fn annotates_each_failure_that_was_not_quarantined() { + let mut result = upload_result(5); + result.quarantine_context.exit_code = ExitCode::Failure; + result.quarantine_context.fetch_status = QuarantineFetchStatus::FetchSucceeded; + result + .quarantine_context + .quarantine_status + .quarantine_results = vec![failure("adds", "MathTests", Some("src/math.rs"))]; + let mut divides = failure("divides", "MathTests", None); + divides.failure_message = Some(String::from("expected 1, got 2\n100% wrong")); + result.quarantine_context.failures = vec![ + failure("subtracts", "Math, Tests: unit", Some("src/math.rs")), + divides, + ]; + + assert_eq!( + upload_annotations(&result), + [ + "::error::2 tests failed and were not quarantined, using exit code: 1", + "::error file=src/math.rs,title=Math%2C Tests%3A unit/subtracts::Failed%0Ahttps://app.trunk.io/test-org/flaky-tests/test/subtracts-id?repo=trunk-io%252Fanalytics-cli", + "::error title=MathTests/divides::expected 1, got 2%0A100%25 wrong%0Ahttps://app.trunk.io/test-org/flaky-tests/test/divides-id?repo=trunk-io%252Fanalytics-cli", + ] + ); +} + +#[test] +fn says_why_failures_could_not_be_quarantined() { + let mut result = upload_result(3); + result.quarantine_context.exit_code = ExitCode::Failure; + result.quarantine_context.fetch_status = + QuarantineFetchStatus::FetchFailed(anyhow::anyhow!("request timed out")); + result.quarantine_context.failures = vec![failure("adds", "MathTests", None)]; + + assert_eq!( + upload_annotations(&result)[0], + "::error::1 test failed and was not quarantined, using exit code: 1%0AWe were unable to determine the quarantine status for tests. Any failing tests will be reported as failures" + ); + + result.quarantine_context.fetch_status = QuarantineFetchStatus::FetchSucceeded; + result.disable_quarantining = true; + assert_eq!( + upload_annotations(&result)[0], + "::error::1 test failed and was not quarantined, using exit code: 1%0AQuarantining was turned off with --disable-quarantining" + ); +} + +#[test] +fn annotates_at_most_nine_failures() { + let mut result = upload_result(12); + result.quarantine_context.exit_code = ExitCode::Failure; + result.quarantine_context.failures = (0..12) + .map(|i| failure(&format!("test{i}"), "MathTests", None)) + .collect(); + + let annotations = upload_annotations(&result); + assert_eq!(annotations.len(), 10); + assert_eq!( + annotations[0], + "::error::12 tests failed and were not quarantined, using exit code: 1%0AThe first 9 are annotated; see the log for the rest." + ); +} + +#[test] +fn says_only_the_error_when_it_blocks_quarantining() { + let mut result = upload_result(3); + result.quarantine_context.exit_code = ExitCode::Failure; + result.quarantine_context.failures = vec![failure("adds", "MathTests", None)]; + result.error_report = Some(ErrorReport::new( + unauthorized_error(), + String::from("test-org"), + Some(String::from(UPLOAD_ERROR_MESSAGE)), + )); + + assert_eq!( + upload_annotations(&result), + [ + "::error::There was an unexpected error that occurred while uploading test results%0AUnathorized access, your Trunk organization URL slug or token may be incorrect%0AHint: You can find it under the settings page at https://app.trunk.io/test-org/settings" + ] + ); +} + +#[test] +fn warns_about_an_upload_error_before_the_failures() { + let mut result = upload_result(3); + result.quarantine_context.exit_code = ExitCode::Failure; + result.quarantine_context.failures = vec![failure("adds", "MathTests", None)]; + result.error_report = Some(ErrorReport::new( + anyhow::anyhow!("HTTP status client error (400 Bad Request)") + .context(ClientErrorExplanation { + code: Some(String::from("SOME_CODE")), + message: Some(String::from("Pass --test-collection-id.")), + }) + .context("Error in create bundle upload endpoint"), + String::from("test-org"), + Some(String::from(UPLOAD_ERROR_MESSAGE)), + )); + + assert_eq!( + upload_annotations(&result)[..2].to_vec(), + [ + "::warning::There was an unexpected error that occurred while uploading test results%0APass --test-collection-id.", + "::error::1 test failed and was not quarantined, using exit code: 1", + ] + ); +} + +#[test] +fn annotates_a_run_that_failed_outright() { + let report = ErrorReport::new( + anyhow::Error::from(InterruptingError::new(String::from( + "No test output files found to upload.", + ))), + String::from("test-org"), + None, + ); + + assert_eq!( + error_annotations(&report), + ["::error::No test output files found to upload."] + ); +} + +fn github_annotations(summary_file: &std::path::Path) -> assert_cmd::assert::Assert { + Command::new(cargo::cargo_bin!()) + .args(["github-annotations", "--summary-file"]) + .arg(summary_file) + .assert() +} + +#[test] +fn the_command_prints_annotations_and_exits_0() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("summary.json"); + let mut result = upload_result(0); + result.quarantine_context.exit_code = ExitCode::Failure; + let report = UploadReport::new(&result, UploadCommand::Upload); + json::write_file(&path, Report::Upload(&report)).unwrap(); + + github_annotations(&path).success().stdout( + "::error::No tests were found in the provided test results, but non zero exit code provided: 1%0AThe test step appears to have failed without running tests -- see above for the original failure.\n", + ); +} + +#[test] +fn the_command_exits_non_zero_with_nothing_to_say() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("summary.json"); + let report = UploadReport::new(&upload_result(5), UploadCommand::Upload); + json::write_file(&path, Report::Upload(&report)).unwrap(); + + github_annotations(&path).code(1).stdout(""); + + fs::remove_file(&path).unwrap(); + github_annotations(&path).code(1).stdout(""); +} diff --git a/cli/tests/summary/main.rs b/cli/tests/summary/main.rs index 07ecdf68..5b5c3eda 100644 --- a/cli/tests/summary/main.rs +++ b/cli/tests/summary/main.rs @@ -1,4 +1,5 @@ mod error_report; +mod github; mod schema; mod upload; mod validate; diff --git a/cli/tests/summary/upload.rs b/cli/tests/summary/upload.rs index a522cdf8..3c10add0 100644 --- a/cli/tests/summary/upload.rs +++ b/cli/tests/summary/upload.rs @@ -32,7 +32,7 @@ use trunk_analytics_cli::{ use crate::unauthorized_error; const BUNDLE_UPLOAD_ID: &str = "4b1d2c3e-5f60-4a7b-8c9d-0e1f2a3b4c5d"; -const UPLOAD_ERROR_MESSAGE: &str = +pub(super) const UPLOAD_ERROR_MESSAGE: &str = "There was an unexpected error that occurred while uploading test results"; fn upload_summary(result: &UploadRunResult) -> Vec { @@ -52,7 +52,7 @@ fn repo() -> RepoUrlParts { } } -fn upload_result(num_tests: usize) -> UploadRunResult { +pub(super) fn upload_result(num_tests: usize) -> UploadRunResult { let mut quarantine_context = QuarantineContext::skip_fetch(Vec::new()); quarantine_context.repo = repo(); quarantine_context.org_url_slug = String::from("test-org"); @@ -117,7 +117,7 @@ fn in_collection(mut result: UploadRunResult) -> UploadRunResult { result } -fn failure(name: &str, parent_name: &str, file: Option<&str>) -> Test { +pub(super) fn failure(name: &str, parent_name: &str, file: Option<&str>) -> Test { Test { name: String::from(name), parent_name: String::from(parent_name), From 0b056b4eaba3e190e875057251963c71eb50c855 Mon Sep 17 00:00:00 2001 From: Dylan Frankland Date: Wed, 28 Oct 2026 16:11:18 +0000 Subject: [PATCH 2/2] refactor(cli): make the summary's JSON helpers methods, and read newer schemas that still parse - Report::{schema, to_json, write_file}, and Summary::read_file plus FromStr, replace json::* free functions. - A newer schemaVersion only fails when the summary doesn't parse; one that does is read with a warning. - An unknown error kind from a newer CLI reads as OTHER instead of failing the whole summary. Co-Authored-By: Claude Opus 5.5 --- cli/src/error_report.rs | 2 + cli/src/main.rs | 4 +- cli/src/summary_output/github.rs | 4 +- cli/src/summary_output/json.rs | 115 ++++++++++++++++++------------ cli/tests/summary/error_report.rs | 4 +- cli/tests/summary/github.rs | 49 ++++++++++--- cli/tests/summary/schema.rs | 2 +- cli/tests/summary/upload.rs | 2 +- test_report/src/report.rs | 2 +- 9 files changed, 117 insertions(+), 67 deletions(-) diff --git a/cli/src/error_report.rs b/cli/src/error_report.rs index 130540b5..c0dc894b 100644 --- a/cli/src/error_report.rs +++ b/cli/src/error_report.rs @@ -105,6 +105,8 @@ pub enum ErrorKind { GitRepository, /// The run stopped early, such as when it found no test reports and empty results aren't allowed. Interrupted, + /// Also what a kind from a newer CLI reads as. + #[serde(other)] Other, } diff --git a/cli/src/main.rs b/cli/src/main.rs index 425ef7dc..80227220 100644 --- a/cli/src/main.rs +++ b/cli/src/main.rs @@ -247,7 +247,7 @@ fn main() -> anyhow::Result<()> { close_out_and_exit(exit_code, guard, render_sender, render_handle) } MainOutcome::Finished(Ok(RunResult::SummaryOutputSchema)) => { - println!("{}", serde_json::to_string_pretty(&json::schema())?); + println!("{}", serde_json::to_string_pretty(&json::Report::schema())?); close_out_and_exit(ExitCode::Success, guard, render_sender, render_handle) } MainOutcome::Finished(Err(error)) => { @@ -276,7 +276,7 @@ fn write_summary_file(path: Option<&Path>, report: Report, render_sender: &Sende let Some(path) = path else { return; }; - let text = SummaryFileText::new(path, json::write_file(path, report)); + let text = SummaryFileText::new(path, report.write_file(path)); send_message( DisplayMessage::Final(Arc::new(text), String::from("summary file display")), render_sender, diff --git a/cli/src/summary_output/github.rs b/cli/src/summary_output/github.rs index 99f96fe2..eeee6a16 100644 --- a/cli/src/summary_output/github.rs +++ b/cli/src/summary_output/github.rs @@ -8,7 +8,7 @@ use pluralizer::pluralize; use super::{ ErrorSummary, FailedTest, Outcome, Quarantining, UploadReport, - json::{self, Summary}, + json::Summary, text::{ FAILED_WITHOUT_RUNNING_TESTS, FAILURE_MESSAGE_MAX_LINES, NO_TEST_FAILURES, NO_TESTS_FOUND, QUARANTINE_LOOKUP_FAILED, SOME_TESTS_MAY_NOT_HAVE_RUN, UNAUTHORIZED_HINT, @@ -40,7 +40,7 @@ pub struct Annotation { /// 0, and otherwise falls back to its own message, so anything else exits non-zero: a summary /// with nothing to say, and one that can't be read. pub fn run(summary_file: &Path) -> ExitCode { - let summary = match json::read_file(summary_file) { + let summary = match Summary::read_file(summary_file) { Ok(summary) => summary, Err(error) => { eprintln!("{error:#}"); diff --git a/cli/src/summary_output/json.rs b/cli/src/summary_output/json.rs index c8f17510..c7915bed 100644 --- a/cli/src/summary_output/json.rs +++ b/cli/src/summary_output/json.rs @@ -1,4 +1,4 @@ -use std::{fs, path::Path}; +use std::{fs, path::Path, str::FromStr}; use anyhow::Context; use schemars::{JsonSchema, Schema, generate::SchemaSettings}; @@ -44,58 +44,79 @@ struct SchemaVersion { schema_version: u32, } -pub fn schema() -> Schema { - SchemaSettings::draft2020_12() - .for_serialize() - .into_generator() - .into_root_schema_for::>() -} +impl Report<'_> { + /// What `summary-output-schema` prints. + pub fn schema() -> Schema { + SchemaSettings::draft2020_12() + .for_serialize() + .into_generator() + .into_root_schema_for::>() + } -pub fn to_string(report: Report) -> serde_json::Result { - serde_json::to_string_pretty(&Versioned { - schema_version: SCHEMA_VERSION, - report, - }) -} + pub fn to_json(self) -> serde_json::Result { + serde_json::to_string_pretty(&Versioned { + schema_version: SCHEMA_VERSION, + report: self, + }) + } -/// Reads a summary that this or an older CLI wrote. A newer schema version is an error, since its -/// fields may have changed meaning. -pub fn from_str(json: &str) -> anyhow::Result { - let SchemaVersion { schema_version } = - serde_json::from_str(json).context("not a summary: no schemaVersion")?; - if schema_version > SCHEMA_VERSION { - anyhow::bail!( - "schemaVersion {schema_version} is newer than this CLI understands ({SCHEMA_VERSION})" - ); + /// Writes a sibling file and renames it into place, so a reader never sees half a summary. + /// Not `tempfile::NamedTempFile`, which would leave the summary readable only by its owner. + pub fn write_file(self, path: &Path) -> anyhow::Result<()> { + let json = self.to_json()?; + let file_name = path + .file_name() + .ok_or_else(|| anyhow::anyhow!("{} is not a file path", path.display()))?; + let dir = path.parent().unwrap_or(Path::new("")); + if !dir.as_os_str().is_empty() { + fs::create_dir_all(dir)?; + } + let temp_path = dir.join(format!( + ".{}.{}.tmp", + file_name.to_string_lossy(), + std::process::id() + )); + let written = fs::write(&temp_path, json).and_then(|()| fs::rename(&temp_path, path)); + if written.is_err() { + let _ = fs::remove_file(&temp_path); + } + Ok(written?) } - Ok(serde_json::from_str(json)?) } -pub fn read_file(path: &Path) -> anyhow::Result { - let json = - fs::read_to_string(path).with_context(|| format!("couldn't read {}", path.display()))?; - from_str(&json).with_context(|| format!("couldn't read the summary in {}", path.display())) +impl Summary { + pub fn read_file(path: &Path) -> anyhow::Result { + let json = fs::read_to_string(path) + .with_context(|| format!("couldn't read {}", path.display()))?; + json.parse() + .with_context(|| format!("couldn't read the summary in {}", path.display())) + } } -/// Writes a sibling file and renames it into place, so a reader never sees half a summary. -/// Not `tempfile::NamedTempFile`, which would leave the summary readable only by its owner. -pub fn write_file(path: &Path, report: Report) -> anyhow::Result<()> { - let json = to_string(report)?; - let file_name = path - .file_name() - .ok_or_else(|| anyhow::anyhow!("{} is not a file path", path.display()))?; - let dir = path.parent().unwrap_or(Path::new("")); - if !dir.as_os_str().is_empty() { - fs::create_dir_all(dir)?; - } - let temp_path = dir.join(format!( - ".{}.{}.tmp", - file_name.to_string_lossy(), - std::process::id() - )); - let written = fs::write(&temp_path, json).and_then(|()| fs::rename(&temp_path, path)); - if written.is_err() { - let _ = fs::remove_file(&temp_path); +impl FromStr for Summary { + type Err = anyhow::Error; + + /// Reads a summary that any CLI wrote. A newer schema version is only an error when the + /// summary doesn't parse; one that does, such as one with only new fields, is read with a + /// warning. + fn from_str(json: &str) -> anyhow::Result { + let SchemaVersion { schema_version } = + serde_json::from_str(json).context("not a summary: no schemaVersion")?; + let newer = schema_version > SCHEMA_VERSION; + let newer_message = || { + format!( + "schemaVersion {schema_version} is newer than this CLI understands ({SCHEMA_VERSION})" + ) + }; + match serde_json::from_str(json) { + Ok(summary) => { + if newer { + tracing::warn!("{}; reading it anyway", newer_message()); + } + Ok(summary) + } + Err(error) if newer => Err(anyhow::Error::from(error).context(newer_message())), + Err(error) => Err(error.into()), + } } - Ok(written?) } diff --git a/cli/tests/summary/error_report.rs b/cli/tests/summary/error_report.rs index 8340e6a4..4475f518 100644 --- a/cli/tests/summary/error_report.rs +++ b/cli/tests/summary/error_report.rs @@ -199,7 +199,7 @@ fn writes_an_error_as_json() { let error = ErrorSummary::new(&report); assert_eq!( - serde_json::from_str::(&json::to_string(Report::Error(&error)).unwrap()) + serde_json::from_str::(&Report::Error(&error).to_json().unwrap()) .unwrap(), serde_json::json!({ "schemaVersion": 1, @@ -220,7 +220,7 @@ fn writes_the_settings_url_and_exit_code_of_an_unauthorized_error_as_json() { let report = ErrorReport::new(unauthorized_error(), String::from("fake-org-slug"), None); let error = ErrorSummary::new(&report); let json: serde_json::Value = - serde_json::from_str(&json::to_string(Report::Error(&error)).unwrap()).unwrap(); + serde_json::from_str(&Report::Error(&error).to_json().unwrap()).unwrap(); assert_eq!(json["kind"], "UNAUTHORIZED"); assert_eq!( diff --git a/cli/tests/summary/github.rs b/cli/tests/summary/github.rs index 79f3ae06..a9823277 100644 --- a/cli/tests/summary/github.rs +++ b/cli/tests/summary/github.rs @@ -5,7 +5,7 @@ use assert_cmd::{Command, cargo}; use pretty_assertions::assert_eq; use trunk_analytics_cli::{ context_quarantine::QuarantineFetchStatus, - error_report::{ErrorReport, InterruptingError}, + error_report::{ErrorKind, ErrorReport, InterruptingError}, exit_code::ExitCode, summary_output::{ ErrorSummary, UploadCommand, UploadReport, @@ -23,7 +23,11 @@ use super::{ /// Writes the summary as `upload` would, then reads it back the way the command does. fn upload_annotations(result: &UploadRunResult) -> Vec { let report = UploadReport::new(result, UploadCommand::Upload); - let summary = json::from_str(&json::to_string(Report::Upload(&report)).unwrap()).unwrap(); + let summary = Report::Upload(&report) + .to_json() + .unwrap() + .parse::() + .unwrap(); annotations(&summary) .iter() .map(ToString::to_string) @@ -32,7 +36,11 @@ fn upload_annotations(result: &UploadRunResult) -> Vec { fn error_annotations(report: &ErrorReport) -> Vec { let error = ErrorSummary::new(report); - let summary = json::from_str(&json::to_string(Report::Error(&error)).unwrap()).unwrap(); + let summary = Report::Error(&error) + .to_json() + .unwrap() + .parse::() + .unwrap(); annotations(&summary) .iter() .map(ToString::to_string) @@ -45,22 +53,41 @@ fn reads_back_the_summary_it_wrote() { result.quarantine_context.exit_code = ExitCode::Failure; result.quarantine_context.failures = vec![failure("adds", "MathTests", Some("src/math.rs"))]; let report = UploadReport::new(&result, UploadCommand::Upload); - let written = json::to_string(Report::Upload(&report)).unwrap(); + let written = Report::Upload(&report).to_json().unwrap(); - let json::Summary::Upload(read) = json::from_str(&written).unwrap() else { + let json::Summary::Upload(read) = written.parse().unwrap() else { panic!("expected an upload summary"); }; - assert_eq!(json::to_string(Report::Upload(&read)).unwrap(), written); + assert_eq!(Report::Upload(&read).to_json().unwrap(), written); } #[test] fn rejects_a_newer_schema_version() { - let error = json::from_str(r#"{"schemaVersion": 2, "type": "ERROR"}"#).unwrap_err(); + let error = r#"{"schemaVersion": 2, "type": "ERROR"}"#.parse::().unwrap_err(); assert_eq!( error.to_string(), "schemaVersion 2 is newer than this CLI understands (1)" ); - assert!(json::from_str("{}").is_err()); + assert!("{}".parse::().is_err()); +} + +#[test] +fn reads_a_newer_schema_version_that_still_parses() { + let error = ErrorSummary::new(&ErrorReport::new( + unauthorized_error(), + String::from("fake-org-slug"), + None, + )); + let mut written: serde_json::Value = + serde_json::from_str(&Report::Error(&error).to_json().unwrap()).unwrap(); + written["schemaVersion"] = serde_json::json!(2); + written["kind"] = serde_json::json!("SOME_NEW_KIND"); + written["someNewField"] = serde_json::json!(true); + + let json::Summary::Error(read) = written.to_string().parse().unwrap() else { + panic!("expected an error summary"); + }; + assert_eq!(read.kind, ErrorKind::Other); } #[test] @@ -109,7 +136,7 @@ fn annotates_each_failure_that_was_not_quarantined() { .quarantine_status .quarantine_results = vec![failure("adds", "MathTests", Some("src/math.rs"))]; let mut divides = failure("divides", "MathTests", None); - divides.failure_message = Some(String::from("expected 1, got 2\n100% wrong")); + divides.failure_text = Some(String::from("expected 1, got 2\n100% wrong")); result.quarantine_context.failures = vec![ failure("subtracts", "Math, Tests: unit", Some("src/math.rs")), divides, @@ -236,7 +263,7 @@ fn the_command_prints_annotations_and_exits_0() { let mut result = upload_result(0); result.quarantine_context.exit_code = ExitCode::Failure; let report = UploadReport::new(&result, UploadCommand::Upload); - json::write_file(&path, Report::Upload(&report)).unwrap(); + Report::Upload(&report).write_file(&path).unwrap(); github_annotations(&path).success().stdout( "::error::No tests were found in the provided test results, but non zero exit code provided: 1%0AThe test step appears to have failed without running tests -- see above for the original failure.\n", @@ -248,7 +275,7 @@ fn the_command_exits_non_zero_with_nothing_to_say() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("summary.json"); let report = UploadReport::new(&upload_result(5), UploadCommand::Upload); - json::write_file(&path, Report::Upload(&report)).unwrap(); + Report::Upload(&report).write_file(&path).unwrap(); github_annotations(&path).code(1).stdout(""); diff --git a/cli/tests/summary/schema.rs b/cli/tests/summary/schema.rs index 4606a1bc..7cfbc3e4 100644 --- a/cli/tests/summary/schema.rs +++ b/cli/tests/summary/schema.rs @@ -14,6 +14,6 @@ fn prints_the_summary_output_schema() { assert_eq!( serde_json::from_slice::(&output).unwrap(), - serde_json::to_value(json::schema()).unwrap() + serde_json::to_value(json::Report::schema()).unwrap() ); } diff --git a/cli/tests/summary/upload.rs b/cli/tests/summary/upload.rs index 3c10add0..e3558fb9 100644 --- a/cli/tests/summary/upload.rs +++ b/cli/tests/summary/upload.rs @@ -887,7 +887,7 @@ fn exits_with_the_test_run_exit_code_when_an_error_has_none() { fn upload_json(result: &UploadRunResult) -> serde_json::Value { let report = UploadReport::new(result, UploadCommand::Upload); - serde_json::from_str(&json::to_string(Report::Upload(&report)).unwrap()).unwrap() + serde_json::from_str(&Report::Upload(&report).to_json().unwrap()).unwrap() } #[test] diff --git a/test_report/src/report.rs b/test_report/src/report.rs index 75b7b022..68361ccf 100644 --- a/test_report/src/report.rs +++ b/test_report/src/report.rs @@ -1146,7 +1146,7 @@ impl MutTestReport { fn write_summary_file(path: &Path, report: Report) { eprintln!( "{}", - SummaryFileText::new(path, json::write_file(path, report)).message() + SummaryFileText::new(path, report.write_file(path)).message() ); }