From 74d789a2e8d5ac45b1bafd679ca2da2c2c3a6530 Mon Sep 17 00:00:00 2001 From: Mehdi ABAAKOUK Date: Mon, 31 Aug 2026 12:07:39 +0200 Subject: [PATCH] fix(ci): skip a test whose name is too large and say which one A test name arrives from JUnit unbounded and goes straight onto the wire: `build_traces` copies it into the span name and two attributes. INC-2436 was a Vitest title interpolating stringified React source at 31,207 characters, and nothing between the runner and the backend refused it. Skip a case whose name exceeds 65,536 bytes and report it through the path already built for cases too large to upload: the human report and, on GitHub Actions, a `::warning::` annotation. The CI outcome is untouched, so a skipped result never breaks a customer's build. A suite left with no cases emits no span rather than an empty suite. Skipped rather than truncated on purpose. The backend identifies a test by `uuid_generate_v5` of its name, so a truncated name is a different test: the result would split its history, flakiness and quarantine state in two. Not uploading it says so plainly, and names it so the owner can rename the test. Names are cut to 120 bytes for display in both surfaces, since the name is itself what made the case too large. Fixes MRGFY-8951 Related to MRGFY-8902 Change-Id: I224cd6b770d1228bc0ae884ec546b05a94102e3d --- .../mergify-ci/src/junit_process/command.rs | 90 ++++++++++-- crates/mergify-ci/src/junit_process/spans.rs | 131 ++++++++++++++++++ 2 files changed, 212 insertions(+), 9 deletions(-) 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,