diff --git a/bundle/src/files.rs b/bundle/src/files.rs index 61a9dc4c..32cb5c21 100644 --- a/bundle/src/files.rs +++ b/bundle/src/files.rs @@ -113,6 +113,18 @@ impl FileSetBuilder { Ok(file_set_builder) } + /// The files `globs` match, in glob order and deduplicated by canonical path. + 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 +132,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 +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>> { 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 +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(), diff --git a/bundle/tests/expand_globs_test.rs b/bundle/tests/expand_globs_test.rs new file mode 100644 index 00000000..48fc1f1b --- /dev/null +++ b/bundle/tests/expand_globs_test.rs @@ -0,0 +1,73 @@ +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:?}"); +} diff --git a/cli/src/context.rs b/cli/src/context.rs index a33b7072..cc0da916 100644 --- a/cli/src/context.rs +++ b/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> { + 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..27a99d6e 100644 --- a/cli/tests/upload.rs +++ b/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)> { 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)>) { 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 +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); +}