trunk-merge/pr-1181/e307d01c-1840-41fd-bcaa-c1854ed514b1 - #1194
Closed
trunk-io[bot] wants to merge 25 commits into
Closed
trunk-merge/pr-1181/e307d01c-1840-41fd-bcaa-c1854ed514b1#1194trunk-io[bot] wants to merge 25 commits into
trunk-io[bot] wants to merge 25 commits into
Conversation
An `.xcresult` records where a failure was *raised*, never where a test is declared — a passing test's summary is 638 bytes with no path at all. So the file we report is inferred from the failure, and a failure raised inside a helper hands the test to whoever owns the helper. Behind `--use-experimental-xcresult-test-locations` (env `TRUNK_USE_EXPERIMENTAL_XCRESULT_TEST_LOCATIONS`), ask a language server instead: `documentSymbol` over the checkout names the type containing each method, which is the `Suite`/`case` pair an xcresult identifier already gives us. `sourcekit-lsp` and `clangd` ship in the Command Line Tools as well as Xcode, so this is the same shape as what we already do — shell out to an Xcode tool, parse structured output — not a new class of dependency. The flag also changes which calls we make. The declaration path issues `get test-results tests` and `get test-results summary`, and never `get object --legacy`, so the unbounded per-test summary fetch — 6 GB of JSON and a 48 GB peak footprint on one timed-out test — is not reachable from it. Ids do not move: `nodeIdentifierURL` on the modern API is the legacy record's `identifierURL` under another name, and an integration test pins both paths to the same ids and timestamps. A test with no declaration to find (Quick, `+testInvocations`) falls back to the modern API's own `sourceLocation`, vetted against the same vendored-path rules as the failure-summary path — the two fail in disjoint situations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A suite nested inside another suite, and every test it declared, was discarded: the traversal took only a suite's direct `Test Case` children, so a `Test Suite` child was never visited. A swift-testing bundle with 7 tests emitted 4 testcases, with the outer suite left as an empty `<testsuite>` — the tests were simply missing from the upload, silently. JUnit has no nested `<testsuite>`, so a nested suite is flattened into one of its own under a dot-qualified name (`Bundle.Outer.Inner`), which is 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 now appear alongside it. This is on the shared traversal, so it applies to the default path, not only to `--use-experimental-xcresult-test-locations`. No checked-in fixture bundle appears to contain a nested suite (none of the expected JUnit files has an empty `<testsuite>`, and the bundle blobs are Apple's compressed encoding, so this could not be confirmed off macOS). If the macOS suite reports a snapshot diff, that is a fixture that did have the bug — the added testcases are the fix working. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ndle Both shapes the two preceding commits changed were proven only by unit test against a canned `Tests` value, because no captured bundle had either: none of the scenarios has a suite nested in a suite, and none has a test that passed. `nested-and-passing` captures both at once. 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, so no failure summary names a file for them at all. Run against the pre-fix traversal the bundle emits `tests="2" failures="0"` — the inner suite is never visited, so its two tests are dropped and a run with a failing test reports no failures. That is the symptom the flattening fix was worth making, and it now has a bundle behind it. The shape is structural rather than a failure, which is the one thing `verify-failure-summaries.py` cannot express, so `regenerate.sh` checks this scenario with a sibling `verify-test-structure.py` that asserts the nested suite, the pass/fail split, and the presence of the `nodeIdentifierURL` the ids derive from. The declaration-path tests now assert each test's status alongside its file, so a fixture that drifted to all-failing could no longer keep the passing case green while proving nothing, and the crash scenario's test is named for the crash it covers rather than only for the reason no failure summary can serve it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…gression net
The flag was proven on the five fixtures built to exercise it, which says nothing
about the bundles it was not designed around — and those are most of them.
Rather than snapshot each bundle a second time, which would mean checking in a
near-duplicate of every expected JUnit differing only where the flag is supposed
to differ, this asserts the invariant directly: for every bundle the suite reads,
the declaration path and the default path agree on suite, name, id, status and
timestamp, and the reported file is never a vendored path. `file` is the one
thing allowed to move, and it is the one thing not compared.
It is not a vacuous check. Reverting the `startTime` rounding fails all twelve
cases, so the millisecond bug this suite only caught on a single fixture would
now be caught on every one of them.
Each case unpacks its own copy of its bundle. `xcresulttool` migrates a bundle in
place on first read, and pointing a second concurrent reader at a freshly
unpacked one races to create its `database.sqlite3`:
Error: "database.sqlite3" couldn't be moved to "test4.xcresult"
because an item with the same name already exists.
Sharing the existing fixtures would have made every bundle a two-reader race and
turned `test_complex_xcresult_with_valid_path` intermittent.
The CLI's own xcresult upload test is parameterised over the flag too, so the
path is covered end-to-end through argument parsing and the upload rather than
only at the crate boundary.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ler's
`xcresulttool` migrates a bundle that predates `database.sqlite3` in place the
first time it is read. Two things follow, neither of them ours to do:
- an upload writes into a build artifact it was only asked to read, and
- the read fails outright when that directory is not writable:
Error: "database.sqlite3" couldn't be moved because you don't have
permission to access "test4.xcresult".
which is `exit 64` and no JUnit at all, on the read-only artifact mounts CI
systems hand out.
It is also why two readers of one bundle race, which is what made
`test_complex_xcresult_with_valid_path` fail once a second test read the same
fixture.
Both constructors now copy the bundle into a `TempDir` and read that, so the
caller's directory is never written to and never needs to be writable. This is
on the shared path, so the default one is fixed too, not just the flag. 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 we already do
in well under a second on a 64 MB bundle.
The declaration path's fallback is instrumented while here. It is meant to catch
runtime-registered tests by reading the modern API's `sourceLocation`, but that
field is emitted in none of the bundles in `tests/data/`, so it never fires and
such a test gets no file at all. `generate_junits` now logs how many files came
from a declaration, from the fallback, and from neither, so whether that holds
against real-world bundles is answerable rather than assumed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n the test The declaration index is built from a checkout scan rather than the build log, because reading a build log costs the legacy `get object` call this path exists to avoid. The trade was that two same-named suites in different modules both declare the same `(suite, case)`, and `declarations` is a `HashMap<TestKey, DeclarationSite>`, so whichever file the scan reached first won and nothing recorded that there had been a choice. Scan order is arbitrary, so that was a coin flip between two modules' files. It is the one way this path can be confidently wrong where the failure-summary path cannot, since that one reads the frame that actually ran — and for codeowners a wrong file is worse than no file at all. `nodeIdentifierURL` is `test://com.apple.xcode/<scheme>/<target>/<suite>/<case>`, so the target is already in hand from the field the ids are derived from. `record` now prefers a candidate lying under a directory named for that target, which needs no extra `xcresulttool` call and works for a passing test as well as a failing one — unlike anything derived from the failure, which a passing test does not have. Where no candidate is under the target, or the test has no target, the first file scanned still wins, so this is strictly a tie-break and never removes a file that would have been reported before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A test declared on a base class runs again under every concrete subclass, and the supertype chain reported the base class's file for both. The reported file is what codeowners are resolved from, so that handed the subclass's failures to whoever owns the base class — the misattribution `file_attribution` exists to prevent, one level up. The concrete suite chose to run the test, so it is the one reported. That deletes the chain, and with it the inheritance-clause regex: `superclass`, `SUPERCLASS`, `DECLARATION_HEAD_LINES`, the `supertypes` map and its cycle guard. Across three large iOS checkouts 99.7% of the edges it built pointed at `XCTestCase` or `NSObject`, which no declaration can ever resolve to. Where a failure surfaced is no longer a fallback either. It names the file the failure was raised in rather than the one the test is written in, so reporting it resolves the wrong codeowners; no file resolves none, which is recoverable. A test with no declaration to find is runtime-registered (Quick, `+testInvocations`) and now lands in the `unresolved` counter. Crates replace the hand-rolled protocol code: - `lsp-server` and `lsp-types` own the framing, method names and payload shapes. That also fixes a latent bug: the old `file_uri` emitted `file://tests/…` for a relative root, where `tests` parses as the authority and a path component is silently lost. `url::Url::from_file_path` encodes and absolutizes, and only the URI is absolute — reported paths are unchanged. - `ignore` walks the checkout. Its extensions are registered explicitly because the built-in Objective-C type maps `.h`, which no clang server can answer `documentSymbol` for on its own, and `SKIPPED_DIRECTORIES` stays as an override over `.gitignore`. `Limits` is settable per run, because the right values depend on the repo: the clang server answers around 9 files/s against sourcekit-lsp's ~180, so an Objective-C heavy checkout needs the budget and file cap well above the defaults. The budget is spent per server kind rather than across both, which a shared deadline let a large Swift tree exhaust before clangd started, and a server that stops answering is replaced rather than abandoned. Two fixtures cover the shapes involved. The Objective-C one earned its keep immediately: it caught the suite fallback being used as the condition for having resolved a test, which ended the scan at the first file naming a suite and would have collapsed every test to its suite's file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eady prove Inline tests were carrying 1033 lines across five modules. Sorting them by why they were inline rather than by where they sat: `file_attribution`'s 22 tests only ever touched public API — `ReportedPath`, `TestIdentity::is_named_by`, `FileCandidate::from_failure_summary` and `from_issue_summary` are all `pub` — so they move to `tests/` verbatim. The one exception called the private `stack_frames`; with no `fileName` and no location every candidate offered is a stack frame, so it reaches the same filtering through `from_failure_summary` and asserts the provenance too. `xcresult`'s tests observed only public output but built their input by struct literal over private fields, and five of them turned out to duplicate coverage that already exists against real bundles: - `nested_suites_are_flattened` and `a_passing_test_case_has_no_file` are both encoded in `test-nested-and-passing.junit.xml`, which is compared byte for byte — the three passing cases carry no `file` attribute and the failing one does. - `a_failure_raised_elsewhere` is the two `prefer_the_tests_own_file_over_*` tests. - `a_nested_test_case_is_attributed` is `test_declaration_locations_give_a_passing_test_its_file` plus the id comparison in the flag-parity test. - `a_category_records_against_the_class_it_extends` and `an_inherited_test_resolves_to_the_concrete_suites_file` are the two fixtures added earlier on this branch, which prove the same thing from a real bundle rather than from hand-written symbol JSON. The sixth is expressible without the private seam: run the declaration path against a checkout that declares nothing, and no test gets a file at all. That leaves `TestLocationIndex::declaring` unused — the `#[cfg(test)]` seam for seeding an index without a language server — so it is deleted. Nothing was made public to get here. What stays inline, and why it cannot move: - `xcresult_legacy` reaches three private associated functions, and its public entry points read a bundle, so its fifteen-case precedence tables would need fifteen `.xcresult` fixtures. - `test_locations` asserts on `TestKey`'s private fields, and the rest are negative cases or private pure functions with no capturable fixture. - `lsp`'s three assertions cover `file_uri`, where both failure modes are silent: a server that cannot parse a URI answers with no symbols. One coverage loss worth naming: the `Skipped` and `Expected Failure` statuses are no longer pinned. `Passed` and `Failed` are covered by real bundles, and `find_test_case_file` keys on `node_identifier` without inspecting `result`. xcresult.rs 841 -> 516 lines, 0 inline tests file_attribution.rs 468 -> 271 lines, 0 inline tests inline test lines 1033 -> 447 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two fixtures added on this branch were checked by appending `#[case::]` entries to `test_the_declaration_flag_moves_the_file_and_nothing_else`, which lives in `tests/xcresult.rs`. That put new tests in the file everything else was being moved out of, and the only alternative at the time looked like duplicating the 135-line host test. Extracting its assertion to `tests/common` removes the choice: the check is now `assert_the_declaration_flag_moves_only_the_file`, and each set of fixtures runs it from wherever its own tests live. The twelve original cases stay in `xcresult.rs` against the older bundles; the two new ones move next to the fixtures they cover. `tests/xcresult.rs` now has no test that was not already there before the branch — its only removals are the four helpers that moved into `tests/common`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copying was introduced to stop `xcresulttool` migrating a bundle in place, which writes into a directory we were only asked to read and fails where it is not writable. It cost a full copy of every bundle on every upload to buy that. The copy is not what it looked like. Removing it and running the suite four times produced no races at all: the shared fixtures are current-format bundles that carry `database.sqlite3` already, so nothing migrates and nothing contends. The only failure was the test written to pin the read-only guarantee, on the one fixture that genuinely predates the file. So the trade is narrower than the copy implied — it protects bundles in a format Xcode no longer writes, and nothing else. Reading in place is what the CLI did before this branch, and the older format is now an accepted limitation rather than something every upload pays to avoid. `tests/bundle_reading.rs` pins both halves so the behaviour is documented rather than rediscovered: a read-only current-format bundle is readable and comes back byte-identical on disk, and a read-only legacy one fails its in-place migration. The second test is the limitation itself; if it ever starts passing, the migration behaviour changed and the note in `xcresult.rs` is stale. Its `entries` and `set_writable` helpers move to `tests/common` on the way, since both halves need them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every test this branch added to `tests/xcresult.rs` is about the declaration path, and they had accumulated at the end of a file that was already the longest in the crate. They move to `tests/declaration_locations.rs`, where the rest of that coverage already lives: - the seven `test_declaration_locations_*` tests, - `test_a_nested_suite_is_flattened_rather_than_dropped`, and - `test_the_declaration_flag_moves_the_file_and_nothing_else`, whose twelve cases rejoin the two that were split off into `declaration_locations.rs` earlier. `assert_junit` is now called from both files, so it joins the rest of the harness in `tests/common`, which also picks up a blanket `allow(dead_code)`: the module is compiled into each test binary separately, so whatever one binary does not call is dead from its point of view. Measured against `main` rather than the branch tip, `tests/xcresult.rs` now has no test this PR did not find there — 601 lines, all of it pre-existing. The suite is unchanged at 110 tests, and no case was dropped in the move. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six of them differed only in which fixture they read: unpack a bundle, ask `declaration_files` for it, assert a case resolves to the file it is written in. They become one table of six cases, which also drops five of the file's fixture `lazy_static`s — each case unpacks its own bundle. `keep_ids_and_timestamps_identical_to_the_legacy_path` looked subsumed by the flag-parity test, whose comparison already covers both fields, and was not: it carried a guard the parity assertion lacked. Two paths that both emit an empty id compare equal, so the comparison can pass while proving nothing — which is the failure mode worth catching, since a missing `nodeIdentifierURL` would silently re-identify every xcresult test case in the product and a `startTime` read against the wrong epoch would put every timestamp three decades out. The guard moves into the shared assertion instead, so all fifteen parity cases get it rather than one bundle, and the test folds in as the fifteenth case. Checked against every fixture first: all 558 cases across the fifteen bundles carry both fields, so the guard holds. `shape` now returns its columns field-wise so the guard reads the id and timestamp rather than sniffing their rendering, and blanking the id in `shape` makes all fifteen cases fail, so the guard is doing something. declaration_locations.rs 486 -> 339 lines, 13 tests -> 7 suite unchanged at 110 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every test in these files is `#[cfg(target_os = "macos")]`, but their imports were not — so on Linux and Windows `declaration_locations.rs` fails to compile against helpers that are gated out of `common`, and the other two carry unused imports. Only the test targets are affected, so `cargo build --all` misses it; it surfaces under `cargo nextest --workspace`, which is what CI runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A test arriving under a suite that declares no such method was inherited -- a suite cannot run a method it does not have -- so the method is written in some base class, and that is the file the test is in. Reporting the concrete suite's file names a file the test does not appear in, which resolves the wrong codeowners. No inheritance graph is needed to know this, and there is no build-free way to get one: `documentSymbol` carries no inheritance at all, sourcekitd's `key.inheritedtypes` gives only the name as written, and resolving a chain is semantic. Upstream's own `SyntacticSwiftXCTestScanner` gives up at the same point. The test having run is the proof of inheritance, so the case name alone identifies the declaration. Two suites in one target can still declare the same case name, and nothing here can say which of them a third inherited from, so an ambiguous answer declines and the suite's own file stands in rather than naming a file at random. This replaces `an_unrelated_suite_does_not_borrow_another_suites_case`, which pinned the opposite: it guarded an input that cannot occur, since a suite that neither declares nor inherits a case never runs it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ing it `didOpen` carries a file's whole text -- the protocol takes it inline, accepting neither a path nor a stream -- so asking a server what a file declares used to mean a `String` of it, `to_owned` into `TextDocumentItem`, `to_value` re-serializing that into the `Value` a `Notification` needs, and `Message::write`'s `to_string` of the finished message. Four copies of every source file in the checkout, one file at a time, unbounded in size. `Content-Length` precedes the body and a pipe cannot be rewound, so the length has to be known before any body byte goes out -- but that forces a measurement, not a buffer. `Serializer::serialize_str` is what forced the buffer: it wants the whole string contiguous. `collect_str` does not, and serde_json overrides it to push each fragment of a `Display` through its escaper straight into the writer. So the file is now read in 64 KiB chunks by a `Display` impl, twice: once escaped into a counting sink to measure, then again into a `BufWriter` over the server's stdin behind the header. Peak footprint per `didOpen` is flat in file size, and `a_file_larger_than_a_chunk_is_never_handed_over_whole` keeps it that way. Three things that fall out of this and are handled rather than discovered: - A read boundary splits multi-byte characters and `write_str` takes only valid UTF-8, so the incomplete tail of a chunk is carried into the next one. - Two reads can disagree if the file is rewritten between them, and a body that does not match its announced length misframes every message after it -- so the emitting pass is tallied too, and a mismatch abandons the server, which the caller already knows how to restart. - `Display::fmt` can only fail with a payload-free `fmt::Error`, so the real cause is stashed the way serde_json's own adapter does it, keeping the diagnostic that `read_to_string` used to give. Also caps file size, which nothing did before: `max_files` bounds how many files are parsed, not how big one may be. The cap is applied during the scan using the size the walker has already stat'd, so an oversized generated file is never opened, and skips are counted and logged rather than silent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`xcrun --find` is the only way to locate a tool inside an Xcode toolchain, so it is right on macOS and useless anywhere else — it returned `None` off macOS by construction. That was fine while the only consumer read `.xcresult` bundles, which cannot exist without Xcode. The Swift toolchain on Linux ships `sourcekit-lsp` on `PATH` and has no `xcrun` at all, so discovery now tries `xcrun` on macOS and falls back to a `PATH` scan. Nothing else in `test_locations.rs` or `lsp.rs` is platform-specific, which makes this the only thing standing between the declaration index and a non-Apple host. The scan also checks the executable bit rather than just for a file of the right name, so a stray non-executable `sourcekit-lsp` reports "not found" instead of failing later at spawn time with something less obvious. Both new tests run on any platform, which is the point. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`swift test --xunit-output` writes no file path for any test, and on Linux there
is no `.xcresult` to fall back to — so today a Swift test run there can never be
attributed to a file. The declaration index needs nothing from Xcode, so it can
answer this too.
The xunit XML carries more than it first appears. `classname` is the target
followed by the dot-qualified suite path, and only collapses to the bare target
for a top-level `@Test func`:
classname="MyCLITests" name="helloworld()"
classname="MyCLITests.AlphaSuite" name="shared()"
classname="MyCLITests.AlphaSuite.Inner" name="deep()"
classname="MyCLITests.BetaSuite" name="shared()"
That maps onto the existing `TestKey` almost unchanged: the innermost component
is the declaring type, exactly as the innermost component of an xcresult
`Suite/Inner/case()` identifier is, so only the separator differs. A top-level
function falls to the suiteless lookup already there for xcresult, and the first
component is the target, so the collision tie-break carries over for free.
Parameterised tests need no special handling either: swift-testing emits one
entry per function rather than one per argument, keeping argument labels
(`squares(n:)`), and that single entry is the declaration site we want.
XCTest needs none either. One run writes **two files** — swift-testing to
`<name>-swift-testing.xml` and XCTest to `<name>` — and the XCTest form is the
same `Module.Type` plus method, minus the `()`:
classname="MyCLITests.LegacyXCTests" name="testOldStyle"
so the same parse resolves it. That second file is only written when `--parallel`
is passed; without it the XCTest cases still run but are silently absent from the
output, which is worth knowing before trusting a project's xunit to be complete.
The fixture is a real package plus both files it actually produced. Its two
suites both declare `shared()` in different files, so a regression that ignored
the suite component would make one borrow the other's file — reverting the suite
component fails those tests rather than neither.
This is the resolver only. Nothing reads a JUnit file or writes a `file`
attribute back yet, and no flag is wired up.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both inputs feed one `TestLocationIndex`, and nothing checked they agree. They were tested against different fixture packages, so a divergence in either adapter would have gone unnoticed until it reached a repository using both. Three layers of parity now: - `an_xcresult_identifier_and_a_junit_classname_key_alike` — the same test keys the same way from either input. The two build `suite` from different places, an identifier's second-to-last component against a classname's innermost, and arrive at `None` by different routes, so only this catches them drifting. - `an_xcresult_url_and_a_junit_classname_name_the_same_target` — the collision tie-break reads the target from `nodeIdentifierURL` on one side and `classname` on the other, and is only correct if they agree. - `parity::both_inputs_resolve_every_test_to_the_same_file` — one package captured both ways, via `swift test --xunit-output` and via `xcodebuild` into an `.xcresult`, asserting the full set of (test, file) pairs is identical. Writing it turned up two things worth having pinned. The real xcresult identifier for a Swift XCTest method is `testOldStyle()`, with parens, where Objective-C reports none — both shapes are now covered rather than the one I assumed. More importantly the two inputs disagree on that name: `xcodebuild` reports `testOldStyle()` and `swift test --xunit-output` reports `testOldStyle`. They resolve to the same file because keying normalises the parens away, but `name` feeds `gen_info_id_base`, so the same test arriving through the two inputs does not currently land on one identity. That is upstream of this crate and is not worked around here, only pinned by `the_two_inputs_spell_an_xctest_method_differently` so it cannot change unnoticed. Comparison is over sorted `(name, file)` pairs rather than a map keyed by name, because two suites in the fixture both declare `shared()` — keyed by name one silently displaced the other and the test failed on roughly half of runs depending on hash order. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two tests can differ only by argument label, and a normalisation that dropped
labels would silently merge them — giving one the other's file, which for
codeowners is the worst outcome available: confidently wrong rather than absent.
Nothing covered that.
`OverloadSuite` declares `check()`, `check(a:)` and `check(b:)`, with `check(b:)`
in a different file via an extension, so a collapse shows up as a wrong file
rather than only a wrong line. Both inputs report all three and agree on how they
spell them:
xcresult OverloadSuite/check() OverloadSuite/check(a:) OverloadSuite/check(b:)
xunit check() check(a:) check(b:)
`normalized_case` trims only trailing parens, so `check()` becomes `check` while
`check(a:)` becomes the unbalanced `check(a:`. That is untidy but correct: the
same function is applied to the language server's symbol name and to the test
identifier, so what matters is that it is identical on both sides rather than
well-formed. Because labels survive it, the three key distinctly and resolve to
their own declarations, which the parity test now covers end to end from both
artifacts.
Rewriting `normalized_case` to split on `(` instead fails the new test with both
labelled overloads resolving to `OverloadA.swift`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… to key on
An XCTest test method takes no arguments, so there is nothing like `check(a:)` to
separate two of them — every one normalises to a bare method name and the class
name carries the entire load. That is the opposite of the swift-testing overload
case, and it was untested end to end on both inputs.
`BaseTests`, `ChildATests` and `ChildBTests` all report a test called
`testInherited`, distinguished only by class:
MyCLITests.BaseTests testInherited -> BaseTests.swift declares it
MyCLITests.ChildATests testInherited -> BaseTests.swift inherits it
MyCLITests.ChildBTests testInherited -> ChildBTests.swift overrides it
The inherited case is resolved by walking `supertypes`, built from the language
server's superclass parse, and the overriding case never needs the walk because
the subclass declares the method itself. Both inputs spell the identifier the
same way (`ChildATests/testInherited()` against classname `MyCLITests.ChildATests`
plus `testInherited`), so this is covered by the parity test too — which is also
why comparison is over pairs rather than a map, since three tests now share one
name.
`supertypes` had only a seeded-index unit test behind it before this. Disabling
the chain walk fails the `inherited` case alone, leaving `declared` and
`overridden` passing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`find_program` looks on `PATH`, which is where swift.org's tarball instructions, Swiftly and the official Docker images all put `sourcekit-lsp`. The Ubuntu runner image is the exception: `install-swift.sh` symlinks only `swift` and `swiftc` into /usr/local/bin and leaves the rest of the toolchain reachable only through $SWIFT_PATH. So put that directory on `PATH` for the Linux test jobs. A missing server leaves the index empty rather than failing, so these tests failed rather than skipped on any machine without a Swift toolchain -- including the self-hosted runner behind the Windows job, which has none. They now skip when no server is found, and `REQUIRE_LANGUAGE_SERVER` turns that skip back into a failure everywhere CI is supposed to have provided one, so the coverage cannot lapse unnoticed on the runners that matter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`swift test --xunit-output` writes no file path for any test, so a Swift test run outside Xcode has never been attributable to a file. This reads those files, resolves each test to where a language server says it is declared, and writes the `file` attribute back before the JUnit is bundled. It is a **path list rather than a boolean on `--junit-paths`**, for three reasons. Provenance: declaring that a file came from `swift test` is what licenses parsing its `classname` as a Swift type path, and JUnit5 emits the same `Type.method()` shape, so guessing would be unsound in a repository holding both. Cost: resolving walks the checkout, and a repository with no Swift at all should not pay that on every upload. Precedence: an `.xcresult` and a `swift test` xunit are different files, not two readings of one file, so with separate lists there is nothing to arbitrate — no mode, and no platform-conditional default. Taking a list rather than one path also matters because a single `swift test` run writes **two** of them: swift-testing to `<name>-swift-testing.xml` and XCTest to `<name>`, the latter only when `--parallel` is passed. A project using both frameworks uploads both, and one index serves them all, so that costs one checkout scan rather than one per file. Unlike `--xcresult-path`, this is not gated to macOS. The dependency is a language server rather than `xcresulttool`, and `sourcekit-lsp` ships with the Swift toolchain on Linux, so the flag is available wherever `swift test` is. The xcresult flags stay macOS-only because a bundle cannot be read without Xcode. A test that already carries a file keeps it. The resolved/unresolved split is logged, and unresolved warns — which is only a meaningful signal because the input was declared rather than guessed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`handle_swift_test_xunit` was inserted directly after the `#[cfg(target_os = "macos")]` belonging to `handle_xcresult`, so the new function took the gate and `handle_xcresult` lost it. That broke every non-macOS build five ways: the swift xunit handler went missing at its unconditional call site, `handle_xcresult` started compiling against `XCResult`/`XCResultOptions` that are gated out, and `write_all` lost the macOS-gated `Write` import. `--swift-test-xunit-paths` is not platform-gated and needs no Xcode, so the handler stays ungated and the gate goes back where it was. `Duration` moves under the gate instead -- it is only used by the macOS-only `XCResultOptions`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…the file markdownlint MD046 defaults to `consistent`, and this was the file's only indented code block against three fenced ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1194 +/- ##
==========================================
+ Coverage 83.19% 83.85% +0.66%
==========================================
Files 72 74 +2
Lines 16445 17573 +1128
==========================================
+ Hits 13681 14736 +1055
- Misses 2764 2837 +73 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Author
trunk-io
Bot
deleted the
trunk-merge/pr-1181/e307d01c-1840-41fd-bcaa-c1854ed514b1
branch
September 10, 2026 07:13
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
2 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.
This pull request was created and is being managed by Trunk Merge.
This pull request is based on the main branch at SHA c362e52cce5fef59f321c237d5de8e5afbee28b8.
See more details here.
When CI completes, this pull request will be closed automatically.
Pull Requests Being Tested
This pull request is testing the changes from pull request 1181, stacked on pull requests 1178, 1179, and 1180.