From 52b49237a0d11f686c23e2ff84d5dd72da9dc76c Mon Sep 17 00:00:00 2001 From: Dylan Frankland Date: Tue, 18 Aug 2026 11:47:58 -0700 Subject: [PATCH] fix(xcresult): attribute a failure to the test's own frame, not a dependency's MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The file we report for a test is whatever the failure summary points at, and for a failure raised inside a helper that is the helper's file: a snapshot trait, a mocking framework, a page object, a launch helper. The path lands under the package checkout (`Tuist/.build/checkouts/...`, `DerivedData/SourcePackages/checkouts/...`), and since codeowners are resolved from that path the test is then owned by whoever owns the vendored directory. Adds `FileSource::TestFrame`, the frame whose symbol names the test. Frames run innermost first, so the test's own frame sits in the middle of the stack — helpers it called below it, the framework that invoked it above — which is why taking the last frame lands on a dependency. `TestIdentity` owns the spellings a frame can use: Swift (`Suite.testCase()`), Objective-C (`-[Suite testCase]`), closures declared inside the test, and top-level swift-testing functions, which have no suite. That frame is the only source that identifies the test rather than the failure, which `FileSource::is_positive_identification` states. Everything else is vetted against `ReportedPath::is_vendored_dependency` in a single `find`, so a source added later cannot quietly skip the check — previously the same filter was repeated at each link of the chain. The stack fallback now yields its frames outermost-first instead of collapsing to a single "last frame", so rejecting one lands on the next frame out rather than giving up. When a test crashes or fails to launch it never reaches its own frame and every source points into a dependency; we then report no file at all rather than one that would re-own the test. Consumers already treat a missing file as "unchanged" rather than "cleared", so the test keeps the path and owners it last had. Note this also changes attribution for a failure raised inside an in-repo helper: the test's own file now wins over the helper's. That is the intended reading of "the file of the test case". Co-Authored-By: Claude Opus 5 --- xcresult/src/file_attribution.rs | 221 +++++++++++++++++++++++++++---- xcresult/src/xcresult_legacy.rs | 145 +++++++++++++++----- 2 files changed, 303 insertions(+), 63 deletions(-) diff --git a/xcresult/src/file_attribution.rs b/xcresult/src/file_attribution.rs index 003c00f3..c7faf95b 100644 --- a/xcresult/src/file_attribution.rs +++ b/xcresult/src/file_attribution.rs @@ -33,8 +33,23 @@ impl ReportedPath { pub fn into_string(self) -> String { self.0 } + + /// Whether this path is vendored dependency source rather than the repo's own + /// code: SPM's build dir (Tuist vendors into `/Tuist/.build/checkouts`), + /// SPM checkouts under Xcode's `DerivedData/SourcePackages/checkouts`, and + /// anything else Xcode generates under DerivedData. + /// + /// Reporting one of these hands the test to whoever owns the vendored + /// directory, because that is where codeowners are resolved from. + pub fn is_vendored_dependency(&self) -> bool { + DEPENDENCY_PATH_SEGMENTS + .iter() + .any(|segment| self.0.contains(segment)) + } } +const DEPENDENCY_PATH_SEGMENTS: [&str; 3] = ["/.build/", "/checkouts/", "/DerivedData/"]; + /// Where a candidate file came from. /// /// Every variant here answers "where did this failure surface", which is only ever @@ -42,6 +57,10 @@ impl ReportedPath { /// and later a caller that wants to vet a candidate — tell the sources apart. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum FileSource { + /// The call-stack frame whose symbol names the test itself. The only source + /// that identifies the test rather than the failure, and so the only one that + /// is right by construction. + TestFrame, /// `failureSummary.fileName`. The site the failure was raised from, which for an /// assertion helper is the helper's file rather than the caller's. RaisedFrom, @@ -57,6 +76,54 @@ pub enum FileSource { DocumentLocation, } +impl FileSource { + /// Whether this source identifies the test itself, or merely where a failure + /// surfaced. Only the latter is a guess, and only a guess needs vetting before + /// it is reported. + pub fn is_positive_identification(&self) -> bool { + matches!(self, Self::TestFrame) + } +} + +/// A test as the bundle names it, and the symbols that name it in a call stack. +/// +/// Swift symbolizes a test method as `Suite.testCase()` and Objective-C as +/// `-[Suite testCase]`; a closure declared inside the test is prefixed +/// (`closure #1 in Suite.testCase()`) but is still defined in the test's file. A +/// swift-testing test declared at the top level has no suite and symbolizes as the +/// bare function. +#[derive(Debug, Clone, Copy)] +pub struct TestIdentity<'a> { + pub suite: Option<&'a str>, + pub case: &'a str, +} + +impl TestIdentity<'_> { + /// Whether `symbol` is this test's own frame, rather than a helper it called or + /// the framework that invoked it. + pub fn is_named_by(&self, symbol: &str) -> bool { + let expected = match self.suite { + Some(suite) => vec![ + format!("{}.{}", suite, self.case), + format!("-[{} {}]", suite, self.case.trim_end_matches("()")), + ], + None => vec![self.case.to_string()], + }; + expected + .iter() + .any(|expected| symbol == expected || symbol.ends_with(&format!(" in {}", expected))) + } + + /// How the action-level issue summaries key this test when Xcode records no + /// producing target. + pub fn fallback_key(&self) -> String { + match self.suite { + Some(suite) => format!("{}.{}", suite, self.case), + None => self.case.to_string(), + } + } +} + /// A file we could report for a test, and the place we found it. #[derive(Debug, Clone)] pub struct FileCandidate { @@ -79,14 +146,16 @@ impl FileCandidate { /// whatever frame happened to be outermost. pub fn from_failure_summary( failure_summary: &legacy_schema::ActionTestFailureSummary, + identity: &TestIdentity, ) -> Vec { [ + test_frame(failure_summary, identity), raised_from(failure_summary), source_code_location(failure_summary), - last_stack_frame(failure_summary), ] .into_iter() .flatten() + .chain(stack_frames(failure_summary)) .collect() } @@ -111,6 +180,34 @@ impl FileCandidate { } } +/// The frame that names the test, and so the test's own file. +/// +/// Frames run innermost first, so this sits in the middle of the stack — helpers it +/// called below it, the framework that invoked it above — which is why taking the +/// last frame lands on a dependency. +/// +/// `imageName` looks like the natural discriminator here and is not: SPM +/// dependencies are statically linked into the test bundle, so every frame reports +/// the test bundle's name. +fn test_frame( + failure_summary: &legacy_schema::ActionTestFailureSummary, + identity: &TestIdentity, +) -> Option { + let call_stack = failure_summary + .source_code_context + .as_ref()? + .call_stack + .as_ref()?; + call_stack.values.iter().find_map(|frame| { + let symbol_info = frame.symbol_info.as_ref()?; + if !identity.is_named_by(&symbol_info.symbol_name.as_ref()?.value) { + return None; + } + let file_path = symbol_info.location.as_ref()?.file_path.as_ref()?; + Some(FileCandidate::new(&file_path.value, FileSource::TestFrame)) + }) +} + fn raised_from(failure_summary: &legacy_schema::ActionTestFailureSummary) -> Option { let file_name = failure_summary.file_name.as_ref()?; Some(FileCandidate::new(&file_name.value, FileSource::RaisedFrom)) @@ -132,14 +229,19 @@ fn source_code_location( )) } -fn last_stack_frame( - failure_summary: &legacy_schema::ActionTestFailureSummary, -) -> Option { - let call_stack = failure_summary +/// The failure's Swift and Objective-C frames, outermost first. +/// +/// Emitted as a sequence rather than a single "last frame" so that a caller +/// rejecting unusable paths lands on the outermost frame it can actually report, +/// rather than giving up because the outermost one happened to be a dependency. +fn stack_frames(failure_summary: &legacy_schema::ActionTestFailureSummary) -> Vec { + let Some(call_stack) = failure_summary .source_code_context - .as_ref()? - .call_stack - .as_ref()?; + .as_ref() + .and_then(|context| context.call_stack.as_ref()) + else { + return Vec::new(); + }; call_stack .values .iter() @@ -164,7 +266,8 @@ fn last_stack_frame( .map(|extension| extension == "swift" || extension == "m") .unwrap_or(false) }) - .last() + .rev() + .collect() } #[cfg(test)] @@ -174,6 +277,9 @@ mod tests { use super::*; + const SUITE: &str = "SnapshotReproTests"; + const CASE: &str = "failingSnapshot()"; + fn xc_string(value: &str) -> Value { json!({ "_value": value }) } @@ -181,20 +287,30 @@ mod tests { fn failure_summary( file_name: Option<&str>, location: Option<&str>, - stack: &[&str], + stack: &[(&str, &str)], ) -> legacy_schema::ActionTestFailureSummary { serde_json::from_value(json!({ "fileName": file_name.map(xc_string), "sourceCodeContext": { "location": { "filePath": location.map(xc_string) }, - "callStack": { "_values": stack.iter().map(|path| json!({ - "symbolInfo": { "location": { "filePath": xc_string(path) } } + "callStack": { "_values": stack.iter().map(|(symbol, path)| json!({ + "symbolInfo": { + "symbolName": xc_string(symbol), + "location": { "filePath": xc_string(path) } + } })).collect::>() } } })) .unwrap() } + fn identity() -> TestIdentity<'static> { + TestIdentity { + suite: Some(SUITE), + case: CASE, + } + } + #[rstest] #[case::spaces_are_encoded("/repo/Tests/My Test.swift", "/repo/Tests/My%20Test.swift")] #[case::already_safe("/repo/Tests/Test.swift", "/repo/Tests/Test.swift")] @@ -202,50 +318,97 @@ mod tests { assert_eq!(ReportedPath::new(path).as_str(), expected); } + #[rstest] + #[case::tuist_checkout("/repo/Tuist/.build/checkouts/Dep/Dep.swift", true)] + #[case::derived_data("/repo/DerivedData/SourcePackages/checkouts/Dep/Dep.swift", true)] + #[case::the_repos_own_code("/repo/Tests/SnapshotReproTests.swift", false)] + fn reported_path_recognizes_vendored_sources(#[case] path: &str, #[case] expected: bool) { + assert_eq!(ReportedPath::new(path).is_vendored_dependency(), expected); + } + + #[rstest] + #[case::swift_symbol("SnapshotReproTests.failingSnapshot()", true)] + #[case::objc_symbol("-[SnapshotReproTests failingSnapshot]", true)] + #[case::closure_inside_test("closure #1 in SnapshotReproTests.failingSnapshot()", true)] + #[case::helper_the_test_called("assertSnapshot(of:as:)", false)] + #[case::same_case_name_in_another_suite("OtherTests.failingSnapshot()", false)] + #[case::trait_that_invoked_the_test( + "closure #1 in _SnapshotsTestTrait.provideScope(for:testCase:performing:)", + false + )] + fn identity_recognizes_only_the_tests_own_frame(#[case] symbol: &str, #[case] expected: bool) { + assert_eq!(identity().is_named_by(symbol), expected); + } + + #[rstest] + #[case::top_level_swift_testing_function("failingSnapshot()", true)] + #[case::closure_inside_it("closure #1 in failingSnapshot()", true)] + #[case::suite_scoped_symbol("SnapshotReproTests.failingSnapshot()", false)] + fn a_suiteless_test_is_matched_by_its_bare_function( + #[case] symbol: &str, + #[case] expected: bool, + ) { + let identity = TestIdentity { + suite: None, + case: CASE, + }; + assert_eq!(identity.is_named_by(symbol), expected); + } + #[test] fn candidates_are_offered_in_preference_order_and_keep_their_provenance() { let summary = failure_summary( Some("/repo/Tests/Raised.swift"), Some("/repo/Tests/Location.swift"), - &["/repo/Tests/Frame.swift"], + &[ + ("helper()", "/repo/Tests/Inner.swift"), + ( + "SnapshotReproTests.failingSnapshot()", + "/repo/Tests/Own.swift", + ), + ("framework()", "/repo/Tests/Outer.swift"), + ], ); - let candidates = FileCandidate::from_failure_summary(&summary); assert_eq!( - candidates + FileCandidate::from_failure_summary(&summary, &identity()) .iter() .map(|candidate| (candidate.path.as_str(), candidate.source)) .collect::>(), vec![ + ("/repo/Tests/Own.swift", FileSource::TestFrame), ("/repo/Tests/Raised.swift", FileSource::RaisedFrom), ("/repo/Tests/Location.swift", FileSource::SourceCodeLocation), - ("/repo/Tests/Frame.swift", FileSource::LastStackFrame), + // Frames run innermost first, so they are offered outermost first. + ("/repo/Tests/Outer.swift", FileSource::LastStackFrame), + ("/repo/Tests/Own.swift", FileSource::LastStackFrame), + ("/repo/Tests/Inner.swift", FileSource::LastStackFrame), ] ); } #[test] fn a_summary_offering_nothing_yields_no_candidates() { - assert!(FileCandidate::from_failure_summary(&failure_summary(None, None, &[])).is_empty()); + let summary = failure_summary(None, None, &[]); + assert!(FileCandidate::from_failure_summary(&summary, &identity()).is_empty()); } #[rstest] - // Frames run innermost first, so the *last* one with source is taken. - #[case::last_wins(&["/repo/Tests/First.swift", "/repo/Tests/Second.m"], Some("/repo/Tests/Second.m"))] #[case::other_languages_skipped( - &["/repo/Tests/Real.swift", "/repo/Tests/Generated.cc", "/repo/Readme.md"], - Some("/repo/Tests/Real.swift") + &[("a", "/repo/Tests/Real.swift"), ("b", "/repo/Tests/Generated.cc"), ("c", "/repo/Readme.md")], + vec!["/repo/Tests/Real.swift"] )] - #[case::nothing_usable(&["/repo/Tests/Generated.cc"], None)] - fn the_stack_fallback_takes_the_outermost_swift_or_objc_frame( - #[case] stack: &[&str], - #[case] expected: Option<&str>, + #[case::nothing_usable(&[("a", "/repo/Tests/Generated.cc")], vec![])] + fn only_swift_and_objc_frames_are_offered( + #[case] stack: &[(&str, &str)], + #[case] expected: Vec<&str>, ) { let summary = failure_summary(None, None, stack); assert_eq!( - FileCandidate::from_failure_summary(&summary) - .first() - .map(|candidate| candidate.path.as_str().to_string()), - expected.map(String::from) + stack_frames(&summary) + .iter() + .map(|candidate| candidate.path.as_str()) + .collect::>(), + expected ); } diff --git a/xcresult/src/xcresult_legacy.rs b/xcresult/src/xcresult_legacy.rs index 9f69c761..b833124d 100644 --- a/xcresult/src/xcresult_legacy.rs +++ b/xcresult/src/xcresult_legacy.rs @@ -9,7 +9,7 @@ use petgraph::{ graph::{DiGraph, NodeIndex}, }; -use crate::file_attribution::FileCandidate; +use crate::file_attribution::{FileCandidate, TestIdentity}; use crate::types::{SWIFT_DEFAULT_TEST_SUITE_NAME, legacy_schema}; use crate::xcrun::{xcresulttool_get_object, xcresulttool_get_object_id}; @@ -25,7 +25,11 @@ pub struct XCResultTestLegacy { } impl XCResultTestLegacy { - fn find_file_in_test_summary(failure_summary_id: &str, path: &OsStr) -> Option { + fn find_file_in_test_summary( + failure_summary_id: &str, + path: &OsStr, + identity: &TestIdentity, + ) -> Option { let summary = xcresulttool_get_object_id(path, failure_summary_id); summary.ok().and_then(|summary| { summary @@ -35,28 +39,45 @@ impl XCResultTestLegacy { // grab the first failure summary if there are multiple failure_summaries.values.first() }) - .and_then(Self::find_file_in_failure_summary) + .and_then(|failure_summary| { + Self::find_file_in_failure_summary(failure_summary, identity) + }) }) } - /// The file to report for a failure, taken from the first source that offers - /// one. See [`crate::file_attribution`] for what each source actually means. + /// The file to report for a failure: the first candidate we are willing to + /// stand behind. + /// + /// Only the test's own frame identifies the test; every other source says where + /// the failure surfaced, which for a snapshot trait, a mocking framework or a + /// page object is inside the dependency. Those are vetted here — in one place, + /// so a source added later cannot quietly skip the check — and a test that + /// crashes or fails to launch never reaches its own frame, leaving nothing + /// reportable. We then report no file at all rather than one that would hand the + /// test to whoever owns the vendored directory. Consumers treat a missing file + /// as "unchanged" rather than "cleared", so it keeps the path and owners it had. fn find_file_in_failure_summary( failure_summary: &legacy_schema::ActionTestFailureSummary, + identity: &TestIdentity, ) -> Option { - FileCandidate::from_failure_summary(failure_summary) + FileCandidate::from_failure_summary(failure_summary, identity) .into_iter() - .next() + .find(Self::is_reportable) .map(|candidate| candidate.path.into_string()) } + fn is_reportable(candidate: &FileCandidate) -> bool { + candidate.source.is_positive_identification() || !candidate.path.is_vendored_dependency() + } + /// The action-level issue summaries keyed by whatever names the test they /// belong to, which is the producing target when Xcode records one and the test /// case name otherwise. fn fallback_file_from_failure_issue_summary( failure_summary: &legacy_schema::TestFailureIssueSummary, ) -> Option<(Option<&str>, String)> { - let candidate = FileCandidate::from_issue_summary(failure_summary)?; + let candidate = + FileCandidate::from_issue_summary(failure_summary).filter(Self::is_reportable)?; let producing_target = failure_summary .producing_target .as_ref() @@ -271,12 +292,11 @@ impl XCResultTestLegacy { let test_suite_name = parent_node.map(|node| node.weight.name); let test_case_name = node.weight.name; - let formatted_test_case_name = - if let Some(test_suite_name) = test_suite_name { - format!("{}.{}", test_suite_name, test_case_name) - } else { - test_case_name.to_string() - }; + let identity = TestIdentity { + suite: test_suite_name, + case: test_case_name, + }; + let formatted_test_case_name = identity.fallback_key(); let failure_summary_id = node.weight.failure_summary_id; let mut file = if use_experimental_failure_summary && failure_summary_id.is_some() @@ -284,6 +304,7 @@ impl XCResultTestLegacy { Self::find_file_in_test_summary( failure_summary_id.unwrap_or_default(), path.as_ref(), + &identity, ) } else { None @@ -497,8 +518,29 @@ mod tests { json!({ "_value": value }) } + const TEST_SUITE: &str = "SnapshotReproTests"; + const TEST_CASE: &str = "failingSnapshot()"; + #[rstest] - #[case::file_name_wins( + // The test's own frame beats the file the failure was raised from, which here is + // the assertion helper inside the dependency. + #[case::test_frame_wins_over_raised_from_file( + Some("/repo/Tests/Assertion.swift"), + Some("/repo/Tests/Assertion.swift"), + &[ + ("assertSnapshot(of:as:)", "/repo/Tuist/.build/checkouts/swift-snapshot-testing/Assert.swift"), + ("SnapshotReproTests.failingSnapshot()", "/repo/Tests/SnapshotReproTests.swift"), + ("closure #1 in _SnapshotsTestTrait.provideScope(for:)", "/repo/Tuist/.build/checkouts/swift-snapshot-testing/Trait.swift"), + ], + Some("/repo/Tests/SnapshotReproTests.swift") + )] + #[case::objc_symbol_and_closure_frames_name_the_test( + None, + None, + &[("closure #1 in -[SnapshotReproTests failingSnapshot]", "/repo/Tests/SnapshotReproTests.m")], + Some("/repo/Tests/SnapshotReproTests.m") + )] + #[case::file_name_wins_when_no_frame_names_the_test( Some("/repo/Tests/My Test.swift"), Some("/repo/Tests/Other.swift"), &[], @@ -507,51 +549,78 @@ mod tests { #[case::location_before_call_stack( None, Some("/repo/Tests/Assertion.swift"), - &["/repo/Packages/SnapshotTesting/SnapshotsTestTrait.swift"], + &[("provideScope(for:)", "/repo/Packages/SnapshotTesting/SnapshotsTestTrait.swift")], + Some("/repo/Tests/Assertion.swift") + )] + #[case::dependency_file_name_falls_through_to_location( + Some("/repo/Tuist/.build/checkouts/UITestSupport/PageObject.swift"), + Some("/repo/Tests/Assertion.swift"), + &[], Some("/repo/Tests/Assertion.swift") )] #[case::last_swift_or_objc_stack_frame( None, None, &[ - "/repo/Tests/Generated.cc", - "/repo/Tests/First.swift", - "/repo/Tests/Second.m", - "/repo/Tests/Readme.md", + ("first", "/repo/Tests/Generated.cc"), + ("second", "/repo/Tests/First.swift"), + ("third", "/repo/Tests/Second.m"), + ("fourth", "/repo/Tests/Readme.md"), ], Some("/repo/Tests/Second.m") )] + // The outermost frame is a dependency, so the next one out is taken instead. + #[case::dependency_frames_skipped_in_stack_fallback( + None, + None, + &[ + ("first", "/repo/Tests/First.swift"), + ("second", "/repo/Tuist/.build/checkouts/UITestSupport/Launching.swift"), + ], + Some("/repo/Tests/First.swift") + )] + // A launch failure or crash never reaches the test's own frame, so every + // remaining source points into the dependency: report no file rather than one + // that would re-own the test. + #[case::only_dependency_sources_yields_nothing( + None, + Some("/repo/DerivedData/SourcePackages/checkouts/UITestSupport/Launching.swift"), + &[("launch", "/repo/Tuist/.build/checkouts/UITestSupport/Launching.swift")], + None + )] #[case::no_usable_file( None, None, - &["/repo/Tests/Generated.cc", "/repo/Tests/Readme.md"], + &[("first", "/repo/Tests/Generated.cc"), ("second", "/repo/Tests/Readme.md")], None )] fn failure_summary_file_sources( #[case] file_name: Option<&str>, #[case] location: Option<&str>, - #[case] stack: &[&str], + #[case] stack: &[(&str, &str)], #[case] expected: Option<&str>, ) { let summary = serde_json::from_value(json!({ "fileName": file_name.map(xc_string), "sourceCodeContext": { "location": { "filePath": location.map(xc_string) }, - "callStack": { "_values": stack.iter().map(|path| { - let stack_frame = json!({ - "symbolInfo": { - "location": { - "filePath": xc_string(path) - } - } - }); - stack_frame - }).collect::>() } + "callStack": { "_values": stack.iter().map(|(symbol, path)| json!({ + "symbolInfo": { + "symbolName": xc_string(symbol), + "location": { "filePath": xc_string(path) } + } + })).collect::>() } } })) .unwrap(); - let file = XCResultTestLegacy::find_file_in_failure_summary(&summary); - assert_eq!(file, expected.map(String::from)); + let identity = TestIdentity { + suite: Some(TEST_SUITE), + case: TEST_CASE, + }; + assert_eq!( + XCResultTestLegacy::find_file_in_failure_summary(&summary, &identity), + expected.map(String::from) + ); } #[rstest] @@ -567,6 +636,14 @@ mod tests { Some("SnapshotReproTests.failingSnapshot()"), Some((Some("SnapshotReproTests.failingSnapshot()"), "/repo/Tests/Test.swift")) )] + #[case::dependency_document_location( + Some( + "file:///repo/Tuist/.build/checkouts/UITestSupport/PageObject.swift#EndingLineNumber=377" + ), + Some("SnapshotReproTests"), + Some("SnapshotReproTests.failingSnapshot()"), + None + )] #[case::missing_document_location( None, None,