From fbd4e5ccbf2d2e95b286a9a591514c14ade4ae3b Mon Sep 17 00:00:00 2001 From: Dylan Frankland Date: Thu, 10 Sep 2026 18:36:53 +0000 Subject: [PATCH 1/4] fix(cli): accept --swift-test-xunit-paths as the only report source `--swift-test-xunit-paths` is absent from every `required_unless_present_any` list, so the usage its own help text documents trunk upload --swift-test-xunit-paths out-swift-testing.xml,out.xml fails at argument parsing: error: the following required arguments were not provided: --junit-paths --bazel-bep-path --test-reports It is a real report source -- `coalesce_junit_path_wrappers` handles it on its own branch and produces junit path wrappers from it -- so it belongs in those lists alongside the other three. The workaround it forces is worse than the error. Callers have to supply a junit glob they do not want, and the obvious value, the report they are already uploading, silently doubles every test: the two lists are appended rather than reconciled, so each test arrives once with the file a language server resolved and once with none. Measured on a 3-case suite uploaded both ways, the bundle holds 6 cases, and because `file` feeds `gen_info_id` those are two distinct tests rather than one test twice. `upload_bundle_using_swift_test_xunit` could not catch either problem. It inherits `CommandBuilder`'s default `--junit-paths ./*`, which both hid the missing requirement and matched the two xunit files it writes into the repo root -- so the test has been exercising the double-upload path all along. Its assertions keyed a HashMap by test name, and a duplicate carries the same name, so the second copy overwrote the first and nothing looked wrong. So the test now passes no paths argument at all, via a new `PathsState::NoPaths` that is distinct from a `None` paths_state, and asserts the exact case count. That makes it a real test of standalone usage, and gives the duplication somewhere to show up if it comes back. Co-Authored-By: Claude Opus 5 (1M context) --- cli/src/upload_command.rs | 8 ++++---- cli/tests/common/command_builder.rs | 10 ++++++++++ cli/tests/upload.rs | 23 +++++++++++++++++++++-- 3 files changed, 35 insertions(+), 6 deletions(-) diff --git a/cli/src/upload_command.rs b/cli/src/upload_command.rs index d3966b31..4cb142cc 100644 --- a/cli/src/upload_command.rs +++ b/cli/src/upload_command.rs @@ -44,7 +44,7 @@ pub const DRY_RUN_OUTPUT_DIR: &str = "bundle_upload"; pub struct UploadArgs { #[arg( long, - required_unless_present_any = [JUNIT_GLOB_REQUIRED_UNLESS_PRESENT_ARG, "bazel_bep_path", "test_reports"], + required_unless_present_any = [JUNIT_GLOB_REQUIRED_UNLESS_PRESENT_ARG, "bazel_bep_path", "test_reports", "swift_test_xunit_paths"], conflicts_with = "bazel_bep_path", value_delimiter = ',', value_parser = clap::builder::NonEmptyStringValueParser::new(), @@ -53,20 +53,20 @@ pub struct UploadArgs { pub junit_paths: Vec, #[arg( long, - required_unless_present_any = [JUNIT_GLOB_REQUIRED_UNLESS_PRESENT_ARG, "junit_paths", "test_reports"], + required_unless_present_any = [JUNIT_GLOB_REQUIRED_UNLESS_PRESENT_ARG, "junit_paths", "test_reports", "swift_test_xunit_paths"], help = "Path to Bazel Build Event Protocol JSON file. BEP files contain test results and build metadata." )] pub bazel_bep_path: Option, #[cfg(target_os = "macos")] #[arg(long, - required_unless_present_any = ["junit_paths", "bazel_bep_path", "test_reports"], + required_unless_present_any = ["junit_paths", "bazel_bep_path", "test_reports", "swift_test_xunit_paths"], conflicts_with_all = ["junit_paths", "bazel_bep_path"], required = false, help = "Path to Xcode XCResult bundle directory (macOS only)." )] pub xcresult_path: Option, #[arg( long, - required_unless_present_any = [JUNIT_GLOB_REQUIRED_UNLESS_PRESENT_ARG, "junit_paths", "bazel_bep_path"], + required_unless_present_any = [JUNIT_GLOB_REQUIRED_UNLESS_PRESENT_ARG, "junit_paths", "bazel_bep_path", "swift_test_xunit_paths"], value_delimiter = ',', value_parser = clap::builder::NonEmptyStringValueParser::new(), help = "Comma-separated list of glob patterns to test report files. Supports JUnit XML, Bazel BEP, and XCResult formats." diff --git a/cli/tests/common/command_builder.rs b/cli/tests/common/command_builder.rs index 2000b896..79ecb679 100644 --- a/cli/tests/common/command_builder.rs +++ b/cli/tests/common/command_builder.rs @@ -436,6 +436,10 @@ pub enum PathsState { JunitPaths(String), BazelBepPath(String), XCResultPath(String), + /// No paths argument at all, for a test whose report source is passed through + /// `extra_args` instead. Distinct from a `None` `paths_state`, which supplies a + /// default `--junit-paths` glob. + NoPaths, } impl PathsState { @@ -444,6 +448,7 @@ impl PathsState { PathsState::JunitPaths(path) => vec![String::from("--junit-paths"), path.clone()], PathsState::BazelBepPath(path) => vec![String::from("--bazel-bep-path"), path.clone()], PathsState::XCResultPath(path) => vec![String::from("--xcresult-path"), path.clone()], + PathsState::NoPaths => Vec::new(), } } } @@ -498,6 +503,11 @@ impl<'b> CommandBuilder<'b> { self } + pub fn no_paths(&mut self) -> &mut Self { + self.paths_state = Some(PathsState::NoPaths); + self + } + pub fn extra_args(&mut self, args: &[&str]) -> &mut Self { self.extra_args = args.iter().map(|arg| String::from(*arg)).collect(); self diff --git a/cli/tests/upload.rs b/cli/tests/upload.rs index 8db92602..c22e5473 100644 --- a/cli/tests/upload.rs +++ b/cli/tests/upload.rs @@ -3142,6 +3142,12 @@ async fn upload_bundle_keeps_the_repo_relative_path_when_a_symlink_leaves_the_re // `swift test --xunit-output` reports no file for any test, so the uploaded JUnit only gets // one if a language server found where each test is declared in the checkout. +// +// `no_paths` is load-bearing twice over. It proves `--swift-test-xunit-paths` is accepted on +// its own, which is the usage its help text documents; and it keeps the default `--junit-paths +// ./*` from also matching these two files, which would upload every test a second time without +// a declared file. The case count below is what catches that if it ever regresses — keying by +// name alone cannot, because the duplicate carries the same name. #[cfg(any(target_os = "macos", target_os = "linux"))] #[tokio::test(flavor = "multi_thread")] async fn upload_bundle_using_swift_test_xunit() { @@ -3194,6 +3200,7 @@ async fn upload_bundle_using_swift_test_xunit() { let state = MockServerBuilder::new().spawn_mock_server().await; CommandBuilder::upload(temp_dir.path(), state.host.clone()) + .no_paths() .extra_args(&[ "--swift-test-xunit-paths", "xunit-swift-testing.xml,xunit.xml", @@ -3208,7 +3215,7 @@ async fn upload_bundle_using_swift_test_xunit() { serde_json::from_reader(fs::File::open(tar_extract_directory.join("meta.json")).unwrap()) .unwrap(); - let mut files = std::collections::HashMap::new(); + let mut cases: Vec<(String, Option)> = Vec::new(); for file_set in &bundle_meta.base_props.file_sets { for file in &file_set.files { let mut parser = JunitParser::new(); @@ -3222,13 +3229,25 @@ async fn upload_bundle_using_swift_test_xunit() { .iter() .find(|(key, _)| key.as_str() == "file") .map(|(_, value)| value.as_str().to_owned()); - files.insert(case.name.as_str().to_owned(), file); + cases.push((case.name.as_str().to_owned(), file)); } } } } } + // Three tests across the two files, each bundled exactly once. A count of six is the + // signature of the same reports arriving through a junit glob as well. + assert_eq!( + cases.len(), + 3, + "expected each test bundled once, got {cases:?}" + ); + + let files = cases + .into_iter() + .collect::>>(); + for (name, expected) in [ ("helloworld()", "Tests/MyCLITests/TopLevel.swift"), ("shared()", "Tests/MyCLITests/Suites.swift"), From 446143a2690c0d60042946a0b6f29aeec12ac81d Mon Sep 17 00:00:00 2001 From: Dylan Frankland Date: Thu, 10 Sep 2026 18:47:22 +0000 Subject: [PATCH 2/4] Model swift xunit paths as a PathsState, not an absence of one `PathsState` is the report-source selector, and this change's whole claim is that --swift-test-xunit-paths is a fourth source alongside junit, BEP, and xcresult. A `NoPaths` variant said the opposite: that the source was something smuggled past the selector through extra_args, with a second flag to suppress the default. Nothing else wanted "no source at all", so the variant existed only to work around its own framing. Co-Authored-By: Claude Opus 5 (1M context) --- cli/tests/common/command_builder.rs | 13 ++++++------- cli/tests/upload.rs | 17 +++++++---------- 2 files changed, 13 insertions(+), 17 deletions(-) diff --git a/cli/tests/common/command_builder.rs b/cli/tests/common/command_builder.rs index 79ecb679..c2aa7657 100644 --- a/cli/tests/common/command_builder.rs +++ b/cli/tests/common/command_builder.rs @@ -436,10 +436,7 @@ pub enum PathsState { JunitPaths(String), BazelBepPath(String), XCResultPath(String), - /// No paths argument at all, for a test whose report source is passed through - /// `extra_args` instead. Distinct from a `None` `paths_state`, which supplies a - /// default `--junit-paths` glob. - NoPaths, + SwiftTestXunitPaths(String), } impl PathsState { @@ -448,7 +445,9 @@ impl PathsState { PathsState::JunitPaths(path) => vec![String::from("--junit-paths"), path.clone()], PathsState::BazelBepPath(path) => vec![String::from("--bazel-bep-path"), path.clone()], PathsState::XCResultPath(path) => vec![String::from("--xcresult-path"), path.clone()], - PathsState::NoPaths => Vec::new(), + PathsState::SwiftTestXunitPaths(paths) => { + vec![String::from("--swift-test-xunit-paths"), paths.clone()] + } } } } @@ -503,8 +502,8 @@ impl<'b> CommandBuilder<'b> { self } - pub fn no_paths(&mut self) -> &mut Self { - self.paths_state = Some(PathsState::NoPaths); + pub fn swift_test_xunit_paths(&mut self, new_paths: &str) -> &mut Self { + self.paths_state = Some(PathsState::SwiftTestXunitPaths(String::from(new_paths))); self } diff --git a/cli/tests/upload.rs b/cli/tests/upload.rs index c22e5473..4b5aa685 100644 --- a/cli/tests/upload.rs +++ b/cli/tests/upload.rs @@ -3143,11 +3143,12 @@ async fn upload_bundle_keeps_the_repo_relative_path_when_a_symlink_leaves_the_re // `swift test --xunit-output` reports no file for any test, so the uploaded JUnit only gets // one if a language server found where each test is declared in the checkout. // -// `no_paths` is load-bearing twice over. It proves `--swift-test-xunit-paths` is accepted on -// its own, which is the usage its help text documents; and it keeps the default `--junit-paths -// ./*` from also matching these two files, which would upload every test a second time without -// a declared file. The case count below is what catches that if it ever regresses — keying by -// name alone cannot, because the duplicate carries the same name. +// Passing the paths through `PathsState` rather than `extra_args` is load-bearing twice over. It +// proves `--swift-test-xunit-paths` is accepted as the only report source, which is the usage its +// help text documents; and it keeps the harness's default `--junit-paths ./*` from also matching +// these two files, which would upload every test a second time without a declared file. The case +// count below is what catches that if it ever regresses — keying by name alone cannot, because the +// duplicate carries the same name. #[cfg(any(target_os = "macos", target_os = "linux"))] #[tokio::test(flavor = "multi_thread")] async fn upload_bundle_using_swift_test_xunit() { @@ -3200,11 +3201,7 @@ async fn upload_bundle_using_swift_test_xunit() { let state = MockServerBuilder::new().spawn_mock_server().await; CommandBuilder::upload(temp_dir.path(), state.host.clone()) - .no_paths() - .extra_args(&[ - "--swift-test-xunit-paths", - "xunit-swift-testing.xml,xunit.xml", - ]) + .swift_test_xunit_paths("xunit-swift-testing.xml,xunit.xml") .command() .assert() .success(); From 6c1b8e2560b02a6ffb2d217c945c64218fd70a82 Mon Sep 17 00:00:00 2001 From: Dylan Frankland Date: Thu, 10 Sep 2026 19:38:00 +0000 Subject: [PATCH 3/4] Drop the comment defending a call site that needs no defence It contrasted PathsState against extra_args, which the code no longer does, so it documented the shape of an earlier edit rather than the test. The assertion keeps its own comment, which says why the count is exact and what six would mean -- neither readable off the code. Co-Authored-By: Claude Opus 5 (1M context) --- cli/tests/upload.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/cli/tests/upload.rs b/cli/tests/upload.rs index 4b5aa685..f74c759e 100644 --- a/cli/tests/upload.rs +++ b/cli/tests/upload.rs @@ -3142,13 +3142,6 @@ async fn upload_bundle_keeps_the_repo_relative_path_when_a_symlink_leaves_the_re // `swift test --xunit-output` reports no file for any test, so the uploaded JUnit only gets // one if a language server found where each test is declared in the checkout. -// -// Passing the paths through `PathsState` rather than `extra_args` is load-bearing twice over. It -// proves `--swift-test-xunit-paths` is accepted as the only report source, which is the usage its -// help text documents; and it keeps the harness's default `--junit-paths ./*` from also matching -// these two files, which would upload every test a second time without a declared file. The case -// count below is what catches that if it ever regresses — keying by name alone cannot, because the -// duplicate carries the same name. #[cfg(any(target_os = "macos", target_os = "linux"))] #[tokio::test(flavor = "multi_thread")] async fn upload_bundle_using_swift_test_xunit() { From 63fcdc84401cae163f880a88eab5c28f0bde0617 Mon Sep 17 00:00:00 2001 From: Dylan Frankland Date: Thu, 10 Sep 2026 13:20:00 -0700 Subject: [PATCH 4/4] Give the declaration tests a clock sized for a loaded CI box `Limits::default()` sizes one resolution on a machine doing nothing else. A test binary starts a `sourcekit-lsp` per test, all at once, under coverage instrumentation and alongside the rest of the workspace's suite -- and a server that has not answered `initialize` inside `request_timeout` is abandoned rather than waited on, so every test in that process resolves to nothing. That is what the x86_64 musl runner hit: three of these failed at exactly 30.016s, the default timeout, while the four that ran once the machine quietened took 10s each. The same tests take 0.4s on aarch64 once warm. Only the clock moves; every value the tests assert on is still resolved the way the defaults resolve it. Co-Authored-By: Claude Opus 5 (1M context) --- xcresult/tests/common/mod.rs | 25 +++++++++++++++++++++---- xcresult/tests/swift_test_xunit.rs | 9 ++++++--- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/xcresult/tests/common/mod.rs b/xcresult/tests/common/mod.rs index 72f6be4d..81820c28 100644 --- a/xcresult/tests/common/mod.rs +++ b/xcresult/tests/common/mod.rs @@ -9,15 +9,32 @@ #![allow(dead_code)] -use std::{fs::File, path::Path}; +use std::{fs::File, path::Path, time::Duration}; use context::repo::RepoUrlParts; use flate2::read::GzDecoder; use lazy_static::lazy_static; use tar::Archive; use temp_testdir::TempDir; +use xcresult::test_locations::Limits; #[cfg(target_os = "macos")] -use xcresult::{test_locations::Limits, xcresult::XCResult}; +use xcresult::xcresult::XCResult; + +/// The limits every test here resolves under. +/// +/// The shipped defaults size one resolution on a machine doing nothing else. A test binary +/// starts one language server per test, all of them at once, under coverage instrumentation +/// and alongside the rest of the workspace's suite — and a `sourcekit-lsp` that has not +/// answered `initialize` inside `request_timeout` is abandoned rather than waited on, so +/// every test in that process resolves to nothing. Only the clock is relaxed: what the tests +/// assert on is still resolved the way the defaults resolve it. +pub fn limits() -> Limits { + Limits { + budget: Duration::from_secs(300), + request_timeout: Duration::from_secs(120), + ..Limits::default() + } +} /// The bundles are checked in as tarballs, so a test reads one by unpacking it into a /// temporary directory that is removed with the `TempDir`. @@ -54,7 +71,7 @@ pub fn declaration_report, U: AsRef>( ORG_URL_SLUG.clone(), REPO_FULL_NAME.clone(), repo_root.as_ref(), - Limits::default(), + limits(), ) .expect("the declaration path reads the bundle"); @@ -151,7 +168,7 @@ pub fn assert_the_declaration_flag_moves_only_the_file( ORG_URL_SLUG.clone(), REPO_FULL_NAME.clone(), root, - Limits::default(), + limits(), ) .expect("the declaration path reads the bundle"); diff --git a/xcresult/tests/swift_test_xunit.rs b/xcresult/tests/swift_test_xunit.rs index 81b8735a..67a1184d 100644 --- a/xcresult/tests/swift_test_xunit.rs +++ b/xcresult/tests/swift_test_xunit.rs @@ -1,10 +1,13 @@ //! `swift test --xunit-output` reports no file at all, and needs no Xcode to produce — so //! this is the shape the declaration path takes on Linux. See the fixture's README. +mod common; + use std::{collections::HashMap, path::Path}; +use common::limits; use rstest::rstest; -use xcresult::test_locations::{Limits, TestKey, TestLocationIndex}; +use xcresult::test_locations::{TestKey, TestLocationIndex}; use xcresult::xcrun::find_program; const FIXTURE_ROOT: &str = "tests/fixture-src/swift-test-xunit"; @@ -73,7 +76,7 @@ fn resolve_from(xunit: &str) -> HashMap<(String, String), String> { ) }) .collect::>(); - let index = TestLocationIndex::resolve(Path::new(FIXTURE_ROOT), &keys, Limits::default()); + let index = TestLocationIndex::resolve(Path::new(FIXTURE_ROOT), &keys, limits()); cases .into_iter() @@ -233,7 +236,7 @@ mod parity { String::from("trunk"), String::from("github.com/trunk-io/analytics-cli"), Path::new(FIXTURE_ROOT), - Limits::default(), + limits(), ) .expect("the declaration path reads the bundle");