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

[dfrankland]

The one caveat, and what it is worth

The caveat: tests registered at runtime

A language server finds a test by reading its declaration in source. A test that has no
declaration — one synthesised at runtime — cannot be found this way, and this is the only
class of test where the failure-summary path can name a file and this path cannot.

That means, concretely:

• Quick (with Nimble) — describe/it closures become test cases at runtime via
  class_addMethod; there is no func testFoo() anywhere in the source to find.
• Anything overriding +testInvocations / testInvocations — the XCTest hook for
  generating cases programmatically.
• Older Objective-C BDD frameworks built on the same trick (Kiwi, Specta).

For these, documentSymbol returns nothing useful and the test gets no file. The two
approaches fail in genuinely disjoint situations, which is why this ships as a flag with the
default path still in place.

Why the caveat is shrinking

The direction of travel is toward tests whose declarations are in the source:

• Swift Testing (@Test, @Suite), which Apple ships in Xcode 16+ and develops in the
  open at swiftlang/swift-testing, is macro-based: @Test func foo() is a real function
  declaration, so it is exactly what documentSymbol reports.
• Even its parameterised tests keep one declaration —
  @Test(arguments: [1, 2, 3]) func f(n: Int) expands to many cases from a single
  declaration site, which is the site we want to attribute to anyway.
• Plain XCTest has always declared func testFoo() in source, so it was never affected.

So the uncovered population is the BDD-closure frameworks specifically, and new Swift test
code is being written against Swift Testing instead. (Directional argument from how these
frameworks work — not a measured adoption figure. generate_junits logs how many files
resolved from a declaration, from the fallback, and from neither, so a run against a real
repository can say how much of that repository this path actually covers.)

What we can delete if runtime-registered tests are not a target

If that population is out of scope, the failure-summary path stops being load-bearing and a
large amount of code goes with it.

Deleted outright

