feat(xcresult): resolve test files from declarations, and stop dropping nested suites

[dfrankland]

Verified on macOS (Xcode xcresulttool 24514) — see What running it on a Mac found.

Five commits. The first two are independent, so the second can be cherry-picked on its own; then two of test coverage, then a fix for a pre-existing bug the coverage exposed.

---

1. feat(xcresult): resolve test files from declarations, not failures

An .xcresult records where a failure was raised, never where a test is declared — there is no per-test declaration site anywhere in the bundle, and a passing test's summary is 638 bytes with no path at all. Everything in file_attribution.rs is therefore a proxy, and a failure raised inside a helper attributes the test to the helper's file, which is where codeowners are resolved from.

Behind --use-experimental-xcresult-test-locations (env TRUNK_USE_EXPERIMENTAL_XCRESULT_TEST_LOCATIONS, hidden on upload), ask a language server instead. textDocument/documentSymbol over the checkout names the type containing each method, which is exactly the Suite/case pair an xcresult identifier already gives us.

This is not a new class of dependency — it is the same shape as what we ship today (shell out to an Xcode tool, parse structured output), and sourcekit-lsp/clangd ship in the Command Line Tools as well as Xcode, whereas xcresulttool ships only in Xcode. documentSymbol rather than workspace/symbol is deliberate: the latter is index-backed and makes the server index the checkout, while this parses one file on demand and needs neither an index nor a build.

It also changes which xcresulttool calls we make

	default	flag on
results	get test-results tests	same
run start time	get object --legacy (whole ActionsInvocationRecord)	get test-results summary
per-test failure summary	get object --id <summaryRef> per failure	never issued
file	failure call stack → file_attribution.rs cascade	documentSymbol over the checkout

The per-test summary fetch is unbounded — one timed-out test has been measured producing 6 GB of JSON at a 48 GB peak footprint, because we fetch the whole object to read failure_summaries.values.first(). The declaration path cannot reach that object at all, so this is structural rather than a bound.

Where it is worse

