From defe391103dcbfa23b62b3a1e45a7fd85da4782a Mon Sep 17 00:00:00 2001 From: Dylan Frankland <2488926+dfrankland@users.noreply.github.com> Date: Thu, 29 Oct 2026 18:37:44 +0000 Subject: [PATCH] feat(cli): count files with old timestamps in the summary Trunk drops every report that has old (> 24 hour) timestamps when it ingests an upload. The summary only said so in each file's warnings, which the file limit cuts off, so a run could show passing tests that Trunk then dropped. The upload and validate summaries now say how many files have old timestamps, that Trunk will drop their results, and that the test counts include them. They also suggest --bazel-bep-path, which skips results that Bazel restored from its cache. The line is never truncated and shows with --validation-report none too. The JSON summary gets filesWithOldTimestamps and a per-file oldTimestamps, and github-annotations adds a warning. Exit codes and uploads don't change. Refs TRUNK-23000 Co-Authored-By: Claude Opus 5.5 --- cli/src/summary_output/github.rs | 11 +++- cli/src/summary_output/text.rs | 36 ++++++++++++ cli/src/summary_output/upload.rs | 20 ++++++- cli/src/summary_output/validate.rs | 7 ++- cli/src/validate_command.rs | 2 + cli/tests/summary/github.rs | 38 ++++++++++++- cli/tests/summary/upload.rs | 88 ++++++++++++++++++++++++++++++ cli/tests/summary/validate.rs | 35 ++++++++++++ cli/tests/upload.rs | 59 ++++++++++++++++++++ context/src/junit/validator.rs | 12 ++++ 10 files changed, 303 insertions(+), 5 deletions(-) diff --git a/cli/src/summary_output/github.rs b/cli/src/summary_output/github.rs index eeee6a16..2aff36b8 100644 --- a/cli/src/summary_output/github.rs +++ b/cli/src/summary_output/github.rs @@ -10,8 +10,9 @@ use super::{ ErrorSummary, FailedTest, Outcome, Quarantining, UploadReport, 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, + BAZEL_BEP_HINT, 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, + old_timestamps_message, }, }; use crate::{ @@ -74,6 +75,12 @@ fn upload_annotations(report: &UploadReport) -> Vec { } annotations.push(Annotation::warning(error_message(error))); } + if report.files_with_old_timestamps > 0 { + annotations.push(Annotation::warning(format!( + "{}, but the test counts include them.\n{BAZEL_BEP_HINT}", + old_timestamps_message(report.files_with_old_timestamps) + ))); + } let exit_code = report.cli_exit_code(); match report.outcome { diff --git a/cli/src/summary_output/text.rs b/cli/src/summary_output/text.rs index 6763f439..1ef0571f 100644 --- a/cli/src/summary_output/text.rs +++ b/cli/src/summary_output/text.rs @@ -1,5 +1,6 @@ use std::path::{Path, PathBuf}; +use context::junit::validator::TIMESTAMP_OLD_HOURS; use pluralizer::pluralize; use superconsole::{ Hyperlink, Line, Lines, Span, SuperConsole, @@ -81,6 +82,16 @@ pub(crate) const SOME_TESTS_MAY_NOT_HAVE_RUN: &str = "This may indicate that som 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(crate) const BAZEL_BEP_HINT: &str = "If Bazel restored these files from its cache, upload with --bazel-bep-path to skip cached results: https://docs.trunk.io/flaky-tests/get-started/frameworks/bazel"; + +/// Shown whatever `--validation-report` says, since Trunk's ingestion drops these files' results. +pub(crate) fn old_timestamps_message(files: usize) -> String { + format!( + "{} {} test results older than {TIMESTAMP_OLD_HOURS} hours. Trunk will drop these results", + pluralize("file", files as isize, true), + if files == 1 { "has" } else { "have" }, + ) +} pub fn render_error(error: &ErrorSummary) -> anyhow::Result> { let mut lines = Vec::new(); @@ -209,6 +220,12 @@ pub fn render_upload(report: &UploadReport, options: &TextOptions) -> anyhow::Re .attribute(Attribute::Bold), )?])); } + if report.files_with_old_timestamps > 0 { + output.extend(render_old_timestamps(format!( + "{}, but the test counts above include them.", + old_timestamps_message(report.files_with_old_timestamps) + ))?); + } output.push(Line::default()); if quarantined > 0 { @@ -310,6 +327,17 @@ pub fn render_upload(report: &UploadReport, options: &TextOptions) -> anyhow::Re Ok(output) } +fn render_old_timestamps(message: String) -> anyhow::Result> { + Ok(vec![ + Line::from_iter([Span::new_styled_lossy( + style(format!(" ⚠️ {message}")) + .with(Color::Yellow) + .attribute(Attribute::Bold), + )]), + Line::from_iter([Span::new_unstyled(format!(" {BAZEL_BEP_HINT}"))?]), + ]) +} + fn render_groups(groups: &[FailureGroup], options: &TextOptions) -> anyhow::Result { let mut output = Vec::new(); for group in groups { @@ -663,6 +691,7 @@ pub fn render_validate(report: &ValidateReport) -> anyhow::Result> { files_with_errors, warnings, errors, + files_with_old_timestamps, } = report.counts; output.push(Line::from_iter([ Span::new_styled(style(format!("{}", valid_files)).with(Color::Green))?, @@ -694,6 +723,13 @@ pub fn render_validate(report: &ValidateReport) -> anyhow::Result> { Span::new_styled(style(format!("{}", errors)).with(Color::Red))?, Span::new_unstyled(format!(" {}", pluralize("error", errors as isize, false)))?, ])); + if files_with_old_timestamps > 0 { + output.push(Line::default()); + output.extend(render_old_timestamps(format!( + "{} when they're uploaded.", + old_timestamps_message(files_with_old_timestamps) + ))?); + } Ok(output) } diff --git a/cli/src/summary_output/upload.rs b/cli/src/summary_output/upload.rs index 1569e31f..8e3f55df 100644 --- a/cli/src/summary_output/upload.rs +++ b/cli/src/summary_output/upload.rs @@ -35,6 +35,10 @@ pub struct UploadReport { pub upload_url: Option, /// One entry per test report file. pub validations: Vec, + /// Files whose results Trunk will drop for having old timestamps. `counts` includes their + /// tests. + #[serde(default)] + pub files_with_old_timestamps: usize, pub counts: TestCounts, pub quarantining: Quarantining, /// Failed tests that were quarantined, grouped like `failures`. @@ -114,6 +118,9 @@ pub struct FileValidation { pub test_suite_count: Option, /// Null when the file couldn't be parsed. pub test_case_count: Option, + /// The file has a test older than 24 hours, so Trunk will drop all of its results. + #[serde(default)] + pub old_timestamps: bool, } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] @@ -179,6 +186,7 @@ impl UploadReport { UploadCommand::RSpec => None, }; let bundle_upload_id = &result.meta.base_props.bundle_upload_id; + let validations = file_validations(&result.validations); Self { command, @@ -186,7 +194,8 @@ impl UploadReport { dry_run: result.dry_run, bundle_upload_id: Some(bundle_upload_id.clone()).filter(|id| !id.is_empty()), upload_url: upload_url(result), - validations: file_validations(&result.validations), + files_with_old_timestamps: files_with_old_timestamps(&validations), + validations, counts, quarantining: quarantine_context.quarantining(result.disable_quarantining), quarantined: group(quarantined, |test| failed_test(result, test, true)), @@ -228,6 +237,13 @@ impl TestCounts { } } +pub(super) fn files_with_old_timestamps(validations: &[FileValidation]) -> usize { + validations + .iter() + .filter(|validation| validation.old_timestamps) + .count() +} + impl FileValidation { pub fn has_issues(&self) -> bool { self.parse_error.is_some() || !self.errors.is_empty() || !self.warnings.is_empty() @@ -336,6 +352,7 @@ fn file_validations(validations: &JunitReportValidations) -> Vec warnings: Vec::new(), test_suite_count: None, test_case_count: None, + old_timestamps: false, }, Ok(validation) => { let issues = validation.all_issues_flat(); @@ -353,6 +370,7 @@ fn file_validations(validations: &JunitReportValidations) -> Vec warnings: messages(JunitValidationLevel::SubOptimal), test_suite_count: Some(validation.test_suites().len()), test_case_count: Some(validation.test_cases().len()), + old_timestamps: validation.has_old_timestamps(), } } }) diff --git a/cli/src/summary_output/validate.rs b/cli/src/summary_output/validate.rs index 35bbc1d2..2a929e5e 100644 --- a/cli/src/summary_output/validate.rs +++ b/cli/src/summary_output/validate.rs @@ -4,7 +4,7 @@ use context::junit::validator::JunitReportValidationFlatIssue; use schemars::JsonSchema; use serde::Serialize; -use super::{FileValidation, Notice, ParseError}; +use super::{FileValidation, Notice, ParseError, upload::files_with_old_timestamps}; use crate::{ exit_code::ExitCode, validate_command::{ParseIssues, TestIssues, ValidateRunResult}, @@ -49,6 +49,8 @@ pub struct ValidationCounts { pub files_with_errors: usize, pub warnings: usize, pub errors: usize, + /// Files whose results Trunk will drop for having old timestamps. + pub files_with_old_timestamps: usize, } impl ValidateReport { @@ -132,6 +134,7 @@ impl ValidationCounts { files_with_errors: files_with_errors.len(), warnings, errors, + files_with_old_timestamps: files_with_old_timestamps(validations), } } } @@ -145,6 +148,7 @@ fn parse_validation(issues: &ParseIssues) -> FileValidation { warnings: issues.warnings.iter().map(ToString::to_string).collect(), test_suite_count: None, test_case_count: None, + old_timestamps: false, } } @@ -162,5 +166,6 @@ fn test_validation(issues: &TestIssues) -> FileValidation { warnings: messages(&issues.warnings), test_suite_count: Some(issues.num_test_suites), test_case_count: Some(issues.num_test_cases), + old_timestamps: issues.old_timestamps, } } diff --git a/cli/src/validate_command.rs b/cli/src/validate_command.rs index 66204f6c..fb7af472 100644 --- a/cli/src/validate_command.rs +++ b/cli/src/validate_command.rs @@ -102,6 +102,7 @@ pub struct TestIssues { pub(crate) num_test_cases: usize, pub(crate) errors: Vec, pub(crate) warnings: Vec, + pub(crate) old_timestamps: bool, } #[derive(Debug)] @@ -448,6 +449,7 @@ fn gen_test_issues(report_validations: &JunitFileToValidation) -> Vec JunitReportValidations { + JunitReportValidations::new( + (0..num_files) + .map(|n| { + ( + format!("test-results/{n:02}.xml"), + validation(vec![ + report_warning(JunitReportValidationIssueSubOptimal::MissingTimestamps), + report_warning(JunitReportValidationIssueSubOptimal::OldTimestamps), + ]), + ) + }) + .collect(), + ) +} + +#[test] +fn counts_every_file_with_old_timestamps_past_the_file_limit() { + let mut result = upload_result(1344); + result.validations = old_timestamp_validations(112); + + let summary = upload_summary(&result); + assert_eq!( + summary[summary.len() - 8..].to_vec(), + [ + "…and 104 more", + "", + "πŸ“š Test Report", + " Total: 1344 Pass: 1344 Fail: 0 Quarantined: 0 Pass Ratio: 100.0%", + " ⚠️ 112 files have test results older than 24 hours. Trunk will drop these results, but the test counts above include them.", + " If Bazel restored these files from its cache, upload with --bazel-bep-path to skip cached results: https://docs.trunk.io/flaky-tests/get-started/frameworks/bazel", + "", + "πŸŽ‰ No test failures found!", + ] + ); +} + +#[test] +fn writes_the_files_with_old_timestamps_as_json() { + let mut result = upload_result(4); + result.validations = JunitReportValidations::new(BTreeMap::from([ + ( + String::from("test-results/new.xml"), + validation(vec![report_warning( + JunitReportValidationIssueSubOptimal::StaleTimestamps, + )]), + ), + ( + String::from("test-results/old.xml"), + validation(vec![report_warning( + JunitReportValidationIssueSubOptimal::OldTimestamps, + )]), + ), + ])); + + let json = upload_json(&result); + assert_eq!(json["filesWithOldTimestamps"], 1); + assert_eq!( + json["validations"] + .as_array() + .unwrap() + .iter() + .map(|validation| (&validation["file"], &validation["oldTimestamps"])) + .collect::>(), + [ + ( + &serde_json::json!("test-results/new.xml"), + &serde_json::json!(false) + ), + ( + &serde_json::json!("test-results/old.xml"), + &serde_json::json!(true) + ), + ] + ); +} + fn upload_json(result: &UploadRunResult) -> serde_json::Value { let report = UploadReport::new(result, UploadCommand::Upload); serde_json::from_str(&Report::Upload(&report).to_json().unwrap()).unwrap() @@ -996,6 +1081,7 @@ fn writes_the_report_as_json() { "warnings": ["test case or parent has no time duration"], "testSuiteCount": 0, "testCaseCount": 0, + "oldTimestamps": false, }, { "file": "test-results/b.xml", @@ -1007,8 +1093,10 @@ fn writes_the_report_as_json() { "warnings": [], "testSuiteCount": null, "testCaseCount": null, + "oldTimestamps": false, }, ], + "filesWithOldTimestamps": 0, "counts": { "total": 10, "passed": 7, diff --git a/cli/tests/summary/validate.rs b/cli/tests/summary/validate.rs index aab17ee3..0fbba1fd 100644 --- a/cli/tests/summary/validate.rs +++ b/cli/tests/summary/validate.rs @@ -121,3 +121,38 @@ async fn prints_bep_parse_errors_as_notices_without_a_codeowners_file() { ] ); } + +/// One test that ran two days ago: old enough for Trunk to drop, too recent for the validator +/// to discard as a defaulted timestamp. +fn old_timestamp_junit_xml() -> String { + let timestamp = (chrono::Utc::now() - chrono::TimeDelta::days(2)).format("%Y-%m-%dT%H:%M:%SZ"); + format!( + r#" + + + + + +"# + ) +} + +#[tokio::test] +async fn says_that_results_with_old_timestamps_will_be_dropped() { + let temp_dir = tempfile::tempdir().unwrap(); + let path = temp_dir.path().join("old.xml"); + std::fs::write(&path, old_timestamp_junit_xml()).unwrap(); + let report = validate_report(&["--junit-paths", path.to_str().unwrap()]).await; + assert_eq!(report.counts.files_with_old_timestamps, 1); + let summary = unstyled(text::render_validate(&report)); + assert_eq!( + summary[summary.len() - 5..].to_vec(), + [ + "0 valid files, 1 file with warnings, and 0 files with errors, with 1 file total (a file is double counted if it has both errors and warnings)", + "1 warning, and 0 errors", + "", + " ⚠️ 1 file has test results older than 24 hours. Trunk will drop these results when they're uploaded.", + " If Bazel restored these files from its cache, upload with --bazel-bep-path to skip cached results: https://docs.trunk.io/flaky-tests/get-started/frameworks/bazel", + ] + ); +} diff --git a/cli/tests/upload.rs b/cli/tests/upload.rs index 67693004..03d8cd05 100644 --- a/cli/tests/upload.rs +++ b/cli/tests/upload.rs @@ -1619,6 +1619,65 @@ async fn upload_writes_summary_output_file() { } } +/// One test that ran two days ago: old enough for Trunk to drop, too recent for the validator +/// to discard as a defaulted timestamp. +fn old_timestamp_junit_xml() -> String { + let timestamp = (chrono::Utc::now() - chrono::TimeDelta::days(2)).format("%Y-%m-%dT%H:%M:%SZ"); + format!( + r#" + + + + + +"# + ) +} + +#[tokio::test(flavor = "multi_thread")] +async fn upload_says_how_many_files_have_old_timestamps_past_the_file_limit() { + let temp_dir = tempdir().unwrap(); + generate_mock_git_repo(&temp_dir); + for n in 0..10 { + fs::write( + temp_dir.path().join(format!("old-{n}.xml")), + old_timestamp_junit_xml(), + ) + .unwrap(); + } + let summary_path = temp_dir.path().join("summary.json"); + + let state = MockServerBuilder::new().spawn_mock_server().await; + let assert = CommandBuilder::upload(temp_dir.path(), state.host.clone()) + .junit_paths("old-*.xml") + .command() + .arg("--summary-output-file") + .arg(&summary_path) + .assert() + .success() + .stdout(predicate::str::contains("…and 2 more")); + + // Listed once, though 2 of the 10 files are cut from the per-file warnings. + let stdout = String::from_utf8_lossy(&assert.get_output().stdout).into_owned(); + assert_eq!( + stdout + .matches( + "10 files have test results older than 24 hours. Trunk will drop these results, but the test counts above include them." + ) + .count(), + 1 + ); + assert!(stdout.contains("upload with --bazel-bep-path to skip cached results")); + + let requests = state.requests.lock().unwrap().clone(); + assert_eq!(requests.len(), 3); + assert_matches!(requests[1], RequestPayload::S3Upload(_)); + + let summary = read_summary_output_file(&summary_path); + assert_eq!(summary["filesWithOldTimestamps"], 10); + assert_eq!(summary["counts"]["total"], 10); +} + #[tokio::test(flavor = "multi_thread")] async fn upload_writes_a_dry_run_to_summary_output_file() { let temp_dir = tempdir().unwrap(); diff --git a/context/src/junit/validator.rs b/context/src/junit/validator.rs index f920e4be..97a4ac30 100644 --- a/context/src/junit/validator.rs +++ b/context/src/junit/validator.rs @@ -358,6 +358,18 @@ impl JunitReportValidation { .collect() } + /// Trunk drops a report with old timestamps when it ingests the upload. + pub fn has_old_timestamps(&self) -> bool { + self.all_issues.iter().any(|issue| { + matches!( + issue, + JunitValidationIssueType::Report(JunitValidationIssue::SubOptimal( + JunitReportValidationIssueSubOptimal::OldTimestamps + )) + ) + }) + } + pub fn test_suites(&self) -> &[JunitTestSuiteValidation] { &self.test_suites }