From df21a3ffc79ae384496e29ba4d89bb5bd155d03d Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 13 Jul 2026 22:49:18 +0000 Subject: [PATCH 1/3] fix(context): emit a run per Playwright rerun/flaky attempt into_test_case_runs() walked each once and pattern-matched the status with `..`, discarding the parsed flaky_runs (on Success) and reruns (on NonSuccess). A passing Playwright testcase carrying a for a failed attempt was therefore emitted as a single SUCCESS run, so pass-on-retry flakes never surfaced. Emit an additional TestCaseRun (as a failure, with its own output and timing) for each parsed rerun/flaky attempt, in addition to the final attempt. The runs flow straight into the uploaded bin report, so no separate change is needed there. Fixes TRUNK-18796. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_017d3M95mohe3LhktThN2rxz --- context/src/junit/parser.rs | 174 ++++++++++++++++++++++++++++++++++++ 1 file changed, 174 insertions(+) diff --git a/context/src/junit/parser.rs b/context/src/junit/parser.rs index d86148e0..6c367bbc 100644 --- a/context/src/junit/parser.rs +++ b/context/src/junit/parser.rs @@ -296,6 +296,11 @@ impl JunitParser { }); } } + let reruns: Vec = match &test_case.status { + TestCaseStatus::Success { flaky_runs } => flaky_runs.clone(), + TestCaseStatus::NonSuccess { reruns, .. } => reruns.clone(), + TestCaseStatus::Skipped { .. } => Vec::new(), + }; test_case_run.status = match test_case.status { TestCaseStatus::Success { .. } => TestCaseRunStatus::Success.into(), TestCaseStatus::Skipped { @@ -449,6 +454,9 @@ impl JunitParser { )); } + for rerun in &reruns { + test_case_runs.push(test_case_run_for_rerun(&test_case_run, rerun)); + } test_case_runs.push(test_case_run); } } @@ -920,6 +928,62 @@ fn resolve_non_success_system_output( ) } +/// Builds a `TestCaseRun` for a prior rerun/flaky attempt, inheriting `base`'s identity (test id, +/// file, codeowners, quarantine state, ...) but recording the attempt as a failure with its own +/// output and timing. Emitting these alongside the final attempt is what lets pass-on-retry flakes +/// surface — otherwise every attempt collapses into the single final run. +fn test_case_run_for_rerun(base: &TestCaseRun, rerun: &TestRerun) -> TestCaseRun { + let mut run = base.clone(); + run.status = TestCaseRunStatus::Failure.into(); + + let message = rerun.message.as_deref(); + let description = rerun.description.as_deref(); + let system_out = rerun.system_out.as_deref(); + let system_err = rerun.system_err.as_deref(); + + run.status_output_message = description + .or(message) + .map(str::to_string) + .unwrap_or_default(); + + run.test_output = if message.is_some() + || description.is_some() + || system_out.is_some() + || system_err.is_some() + { + Some(TestOutput { + message: message.map(str::to_string).unwrap_or_default(), + text: description.map(str::to_string).unwrap_or_default(), + system_out: system_out.map(str::to_string).unwrap_or_default(), + system_err: system_err.map(str::to_string).unwrap_or_default(), + }) + } else { + None + }; + + if let Some(timestamp) = rerun.timestamp { + run.started_at = Some(Timestamp { + seconds: timestamp.timestamp(), + nanos: timestamp.timestamp_subsec_nanos() as i32, + }); + } + if let Some(time) = rerun.time { + if run.started_at.is_none() { + run.started_at = Some(Timestamp { + seconds: 0, + nanos: 0, + }); + } + run.finished_at = run.started_at.clone().map(|mut v| { + v.seconds += time.as_secs() as i64; + v.nanos += time.subsec_nanos() as i32; + v + }); + } + + run +} + /// Converts `//pkg/path:target` to `Some("pkg/path")` for codeowners lookup. /// Returns `None` if the label has no package component (e.g. `//:target`). fn bazel_label_to_package_path(label: &str) -> Option { @@ -1881,4 +1945,114 @@ failures: let test_output = failing.test_output.as_ref().unwrap(); assert!(test_output.system_out.is_empty()); } + + #[test] + fn test_into_test_case_runs_emits_run_per_flaky_rerun() { + // Playwright reports a pass-on-retry as a passing carrying a for + // each failed attempt. Each attempt must surface as its own run so the failed attempt is + // not lost and the test can be classified flaky. + let mut junit_parser = JunitParser::new(); + let file_contents = r#" + + + + + + + + + + + + "#; + junit_parser + .parse(BufReader::new(file_contents.as_bytes())) + .unwrap(); + + let test_case_runs = junit_parser.into_test_case_runs(IntoTestCaseRunsOptions { + org_slug: "org", + repo: &RepoUrlParts { + host: "host".into(), + owner: "owner".into(), + name: "name".into(), + }, + codeowners: None, + quarantined_test_ids: &[], + variant: "", + test_runner_config: None, + }); + + assert_eq!(test_case_runs.len(), 2); + + let rerun = &test_case_runs[0]; + let final_run = &test_case_runs[1]; + + // The failed attempt comes first, the final passing attempt last. + assert_eq!(rerun.status, TestCaseRunStatus::Failure as i32); + assert_eq!(final_run.status, TestCaseRunStatus::Success as i32); + + // Both attempts describe the same test. + assert_eq!(rerun.name, "should show error toast"); + assert_eq!(final_run.name, "should show error toast"); + assert_eq!(rerun.parent_name, final_run.parent_name); + assert_eq!(rerun.classname, final_run.classname); + + let test_output = rerun + .test_output + .as_ref() + .expect("expected test output for flaky attempt"); + assert_eq!(test_output.message, "Test timeout of 5000ms exceeded."); + assert_eq!(test_output.system_out, "attempt output"); + assert!(final_run.test_output.is_none()); + } + + #[test] + fn test_into_test_case_runs_emits_run_per_failing_rerun() { + // A test that fails every attempt records the earlier attempts as their own + // runs in addition to the final failure. + let mut junit_parser = JunitParser::new(); + let file_contents = r#" + + + + + + + + + + "#; + junit_parser + .parse(BufReader::new(file_contents.as_bytes())) + .unwrap(); + + let test_case_runs = junit_parser.into_test_case_runs(IntoTestCaseRunsOptions { + org_slug: "org", + repo: &RepoUrlParts { + host: "host".into(), + owner: "owner".into(), + name: "name".into(), + }, + codeowners: None, + quarantined_test_ids: &[], + variant: "", + test_runner_config: None, + }); + + assert_eq!(test_case_runs.len(), 2); + assert!( + test_case_runs + .iter() + .all(|run| run.status == TestCaseRunStatus::Failure as i32) + ); + // The earlier rerun attempt is emitted first, the final failure last. + assert_eq!( + test_case_runs[0].test_output.as_ref().unwrap().message, + "first failure" + ); + assert_eq!( + test_case_runs[1].test_output.as_ref().unwrap().message, + "final failure" + ); + } } From 6f446901743bcfa97c0b423e541073d0c168057d Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 13 Jul 2026 23:18:57 +0000 Subject: [PATCH 2/3] test(cli): derive expected run count from parsed reruns upload_bundle and upload_bundle_using_bep hardcoded 500 test_case_runs, which only held because reruns were dropped. Now that each parsed rerun/flaky attempt is emitted as its own run, the total equals the test cases plus their (randomly generated) reruns. Compute the expected count from the bundled JUnit so the assertion is seed-independent. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_017d3M95mohe3LhktThN2rxz --- cli/tests/upload.rs | 39 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-) diff --git a/cli/tests/upload.rs b/cli/tests/upload.rs index db3ca4c8..5f83445a 100644 --- a/cli/tests/upload.rs +++ b/cli/tests/upload.rs @@ -44,6 +44,33 @@ use test_utils::{ }; use trunk_analytics_cli::upload_command::{DRY_RUN_OUTPUT_DIR, get_bundle_upload_id_message}; +// The mock JUnit generator adds a random number of reruns per test, so the number of +// `TestCaseRun`s equals the test cases plus every parsed rerun/flaky attempt. Mirror +// `into_test_case_runs` to derive the expected count from the bundled JUnit rather than hardcoding +// a seed-dependent number. +fn expected_test_case_run_count(reports: &[quick_junit::Report]) -> usize { + reports + .iter() + .flat_map(|report| report.test_suites.iter()) + .flat_map(|test_suite| test_suite.test_cases.iter()) + .map(|test_case| { + 1 + match &test_case.status { + quick_junit::TestCaseStatus::Success { flaky_runs } => flaky_runs.len(), + quick_junit::TestCaseStatus::NonSuccess { reruns, .. } => reruns.len(), + quick_junit::TestCaseStatus::Skipped { .. } => 0, + } + }) + .sum() +} + +fn expected_test_case_run_count_from_junit(path: &std::path::Path) -> usize { + let mut junit_parser = JunitParser::new(); + junit_parser + .parse(BufReader::new(fs::File::open(path).unwrap())) + .unwrap(); + expected_test_case_run_count(&junit_parser.into_reports()) +} + // NOTE: must be multi threaded to start a mock server #[tokio::test(flavor = "multi_thread")] async fn upload_bundle() { @@ -218,7 +245,10 @@ async fn upload_bundle() { assert_eq!(report.test_results.len(), 1); let report = report.test_results.first().unwrap(); assert_eq!(report.test_build_information, None); - assert_eq!(report.test_case_runs.len(), 500); + assert_eq!( + report.test_case_runs.len(), + expected_test_case_run_count_from_junit(&tar_extract_directory.join(&bundled_file.path)) + ); let test_case_run = &report.test_case_runs[0]; assert!(test_case_run.id.is_empty()); assert!(!test_case_run.name.is_empty()); @@ -328,6 +358,7 @@ async fn upload_bundle_using_bep() { assert!(!bundle_meta.base_props.file_sets.is_empty()); assert_eq!(bundle_meta.base_props.test_collection, None); + let mut expected_runs = 0usize; bundle_meta .base_props .file_sets @@ -340,7 +371,9 @@ async fn upload_bundle_using_bep() { assert!(junit_parser.parse(BufReader::new(junit_file)).is_ok()); assert!(junit_parser.issues().is_empty()); }); - let report = junit_parser.into_reports().pop().unwrap(); + let mut reports = junit_parser.into_reports(); + expected_runs += expected_test_case_run_count(&reports); + let report = reports.pop().unwrap(); let test_runner_report = file_set.test_runner_report.clone().unwrap(); assert_eq!( test_runner_report.resolved_status, @@ -376,7 +409,7 @@ async fn upload_bundle_using_bep() { assert_eq!(report.test_results.len(), 1); let report = report.test_results.first().unwrap(); - assert_eq!(report.test_case_runs.len(), 500); + assert_eq!(report.test_case_runs.len(), expected_runs); assert!(report.test_build_information.is_some()); let test_build_information = assert_matches!( report.test_build_information.as_ref(), From 73777091fdd270b6394566f0b0d4331a4a433938 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 14 Jul 2026 06:04:21 +0000 Subject: [PATCH 3/3] refactor(context): iterate reruns by reference; assert full run vec in tests Address review feedback: - Borrow test_case.status when setting the run status so the rerun/flaky attempts can be iterated by reference at emit time, dropping the cloned Vec. - Assert over the entire test_case_runs vec in the rerun tests rather than field-by-field. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_017d3M95mohe3LhktThN2rxz --- context/src/junit/parser.rs | 163 +++++++++++++++++++++++++----------- 1 file changed, 115 insertions(+), 48 deletions(-) diff --git a/context/src/junit/parser.rs b/context/src/junit/parser.rs index 6c367bbc..e0e0c2b5 100644 --- a/context/src/junit/parser.rs +++ b/context/src/junit/parser.rs @@ -296,12 +296,7 @@ impl JunitParser { }); } } - let reruns: Vec = match &test_case.status { - TestCaseStatus::Success { flaky_runs } => flaky_runs.clone(), - TestCaseStatus::NonSuccess { reruns, .. } => reruns.clone(), - TestCaseStatus::Skipped { .. } => Vec::new(), - }; - test_case_run.status = match test_case.status { + test_case_run.status = match &test_case.status { TestCaseStatus::Success { .. } => TestCaseRunStatus::Success.into(), TestCaseStatus::Skipped { message, @@ -321,8 +316,14 @@ impl JunitParser { || system_err.is_some() { test_case_run.test_output = Some(TestOutput { - message: message.map(|m| m.to_string()).unwrap_or_default(), - text: description.map(|d| d.to_string()).unwrap_or_default(), + message: message + .as_ref() + .map(|m| m.to_string()) + .unwrap_or_default(), + text: description + .as_ref() + .map(|d| d.to_string()) + .unwrap_or_default(), system_out: system_out .map(|s| s.to_string()) .unwrap_or_default(), @@ -356,8 +357,14 @@ impl JunitParser { || system_err.is_some() { test_case_run.test_output = Some(TestOutput { - message: message.map(|m| m.to_string()).unwrap_or_default(), - text: description.map(|d| d.to_string()).unwrap_or_default(), + message: message + .as_ref() + .map(|m| m.to_string()) + .unwrap_or_default(), + text: description + .as_ref() + .map(|d| d.to_string()) + .unwrap_or_default(), system_out: system_out .map(|s| s.to_string()) .unwrap_or_default(), @@ -454,8 +461,15 @@ impl JunitParser { )); } - for rerun in &reruns { - test_case_runs.push(test_case_run_for_rerun(&test_case_run, rerun)); + let reruns = match &test_case.status { + TestCaseStatus::Success { flaky_runs } => Some(flaky_runs.iter()), + TestCaseStatus::NonSuccess { reruns, .. } => Some(reruns.iter()), + TestCaseStatus::Skipped { .. } => None, + }; + if let Some(reruns) = reruns { + for rerun in reruns { + test_case_runs.push(test_case_run_for_rerun(&test_case_run, rerun)); + } } test_case_runs.push(test_case_run); } @@ -1186,7 +1200,9 @@ mod tests { use std::io::BufReader; use prost_wkt_types::Timestamp; - use proto::test_context::test_run::{AttemptNumber, LineNumber, TestCaseRunStatus, TestOutput}; + use proto::test_context::test_run::{ + AttemptNumber, LineNumber, TestCaseRun, TestCaseRunStatus, TestOutput, + }; use crate::{ junit::parser::{IntoTestCaseRunsOptions, JunitParser, bazel_label_to_package_path}, @@ -1947,6 +1963,7 @@ failures: } #[test] + #[allow(deprecated)] fn test_into_test_case_runs_emits_run_per_flaky_rerun() { // Playwright reports a pass-on-retry as a passing carrying a for // each failed attempt. Each attempt must surface as its own run so the failed attempt is @@ -1982,31 +1999,50 @@ failures: test_runner_config: None, }); - assert_eq!(test_case_runs.len(), 2); - - let rerun = &test_case_runs[0]; - let final_run = &test_case_runs[1]; - - // The failed attempt comes first, the final passing attempt last. - assert_eq!(rerun.status, TestCaseRunStatus::Failure as i32); - assert_eq!(final_run.status, TestCaseRunStatus::Success as i32); - - // Both attempts describe the same test. - assert_eq!(rerun.name, "should show error toast"); - assert_eq!(final_run.name, "should show error toast"); - assert_eq!(rerun.parent_name, final_run.parent_name); - assert_eq!(rerun.classname, final_run.classname); - - let test_output = rerun - .test_output - .as_ref() - .expect("expected test output for flaky attempt"); - assert_eq!(test_output.message, "Test timeout of 5000ms exceeded."); - assert_eq!(test_output.system_out, "attempt output"); - assert!(final_run.test_output.is_none()); + // The failed attempt is emitted first (as a failure carrying its own output), the final + // passing attempt last; both describe the same test. + let shared = TestCaseRun { + name: "should show error toast".into(), + parent_name: "team-settings.pwtest.ts".into(), + classname: "team-settings.pwtest.ts".into(), + started_at: Some(Timestamp { + seconds: 1783471539, + nanos: 0, + }), + ..Default::default() + }; + assert_eq!( + test_case_runs, + vec![ + TestCaseRun { + status: TestCaseRunStatus::Failure as i32, + status_output_message: "Test timeout of 5000ms exceeded.".into(), + test_output: Some(TestOutput { + message: "Test timeout of 5000ms exceeded.".into(), + text: String::new(), + system_out: "attempt output".into(), + system_err: String::new(), + }), + finished_at: Some(Timestamp { + seconds: 1783471544, + nanos: 595_000_000, + }), + ..shared.clone() + }, + TestCaseRun { + status: TestCaseRunStatus::Success as i32, + finished_at: Some(Timestamp { + seconds: 1783471540, + nanos: 799_000_000, + }), + ..shared + }, + ] + ); } #[test] + #[allow(deprecated)] fn test_into_test_case_runs_emits_run_per_failing_rerun() { // A test that fails every attempt records the earlier attempts as their own // runs in addition to the final failure. @@ -2039,20 +2075,51 @@ failures: test_runner_config: None, }); - assert_eq!(test_case_runs.len(), 2); - assert!( - test_case_runs - .iter() - .all(|run| run.status == TestCaseRunStatus::Failure as i32) - ); - // The earlier rerun attempt is emitted first, the final failure last. - assert_eq!( - test_case_runs[0].test_output.as_ref().unwrap().message, - "first failure" - ); + // The earlier rerun attempt is emitted first, the final failure last; both are recorded as + // failures. + let shared = TestCaseRun { + name: "always_fails".into(), + parent_name: "suite".into(), + classname: "suite".into(), + status: TestCaseRunStatus::Failure as i32, + started_at: Some(Timestamp { + seconds: 1783471539, + nanos: 0, + }), + ..Default::default() + }; assert_eq!( - test_case_runs[1].test_output.as_ref().unwrap().message, - "final failure" + test_case_runs, + vec![ + TestCaseRun { + status_output_message: "first failure".into(), + test_output: Some(TestOutput { + message: "first failure".into(), + text: String::new(), + system_out: String::new(), + system_err: String::new(), + }), + finished_at: Some(Timestamp { + seconds: 1783471539, + nanos: 500_000_000, + }), + ..shared.clone() + }, + TestCaseRun { + status_output_message: "final failure".into(), + test_output: Some(TestOutput { + message: "final failure".into(), + text: String::new(), + system_out: String::new(), + system_err: String::new(), + }), + finished_at: Some(Timestamp { + seconds: 1783471540, + nanos: 0, + }), + ..shared + }, + ] ); } }