feat(cli): accept globs in --swift-test-xunit-paths

bundle/src/files.rs

@@ -113,14 +113,30 @@ impl FileSetBuilder {
         Ok(file_set_builder)
     }
 
+    /// The files `globs` match, in glob order and deduplicated by canonical path.
+    pub fn expand_globs<T: AsRef<str>>(
+        repo_root: T,
+        globs: &[String],
+    ) -> anyhow::Result<Vec<PathBuf>> {
+        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],
         codeowners: Option<CodeOwners>,
         exec_start: Option<SystemTime>,
     ) -> anyhow::Result<Self> {
         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::<Vec<_>>();
+        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 +172,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<Vec<Vec<PathBuf>>> {
         let mut claimed: HashSet<PathBuf> = 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<PathBuf> = matches
@@ -182,7 +198,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(),

bundle/tests/expand_globs_test.rs

@@ -0,0 +1,73 @@
+use std::path::PathBuf;
+
+use bundle::FileSetBuilder;
+
+fn repo_with<const N: usize>(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:?}");
+}

cli/src/context.rs

@@ -915,17 +915,19 @@ 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<Vec<JunitReportFileWithTestRunnerReport>> {
+    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());
     }
 

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 `<name>-swift-testing.xml` and XCTest to `<name>`, 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 `<name>-swift-testing.xml` and XCTest to \
+                `<name>`, the latter only when `--parallel` is also passed. Upload both if \
+                the project uses both frameworks.",
         required = false
     )]
     pub swift_test_xunit_paths: Vec<String>,

cli/tests/upload.rs

@@ -3194,14 +3194,10 @@ 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, each declared 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 +3241,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 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,
+) -> Vec<(String, Option<String>)> {
     let bundle_meta: BundleMeta =
         serde_json::from_reader(fs::File::open(tar_extract_directory.join("meta.json")).unwrap())
             .unwrap();
@@ -3279,24 +3274,24 @@ 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 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"),
+    ("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<String>)>) {
     let files = cases
         .into_iter()
         .collect::<std::collections::HashMap<String, Option<String>>>();
 
-    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 +3303,95 @@ 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);
+}
+
+#[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);
+
+    assert_eq!(
+        cases.len(),
+        3,
+        "expected the glob to reach both files once each, got {cases:?}"
+    );
+    assert_declarations_resolved(cases);
+}
+
+// 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() {
+    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);
+}