From 17f8da61c655f6aa7b785efd8a5ad752fea78692 Mon Sep 17 00:00:00 2001 From: Dylan Frankland Date: Mon, 31 Aug 2026 14:22:30 -0700 Subject: [PATCH] fix(xcresult): stop dropping nested test suites MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A suite nested inside another suite, and every test it declared, was silently discarded. The traversal took only a suite's direct `Test Case` children, so a `Test Suite` child was never visited. The tests did not merely go missing — their results went with them. Against a swift-testing bundle with a nested suite, the pre-fix traversal emits tests="2" failures="0" where the correct output is `tests="4" failures="1"`, so a run containing a failing test reported a clean bill of health. JUnit has no nested ``, so a nested suite now becomes one of its own under a dot-qualified name (`Bundle.Outer.Inner`) — the convention the bundle prefix already used. The change is additive: an outer suite with no direct cases still emits its own `` exactly as before, and the inner ones appear alongside it. No existing expected JUnit moves, because no fixture in `tests/data/` had a nested suite — which is why this went unnoticed. Co-Authored-By: Claude Opus 5 (1M context) --- xcresult/CONTRIBUTING.md | 9 ++- xcresult/src/xcresult.rs | 136 ++++++++++++++++++++++++++++++++++----- 2 files changed, 127 insertions(+), 18 deletions(-) diff --git a/xcresult/CONTRIBUTING.md b/xcresult/CONTRIBUTING.md index 42484e7a..afb38e89 100644 --- a/xcresult/CONTRIBUTING.md +++ b/xcresult/CONTRIBUTING.md @@ -4,11 +4,16 @@ The `xcresult` crate exists to handle converting between xcresult and JUnit form ## Purpose -This crate serves two main purposes: +This crate serves three main purposes: 1. **Format Conversion**: Converts xcresult bundles (produced by Xcode test runs) into JUnit XML format for compatibility with various CI/CD systems and test reporting tools. -2. **Conditional File Path Specification**: While there are other xcresult parses, this crate handles specifying file paths in the JUnit output, which are conditionally present based on whether a failure (not error) has occurred. File paths are only included in the JUnit output when a test case has failed, as they are extracted from failure summaries in the xcresult bundle. This also handles generating stable identfiers because, by default, one of the values we generate IDs from is the file path. Without this crate, we wouldn't be able to safely map files to tests nor have codeowners support for xcresult. +2. **Suite flattening**: JUnit has no nested ``, so a suite nested inside another + becomes its own with a dot-qualified name (`Bundle.Outer.Inner`). This is not cosmetic — + emitting only the outer suite silently drops every test the inner ones declare, and their + failures with them, which is what used to happen to nested swift-testing suites. + +3. **Conditional File Path Specification**: While there are other xcresult parses, this crate handles specifying file paths in the JUnit output, which are conditionally present based on whether a failure (not error) has occurred. File paths are only included in the JUnit output when a test case has failed, as they are extracted from failure summaries in the xcresult bundle. This also handles generating stable identfiers because, by default, one of the values we generate IDs from is the file path. Without this crate, we wouldn't be able to safely map files to tests nor have codeowners support for xcresult. ## Running the Binary diff --git a/xcresult/src/xcresult.rs b/xcresult/src/xcresult.rs index 18335f1e..167c7188 100644 --- a/xcresult/src/xcresult.rs +++ b/xcresult/src/xcresult.rs @@ -135,12 +135,7 @@ impl XCResult { ) } else if matches!(test_bundle_or_test_suite.node_type, TestNodeType::TestSuite) { let test_suite = test_bundle_or_test_suite; - vec![ - self.xcresult_test_suite_to_junit_test_suite( - test_suite, - Option::<&str>::None, - ), - ] + self.xcresult_test_suite_to_junit_test_suites(test_suite, None) } else { vec![] } @@ -153,11 +148,12 @@ impl XCResult { test_nodes: &[TestNode], bundle_name: Option, ) -> Vec { + let qualifier = bundle_name.as_ref().map(|bn| bn.as_ref()); let mut test_suites = test_nodes .iter() .filter(|tn| matches!(tn.node_type, TestNodeType::TestSuite)) - .map(|test_suite| { - self.xcresult_test_suite_to_junit_test_suite(test_suite, bundle_name.as_ref()) + .flat_map(|test_suite| { + self.xcresult_test_suite_to_junit_test_suites(test_suite, qualifier) }) .collect::>(); // test cases can be at the top level @@ -178,20 +174,33 @@ impl XCResult { test_suites } - fn xcresult_test_suite_to_junit_test_suite>( + /// JUnit has no nested ``, so a suite nested inside another becomes one of its + /// own under a dot-qualified name. Returning a `Vec` is the fix: taking only the direct + /// `Test Case` children silently dropped every test an inner suite declared. + fn xcresult_test_suite_to_junit_test_suites( &self, xcresult_test_suite: &TestNode, - bundle_name: Option, - ) -> TestSuite { - let name = bundle_name - .as_ref() - .map(|bn| format!("{}.{}", bn.as_ref(), xcresult_test_suite.name)) + qualifier: Option<&str>, + ) -> Vec { + let name = qualifier + .map(|qualifier| format!("{}.{}", qualifier, xcresult_test_suite.name)) .unwrap_or_else(|| String::from(&xcresult_test_suite.name)); - let mut test_suite = TestSuite::new(name); + let mut test_suite = TestSuite::new(name.clone()); test_suite.add_test_cases( self.xcresult_test_cases_to_junit_test_cases(xcresult_test_suite.children.as_slice()), ); - test_suite + + let mut test_suites = vec![test_suite]; + test_suites.extend( + xcresult_test_suite + .children + .iter() + .filter(|tn| matches!(tn.node_type, TestNodeType::TestSuite)) + .flat_map(|nested| { + self.xcresult_test_suite_to_junit_test_suites(nested, Some(name.as_str())) + }), + ); + test_suites } fn xcresult_test_cases_to_junit_test_cases(&self, test_nodes: &[TestNode]) -> Vec { @@ -358,3 +367,98 @@ impl XCResult { None } } + +#[cfg(test)] +mod tests { + use serde_json::{Value, json}; + + use super::*; + + fn suite(name: &str, children: Vec) -> Value { + json!({ "nodeType": "Test Suite", "name": name, "children": children }) + } + + fn case(name: &str) -> Value { + json!({ + "nodeType": "Test Case", + "name": name, + "nodeIdentifier": name, + "result": "Passed", + "children": [] + }) + } + + fn bundle(name: &str, children: Vec) -> Tests { + serde_json::from_value(json!({ + "testPlanConfigurations": [], + "devices": [], + "testNodes": [{ + "nodeType": "Test Plan", + "name": "ExamplePlan", + "children": [{ + "nodeType": "Unit test bundle", + "name": name, + "children": children + }] + }] + })) + .unwrap() + } + + fn suites_and_cases(tests: Tests) -> Vec<(String, Vec)> { + let xcresult = XCResult { + tests, + org_url_slug: String::from("trunk"), + repo_full_name: String::from("github.com/trunk-io/analytics-cli"), + legacy_xcresult_tests: HashMap::new(), + test_run_started_at: None, + }; + let mut reports = xcresult.generate_junits(); + assert_eq!(reports.len(), 1); + reports + .pop() + .unwrap() + .test_suites + .iter() + .map(|test_suite| { + ( + test_suite.name.as_str().to_owned(), + test_suite + .test_cases + .iter() + .map(|test_case| test_case.name.as_str().to_owned()) + .collect(), + ) + }) + .collect() + } + + // A `Test Suite` nested in a `Test Suite` was never visited, so every test it declared + // vanished from the upload — and any failure among them with it. + #[test] + fn nested_suites_are_flattened_rather_than_dropped() { + let tests = bundle( + "ExampleTests", + vec![suite( + "OuterSuite", + vec![ + case("outerCase()"), + suite("InnerSuite", vec![case("innerCase()")]), + ], + )], + ); + assert_eq!( + suites_and_cases(tests), + vec![ + ( + String::from("ExampleTests.OuterSuite"), + vec![String::from("outerCase()")] + ), + ( + String::from("ExampleTests.OuterSuite.InnerSuite"), + vec![String::from("innerCase()")] + ), + ] + ); + } +}