fix(xcresult): attribute a failure to the test's own frame, not a dependency's - #1157
Closed
mmatheson wants to merge 1 commit into
Closed
fix(xcresult): attribute a failure to the test's own frame, not a dependency's#1157mmatheson wants to merge 1 commit into
mmatheson wants to merge 1 commit into
Conversation
…endency's The file we report for an XCTest/swift-testing case 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. Two changes: - Select the call-stack frame whose symbol names the test itself, and take its file. 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. Symbols are matched for Swift (`Suite.testCase()`), Objective-C (`-[Suite testCase]`), closures declared inside the test (`closure #1 in ...`), and top-level swift-testing functions, which have no suite. `imageName` looks like the natural discriminator here and is not: SPM dependencies are statically linked into the test bundle, so every frame in our own snapshot-testing fixture reports the test bundle's name. - Reject vendored dependency paths from the remaining sources — the raised-from `fileName`, the source-code-context location, the last-frame fallback, and the document location used by the non-experimental path. When a test crashes or fails to launch it never reaches its own frame and every source points into a dependency; we now 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".
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1157 +/- ##
==========================================
+ Coverage 82.97% 83.29% +0.31%
==========================================
Files 71 71
Lines 16044 16117 +73
==========================================
+ Hits 13312 13424 +112
+ Misses 2732 2693 -39 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
Member
|
Superseded by a three-PR stack that separates the refactor from the fix:
Same behaviour, same seven cases fail if the fix is reverted. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The file we report for an XCTest / swift-testing case comes from the failure summary, 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. Those live under the package checkout, so we report e.g.:
Codeowners are resolved from that path (
context/src/junit/parser.rs), so the test then belongs to whoever owns the vendored directory. For ZillowGroup that isTuist/ @reviewers/mobile-app-platform-ios, and it is a recurring support thread — 683 of their currently-active test cases sit on such a path, 940 rows owned by that team as a result.The mechanism
From our own fixture's
database.sqlite3(SourceCodeFrames.orderInContainer, innermost first):recordIssue(_:fileID:filePath:line:column:)…/SourcePackages/checkouts/swift-snapshot-testingassertSnapshot<A, B>(of:as:…)…/SourcePackages/checkouts/swift-snapshot-testingSnapshotReproTests.failingSnapshot()…/Tests/SnapshotReproTests/SnapshotReproTests.swiftclosure #1 in _SnapshotsTestTrait.provideScope(…)…/checkouts/swift-snapshot-testingThe test's own frame is in the middle — helpers it called below it, the framework that invoked it above — so
find_file_in_source_code_context_call_stack's.last()selects a dependency by construction. It returns the right answer on this fixture only incidentally: those dependency locations point at the package directory, so the.swift/.mextension filter drops them. Zillow's dependency frames are real.swiftfiles, so nothing stops them.What changed
1. Select the frame whose symbol names the test. Matched for Swift (
Suite.testCase()), Objective-C (-[Suite testCase]), closures declared inside the test (closure #1 in …, still the test's file), and top-level swift-testing functions, which have no suite — a shape Zillow raised separately. That frame's file is a positive identification rather than a blocklist of everything else.imageNamelooks like the natural discriminator and is not: SPM dependencies are statically linked into the test bundle, so every frame in the fixture — including the ones inside swift-snapshot-testing — reportsimageName = SnapshotReproTests. Worth recording so nobody reaches for it later.2. Reject vendored dependency paths from the remaining sources — the raised-from
fileName, the source-code-context location, the last-frame fallback, and the document location used by the non-experimental path (otherwise the fix leaks straight through it). Segments:/.build/,/checkouts/,/DerivedData/, which covers both the Tuist layout and stock SPM under DerivedData.When a test crashes or fails to launch it never reaches its own frame and every remaining source points into a dependency — we now report no file rather than one that would re-own the test. Consumers treat a missing file as "unchanged", not "cleared" (the ETL only takes non-empty values), so the test keeps the path and owners it last had.
Behaviour change worth a reviewer's judgement
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", and it is what makes ownership follow the test rather than the utility it called — but it is broader than the dependency case in the title, so flagging it explicitly.
Tests
cargo test -p xcresult --lib— 24 pass, including new cases for: the test frame outranking a raised-from path; ObjC and closure symbols; a dependencyfileNamefalling through to the location; dependency frames skipped in the last-frame fallback; a dependency-only summary (the crash/launch case) yieldingNone; a dependency document location rejected in the fallback map; and suite-less swift-testing functions.cargo fmtclean;cargo clippy -p xcresultsurfaces nothing new (the two warnings on this file,Iterator::lastand an elidable lifetime, are pre-existing).Not run: the
#[cfg(target_os = "macos")]integration tests, which needxcresulttool— I only have Linux here.test_swift_snapshot_testing_trait_failure_uses_assertion_fileshould still pass unchanged (its expected file is the test file, which is exactly what the symbol-matched frame yields), but please confirm on a Mac before merge.Also worth adding before this ships: a fixture built from one of the xcresults Zillow shared in #trunk-zillow — specifically a UI-test launch failure and a
PageObjecttimeout, since those are the shapes I could only reconstruct synthetically here.Related
trunk2 #5254 fixes the read side — the ClickHouse views that were showing a test's creation-time path forever and blanking collection-scoped codeowners on any upload that reported no file. This PR stops the bad path being written; that one stops a corrected path being ignored.