diff --git a/crates/mergify-ci/src/junit_process/command.rs b/crates/mergify-ci/src/junit_process/command.rs index 724ba6ee..d32b19f1 100644 --- a/crates/mergify-ci/src/junit_process/command.rs +++ b/crates/mergify-ci/src/junit_process/command.rs @@ -171,12 +171,12 @@ async fn run_with_cap( .map(|c| c.name.clone()) .collect(), }; - let built = spans::build_traces(&parsed, &metadata); + let mut built = spans::build_traces(&parsed, &metadata); // Cap each gzipped upload at MAX_GZIPPED_UPLOAD_BYTES. A normal // report is one chunk (byte-identical to before); only an // oversized payload fans out into several uploads. - let (chunks, oversized_cases, mut upload_error) = + let (chunks, mut oversized_cases, mut upload_error) = match split::split_request(&built.request, upload_cap) { Ok(outcome) => (outcome.chunks, outcome.oversized_cases, None), // gzip is an in-memory write and effectively never fails, @@ -192,6 +192,16 @@ async fn run_with_cap( ), }; + // Cases the span builder refused on name length join the ones the split + // refused on payload size: from the user's side both are "this result was + // not uploaded", and one list is what they need to act on. + // + // Moved, not cloned. Every string in here is over MAX_TEST_NAME_BYTES -- + // that is why it was refused -- so copying them would allocate another + // 65 kB apiece on the one path guaranteed to be holding the biggest names + // in the run. `built` is not read for this field again. + oversized_cases.append(&mut built.oversized_case_names); + let client = upload::default_client(); // Nothing reached the backend when the split produced no chunks // (every case individually oversized) or gzip failed. Captured @@ -603,10 +613,16 @@ fn gha_oversized_annotation(names: &[String]) -> Option { return None; } Some(format!( - "::warning title=Mergify Test Insights::{n} test result(s) exceeded the upload size \ - limit and were skipped: {names}. The rest of the run was uploaded.", + "::warning title=Mergify Test Insights::{n} test result(s) were too large to upload \ + and were skipped: {names}. The rest of the run was uploaded.", n = names.len(), - names = gha_escape_data(&names.join(", ")), + names = gha_escape_data( + &names + .iter() + .map(|n| display_name(n)) + .collect::>() + .join(", "), + ), )) } @@ -640,14 +656,33 @@ fn write_upload_error_block(out: &mut String, error: &str, rejected: bool) { /// is no upload it can fit into. The CI verdict is unaffected (it's /// computed from the parsed cases, not the upload), so this is a /// best-effort data-loss notice, not a failure. +/// How much of a skipped test's name the report prints. A name can be the +/// reason it was skipped, so printing it whole would bury the report under +/// the same 65 kB that caused the problem. +const SKIPPED_NAME_DISPLAY_BYTES: usize = 120; + +/// `name` cut to [`SKIPPED_NAME_DISPLAY_BYTES`] on a character boundary, +/// marked when it was cut so nobody copies the prefix as the real name. +fn display_name(name: &str) -> String { + if name.len() <= SKIPPED_NAME_DISPLAY_BYTES { + return name.to_string(); + } + let mut end = SKIPPED_NAME_DISPLAY_BYTES; + while end > 0 && !name.is_char_boundary(end) { + end -= 1; + } + format!("{}… ({} bytes total)", &name[..end], name.len()) +} + fn write_oversized_cases(out: &mut String, names: &[String]) { - out.push_str("\n ⚠️ Some test results were too large to upload\n"); - out.push_str(" A single test's output exceeded the upload size limit and was skipped.\n"); + out.push_str("\n ⚠️ Some test results were skipped\n"); + out.push_str(" A test whose name or output is too large to upload is skipped; the rest\n"); + out.push_str(" of the run was uploaded. Rename the test to get its history back.\n"); out.push_str(" Quarantine status and CI outcome are unaffected.\n"); out.push('\n'); out.push_str(" ┌ Skipped\n"); for name in names { - out.push_str(&format!(" │ {name}\n")); + out.push_str(&format!(" │ {}\n", display_name(name))); } out.push_str(" └─\n"); } @@ -1025,7 +1060,44 @@ mod tests { .unwrap(); assert!(ann.starts_with("::warning"), "{ann}"); assert!(ann.contains("a.big, b.huge"), "{ann}"); - assert!(ann.contains("exceeded the upload size limit"), "{ann}"); + assert!(ann.contains("too large to upload"), "{ann}"); + } + + /// A name can itself be the reason a case was skipped, so neither the + /// report nor the annotation may print it whole -- 65 kB of generated + /// title would bury the very message telling you which test to rename. + #[test] + fn a_skipped_name_is_truncated_for_display() { + let long = "z".repeat(super::SKIPPED_NAME_DISPLAY_BYTES + 500); + + let shown = display_name(&long); + assert!(shown.len() < long.len(), "{shown}"); + // Marked as cut, and carrying the real size, so nobody copies the + // prefix believing it is the test's name. + assert!(shown.contains('…'), "{shown}"); + assert!( + shown.contains(&format!("{} bytes total", long.len())), + "{shown}" + ); + + let mut report = String::new(); + write_oversized_cases(&mut report, std::slice::from_ref(&long)); + assert!(!report.contains(&long), "the report printed the whole name"); + + let ann = temp_env::with_var("GITHUB_ACTIONS", Some("true"), || { + gha_oversized_annotation(std::slice::from_ref(&long)) + }) + .unwrap(); + assert!( + !ann.contains(&long), + "the annotation printed the whole name" + ); + } + + /// A short name is printed exactly, with no truncation marker. + #[test] + fn a_short_name_is_printed_verbatim() { + assert_eq!(display_name("tests.test_a"), "tests.test_a"); } // ── End-to-end orchestrator tests. Drive the full `run()` diff --git a/crates/mergify-ci/src/junit_process/spans.rs b/crates/mergify-ci/src/junit_process/spans.rs index d4542182..f8d03ee1 100644 --- a/crates/mergify-ci/src/junit_process/spans.rs +++ b/crates/mergify-ci/src/junit_process/spans.rs @@ -47,6 +47,22 @@ pub struct UploadMetadata { pub quarantined: BTreeSet, } +/// The widest test name this client will upload, in bytes. +/// +/// Not a storage limit. The engine keys every table an oversized name can +/// reach on `uuid_generate_v5` of the name rather than the name itself, so +/// the name is stored whole and no btree tuple limit is tripped (MRGFY-8902). +/// What this bounds is what a wire payload and a UI can usefully carry: a +/// name at this size is 65 kB of generated text — INC-2436 was a Vitest title +/// interpolating stringified React source — that no one reads and that costs +/// every downstream reader who has to render it. +/// +/// Skipped rather than truncated, and reported rather than dropped quietly. +/// Truncating would mint a second identity for the same test, which is the +/// one outcome worse than not having the result: the test would split into +/// two histories and its flakiness and quarantine state with it. +pub(crate) const MAX_TEST_NAME_BYTES: usize = 65_536; + /// Result of converting a [`ParseResult`] (one or more `JUnit` /// files) into a wire-ready OTLP request. #[derive(Debug, Clone)] @@ -55,6 +71,10 @@ pub struct BuiltTraces { /// Same value populates the `test.run.id` resource attribute. pub run_id: String, pub request: ExportTraceServiceRequest, + /// Names of cases left out because they exceed + /// [`MAX_TEST_NAME_BYTES`]. Reported to the user by the caller; + /// empty for every ordinary report. + pub oversized_case_names: Vec, } /// Convert a [`ParseResult`] (the union of every parsed `JUnit` @@ -87,6 +107,7 @@ fn build_traces_with( let common_attrs = common_attributes(metadata); let mut spans: Vec = Vec::new(); + let mut oversized_case_names: Vec = Vec::new(); // Suite spans are appended after we know each suite's earliest // case start (so the suite's start_time covers all its cases). @@ -101,7 +122,17 @@ fn build_traces_with( let suite_span_id = rng.bytes8(); let mut suite_start_time_unix_nanos = now_unix_nanos; + let mut suite_case_count = 0_usize; + for case in suite_cases { + // `len()` on a Rust `String` is already bytes, which is the unit + // the limit is stated in. + if case.name.len() > MAX_TEST_NAME_BYTES { + oversized_case_names.push(case.name.clone()); + continue; + } + suite_case_count += 1; + let case_span_id = rng.bytes8(); let start_time_unix_nanos = case_start_time(now_unix_nanos, case.duration); suite_start_time_unix_nanos = suite_start_time_unix_nanos.min(start_time_unix_nanos); @@ -164,6 +195,13 @@ fn build_traces_with( }); } + // A suite that kept nothing emits nothing: a suite span with no + // children is a test suite the backend would show as having run zero + // cases, which is a different lie from the one being fixed. + if suite_case_count == 0 { + continue; + } + session_start_time_unix_nanos = session_start_time_unix_nanos.min(suite_start_time_unix_nanos); @@ -234,6 +272,7 @@ fn build_traces_with( request: ExportTraceServiceRequest { resource_spans: vec![resource_spans], }, + oversized_case_names, } } @@ -470,6 +509,98 @@ mod tests { } } + /// A name past the cap costs its own result and nothing else. + #[test] + fn an_oversized_name_is_skipped_and_its_suite_mates_still_upload() { + let mut parsed = sample_parsed(); + // One byte over, so the assertion is about the boundary and not about + // some comfortably-huge number that a wrong `>=` would also pass. + let oversized = "x".repeat(MAX_TEST_NAME_BYTES + 1); + parsed.cases.push(TestCase { + name: oversized.clone(), + suite_name: "pytest".to_string(), + duration: Some(Duration::from_secs_f64(0.003)), + file: None, + line: None, + status: TestStatus::Passed, + failure: Failure::default(), + }); + + let mut bytes: Vec = Vec::with_capacity(16 + 4 * 8); + bytes.extend(std::iter::repeat_n(0xAA, 16)); + bytes.extend(std::iter::repeat_n(0x11, 8)); + bytes.extend(std::iter::repeat_n(0x22, 8)); + bytes.extend(std::iter::repeat_n(0x33, 8)); + bytes.extend(std::iter::repeat_n(0x44, 8)); + let mut rng = FixedRng::new(bytes); + + let now: u64 = 1_700_000_000_000_000_000; + let metadata = UploadMetadata::default(); + let built = with_ci_env(&[], || build_traces_with(&parsed, &metadata, now, &mut rng)); + + assert_eq!(built.oversized_case_names, vec![oversized.clone()]); + + let spans = &built.request.resource_spans[0].scope_spans[0].spans; + // Reported, not uploaded -- in no span name and no attribute value. + assert!(!spans.iter().any(|s| s.name == oversized)); + assert!(!spans.iter().any(|s| s.attributes.iter().any(|a| { + matches!( + &a.value.as_ref().and_then(|v| v.value.as_ref()), + Some(AnyValueOneof::StringValue(v)) if v == &oversized + ) + }))); + // The two well-named cases in the same suite are untouched. + assert!( + spans + .iter() + .any(|s| s.name == "tests.test_func.test_success") + ); + assert!( + spans + .iter() + .any(|s| s.name == "tests.test_func.test_failed") + ); + } + + /// A name exactly at the cap is uploaded: the limit is inclusive, and a + /// test sitting on it must not silently lose its history. + #[test] + fn a_name_exactly_at_the_cap_is_still_uploaded() { + let mut parsed = sample_parsed(); + let at_cap = "y".repeat(MAX_TEST_NAME_BYTES); + parsed.cases.push(TestCase { + name: at_cap.clone(), + suite_name: "pytest".to_string(), + duration: Some(Duration::from_secs_f64(0.003)), + file: None, + line: None, + status: TestStatus::Passed, + failure: Failure::default(), + }); + + let mut bytes: Vec = Vec::with_capacity(16 + 5 * 8); + bytes.extend(std::iter::repeat_n(0xAA, 16)); + bytes.extend(std::iter::repeat_n(0x11, 8)); + bytes.extend(std::iter::repeat_n(0x22, 8)); + bytes.extend(std::iter::repeat_n(0x33, 8)); + bytes.extend(std::iter::repeat_n(0x44, 8)); + bytes.extend(std::iter::repeat_n(0x55, 8)); + let mut rng = FixedRng::new(bytes); + + let built = with_ci_env(&[], || { + build_traces_with( + &parsed, + &UploadMetadata::default(), + 1_700_000_000_000_000_000, + &mut rng, + ) + }); + + assert!(built.oversized_case_names.is_empty()); + let spans = &built.request.resource_spans[0].scope_spans[0].spans; + assert!(spans.iter().any(|s| s.name == at_cap)); + } + #[test] fn builds_session_suite_and_case_spans_with_consistent_parent_chain() { // 16 bytes for trace_id; 4×8 bytes for session, suite,