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 }