Tests registered at runtime (Quick's class_addMethod, +testInvocations) have no declaration to find. The two approaches fail in disjoint situations, so such a test falls back to the modern API's own sourceLocation, vetted against the same vendored-path rules in file_attribution.rs. That is why this is a flag and not a replacement.

New files

• xcresult/src/lsp.rs — minimal LSP JSON-RPC client. Reader thread + recv_timeout; once a request times out the stream cannot be resynchronised (a late reply would be read as the answer to the next request), so the process is killed and later calls refused rather than an upload waiting on a dead server. Replies null to client/registerCapability/workspace/configuration instead of ignoring them.
• xcresult/src/test_locations.rs — the (suite, case) → file:line index: ObjC ± prefixes, Class(Category) normalisation, suiteless top-level tests, superclass chaining, and a checkout scan ranked so files named after a suite go first.

The file list comes from a checkout scan, not the build log. The build log gives exact target ownership, but reading it costs a legacy get object call — the thing this path exists to avoid. The trade is that two same-named suites in different modules can collide.

2. fix(xcresult): stop dropping nested test suites

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 symptom is worse than losing tests. Against the new fixture the pre-fix traversal emits:

tests="2" failures="0"     # before
tests="4" failures="1"     # after

The inner suite's two tests are dropped and its failure goes with them, so a run containing a failing test reports a clean bill of health.

JUnit has no nested <testsuite>, so a nested suite is now flattened into 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 empty <testsuite> exactly as before, and the inner ones appear alongside it.

This is on the shared traversal, so it applies to the default path, not just the new flag.

3. test(xcresult): cover a nested suite and passing tests with a real bundle

Adds the nested-and-passing scenario, which closes both fixture gaps at once — see Fixtures.

4. test(xcresult): run every bundle through the declaration path as a regression net

Everything the flag was not designed around, checked for perturbation — see Every other bundle.

5. fix(xcresult): read a copy of the bundle instead of migrating the caller's

A pre-existing bug on the shared path, found because the regression net made two tests read one fixture. xcresulttool migrates a bundle that predates database.sqlite3 in place on first read, so today the uploader either writes into a build artifact it was only asked to read, or fails outright when it cannot:

bundle format	writable	read-only
older (Data + Info.plist)	mutated — database.sqlite3 written in	exit 64, no JUnit
already migrated	untouched	fine

Error: "database.sqlite3" couldn't be moved because you don't have
permission to access "test4.xcresult".

Read-only artifact mounts are ordinary in CI, so this is a real failure mode, not a theoretical one. It is also why two concurrent readers of one bundle race.

Both constructors now copy into a TempDir and read that. The copy is unconditional rather than keyed on whether a migration would happen — sniffing the format to save a copy trades a correctness guarantee for work that takes well under a second on a 64 MB bundle. This is on the shared path, so the default path is fixed too, not just the flag.

test_reading_a_bundle_neither_writes_to_it_nor_needs_it_writable pins both halves; it fails without the copy.

---

Testing

cargo test -p xcresult — 117 tests pass on macOS (74 unit, 43 integration), plus the CLI's own suite.

The split is deliberate, because the parts that genuinely need macOS are narrower than they look:

• src/test_locations.rs — symbol mapping, inheritance walk (including a cycle), category normalisation, superclass regex, scan ranking and caps, LSP wire framing (byte-vs-char, lowercase header, truncation).
• src/xcresult.rs — suite flattening and the attribution join, against a canned Tests value and a seeded index (TestLocationIndex::declaring, test-only).
• tests/xcresult.rs — the macOS tests that actually drive sourcekit-lsp and clangd over the checked-in packages in tests/fixture-src/.

What the declaration path is proven to do

Each of these drives a real language server over a real bundle. The last two columns are
what the existing paths report for the same test, so the comparison is against what we
ship rather than against nothing.

test	shape	legacy	failure-summary
..._give_a_crashed_test_its_file	fatalError in a dependency with zero call-stack frames; and a trait failure raised after the test's own frame is gone	none	none
..._give_a_passing_test_its_file	three tests that passed, so no failure summary exists to read	none	none
..._prefer_the_tests_own_file_over_a_vendored_dependency	helper under SourcePackages/checkouts/	none	the test's own file
..._resolve_an_objc_test_through_clangd	ObjC XCTFail in a shared category	none	the test's own file
..._prefer_the_tests_own_file_over_an_in_repo_helper	helper in the test target	the helper's file	the test's own file
..._find_a_top_level_swift_testing_function	suiteless @Test func failed by a helper	the helper's file	the test's own file
..._keep_ids_and_timestamps_identical_to_the_legacy_path	equivalence against the path in production	—	—

So the declaration path matches the experimental failure-summary path wherever that path
can name a file at all, beats the legacy path everywhere, and is the only one of the
three that can name a file for a crash with no stack or for a test that passed.

It is not a superset, in two directions

Worth being precise, because the flag's fallback is what is supposed to cover the gap:

• Coverage. A runtime-registered test (Quick's class_addMethod, +testInvocations) has no declaration to find. It is meant to fall back to the modern API's sourceLocation — but that field, while in the schema, is emitted in none of the 14 bundles here, so the fallback never fires and such a test gets no file, where the failure-summary path reads the call stack and would name one. The declaration path never issues get object, so it cannot see a stack; the failure path never scans the checkout, so it cannot see a declaration. Genuinely disjoint.
• Correctness. The index comes from a checkout scan rather than the build log, so two same-named suites in different modules collide. declarations is a HashMap<TestKey, DeclarationSite>, so the loser is silently overwritten and nothing records that there was a choice. For codeowners a wrong file is worse than no file, and the failure-summary path cannot make that mistake.

generate_junits now logs the split — N from a declaration, N from the fallback, N unresolved — so whether the fallback ever fires on real bundles is answerable rather than assumed.

If the collision starts to matter, the fix is already in hand and does not need the call stack: nodeIdentifierURL is test://com.apple.xcode/<scheme>/<target>/<suite>/<case>, so it names the target, and it is already parsed for ids. Ranking candidate files by whether their path matches the target disambiguates modules for passing and failing tests alike. Disambiguating from the failure instead would not: the modern API gives only the failure message's File.swift:12: basename, that basename is where the failure was raised (usually a helper, matching neither candidate), and a passing test has no failure message at all — so the signal is absent exactly where the collision is most likely to go unnoticed.

The passing-test case asserts each test's status alongside its file, so a fixture that
drifted to all-failing could not keep it green while proving nothing.

Every other bundle, as a regression net

The five scenarios above were built to exercise this path, which says nothing about the
bundles that were not. test_the_declaration_flag_moves_the_file_and_nothing_else runs
every bundle in the suite through both paths and asserts they agree on suite, name, id,
status and timestamp — file is the one thing allowed to move and the one thing not
compared. That is cheaper and far less brittle than checking in a near-duplicate of every
expected JUnit differing only where the flag is meant to differ.

It is not vacuous: reverting the startTime rounding below fails all twelve cases, so
the bug that originally showed up on one fixture would now be caught on every one of them.

upload_bundle_using_xcresult in the CLI is parameterised over the flag too, so it is
covered end-to-end through argument parsing and the upload, not only at the crate boundary.

Each case unpacks its own copy of its bundle, which is load-bearing: xcresulttool
migrates a bundle in place on first read, so a second concurrent reader of a freshly
unpacked one races to create its database.sqlite3 and fails with

Error: "database.sqlite3" couldn't be moved to "test4.xcresult" because an item with the same name already exists.

Sharing the existing fixtures made test_complex_xcresult_with_valid_path fail under
parallelism until each case got its own copy.

Fixtures

Both gaps that were open are now closed by nested-and-passing, captured through regenerate.sh like every other scenario (93MB of unreferenced symbolication data pruned to 0; 30KB checked in). Its inner @Suite is declared in a different file from the suite containing it, so resolving it needs a per-test declaration rather than the enclosing suite's file, and three of its four tests pass.

Its shape is structural rather than a failure, which is the one thing verify-failure-summaries.py cannot express, so it is checked by a sibling verify-test-structure.py that asserts the nested suite, the pass/fail split, and the presence of the nodeIdentifierURL ids derive from.

---

What running it on a Mac found

Three things were flagged as unverifiable off macOS. All three were checked; two held, and the third turned up a real bug.

nodeIdentifierURL is emitted. Parsed out of the tree of every bundle in tests/data/: 546 test cases, 0 missing it. The silent fallback to nodeIdentifier — which would have given every xcresult test case in the product a new identity — is never reached. The new fixture's structural verifier now pins this so it cannot regress unnoticed.

startTime is Unix epoch seconds, not an Apple reference-date offset. test1.xcresult reports 1727723571.159 → 2024-09-30 19:12:51Z, and the legacy record's own string for the same run is "2024-09-30T12:12:51.159-0700".

But the conversion was lossy, which is what the equivalence test actually caught:

declarations: 2024-09-30 19:12:51.158999919 +00:00
legacy:       2024-09-30 19:12:51.159       +00:00

(start_time.fract() * 1e9) as u32 extracts nanoseconds from an f64 whose ULP at epoch magnitude is 238 ns, so those digits were float noise. Both sources carry milliseconds — xcresulttool prints three decimals and the legacy parser reads %.3f — so this now rounds to the millisecond via from_timestamp_millis. Ids were byte-identical throughout; only the timestamps moved.

No snapshot diffs. Every existing expected JUnit was left untouched, because no pre-existing fixture had a nested suite.

The five LSP-driving tests passed first run. sourcekit-lsp and clangd both resolve under the installed Xcode; no debugging was needed.

A compile break the test suite could not see

coalesce_junit_path_wrappers was refactored to take an XCResultOptions, but the unit tests inside cli/src/context.rs still passed the old eight arguments. cargo test -p xcresult never compiles them, so this survived — CI would have failed. Fixed in the commit that introduced it, which now builds and passes on its own.

---

Known warts

• conflicts_with and an env var set to false. The flag is incompatible with --use-experimental-failure-summary (which tunes a code path this one does not run), enforced by clap rather than a runtime warning. clap correctly ignores a default-sourced value for conflicts, but treats an env-supplied value as present regardless of what it says — so TRUNK_USE_EXPERIMENTAL_XCRESULT_TEST_LOCATIONS=false together with --use-experimental-failure-summary is a hard conflict error. Unset the variable to roll back rather than setting it to false. Documented in CONTRIBUTING.md.
• context/src/junit/parser.rs is in this diff and is unrelated. trunk fmt runs cargo fmt workspace-wide, and that file is not rustfmt-clean on main under the pinned toolchain. Reverting it is unstable — anyone running trunk fmt re-applies it.
• trunk check's clippy still cannot run to completion, for an environmental reason rather than a code one: --all-targets --all-features pulls in rb-sys, which fails with Failed to setup stable API. Confirmed to fail identically on main. cargo clippy -p xcresult -p trunk-analytics-cli --all-targets is clean, and trunk fmt is clean on every changed file.
• classname on a nested test names the outer suite (OuterSuite, not OuterSuite.InnerSuite), because it is the first component of the identifier. Pre-existing behaviour on the shared traversal that only becomes visible now that nested tests are emitted at all; it does not feed the id, which is a UUIDv5 over org#repo#identifierURL. Left alone deliberately.

Docs

xcresult/CONTRIBUTING.md covers suite flattening as a stated purpose of the crate, the new flag and --repo-root, what the declaration path buys and where it is worse, the clap wart, the id-stability argument, cost and limits, and the fixture coverage above. tests/fixture-src/README.md documents the new scenario and which verifier enforces it.

🤖 Generated with Claude Code

[trunk-io]

Merging to main in this repository is managed by Trunk.

• To merge this pull request, check the box to the left or comment /trunk merge below.

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