From 12c3c0af7ee79df7e53e802a05b6dbebbcf04bb4 Mon Sep 17 00:00:00 2001 From: Dylan Frankland Date: Mon, 14 Sep 2026 21:36:24 +0000 Subject: [PATCH 1/3] feat(cli): accept globs in --swift-test-xunit-paths The argument took literal filenames, so a repository with more than one test target had to spell out every file `swift test --xunit-output` wrote, and a pattern reached `File::open` verbatim and failed. `--junit-paths` has always globbed, so the same spelling meant different things depending on which argument carried it. Expansion now goes through the junit path's own, which also brings its dedupe: matches are keyed by canonical path, so a file two patterns both match -- or one reached through a symlink as well as directly -- is parsed once instead of uploading every test it holds twice. `collect_files_per_glob` took junit wrappers to read one field off each, so it now takes the globs themselves and `expand_globs` exposes the flattened result. One implementation, rather than a second one that drifts. Two consequences worth knowing. A relative value now resolves against the repo root rather than the working directory, which is what `--junit-paths` has always done and only differs when the uploader runs outside the repo. And a value naming a file that is not there no longer fails on open -- it matches nothing, exactly as a junit glob does, leaving the existing empty-results check to decide whether that is an error. Co-Authored-By: Claude Opus 5 (1M context) --- bundle/src/files.rs | 112 ++++++++++++++++++++++++++-- cli/src/context.rs | 17 +++-- cli/src/upload_command.rs | 13 ++-- cli/tests/upload.rs | 149 +++++++++++++++++++++++++++++++------- 4 files changed, 245 insertions(+), 46 deletions(-) diff --git a/bundle/src/files.rs b/bundle/src/files.rs index 61a9dc4c..865c35fa 100644 --- a/bundle/src/files.rs +++ b/bundle/src/files.rs @@ -113,6 +113,23 @@ impl FileSetBuilder { Ok(file_set_builder) } + /// The files `globs` match, in glob order and deduplicated exactly as junit globs are. + /// + /// For a caller that reads the matched files itself rather than handing them straight to + /// [`Self::build_file_sets`] -- the `swift test --xunit-output` path rewrites each report + /// before bundling it -- so that one spelling of a path does not expand differently + /// depending on which argument carried it. + pub fn expand_globs>( + repo_root: T, + globs: &[String], + ) -> anyhow::Result> { + let repo_root = RepoRoot::canonical(repo_root.as_ref()); + Ok(Self::collect_files_per_glob(&repo_root, globs)? + .into_iter() + .flatten() + .collect()) + } + fn file_sets_from_glob( repo_root: &str, junit_paths: &[JunitReportFileWithTestRunnerReport], @@ -120,7 +137,11 @@ impl FileSetBuilder { exec_start: Option, ) -> anyhow::Result { let repo_root = RepoRoot::canonical(repo_root); - let files_per_glob = Self::collect_files_per_glob(&repo_root, junit_paths)?; + let globs = junit_paths + .iter() + .map(|junit_wrapper| junit_wrapper.junit_path.clone()) + .collect::>(); + let files_per_glob = Self::collect_files_per_glob(&repo_root, &globs)?; let (count, file_sets) = junit_paths.iter().zip(files_per_glob).try_fold( (0, Vec::with_capacity(junit_paths.len())), @@ -156,14 +177,14 @@ impl FileSetBuilder { /// already claimed owns nothing, which keeps its (now empty) file set in place. fn collect_files_per_glob( repo_root: &RepoRoot, - junit_paths: &[JunitReportFileWithTestRunnerReport], + globs: &[String], ) -> anyhow::Result>> { let mut claimed: HashSet = HashSet::new(); - junit_paths + globs .iter() - .map(|junit_wrapper| { - let matches = Self::scan_from_glob(&junit_wrapper.junit_path, repo_root.as_str())?; + .map(|glob_path| { + let matches = Self::scan_from_glob(glob_path, repo_root.as_str())?; let matched = matches.len(); let mut owned: Vec = matches @@ -182,7 +203,7 @@ impl FileSetBuilder { tracing::warn!( "glob {:?} matched {} paths resolving to {} files not already \ collected; {} duplicate routes were dropped", - junit_wrapper.junit_path, + glob_path, matched, owned.len(), matched - owned.len(), @@ -499,3 +520,82 @@ impl BundledFile { .unwrap_or(&self.original_path) } } + +/// What every argument taking a path shares, so these pin the behaviour once rather than +/// once per caller. Deliberately free of any language server or test runner: expansion is +/// the half that can be proven anywhere, and the `swift test` integration tests that cover +/// the other half need a Swift toolchain to say anything at all. +#[cfg(test)] +mod expand_globs_tests { + use super::*; + + fn repo_with(files: [&str; N]) -> (tempfile::TempDir, PathBuf) { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().canonicalize().unwrap(); + for file in files { + let path = root.join(file); + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).unwrap(); + } + std::fs::write(path, "").unwrap(); + } + (dir, root) + } + + #[test] + fn a_file_two_globs_both_match_is_returned_once() { + let (_dir, root) = repo_with(["junit.xml", "junit-swift-testing.xml"]); + + let files = FileSetBuilder::expand_globs( + root.to_string_lossy(), + &[String::from("junit*.xml"), String::from("junit.xml")], + ) + .unwrap(); + + // The second pattern owns nothing: the first already claimed the file it names. + assert_eq!( + files, + vec![root.join("junit-swift-testing.xml"), root.join("junit.xml")] + ); + } + + /// The dedupe is on the canonical path rather than the matched one, which is the only + /// way a link and its target are recognised as one file. + #[cfg(unix)] + #[test] + fn a_symlink_to_an_already_matched_file_is_not_returned_again() { + let (_dir, root) = repo_with(["real.xml"]); + std::os::unix::fs::symlink(root.join("real.xml"), root.join("link.xml")).unwrap(); + + let files = + FileSetBuilder::expand_globs(root.to_string_lossy(), &[String::from("*.xml")]).unwrap(); + + assert_eq!(files, vec![root.join("real.xml")]); + } + + /// A relative pattern is resolved against the repo root, not the working directory, so + /// where the uploader was invoked from does not change which files it finds. + #[test] + fn relative_globs_resolve_against_the_repo_root() { + let (_dir, root) = repo_with(["reports/junit.xml"]); + + let files = + FileSetBuilder::expand_globs(root.to_string_lossy(), &[String::from("reports/*.xml")]) + .unwrap(); + + assert_eq!(files, vec![root.join("reports/junit.xml")]); + } + + #[test] + fn a_glob_matching_nothing_owns_nothing() { + let (_dir, root) = repo_with(["junit.xml"]); + + let files = FileSetBuilder::expand_globs( + root.to_string_lossy(), + &[String::from("nothing-here/*.xml")], + ) + .unwrap(); + + assert!(files.is_empty(), "got {files:?}"); + } +} diff --git a/cli/src/context.rs b/cli/src/context.rs index a33b7072..14564e3c 100644 --- a/cli/src/context.rs +++ b/cli/src/context.rs @@ -915,17 +915,22 @@ pub async fn gather_upload_id_context( /// where a language server says it is declared. Needs no Xcode, unlike the `.xcresult` path. fn handle_swift_test_xunit( junit_temp_dir: &tempfile::TempDir, - paths: &[String], + globs: &[String], repo_root: &str, ) -> anyhow::Result> { + // Expanded the way junit globs are, so the same pattern reaches the same files through + // either argument, and a file reached twice -- by two patterns, or by two symlinked + // routes to one canonical path -- is parsed once rather than uploading its tests twice. + let paths = FileSetBuilder::expand_globs(repo_root, globs)?; + let mut reports = Vec::new(); - for path in paths { + for path in &paths { let file = std::fs::File::open(path) - .map_err(|e| anyhow::anyhow!("failed to open {}: {}", path, e))?; + .map_err(|e| anyhow::anyhow!("failed to open {}: {}", path.display(), e))?; let mut parser = JunitParser::new(); - parser - .parse(BufReader::new(file)) - .map_err(|e| anyhow::anyhow!("failed to parse {} as JUnit XML: {}", path, e))?; + parser.parse(BufReader::new(file)).map_err(|e| { + anyhow::anyhow!("failed to parse {} as JUnit XML: {}", path.display(), e) + })?; reports.extend(parser.into_reports()); } diff --git a/cli/src/upload_command.rs b/cli/src/upload_command.rs index 4c4b0175..3ac7873a 100644 --- a/cli/src/upload_command.rs +++ b/cli/src/upload_command.rs @@ -73,12 +73,13 @@ pub struct UploadArgs { long, env = constants::TRUNK_SWIFT_TEST_XUNIT_PATHS_ENV, value_delimiter = ',', - help = "Comma-separated list of JUnit files written by `swift test --xunit-output`. \ - These carry no file path, so each test's file is taken from where a language \ - server says it is declared in the repository. One run writes two files: \ - swift-testing to `-swift-testing.xml` and XCTest to ``, the \ - latter only when `--parallel` is also passed. Upload both if the project \ - uses both frameworks.", + help = "Comma-separated list of glob patterns to JUnit files written by \ + `swift test --xunit-output` (e.g. 'junit*.xml'), resolved against the \ + repository root. These carry no file path, so each test's file is taken from \ + where a language server says it is declared in the repository. One run writes \ + two files: swift-testing to `-swift-testing.xml` and XCTest to \ + ``, the latter only when `--parallel` is also passed. Upload both if \ + the project uses both frameworks.", required = false )] pub swift_test_xunit_paths: Vec, diff --git a/cli/tests/upload.rs b/cli/tests/upload.rs index 9dc7cc24..50774b56 100644 --- a/cli/tests/upload.rs +++ b/cli/tests/upload.rs @@ -3194,14 +3194,11 @@ async fn upload_bundle_keeps_the_repo_relative_path_when_a_symlink_leaves_the_re println!("{assert}"); } -// `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. +/// A package whose three tests are split across the two files one `swift test --xunit-output` +/// run writes, so a glob reaching both is distinguishable from one reaching only the +/// swift-testing half, and each test's declaration lives in a file of its own. #[cfg(any(target_os = "macos", target_os = "linux"))] -#[tokio::test(flavor = "multi_thread")] -async fn upload_bundle_using_swift_test_xunit() { - let temp_dir = tempdir().unwrap(); - generate_mock_git_repo(&temp_dir); - +fn write_swift_test_xunit_fixture(temp_dir: &tempfile::TempDir) { let tests_dir = temp_dir.path().join("Tests/MyCLITests"); fs::create_dir_all(&tests_dir).unwrap(); fs::write( @@ -3245,16 +3242,15 @@ async fn upload_bundle_using_swift_test_xunit() { ), ) .unwrap(); +} - let state = MockServerBuilder::new().spawn_mock_server().await; - CommandBuilder::upload(temp_dir.path(), state.host.clone()) - .swift_test_xunit_paths("xunit-swift-testing.xml,xunit.xml") - .command() - .assert() - .success(); - - let requests = state.requests.lock().unwrap().clone(); - let tar_extract_directory = assert_matches!(&requests[1], RequestPayload::S3Upload(d) => d); +/// Every test case in the uploaded bundle, paired with the file it was attributed to. Reads +/// every file set, so a report bundled twice shows up as a longer list rather than as a +/// silently overwritten entry. +#[cfg(any(target_os = "macos", target_os = "linux"))] +fn bundled_cases_with_files( + tar_extract_directory: &std::path::Path, +) -> Vec<(String, Option)> { let bundle_meta: BundleMeta = serde_json::from_reader(fs::File::open(tar_extract_directory.join("meta.json")).unwrap()) .unwrap(); @@ -3279,24 +3275,25 @@ async fn upload_bundle_using_swift_test_xunit() { } } } + cases +} - // 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:?}" - ); +/// The file each test is expected to be attributed to, which is the file it is declared in +/// rather than any the xunit XML mentions -- it mentions none. +#[cfg(any(target_os = "macos", target_os = "linux"))] +const SWIFT_TEST_XUNIT_DECLARATIONS: [(&str, &str); 3] = [ + ("helloworld()", "Tests/MyCLITests/TopLevel.swift"), + ("shared()", "Tests/MyCLITests/Suites.swift"), + ("testOldStyle", "Tests/MyCLITests/Legacy.swift"), +]; +#[cfg(any(target_os = "macos", target_os = "linux"))] +fn assert_declarations_resolved(cases: Vec<(String, Option)>) { let files = cases .into_iter() .collect::>>(); - for (name, expected) in [ - ("helloworld()", "Tests/MyCLITests/TopLevel.swift"), - ("shared()", "Tests/MyCLITests/Suites.swift"), - ("testOldStyle", "Tests/MyCLITests/Legacy.swift"), - ] { + for (name, expected) in SWIFT_TEST_XUNIT_DECLARATIONS { let file = files .get(name) .unwrap_or_else(|| panic!("{name} is missing from the bundle")) @@ -3308,3 +3305,99 @@ async fn upload_bundle_using_swift_test_xunit() { ); } } + +// `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. +#[cfg(any(target_os = "macos", target_os = "linux"))] +#[tokio::test(flavor = "multi_thread")] +async fn upload_bundle_using_swift_test_xunit() { + let temp_dir = tempdir().unwrap(); + generate_mock_git_repo(&temp_dir); + write_swift_test_xunit_fixture(&temp_dir); + + let state = MockServerBuilder::new().spawn_mock_server().await; + CommandBuilder::upload(temp_dir.path(), state.host.clone()) + .swift_test_xunit_paths("xunit-swift-testing.xml,xunit.xml") + .command() + .assert() + .success(); + + let requests = state.requests.lock().unwrap().clone(); + let tar_extract_directory = assert_matches!(&requests[1], RequestPayload::S3Upload(d) => d); + let cases = bundled_cases_with_files(tar_extract_directory); + + // 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:?}" + ); + assert_declarations_resolved(cases); +} + +// These are globs, expanded exactly as `--junit-paths` expands its own, so the two files one +// `swift test` run writes can be named by the pattern that produced them. +#[cfg(any(target_os = "macos", target_os = "linux"))] +#[tokio::test(flavor = "multi_thread")] +async fn swift_test_xunit_paths_are_globs() { + let temp_dir = tempdir().unwrap(); + generate_mock_git_repo(&temp_dir); + write_swift_test_xunit_fixture(&temp_dir); + + let state = MockServerBuilder::new().spawn_mock_server().await; + CommandBuilder::upload(temp_dir.path(), state.host.clone()) + .swift_test_xunit_paths("xunit*.xml") + .command() + .assert() + .success(); + + let requests = state.requests.lock().unwrap().clone(); + let tar_extract_directory = assert_matches!(&requests[1], RequestPayload::S3Upload(d) => d); + let cases = bundled_cases_with_files(tar_extract_directory); + + // One pattern reaching both files, so the XCTest half is not silently left behind. + assert_eq!( + cases.len(), + 3, + "expected the glob to reach both files once each, got {cases:?}" + ); + assert_declarations_resolved(cases); +} + +// A file reached twice is bundled once. The dedupe is on the canonical path, so a symlink +// pointing at a file another pattern already claimed resolves to it rather than doubling +// every test it holds -- which is what an uploaded duplicate would look like downstream. +#[cfg(any(target_os = "macos", target_os = "linux"))] +#[tokio::test(flavor = "multi_thread")] +async fn a_swift_test_xunit_file_reached_twice_is_uploaded_once() { + let temp_dir = tempdir().unwrap(); + generate_mock_git_repo(&temp_dir); + write_swift_test_xunit_fixture(&temp_dir); + + std::os::unix::fs::symlink( + temp_dir.path().join("xunit.xml"), + temp_dir.path().join("xunit-link.xml"), + ) + .unwrap(); + + let state = MockServerBuilder::new().spawn_mock_server().await; + CommandBuilder::upload(temp_dir.path(), state.host.clone()) + // Three routes to two files: the glob matches both real files and the symlink, and + // the literal path names one of them a second time. + .swift_test_xunit_paths("xunit*.xml,xunit.xml") + .command() + .assert() + .success(); + + let requests = state.requests.lock().unwrap().clone(); + let tar_extract_directory = assert_matches!(&requests[1], RequestPayload::S3Upload(d) => d); + let cases = bundled_cases_with_files(tar_extract_directory); + + assert_eq!( + cases.len(), + 3, + "expected duplicate routes to one file to collapse, got {cases:?}" + ); + assert_declarations_resolved(cases); +} From 1c92055777236441bcfaea75b4f1628bb162e555 Mon Sep 17 00:00:00 2001 From: Dylan Frankland Date: Mon, 14 Sep 2026 21:46:00 +0000 Subject: [PATCH 2/3] test(bundle): move the expand_globs tests to bundle/tests/ They only exercise `FileSetBuilder::expand_globs`, which is public, so nothing about them needs to sit inside the module. As an integration test they exercise the crate the way its callers do and cannot quietly come to depend on a private detail. Co-Authored-By: Claude Opus 5 (1M context) --- bundle/src/files.rs | 79 ------------------------------- bundle/tests/expand_globs_test.rs | 79 +++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 79 deletions(-) create mode 100644 bundle/tests/expand_globs_test.rs diff --git a/bundle/src/files.rs b/bundle/src/files.rs index 865c35fa..820a22a5 100644 --- a/bundle/src/files.rs +++ b/bundle/src/files.rs @@ -520,82 +520,3 @@ impl BundledFile { .unwrap_or(&self.original_path) } } - -/// What every argument taking a path shares, so these pin the behaviour once rather than -/// once per caller. Deliberately free of any language server or test runner: expansion is -/// the half that can be proven anywhere, and the `swift test` integration tests that cover -/// the other half need a Swift toolchain to say anything at all. -#[cfg(test)] -mod expand_globs_tests { - use super::*; - - fn repo_with(files: [&str; N]) -> (tempfile::TempDir, PathBuf) { - let dir = tempfile::tempdir().unwrap(); - let root = dir.path().canonicalize().unwrap(); - for file in files { - let path = root.join(file); - if let Some(parent) = path.parent() { - std::fs::create_dir_all(parent).unwrap(); - } - std::fs::write(path, "").unwrap(); - } - (dir, root) - } - - #[test] - fn a_file_two_globs_both_match_is_returned_once() { - let (_dir, root) = repo_with(["junit.xml", "junit-swift-testing.xml"]); - - let files = FileSetBuilder::expand_globs( - root.to_string_lossy(), - &[String::from("junit*.xml"), String::from("junit.xml")], - ) - .unwrap(); - - // The second pattern owns nothing: the first already claimed the file it names. - assert_eq!( - files, - vec![root.join("junit-swift-testing.xml"), root.join("junit.xml")] - ); - } - - /// The dedupe is on the canonical path rather than the matched one, which is the only - /// way a link and its target are recognised as one file. - #[cfg(unix)] - #[test] - fn a_symlink_to_an_already_matched_file_is_not_returned_again() { - let (_dir, root) = repo_with(["real.xml"]); - std::os::unix::fs::symlink(root.join("real.xml"), root.join("link.xml")).unwrap(); - - let files = - FileSetBuilder::expand_globs(root.to_string_lossy(), &[String::from("*.xml")]).unwrap(); - - assert_eq!(files, vec![root.join("real.xml")]); - } - - /// A relative pattern is resolved against the repo root, not the working directory, so - /// where the uploader was invoked from does not change which files it finds. - #[test] - fn relative_globs_resolve_against_the_repo_root() { - let (_dir, root) = repo_with(["reports/junit.xml"]); - - let files = - FileSetBuilder::expand_globs(root.to_string_lossy(), &[String::from("reports/*.xml")]) - .unwrap(); - - assert_eq!(files, vec![root.join("reports/junit.xml")]); - } - - #[test] - fn a_glob_matching_nothing_owns_nothing() { - let (_dir, root) = repo_with(["junit.xml"]); - - let files = FileSetBuilder::expand_globs( - root.to_string_lossy(), - &[String::from("nothing-here/*.xml")], - ) - .unwrap(); - - assert!(files.is_empty(), "got {files:?}"); - } -} diff --git a/bundle/tests/expand_globs_test.rs b/bundle/tests/expand_globs_test.rs new file mode 100644 index 00000000..c93346ba --- /dev/null +++ b/bundle/tests/expand_globs_test.rs @@ -0,0 +1,79 @@ +//! What every argument taking a path shares, pinned once rather than once per caller. +//! +//! Deliberately free of any language server or test runner: expansion is the half that can be +//! proven anywhere, and the `swift test --xunit-output` integration tests that cover the other +//! half need a Swift toolchain to say anything at all. + +use std::path::PathBuf; + +use bundle::FileSetBuilder; + +fn repo_with(files: [&str; N]) -> (tempfile::TempDir, PathBuf) { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().canonicalize().unwrap(); + for file in files { + let path = root.join(file); + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).unwrap(); + } + std::fs::write(path, "").unwrap(); + } + (dir, root) +} + +#[test] +fn a_file_two_globs_both_match_is_returned_once() { + let (_dir, root) = repo_with(["junit.xml", "junit-swift-testing.xml"]); + + let files = FileSetBuilder::expand_globs( + root.to_string_lossy(), + &[String::from("junit*.xml"), String::from("junit.xml")], + ) + .unwrap(); + + // The second pattern owns nothing: the first already claimed the file it names. + assert_eq!( + files, + vec![root.join("junit-swift-testing.xml"), root.join("junit.xml")] + ); +} + +/// The dedupe is on the canonical path rather than the matched one, which is the only way a +/// link and its target are recognised as one file. +#[cfg(unix)] +#[test] +fn a_symlink_to_an_already_matched_file_is_not_returned_again() { + let (_dir, root) = repo_with(["real.xml"]); + std::os::unix::fs::symlink(root.join("real.xml"), root.join("link.xml")).unwrap(); + + let files = + FileSetBuilder::expand_globs(root.to_string_lossy(), &[String::from("*.xml")]).unwrap(); + + assert_eq!(files, vec![root.join("real.xml")]); +} + +/// A relative pattern is resolved against the repo root, not the working directory, so where +/// the uploader was invoked from does not change which files it finds. +#[test] +fn relative_globs_resolve_against_the_repo_root() { + let (_dir, root) = repo_with(["reports/junit.xml"]); + + let files = + FileSetBuilder::expand_globs(root.to_string_lossy(), &[String::from("reports/*.xml")]) + .unwrap(); + + assert_eq!(files, vec![root.join("reports/junit.xml")]); +} + +#[test] +fn a_glob_matching_nothing_owns_nothing() { + let (_dir, root) = repo_with(["junit.xml"]); + + let files = FileSetBuilder::expand_globs( + root.to_string_lossy(), + &[String::from("nothing-here/*.xml")], + ) + .unwrap(); + + assert!(files.is_empty(), "got {files:?}"); +} From a41d5205c718c44a5848c1631421045bfdc189c3 Mon Sep 17 00:00:00 2001 From: Dylan Frankland Date: Mon, 14 Sep 2026 21:53:16 +0000 Subject: [PATCH 3/3] refactor: cut narration from the swift xunit glob comments Say what the code does and drop the rest: which argument historically expanded differently, why a test needs no toolchain, what an uploaded duplicate would look like downstream. Co-Authored-By: Claude Opus 5 (1M context) --- bundle/src/files.rs | 7 +------ bundle/tests/expand_globs_test.rs | 6 ------ cli/src/context.rs | 3 --- cli/tests/upload.rs | 18 ++++++------------ 4 files changed, 7 insertions(+), 27 deletions(-) diff --git a/bundle/src/files.rs b/bundle/src/files.rs index 820a22a5..32cb5c21 100644 --- a/bundle/src/files.rs +++ b/bundle/src/files.rs @@ -113,12 +113,7 @@ impl FileSetBuilder { Ok(file_set_builder) } - /// The files `globs` match, in glob order and deduplicated exactly as junit globs are. - /// - /// For a caller that reads the matched files itself rather than handing them straight to - /// [`Self::build_file_sets`] -- the `swift test --xunit-output` path rewrites each report - /// before bundling it -- so that one spelling of a path does not expand differently - /// depending on which argument carried it. + /// The files `globs` match, in glob order and deduplicated by canonical path. pub fn expand_globs>( repo_root: T, globs: &[String], diff --git a/bundle/tests/expand_globs_test.rs b/bundle/tests/expand_globs_test.rs index c93346ba..48fc1f1b 100644 --- a/bundle/tests/expand_globs_test.rs +++ b/bundle/tests/expand_globs_test.rs @@ -1,9 +1,3 @@ -//! What every argument taking a path shares, pinned once rather than once per caller. -//! -//! Deliberately free of any language server or test runner: expansion is the half that can be -//! proven anywhere, and the `swift test --xunit-output` integration tests that cover the other -//! half need a Swift toolchain to say anything at all. - use std::path::PathBuf; use bundle::FileSetBuilder; diff --git a/cli/src/context.rs b/cli/src/context.rs index 14564e3c..cc0da916 100644 --- a/cli/src/context.rs +++ b/cli/src/context.rs @@ -918,9 +918,6 @@ fn handle_swift_test_xunit( globs: &[String], repo_root: &str, ) -> anyhow::Result> { - // Expanded the way junit globs are, so the same pattern reaches the same files through - // either argument, and a file reached twice -- by two patterns, or by two symlinked - // routes to one canonical path -- is parsed once rather than uploading its tests twice. let paths = FileSetBuilder::expand_globs(repo_root, globs)?; let mut reports = Vec::new(); diff --git a/cli/tests/upload.rs b/cli/tests/upload.rs index 50774b56..27a99d6e 100644 --- a/cli/tests/upload.rs +++ b/cli/tests/upload.rs @@ -3195,8 +3195,7 @@ async fn upload_bundle_keeps_the_repo_relative_path_when_a_symlink_leaves_the_re } /// A package whose three tests are split across the two files one `swift test --xunit-output` -/// run writes, so a glob reaching both is distinguishable from one reaching only the -/// swift-testing half, and each test's declaration lives in a file of its own. +/// run writes, each declared in a file of its own. #[cfg(any(target_os = "macos", target_os = "linux"))] fn write_swift_test_xunit_fixture(temp_dir: &tempfile::TempDir) { let tests_dir = temp_dir.path().join("Tests/MyCLITests"); @@ -3245,8 +3244,8 @@ fn write_swift_test_xunit_fixture(temp_dir: &tempfile::TempDir) { } /// Every test case in the uploaded bundle, paired with the file it was attributed to. Reads -/// every file set, so a report bundled twice shows up as a longer list rather than as a -/// silently overwritten entry. +/// every file set, so a report bundled twice lengthens the list rather than overwriting an +/// entry in it. #[cfg(any(target_os = "macos", target_os = "linux"))] fn bundled_cases_with_files( tar_extract_directory: &std::path::Path, @@ -3278,8 +3277,7 @@ fn bundled_cases_with_files( cases } -/// The file each test is expected to be attributed to, which is the file it is declared in -/// rather than any the xunit XML mentions -- it mentions none. +/// The file each test is declared in, which the xunit XML does not name. #[cfg(any(target_os = "macos", target_os = "linux"))] const SWIFT_TEST_XUNIT_DECLARATIONS: [(&str, &str); 3] = [ ("helloworld()", "Tests/MyCLITests/TopLevel.swift"), @@ -3336,8 +3334,6 @@ async fn upload_bundle_using_swift_test_xunit() { assert_declarations_resolved(cases); } -// These are globs, expanded exactly as `--junit-paths` expands its own, so the two files one -// `swift test` run writes can be named by the pattern that produced them. #[cfg(any(target_os = "macos", target_os = "linux"))] #[tokio::test(flavor = "multi_thread")] async fn swift_test_xunit_paths_are_globs() { @@ -3356,7 +3352,6 @@ async fn swift_test_xunit_paths_are_globs() { let tar_extract_directory = assert_matches!(&requests[1], RequestPayload::S3Upload(d) => d); let cases = bundled_cases_with_files(tar_extract_directory); - // One pattern reaching both files, so the XCTest half is not silently left behind. assert_eq!( cases.len(), 3, @@ -3365,9 +3360,8 @@ async fn swift_test_xunit_paths_are_globs() { assert_declarations_resolved(cases); } -// A file reached twice is bundled once. The dedupe is on the canonical path, so a symlink -// pointing at a file another pattern already claimed resolves to it rather than doubling -// every test it holds -- which is what an uploaded duplicate would look like downstream. +// The dedupe is on the canonical path, so a symlink resolves to the file another pattern +// already claimed rather than doubling every test it holds. #[cfg(any(target_os = "macos", target_os = "linux"))] #[tokio::test(flavor = "multi_thread")] async fn a_swift_test_xunit_file_reached_twice_is_uploaded_once() {