what	size	why it goes
src/xcresult_legacy.rs	~700 lines	is the legacy path
legacy JSON schema	~2,980 lines	only feeds the types that parse get object
its generator script	~320 lines	maintains that schema
dump- / verify-failure-summaries.py	2 scripts	fixture tooling for failure shapes
tests/data/*.legacy.junit.xml	4 files	expected output for a path that is gone
petgraph dependency	—	only xcresult_legacy.rs uses it, nowhere else in the workspace

Reduced

what	from → to	what is left
src/file_attribution.rs	~468 → ~90 lines	only ReportedPath; the whole candidate cascade, TestIdentity and FileCandidate go, with 7 of its 9 test groups
src/xcresult.rs	—	the FileAttribution enum collapses entirely and the two constructors merge back into one
src/xcrun.rs	—	xcresulttool_get_object and xcresulttool_get_object_id go; their only callers are legacy
build.rs, src/types.rs	two schemas → one	legacy_schema module goes
src/main.rs, cli/	—	both experimental flags (12 references) and the clap conflicts_with + env=false wart go with them
tests/xcresult.rs	—	37 references to the failure-summary flag; every #[case] pair collapses to a single test

Roughly ~4,000 lines of checked-in code and schema, ~1,200 of it hand-written Rust.

The point is not the line count. get object disappears, so the unbounded per-test
summary fetch — 6 GB of JSON at a 48 GB peak on one timed-out test — becomes unreachable
by construction rather than by a bound, and the xcresulttool surface drops from four
commands to two.

---

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

---

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 have no declaration to find, and the two approaches fail in disjoint situations — which is why this is a flag and not a replacement. See The one caveat above for which frameworks, and why the intended sourceLocation fallback does not currently cover them.

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 — now #1183

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.

6. feat(xcresult): break a declaration collision with the target that ran the test

Scan order used to decide which of two same-named suites in different modules won a (suite, case) — a coin flip between two modules' files, and the one way this path can be confidently wrong where the failure-summary path cannot. nodeIdentifierURL is test://com.apple.xcode/<scheme>/<target>/<suite>/<case>, so the target is already in hand from the field the ids come from; record now prefers a candidate under a directory named for it. No extra xcresulttool call, and it works for a passing test, unlike anything derived from the failure.

Strictly a tie-break: where no candidate is under the target, or the test has none, the first file scanned still wins, so no file that was reported before stops being reported.

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

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 — 125 tests pass on macOS (82 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 can both declare the same (suite, case). That is now broken by target rather than by scan order — see commit 6.

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.

Disambiguating from the failure instead would not have worked: 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

[dfrankland]

@claude review

[dfrankland]

In general, let's try to use crates (if a popular, well supported option exists) instead of writing our own code. And in addition, make sure tests attempt to use the public API surface via tests dir instead of inline. Inline tests are still allowed, but should be reserved for small assertions that are meant to enforce things that can't be done at compile time.

In addition, are we able to check how fast searching for a document symbol is in a large Swift / ObjC repo?

[dfrankland]

Review addressed in efa56ef — all 11 inline threads replied to and resolved. Answering the two general points and the open question here, since they span several threads.

"Is documentSymbol fast enough in a large Swift / Objective-C repo?"

Measured, rather than estimated. Harness replicates Resolver::parse exactly — same server args, didOpen(full text) → documentSymbol → didClose, one server process for the whole batch — over 1000 files per repo against real checkouts (firefox-ios 3173 .swift, WordPress-iOS 3287 .swift, firebase-ios-sdk 866 .m/.mm).

	firefox-ios	WordPress-iOS	firebase-ios-sdk
server	sourcekit-lsp	sourcekit-lsp	clangd
total wall (1000 files)	6.2 s	5.2 s	91.6 s
throughput	162 files/s	193 files/s	9 files/s
p50 / p90 / p99	2.6 / 5.3 / 16 ms	2.4 / 4.9 / 13 ms	46 / 339 / 682 ms
files inside a 60s budget	~9,900	~12,200	~570

Swift is a non-issue and does not degrade across the batch (first 100 ≈ 8.6 ms, last 100 ≈ 2.0 ms — the early cost is warmup). clangd is ~20× slower per file and is the binding constraint, so max_files: 2000 is comfortable for Swift and unreachable for Objective-C. Hence making all of it configurable rather than picking better constants.

Two things the measurement turned up that were not in the review:

• Both servers shared one deadline set once in resolve, and Swift is parsed first, so a large Swift tree could exhaust the budget before clangd began. Budget is now per server kind.
• 71 of 866 Objective-C responses (8%) came back empty. Not chased in this PR, but noting it — CLANG_EXTENSIONS excludes .h, and adding headers would roughly double the slowest path, so that is a trade to make with these numbers in hand.

"Prefer crates over our own code"

lsp-server (framing + JSON-RPC), lsp-types (methods, payloads, SymbolKind), url (path → URI), ignore (the walk). Net effect on the two files involved is −524 lines against +902, most of the additions being fixtures and tests. Two departures are argued on their threads: lsp-server rather than jsonrpsee, since jsonrpsee has no stdio transport and would not have removed the framing; and explicit globs rather than ignore’s add_defaults(), which maps .h.

"Tests against the public surface in tests/"

New coverage is all real bundles in tests/: two fixtures (inherited-test, objc-category) in a new tests/declaration_locations.rs over a shared tests/common/mod.rs. Inline tests that duplicated existing integration coverage are deleted rather than moved; four more went with the hand-rolled framing they tested. tests/xcresult.rs is down from 1226 to 1091 lines with no existing test relocated. Details and what stays inline are on the two tests/ threads.

Worth a second look: the objc-category fixture caught a bug I had introduced, which is the one change here I would most want reviewed. The suite fallback was being used as the condition for a test having been resolved, so the scan stopped at the first file naming a suite — collapsing every test to its suite’s file instead of its own. lookup (reporting) and method_declaration (stop condition) are now separate, with the reasoning in a doc comment and an inline test pinning the invariant.

Verification: 220 tests passing across xcresult and trunk-analytics-cli, cargo fmt clean, trunk check clean on all 36 modified files.