diff --git a/.github/workflows/pull_request.yml b/.github/workflows/pull_request.yml index 2c830c25..be778c31 100644 --- a/.github/workflows/pull_request.yml +++ b/.github/workflows/pull_request.yml @@ -198,10 +198,29 @@ jobs: exit 1 fi + # The declaration path finds `sourcekit-lsp` on `PATH`, which is where every documented + # Linux install puts it. The Ubuntu runner image is the exception: it symlinks only + # `swift` and `swiftc` into /usr/local/bin and leaves the rest of the toolchain reachable + # only through $SWIFT_PATH. Without this the swift-test-xunit tests find no server. + - name: Put the Swift toolchain on PATH + if: ${{ startsWith(matrix.platform.os-name, 'linux-') }} + shell: bash + run: | + if [[ -z "${SWIFT_PATH:-}" ]]; then + echo "SWIFT_PATH is unset - the runner image no longer ships Swift where expected" >&2 + exit 1 + fi + echo "$SWIFT_PATH" >> "$GITHUB_PATH" + - name: Run tests uses: ./.github/actions/run_tests id: tests continue-on-error: true + env: + # macOS finds the server through `xcrun`, and the step above puts it on `PATH` for + # Linux -- so anywhere but the self-hosted Windows runner, which has no Swift at all, + # a missing server means the environment broke rather than that these should skip. + REQUIRE_LANGUAGE_SERVER: ${{ matrix.platform.os-name != 'windows-x86_64' && '1' || '' }} with: target: ${{ matrix.platform.target }} codecov-token: ${{ secrets.CODECOV_TOKEN }} diff --git a/Cargo.lock b/Cargo.lock index 75b576d9..d6d6dc1b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1452,6 +1452,15 @@ dependencies = [ "num-traits", ] +[[package]] +name = "fluent-uri" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "17c704e9dbe1ddd863da1e6ff3567795087b1eb201ce80d8fa81162e1516500d" +dependencies = [ + "bitflags 1.3.2", +] + [[package]] name = "fnv" version = "1.0.7" @@ -2203,6 +2212,19 @@ version = "0.3.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0cc23270f6e1808e30a928bdc84dea0b9b4136a8bc82338574f23baf47bbd280" +[[package]] +name = "globset" +version = "0.4.20" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "07c34a9410465b45bd9787443bc7370f37735bad04b0f0cd57ff1a3186c98988" +dependencies = [ + "aho-corasick", + "bstr", + "log", + "regex-automata", + "regex-syntax", +] + [[package]] name = "gloo-timers" version = "0.3.0" @@ -2642,6 +2664,22 @@ dependencies = [ "icu_properties", ] +[[package]] +name = "ignore" +version = "0.4.33" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "00b69833ed729dc5aa7d19541d96d6cf8e9137194207a04916d658e43168402f" +dependencies = [ + "crossbeam-deque", + "globset", + "log", + "memchr", + "regex-automata", + "same-file", + "walkdir", + "winapi-util", +] + [[package]] name = "indexmap" version = "1.9.3" @@ -3025,6 +3063,32 @@ version = "0.1.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "112b39cec0b298b6c1999fee3e31427f74f676e4cb9879ed1a121b43661a4154" +[[package]] +name = "lsp-server" +version = "0.10.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3ee25a31f2e571e426eef2896179450cafc7e2f5be00d8a93b1c2d21c0ff7656" +dependencies = [ + "crossbeam-channel", + "log", + "serde", + "serde_derive", + "serde_json", +] + +[[package]] +name = "lsp-types" +version = "0.97.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "53353550a17c04ac46c585feb189c2db82154fc84b79c7a66c96c2c644f66071" +dependencies = [ + "bitflags 1.3.2", + "fluent-uri", + "serde", + "serde_json", + "serde_repr", +] + [[package]] name = "magnus" version = "0.8.2" @@ -4579,9 +4643,9 @@ dependencies = [ [[package]] name = "regex-automata" -version = "0.4.13" +version = "0.4.18" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5276caf25ac86c8d810222b3dbb938e512c55c6831a10f3e6ed1c93b84041f1c" +checksum = "ad8553b9b26413251cbf30e620595c7a41b3887f03da04579c0e6b0d6a06b4b2" dependencies = [ "aho-corasick", "memchr", @@ -5103,6 +5167,17 @@ dependencies = [ "serde_core", ] +[[package]] +name = "serde_repr" +version = "0.1.21" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8d3b1629de253c70a0508c3899572da79ca359fdab27c7920ff00406df418906" +dependencies = [ + "proc-macro2", + "quote", + "syn 3.0.4", +] + [[package]] name = "serde_spanned" version = "0.6.9" @@ -5462,6 +5537,17 @@ dependencies = [ "unicode-ident", ] +[[package]] +name = "syn" +version = "3.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e6275cddf4610d1775e6d1fe9469b2e77d0f39fd98fb7450901b821e0c53649f" +dependencies = [ + "proc-macro2", + "quote", + "unicode-ident", +] + [[package]] name = "sync_wrapper" version = "0.1.2" @@ -6156,6 +6242,7 @@ dependencies = [ "quick-junit", "regex", "reqwest", + "rstest", "sentry", "sentry-tracing", "serde_json", @@ -7034,9 +7121,13 @@ dependencies = [ "anyhow", "chrono", "clap", + "constants", "context", "flate2", + "ignore", "lazy_static", + "lsp-server", + "lsp-types", "petgraph 0.7.1", "pretty_assertions", "prettyplease", @@ -7050,9 +7141,11 @@ dependencies = [ "syn 2.0.110", "tar", "temp_testdir", + "tempfile", "tracing", "tracing-subscriber", "typify", + "url", "uuid", ] diff --git a/cli/Cargo.toml b/cli/Cargo.toml index 22230162..e25e519c 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -71,6 +71,7 @@ more-asserts = "0.3.1" predicates = "3.0.3" pretty_assertions = "0.6" prost-wkt-types = { version = "0.5.1", features = ["vendored-protox"] } +rstest = "0.26.1" [build-dependencies] vergen = { version = "8.3.1", features = [ diff --git a/cli/src/context.rs b/cli/src/context.rs index 4363f3d2..a33b7072 100644 --- a/cli/src/context.rs +++ b/cli/src/context.rs @@ -1,10 +1,10 @@ #[cfg(target_os = "macos")] -use std::io::Write; +use std::time::Duration; use std::{collections::BTreeMap, io::Read}; use std::{ collections::HashMap, env, - io::BufReader, + io::{BufReader, Write}, path::Path, time::{SystemTime, UNIX_EPOCH}, }; @@ -39,6 +39,7 @@ use proto::test_context::test_run::{ }; use regex::Regex; use tempfile::TempDir; +use xcresult::test_locations::{Limits, TestKey, TestLocationIndex}; #[cfg(target_os = "macos")] use xcresult::xcresult::XCResult; @@ -125,6 +126,7 @@ pub fn gather_initial_test_context( #[cfg(target_os = "macos")] xcresult_path, bazel_bep_path, + swift_test_xunit_paths, test_reports, org_url_slug, repo_root, @@ -137,6 +139,18 @@ pub fn gather_initial_test_context( pr_number, #[cfg(target_os = "macos")] use_experimental_failure_summary, + #[cfg(target_os = "macos")] + use_experimental_xcresult_test_locations, + #[cfg(target_os = "macos")] + xcresult_test_locations_max_files, + #[cfg(target_os = "macos")] + xcresult_test_locations_budget_secs, + #[cfg(target_os = "macos")] + xcresult_test_locations_request_timeout_secs, + #[cfg(target_os = "macos")] + xcresult_test_locations_retries, + #[cfg(target_os = "macos")] + xcresult_test_locations_max_file_bytes, .. } = upload_args; @@ -151,6 +165,22 @@ pub fn gather_initial_test_context( )?; tracing::debug!("Found repo state: {:?}", repo); + #[cfg(target_os = "macos")] + let xcresult_options = XCResultOptions { + repo: &repo.repo, + org_url_slug: &org_url_slug, + repo_root: &repo.repo_root, + use_experimental_failure_summary, + use_experimental_test_locations: use_experimental_xcresult_test_locations, + limits: Limits { + max_files: xcresult_test_locations_max_files, + budget: Duration::from_secs(xcresult_test_locations_budget_secs), + request_timeout: Duration::from_secs(xcresult_test_locations_request_timeout_secs), + retries: xcresult_test_locations_retries, + max_file_bytes: xcresult_test_locations_max_file_bytes, + }, + }; + let (junit_path_wrappers, bep_result, junit_path_wrappers_temp_dir) = coalesce_junit_path_wrappers( junit_paths, @@ -158,11 +188,9 @@ pub fn gather_initial_test_context( #[cfg(target_os = "macos")] xcresult_path, #[cfg(target_os = "macos")] - &repo.repo, - #[cfg(target_os = "macos")] - org_url_slug.clone(), - #[cfg(target_os = "macos")] - use_experimental_failure_summary, + &xcresult_options, + swift_test_xunit_paths, + &repo.repo_root, test_reports, allow_empty_test_results, )?; @@ -620,13 +648,25 @@ fn parse_as_bep(dir: String) -> anyhow::Result { result } +/// What the xcresult conversion needs beyond the bundle, kept together so an option added +/// later does not thread another `#[cfg]`-gated parameter through three signatures. +#[cfg(target_os = "macos")] +struct XCResultOptions<'a> { + repo: &'a RepoUrlParts, + org_url_slug: &'a str, + repo_root: &'a str, + use_experimental_failure_summary: bool, + use_experimental_test_locations: bool, + limits: Limits, +} + fn coalesce_junit_path_wrappers( junit_paths: Vec, bazel_bep_path: Option, #[cfg(target_os = "macos")] xcresult_path: Option, - #[cfg(target_os = "macos")] repo: &RepoUrlParts, - #[cfg(target_os = "macos")] org_url_slug: String, - #[cfg(target_os = "macos")] use_experimental_failure_summary: bool, + #[cfg(target_os = "macos")] xcresult_options: &XCResultOptions, + swift_test_xunit_paths: Vec, + repo_root: &str, test_reports: Vec, allow_empty_test_results: bool, ) -> anyhow::Result<( @@ -673,13 +713,7 @@ fn coalesce_junit_path_wrappers( #[cfg(target_os = "macos")] if xcresult_path.is_some() { let temp_dir = tempfile::tempdir()?; - let temp_paths = handle_xcresult( - &temp_dir, - xcresult_path, - repo, - &org_url_slug, - use_experimental_failure_summary, - )?; + let temp_paths = handle_xcresult(&temp_dir, xcresult_path, xcresult_options)?; _junit_path_wrappers_temp_dir = Some(temp_dir); junit_path_wrappers = [junit_path_wrappers.as_slice(), temp_paths.as_slice()].concat(); if junit_path_wrappers.is_empty() { @@ -693,6 +727,25 @@ fn coalesce_junit_path_wrappers( } } + if !swift_test_xunit_paths.is_empty() { + let temp_dir = match _junit_path_wrappers_temp_dir.take() { + Some(temp_dir) => temp_dir, + None => tempfile::tempdir()?, + }; + let temp_paths = handle_swift_test_xunit(&temp_dir, &swift_test_xunit_paths, repo_root)?; + _junit_path_wrappers_temp_dir = Some(temp_dir); + junit_path_wrappers = [junit_path_wrappers.as_slice(), temp_paths.as_slice()].concat(); + if junit_path_wrappers.is_empty() { + if allow_empty_test_results { + tracing::warn!("No tests found in the provided swift test xunit paths."); + } else { + return Err(anyhow::anyhow!( + "No tests found in the provided swift test xunit paths." + )); + } + } + } + if !test_reports.is_empty() { for test_report in test_reports { if let Ok(bazel_result) = parse_as_bep(test_report.clone()) { @@ -711,11 +764,7 @@ fn coalesce_junit_path_wrappers( #[cfg(target_os = "macos")] &test_report, #[cfg(target_os = "macos")] - repo, - #[cfg(target_os = "macos")] - &org_url_slug, - #[cfg(target_os = "macos")] - use_experimental_failure_summary, + xcresult_options, ) { #[cfg(target_os = "macos")] { @@ -741,20 +790,12 @@ fn coalesce_junit_path_wrappers( fn parse_as_xcresult( #[cfg(target_os = "macos")] test_report: &String, - #[cfg(target_os = "macos")] repo: &RepoUrlParts, - #[cfg(target_os = "macos")] org_url_slug: &String, - #[cfg(target_os = "macos")] use_experimental_failure_summary: bool, + #[cfg(target_os = "macos")] xcresult_options: &XCResultOptions, ) -> Option { #[cfg(target_os = "macos")] { let temp_dir = tempfile::tempdir().ok()?; - let temp_paths = handle_xcresult( - &temp_dir, - Some(test_report.clone()), - repo, - &org_url_slug, - use_experimental_failure_summary, - ); + let temp_paths = handle_xcresult(&temp_dir, Some(test_report.clone()), xcresult_options); if temp_paths.is_ok() { return Some(temp_dir); } else { @@ -870,22 +911,122 @@ pub async fn gather_upload_id_context( Ok(upload) } +/// `swift test --xunit-output` writes no file for any test, so each one's file is taken from +/// 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], + repo_root: &str, +) -> anyhow::Result> { + let mut reports = Vec::new(); + for path in paths { + let file = std::fs::File::open(path) + .map_err(|e| anyhow::anyhow!("failed to open {}: {}", path, e))?; + let mut parser = JunitParser::new(); + parser + .parse(BufReader::new(file)) + .map_err(|e| anyhow::anyhow!("failed to parse {} as JUnit XML: {}", path, e))?; + reports.extend(parser.into_reports()); + } + + // One index for every file, so a run uploading several does one scan rather than one each. + let keys = reports + .iter() + .flat_map(|report| report.test_suites.iter()) + .flat_map(|test_suite| test_suite.test_cases.iter()) + .filter_map(|test_case| { + let classname = test_case.classname.as_ref()?.as_str(); + Some(( + TestKey::from_junit_classname(classname, test_case.name.as_str()), + TestKey::target_from_junit_classname(classname), + )) + }) + .collect::>(); + let index = TestLocationIndex::resolve(Path::new(repo_root), &keys, Limits::default()); + + let mut resolved = 0_usize; + let mut unresolved = 0_usize; + for report in &mut reports { + for test_suite in &mut report.test_suites { + for test_case in &mut test_suite.test_cases { + if test_case.extra.contains_key("file") { + continue; + } + let site = test_case + .classname + .as_ref() + .map(|classname| { + TestKey::from_junit_classname(classname.as_str(), test_case.name.as_str()) + }) + .and_then(|key| index.lookup(&key)); + match site { + Some(site) => { + resolved += 1; + test_case + .extra + .insert("file".into(), site.file.as_str().into()); + } + None => unresolved += 1, + } + } + } + } + if unresolved > 0 { + tracing::warn!( + "{} of {} swift test case(s) have no declaration under {}", + unresolved, + resolved + unresolved, + repo_root + ); + } + tracing::info!("swift test files: {resolved} from a declaration, {unresolved} unresolved"); + + let mut temp_paths = Vec::new(); + for (i, report) in reports.iter().enumerate() { + let mut writer: Vec = Vec::new(); + report.serialize(&mut writer)?; + let temp_path = junit_temp_dir + .path() + .join(format!("swift_test_junit_{i}.xml")); + std::fs::File::create(&temp_path)? + .write_all(&writer) + .map_err(|e| anyhow::anyhow!("failed to write junit file: {}", e))?; + let temp_path_str = temp_path + .to_str() + .ok_or_else(|| anyhow::anyhow!("failed to convert junit temp path to string"))?; + temp_paths.push(JunitReportFileWithTestRunnerReport::from( + temp_path_str.to_string(), + )); + } + Ok(temp_paths) +} + #[cfg(target_os = "macos")] fn handle_xcresult( junit_temp_dir: &tempfile::TempDir, xcresult_path: Option, - repo: &RepoUrlParts, - org_url_slug: &String, - use_experimental_failure_summary: bool, + options: &XCResultOptions, ) -> Result, anyhow::Error> { let mut temp_paths = Vec::new(); if let Some(xcresult_path) = xcresult_path { - let xcresult = XCResult::new( - xcresult_path, - org_url_slug.clone(), - repo.repo_full_name(), - use_experimental_failure_summary, - )?; + let org_url_slug = options.org_url_slug.to_string(); + let repo_full_name = options.repo.repo_full_name(); + let xcresult = if options.use_experimental_test_locations { + XCResult::new_with_declaration_locations( + xcresult_path, + org_url_slug, + repo_full_name, + options.repo_root, + options.limits, + )? + } else { + XCResult::new( + xcresult_path, + org_url_slug, + repo_full_name, + options.use_experimental_failure_summary, + )? + }; let junits = xcresult.generate_junits(); if junits.is_empty() { return Err(anyhow::anyhow!( @@ -997,6 +1138,8 @@ mod tests { #[cfg(target_os = "macos")] use context::repo::RepoUrlParts; + #[cfg(target_os = "macos")] + use crate::context::XCResultOptions; use crate::context::{coalesce_junit_path_wrappers, gather_initial_test_context}; use crate::upload_command::UploadArgs; @@ -1060,17 +1203,24 @@ mod tests { owner: "trunk-io".to_string(), name: "analytics-cli".to_string(), }; + #[cfg(target_os = "macos")] + let xcresult_options = XCResultOptions { + repo: &repo, + org_url_slug: "test", + repo_root: "test", + use_experimental_failure_summary: false, + use_experimental_test_locations: false, + limits: xcresult::test_locations::Limits::default(), + }; let result_err = coalesce_junit_path_wrappers( vec!["test".into()], Some("test".into()), #[cfg(target_os = "macos")] Some("test".into()), #[cfg(target_os = "macos")] - &repo, - #[cfg(target_os = "macos")] - "test".into(), - #[cfg(target_os = "macos")] - false, + &xcresult_options, + Vec::new(), + "test", Vec::new(), false, ); @@ -1081,11 +1231,9 @@ mod tests { #[cfg(target_os = "macos")] Some("test".into()), #[cfg(target_os = "macos")] - &repo, - #[cfg(target_os = "macos")] - "test".into(), - #[cfg(target_os = "macos")] - false, + &xcresult_options, + Vec::new(), + "test", Vec::new(), true, ); @@ -1106,17 +1254,24 @@ mod tests { owner: "trunk-io".to_string(), name: "analytics-cli".to_string(), }; + #[cfg(target_os = "macos")] + let xcresult_options = XCResultOptions { + repo: &repo, + org_url_slug: "test", + repo_root: "test", + use_experimental_failure_summary: false, + use_experimental_test_locations: false, + limits: xcresult::test_locations::Limits::default(), + }; let result_ok = coalesce_junit_path_wrappers( Vec::new(), None, #[cfg(target_os = "macos")] None, #[cfg(target_os = "macos")] - &repo, - #[cfg(target_os = "macos")] - "test".into(), - #[cfg(target_os = "macos")] - false, + &xcresult_options, + Vec::new(), + "test", vec!["test".into()], true, ); diff --git a/cli/src/upload_command.rs b/cli/src/upload_command.rs index ca6bbb28..d3966b31 100644 --- a/cli/src/upload_command.rs +++ b/cli/src/upload_command.rs @@ -72,6 +72,19 @@ pub struct UploadArgs { help = "Comma-separated list of glob patterns to test report files. Supports JUnit XML, Bazel BEP, and XCResult formats." )] pub test_reports: Vec, + #[arg( + 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.", + required = false + )] + pub swift_test_xunit_paths: Vec, /// Always required — do not make this optional. It is what attributes a run to an organization /// in our telemetry: it is tagged onto every error we forward to Sentry (see `setup_logger` in /// `main.rs`), recorded on the bundle as `base_props.org`, and scopes the upload metrics we @@ -280,6 +293,71 @@ pub struct UploadArgs { hide = true )] pub use_experimental_failure_summary: bool, + #[cfg(target_os = "macos")] + #[arg( + long, + env = constants::TRUNK_USE_EXPERIMENTAL_XCRESULT_TEST_LOCATIONS_ENV, + help = "Flag to take an xcresult test's file from where a language server says it is declared, rather than from the failure that surfaced it. Reads the bundle with no legacy `xcresulttool get object` calls.", + action = ArgAction::Set, + required = false, + require_equals = true, + num_args = 0..=1, + default_value = "false", + default_missing_value = "true", + hide = true, + conflicts_with = "use_experimental_failure_summary" + )] + pub use_experimental_xcresult_test_locations: bool, + #[cfg(target_os = "macos")] + #[arg( + long, + env = constants::TRUNK_XCRESULT_TEST_LOCATIONS_MAX_FILES_ENV, + help = "Most source files to parse when resolving xcresult test declarations.", + required = false, + default_value_t = xcresult::test_locations::Limits::default().max_files, + hide = true + )] + pub xcresult_test_locations_max_files: usize, + #[cfg(target_os = "macos")] + #[arg( + long, + env = constants::TRUNK_XCRESULT_TEST_LOCATIONS_BUDGET_SECS_ENV, + help = "Seconds to spend per language server when resolving xcresult test declarations. The clang server answers far slower per file than the Swift one, so an Objective-C heavy repo wants this raised.", + required = false, + default_value_t = xcresult::test_locations::Limits::default().budget.as_secs(), + hide = true + )] + pub xcresult_test_locations_budget_secs: u64, + #[cfg(target_os = "macos")] + #[arg( + long, + env = constants::TRUNK_XCRESULT_TEST_LOCATIONS_REQUEST_TIMEOUT_SECS_ENV, + help = "Seconds to wait for a single language server reply before giving up on it.", + required = false, + default_value_t = xcresult::test_locations::Limits::default().request_timeout.as_secs(), + hide = true + )] + pub xcresult_test_locations_request_timeout_secs: u64, + #[cfg(target_os = "macos")] + #[arg( + long, + env = constants::TRUNK_XCRESULT_TEST_LOCATIONS_RETRIES_ENV, + help = "How many times to replace a language server that stops answering with a fresh one.", + required = false, + default_value_t = xcresult::test_locations::Limits::default().retries, + hide = true + )] + pub xcresult_test_locations_retries: usize, + #[cfg(target_os = "macos")] + #[arg( + long, + env = constants::TRUNK_XCRESULT_TEST_LOCATIONS_MAX_FILE_BYTES_ENV, + help = "Largest source file to parse for declarations. One request carries the whole file, so an outsized generated one costs memory and usually a timeout for symbols it does not declare. Skipped without being read.", + required = false, + default_value_t = xcresult::test_locations::Limits::default().max_file_bytes, + hide = true + )] + pub xcresult_test_locations_max_file_bytes: u64, #[arg( long, env = constants::TRUNK_VALIDATION_REPORT_ENV, diff --git a/cli/tests/common/command_builder.rs b/cli/tests/common/command_builder.rs index f8cbb955..2000b896 100644 --- a/cli/tests/common/command_builder.rs +++ b/cli/tests/common/command_builder.rs @@ -452,6 +452,7 @@ pub struct CommandBuilder<'a> { command_type: CommandType, current_dir: &'a Path, paths_state: Option, + extra_args: Vec, } impl<'b> CommandBuilder<'b> { @@ -463,6 +464,7 @@ impl<'b> CommandBuilder<'b> { }, current_dir, paths_state: None, + extra_args: Vec::new(), } } @@ -475,6 +477,7 @@ impl<'b> CommandBuilder<'b> { }, current_dir, paths_state: None, + extra_args: Vec::new(), } } @@ -486,6 +489,7 @@ impl<'b> CommandBuilder<'b> { }, current_dir, paths_state: None, + extra_args: Vec::new(), } } @@ -494,6 +498,11 @@ impl<'b> CommandBuilder<'b> { self } + pub fn extra_args(&mut self, args: &[&str]) -> &mut Self { + self.extra_args = args.iter().map(|arg| String::from(*arg)).collect(); + self + } + pub fn xcresult_path(&mut self, new_paths: &str) -> &mut Self { self.paths_state = Some(PathsState::XCResultPath(String::from(new_paths))); self @@ -634,6 +643,7 @@ impl<'b> CommandBuilder<'b> { .into_iter() .chain(paths_args) .chain(self.command_type.build_args()) + .chain(self.extra_args.clone()) .collect() } diff --git a/cli/tests/upload.rs b/cli/tests/upload.rs index d4088329..8db92602 100644 --- a/cli/tests/upload.rs +++ b/cli/tests/upload.rs @@ -802,9 +802,14 @@ async fn upload_bundle_without_canonical_test_collection_metadata_keeps_bundle_g assert_eq!(bundle_meta.bundle_upload_id_v2, "test-bundle-upload-id-v2"); } -#[tokio::test(flavor = "multi_thread")] +// The declaration path is exercised end-to-end here too, so a bundle that uploads cleanly +// today cannot start failing behind the flag without this noticing. #[cfg(target_os = "macos")] -async fn upload_bundle_using_xcresult() { +#[rstest::rstest] +#[case::default_path(&[])] +#[case::declaration_locations(&["--use-experimental-xcresult-test-locations=true"])] +#[tokio::test(flavor = "multi_thread")] +async fn upload_bundle_using_xcresult(#[case] extra_args: &[&str]) { let temp_dir = tempdir().unwrap(); generate_mock_git_repo(&temp_dir); unpack_archive_to_dir( @@ -816,6 +821,7 @@ async fn upload_bundle_using_xcresult() { let assert = CommandBuilder::upload(temp_dir.path(), state.host.clone()) .xcresult_path("test1.xcresult") + .extra_args(extra_args) .command() .assert() .success() @@ -3133,3 +3139,109 @@ async fn upload_bundle_keeps_the_repo_relative_path_when_a_symlink_leaves_the_re // HINT: View CLI output with `cargo test -- --nocapture` 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. +#[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); + + let tests_dir = temp_dir.path().join("Tests/MyCLITests"); + fs::create_dir_all(&tests_dir).unwrap(); + fs::write( + temp_dir.path().join("Package.swift"), + "// swift-tools-version: 6.0\n", + ) + .unwrap(); + fs::write( + tests_dir.join("TopLevel.swift"), + "import Testing\n\n@Test func helloworld() {}\n", + ) + .unwrap(); + fs::write( + tests_dir.join("Suites.swift"), + "import Testing\n\n@Suite struct AlphaSuite {\n @Test func shared() {}\n}\n", + ) + .unwrap(); + fs::write( + tests_dir.join("Legacy.swift"), + "import XCTest\n\nfinal class LegacyXCTests: XCTestCase {\n func testOldStyle() {}\n}\n", + ) + .unwrap(); + fs::write( + temp_dir.path().join("xunit-swift-testing.xml"), + concat!( + r#""#, + r#""#, + r#""#, + r#""#, + r#""#, + ), + ) + .unwrap(); + fs::write( + temp_dir.path().join("xunit.xml"), + concat!( + r#""#, + r#""#, + r#""#, + r#""#, + ), + ) + .unwrap(); + + let state = MockServerBuilder::new().spawn_mock_server().await; + CommandBuilder::upload(temp_dir.path(), state.host.clone()) + .extra_args(&[ + "--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 bundle_meta: BundleMeta = + serde_json::from_reader(fs::File::open(tar_extract_directory.join("meta.json")).unwrap()) + .unwrap(); + + let mut files = std::collections::HashMap::new(); + for file_set in &bundle_meta.base_props.file_sets { + for file in &file_set.files { + let mut parser = JunitParser::new(); + let junit = fs::File::open(tar_extract_directory.join(&file.path)).unwrap(); + parser.parse(BufReader::new(junit)).unwrap(); + for report in parser.into_reports() { + for suite in &report.test_suites { + for case in &suite.test_cases { + let file = case + .extra + .iter() + .find(|(key, _)| key.as_str() == "file") + .map(|(_, value)| value.as_str().to_owned()); + files.insert(case.name.as_str().to_owned(), file); + } + } + } + } + } + + for (name, expected) in [ + ("helloworld()", "Tests/MyCLITests/TopLevel.swift"), + ("shared()", "Tests/MyCLITests/Suites.swift"), + ("testOldStyle", "Tests/MyCLITests/Legacy.swift"), + ] { + let file = files + .get(name) + .unwrap_or_else(|| panic!("{name} is missing from the bundle")) + .as_deref() + .unwrap_or_else(|| panic!("{name} got no file from its declaration")); + assert!( + file.ends_with(expected), + "expected {name} in {expected}, got {file}" + ); + } +} diff --git a/constants/src/lib.rs b/constants/src/lib.rs index a2afd054..b442946e 100644 --- a/constants/src/lib.rs +++ b/constants/src/lib.rs @@ -57,8 +57,24 @@ pub const TRUNK_VALIDATION_REPORT_ENV: &str = "TRUNK_VALIDATION_REPORT"; pub const TRUNK_SHOW_FAILURE_MESSAGES_ENV: &str = "TRUNK_SHOW_FAILURE_MESSAGES"; pub const TRUNK_HIDE_TEST_COLLECTION_LINKS_ENV: &str = "TRUNK_HIDE_TEST_COLLECTION_LINKS"; pub const TRUNK_DEBUG_ENV: &str = "TRUNK_DEBUG"; +pub const TRUNK_USE_EXPERIMENTAL_XCRESULT_TEST_LOCATIONS_ENV: &str = + "TRUNK_USE_EXPERIMENTAL_XCRESULT_TEST_LOCATIONS"; +// Tuning for the xcresult declaration path. The clang server answers roughly an order of +// magnitude slower per file than the Swift one, so an Objective-C heavy repo may need the +// budget and the file cap raised well above their defaults. +pub const TRUNK_XCRESULT_TEST_LOCATIONS_MAX_FILES_ENV: &str = + "TRUNK_XCRESULT_TEST_LOCATIONS_MAX_FILES"; +pub const TRUNK_XCRESULT_TEST_LOCATIONS_BUDGET_SECS_ENV: &str = + "TRUNK_XCRESULT_TEST_LOCATIONS_BUDGET_SECS"; +pub const TRUNK_XCRESULT_TEST_LOCATIONS_REQUEST_TIMEOUT_SECS_ENV: &str = + "TRUNK_XCRESULT_TEST_LOCATIONS_REQUEST_TIMEOUT_SECS"; +pub const TRUNK_XCRESULT_TEST_LOCATIONS_RETRIES_ENV: &str = "TRUNK_XCRESULT_TEST_LOCATIONS_RETRIES"; +pub const TRUNK_XCRESULT_TEST_LOCATIONS_MAX_FILE_BYTES_ENV: &str = + "TRUNK_XCRESULT_TEST_LOCATIONS_MAX_FILE_BYTES"; + // RSpec-only: when set to "true", aborts the RSpec run if quarantine lookup fails. // Handled in rspec-trunk-flaky-tests/lib/trunk_spec_helper.rb, not the CLI. +pub const TRUNK_SWIFT_TEST_XUNIT_PATHS_ENV: &str = "TRUNK_SWIFT_TEST_XUNIT_PATHS"; pub const TRUNK_QUARANTINE_QUERY_FAILURE_EXIT_ENV: &str = "TRUNK_QUARANTINE_QUERY_FAILURE_EXIT"; // TRUNK_* environment variables to capture in bundle metadata for debugging. @@ -92,6 +108,7 @@ pub const TRUNK_ENVS_TO_CAPTURE: &[&str] = &[ TRUNK_SHOW_FAILURE_MESSAGES_ENV, TRUNK_HIDE_TEST_COLLECTION_LINKS_ENV, TRUNK_DEBUG_ENV, + TRUNK_USE_EXPERIMENTAL_XCRESULT_TEST_LOCATIONS_ENV, ]; pub const ENVS_TO_GET: &[&str] = &[ diff --git a/context/src/junit/parser.rs b/context/src/junit/parser.rs index 6eb93e79..be7c777d 100644 --- a/context/src/junit/parser.rs +++ b/context/src/junit/parser.rs @@ -2198,14 +2198,8 @@ failures: .filter_map(|run| run.test_output.as_ref()) .map(|out| out.message.as_str()) .collect(); - assert!( - messages - .contains(&"/^Pacific\\b/ hour 11 from \"11:00:03 A.M.\" should be even") - ); - assert!( - messages - .contains(&"/^Pacific\\b/ hour 11 from \"11:00:23 A.M.\" should be even") - ); + assert!(messages.contains(&"/^Pacific\\b/ hour 11 from \"11:00:03 A.M.\" should be even")); + assert!(messages.contains(&"/^Pacific\\b/ hour 11 from \"11:00:23 A.M.\" should be even")); assert!(messages.contains(&"Test timeout of 10000ms exceeded.")); } } diff --git a/xcresult/CONTRIBUTING.md b/xcresult/CONTRIBUTING.md index 42484e7a..ca597ce8 100644 --- a/xcresult/CONTRIBUTING.md +++ b/xcresult/CONTRIBUTING.md @@ -4,11 +4,16 @@ The `xcresult` crate exists to handle converting between xcresult and JUnit form ## Purpose -This crate serves two main purposes: +This crate serves three main purposes: 1. **Format Conversion**: Converts xcresult bundles (produced by Xcode test runs) into JUnit XML format for compatibility with various CI/CD systems and test reporting tools. -2. **Conditional File Path Specification**: While there are other xcresult parses, this crate handles specifying file paths in the JUnit output, which are conditionally present based on whether a failure (not error) has occurred. File paths are only included in the JUnit output when a test case has failed, as they are extracted from failure summaries in the xcresult bundle. This also handles generating stable identfiers because, by default, one of the values we generate IDs from is the file path. Without this crate, we wouldn't be able to safely map files to tests nor have codeowners support for xcresult. +2. **Suite flattening**: JUnit has no nested ``, so a suite nested inside another + becomes its own with a dot-qualified name (`Bundle.Outer.Inner`). This is not cosmetic — + emitting only the outer suite silently drops every test the inner ones declare, which is + what used to happen to nested swift-testing suites. + +3. **Conditional File Path Specification**: While there are other xcresult parses, this crate handles specifying file paths in the JUnit output, which are conditionally present based on whether a failure (not error) has occurred. File paths are only included in the JUnit output when a test case has failed, as they are extracted from failure summaries in the xcresult bundle. This also handles generating stable identfiers because, by default, one of the values we generate IDs from is the file path. Without this crate, we wouldn't be able to safely map files to tests nor have codeowners support for xcresult. ## Running the Binary @@ -41,6 +46,105 @@ cargo run --bin xcresult-to-junit -- \ - `--repo-url`: Repository URL, e.g. `https://github.com/trunk-io/analytics-cli` (optional) - `--output-file-path`: JUnit XML output file path (optional, defaults to stdout) - `--use-experimental-failure-summary`: Use experimental failure summary parsing (optional boolean flag) +- `--use-experimental-xcresult-test-locations`: Take each test's file from where it is declared rather than from a failure (optional boolean flag, also settable via `TRUNK_USE_EXPERIMENTAL_XCRESULT_TEST_LOCATIONS`) +- `--repo-root`: Checkout to resolve declarations in, defaults to the working directory (clap `requires` the flag above) + +## Experimental: test locations from declarations + +`--use-experimental-xcresult-test-locations` replaces the file-attribution half of this +crate, and is the same flag on `trunk-analytics-cli upload` (hidden, and equally +xcresult-only — it does nothing for JUnit or Bazel BEP uploads). + +**What it changes.** Everything else here answers "where was this failure raised", because +that is all an `.xcresult` records: there is no per-test declaration site anywhere in the +bundle, and a passing test's summary carries no path at all. This flag asks a language +server instead — `sourcekit-lsp` for Swift, `clangd` for Objective-C, both of which ship +in the Command Line Tools as well as Xcode — for `textDocument/documentSymbol` over the +checkout's own sources, and joins the `(suite, case)` it gets back to the xcresult +identifier. So a failure raised inside a helper is attributed to the test's file rather +than the helper's, a crash with no call stack is attributed at all, and a **passing** test +gets a file for the first time. + +**What it also changes, and is easy to miss.** The declaration path makes exactly one +`xcresulttool` call for results (`get test-results tests`) plus one for the run start time +(`get test-results summary`). It never issues `get object --legacy`, so the unbounded +per-test summary fetch — measured at 6 GB of JSON and a 48 GB peak footprint for a single +timed-out test — is not reachable from it. + +**Where it is worse, and it is not a superset.** Tests registered at runtime (Quick's +`class_addMethod`, `+testInvocations`) have no declaration to find; the two approaches fail +in disjoint situations. Such a test falls back to the modern API's own `sourceLocation`, +vetted against the same vendored-path rules as everything in `src/file_attribution.rs`. + +That fallback is thinner than it looks: `sourceLocation` is in the modern schema +(`TestNode.sourceLocation`) but is emitted in **none** of the bundles in `tests/data/`, so +in practice it never fires and such a test gets no file at all — where the failure-summary +path, which reads the call stack this path never fetches, would name one. `generate_junits` +logs the split (`N from a declaration, N from the fallback, N unresolved`) so it is visible +whether the fallback ever fires against real bundles. + +**Collisions are broken by target.** The index is built from a checkout scan, not the build +log, so two same-named suites in different modules can both declare the same +`(suite, case)`. Scan order decides which is seen first and that order is arbitrary, so +`record` prefers a candidate lying under a directory named for the target that actually ran +the test — `nodeIdentifierURL` is +`test://com.apple.xcode////`, so the target is already in hand +from the field the ids come from, and no extra `xcresulttool` call is needed to get it. +Where no candidate is under the target, or the test has no target, the first file scanned +still wins. This matters because being confidently wrong is worse for codeowners than +reporting nothing, and it is the one way this path can be wrong where the failure-summary +path cannot, since that one reads the frame that actually ran. + +**Incompatible with `--use-experimental-failure-summary`,** which tunes a code path this +one does not run, so clap rejects the pair rather than letting one silently win. One wart +comes with that: clap treats an env-supplied value as present regardless of what it says, +so `TRUNK_USE_EXPERIMENTAL_XCRESULT_TEST_LOCATIONS=false` **and** +`--use-experimental-failure-summary` is a hard conflict error. Unset the variable to roll +back rather than setting it to `false`. + +**Every read goes through a copy.** `xcresulttool` migrates a pre-`database.sqlite3` bundle +in place the first time it is read, which writes into a directory the uploader was only +asked to read, fails outright with `exit 64` when that directory is not writable, and makes +two concurrent readers of one bundle race. Both constructors copy the bundle into a +`TempDir` first and read that, so the caller's bundle is never touched. This is on the +shared path, so it applies to the default one too. + +**Ids.** `generate_id` prefers `nodeIdentifierURL` here, which is the legacy record's +`identifierURL` under another name, so ids do not move between the two paths. + +**An xcresult and a `swift test` xunit give the same test different ids, deliberately for +now.** The two take different branches in `context/src/junit/parser.rs`: an xcresult test +case carries an `id` extra, a v5 UUID over `org#repo#identifierURL`, which is used verbatim; +a `swift test` xunit carries none, so identity falls to `gen_info_id` over +`(org, repo, file, classname, parent_name, name)` — the same scheme every other JUnit +uploader uses. Measured on one package captured both ways: + +``` +shared() xcresult e40658ab-… xunit 73301b89-… +``` + +So a repository uploading both formats sees every test twice, and moving a repository from +one format to the other resets its history. + +They are not unified yet because neither half is free. `identifierURL` leads with the +**xcodebuild scheme name**, which appears nowhere in xunit output, so the xunit path cannot +reproduce an xcresult id; and rederiving the xcresult id from normalised components would +reset history for every repository uploading `.xcresult` today. Giving the xunit path its own +third scheme in the meantime would cost those users two resets rather than one. The intent is +a single migration that moves both onto one format. + +Two consequences worth knowing while that is outstanding. `file` is a `gen_info_id` input, so +turning `--swift-test-xunit-paths` on for a repository that previously uploaded the same file +via `--junit-paths` changes every id once, and a test whose declaration later moves to +another file changes id again — the xcresult scheme is immune to both, since its id ignores +`file`. And `name` is an input too, which is why +`the_two_inputs_spell_an_xctest_method_differently` pins `testOldStyle` against +`testOldStyle()`: that difference feeds the checksum. + +**Cost.** Roughly 13 ms per file parsed. The scan is ranked so files named after a suite go +first and stops as soon as every test resolves; `Limits` caps files parsed, total wall +clock, and per-request time, and a server that stops answering is killed rather than waited +on. ## JSON Schema Generation @@ -95,6 +199,10 @@ The generated types are used in: - `src/xcresult.rs` - Main conversion logic - `src/xcresult_legacy.rs` - Legacy format handling +The declaration path adds `src/lsp.rs` (a minimal JSON-RPC client) and +`src/test_locations.rs` (the `(suite, case) -> file:line` index), neither of which reads a +generated schema. + ## Testing Tests are located in `tests/xcresult.rs` and use sample xcresult bundles from `tests/data/`. To run tests: @@ -108,3 +216,27 @@ cargo test -p xcresult ``` Note: Almost all tests are macOS-specific (marked with `#[cfg(target_os = "macos")]`) as they require `xcrun` to be available. + +The split is deliberate, because the parts that need macOS are narrower than they look: + +- `src/test_locations.rs` unit-tests the symbol mapping, inheritance walk and source scan + against canned `documentSymbol` responses. +- `src/xcresult.rs` unit-tests suite flattening and the attribution join against a canned + `Tests` value and a seeded `TestLocationIndex` (`TestLocationIndex::declaring`, test-only). + This is where "a **passing** test gets a file", "a failure raised in a helper or a + dependency is still attributed to the test's file", and "no nested suite is dropped" are + proven — none of which needs `xcrun`. +- `tests/xcresult.rs` holds the macOS tests that actually drive `sourcekit-lsp` and + `clangd` over the checked-in packages in `tests/fixture-src/`. They pass a scenario's + package directory as the repo root, which is why they assert the file a test is _written + in_ rather than the absolute path baked into the bundle at capture time. + +Both shapes that once had no fixture — a passing test and a nested suite — are covered by +`nested-and-passing`, captured from a package whose inner `@Suite` lives in a different +file from the suite containing it. It is the only scenario whose shape is structural +rather than a failure, so `regenerate.sh` verifies it with `verify-test-structure.py`. + +Against that bundle the pre-fix traversal emits `tests="2" failures="0"`: the inner suite +is never visited, so its two tests are dropped and the run reports no failures despite +having one. Its three passing tests get a file only on the declaration path, since a test +that did not fail has no failure summary to name one. diff --git a/xcresult/Cargo.toml b/xcresult/Cargo.toml index 2cf90e27..df4bc422 100644 --- a/xcresult/Cargo.toml +++ b/xcresult/Cargo.toml @@ -15,15 +15,21 @@ path = "src/lib.rs" anyhow = "1.0.89" chrono = "0.4.38" clap = { version = "4.4.18", features = ["derive", "env"] } +constants = { path = "../constants" } context = { path = "../context", features = ["bindings"] } +ignore = "0.4.33" lazy_static = "1.5.0" +lsp-server = "0.10.0" +lsp-types = "0.97.0" tracing-subscriber = "0.3.19" petgraph = { version = "0.7.1", default-features = false } quick-junit = "0.5.0" regex = "1.11.0" serde = { version = "1.0.215", default-features = false } serde_json = "1.0.133" +tempfile = "3.2.0" tracing = "0.1.41" +url = "2.5.7" uuid = { version = "1.10.0", features = ["v5"] } [dev-dependencies] diff --git a/xcresult/src/file_attribution.rs b/xcresult/src/file_attribution.rs index 6c8cc44e..e8a17e07 100644 --- a/xcresult/src/file_attribution.rs +++ b/xcresult/src/file_attribution.rs @@ -269,200 +269,3 @@ fn stack_frames(failure_summary: &legacy_schema::ActionTestFailureSummary) -> Ve .rev() .collect() } - -#[cfg(test)] -mod tests { - use rstest::rstest; - use serde_json::{Value, json}; - - use super::*; - - const SUITE: &str = "SnapshotReproTests"; - const CASE: &str = "failingSnapshot()"; - - fn xc_string(value: &str) -> Value { - json!({ "_value": value }) - } - - fn failure_summary( - file_name: Option<&str>, - location: Option<&str>, - stack: &[(&str, &str)], - ) -> legacy_schema::ActionTestFailureSummary { - serde_json::from_value(json!({ - "fileName": file_name.map(xc_string), - "sourceCodeContext": { - "location": { "filePath": location.map(xc_string) }, - "callStack": { "_values": stack.iter().map(|(symbol, path)| json!({ - "symbolInfo": { - "symbolName": xc_string(symbol), - "location": { "filePath": xc_string(path) } - } - })).collect::>() } - } - })) - .unwrap() - } - - fn identity() -> TestIdentity<'static> { - TestIdentity { - suite: Some(SUITE), - case: CASE, - } - } - - #[rstest] - #[case::spaces_are_encoded("/repo/Tests/My Test.swift", "/repo/Tests/My%20Test.swift")] - #[case::already_safe("/repo/Tests/Test.swift", "/repo/Tests/Test.swift")] - fn reported_path_normalizes_once(#[case] path: &str, #[case] expected: &str) { - assert_eq!(ReportedPath::new(path).as_str(), expected); - } - - #[rstest] - #[case::tuist_checkout("/repo/Tuist/.build/checkouts/Dep/Dep.swift", true)] - #[case::derived_data("/repo/DerivedData/SourcePackages/checkouts/Dep/Dep.swift", true)] - #[case::the_repos_own_code("/repo/Tests/SnapshotReproTests.swift", false)] - fn reported_path_recognizes_vendored_sources(#[case] path: &str, #[case] expected: bool) { - assert_eq!(ReportedPath::new(path).is_vendored_dependency(), expected); - } - - #[rstest] - #[case::swift_symbol("SnapshotReproTests.failingSnapshot()", true)] - #[case::objc_symbol("-[SnapshotReproTests failingSnapshot]", true)] - #[case::closure_inside_test("closure #1 in SnapshotReproTests.failingSnapshot()", true)] - #[case::helper_the_test_called("assertSnapshot(of:as:)", false)] - #[case::same_case_name_in_another_suite("OtherTests.failingSnapshot()", false)] - #[case::trait_that_invoked_the_test( - "closure #1 in _SnapshotsTestTrait.provideScope(for:testCase:performing:)", - false - )] - fn identity_recognizes_only_the_tests_own_frame(#[case] symbol: &str, #[case] expected: bool) { - assert_eq!(identity().is_named_by(symbol), expected); - } - - #[rstest] - #[case::top_level_swift_testing_function("failingSnapshot()", true)] - #[case::closure_inside_it("closure #1 in failingSnapshot()", true)] - #[case::suite_scoped_symbol("SnapshotReproTests.failingSnapshot()", false)] - fn a_suiteless_test_is_matched_by_its_bare_function( - #[case] symbol: &str, - #[case] expected: bool, - ) { - let identity = TestIdentity { - suite: None, - case: CASE, - }; - assert_eq!(identity.is_named_by(symbol), expected); - } - - #[test] - fn candidates_are_offered_in_preference_order_and_keep_their_provenance() { - let summary = failure_summary( - Some("/repo/Tests/Raised.swift"), - Some("/repo/Tests/Location.swift"), - &[ - ("helper()", "/repo/Tests/Inner.swift"), - ( - "SnapshotReproTests.failingSnapshot()", - "/repo/Tests/Own.swift", - ), - ("framework()", "/repo/Tests/Outer.swift"), - ], - ); - assert_eq!( - FileCandidate::from_failure_summary(&summary, &identity()) - .iter() - .map(|candidate| (candidate.path.as_str(), candidate.source)) - .collect::>(), - vec![ - ("/repo/Tests/Own.swift", FileSource::TestFrame), - ("/repo/Tests/Raised.swift", FileSource::RaisedFrom), - ("/repo/Tests/Location.swift", FileSource::SourceCodeLocation), - // Frames run innermost first, so they are offered outermost first. - ("/repo/Tests/Outer.swift", FileSource::LastStackFrame), - ("/repo/Tests/Own.swift", FileSource::LastStackFrame), - ("/repo/Tests/Inner.swift", FileSource::LastStackFrame), - ] - ); - } - - #[test] - fn a_summary_offering_nothing_yields_no_candidates() { - let summary = failure_summary(None, None, &[]); - assert!(FileCandidate::from_failure_summary(&summary, &identity()).is_empty()); - } - - #[rstest] - #[case::other_languages_skipped( - &[("a", "/repo/Tests/Real.swift"), ("b", "/repo/Tests/Generated.cc"), ("c", "/repo/Readme.md")], - vec!["/repo/Tests/Real.swift"] - )] - #[case::nothing_usable(&[("a", "/repo/Tests/Generated.cc")], vec![])] - fn only_swift_and_objc_frames_are_offered( - #[case] stack: &[(&str, &str)], - #[case] expected: Vec<&str>, - ) { - let summary = failure_summary(None, None, stack); - assert_eq!( - stack_frames(&summary) - .iter() - .map(|candidate| candidate.path.as_str()) - .collect::>(), - expected - ); - } - - // Apple declares `SortedKeyValueArrayPair.value` as `SchemaSerializable`, a type - // the format description never defines, so the generator cannot model it and drops - // the property. The data still carries it, and an object that is both missing the - // property and declared exhaustive fails to deserialize — which silently disabled - // the whole experimental path for any bundle with test attachments. - #[test] - fn a_summary_parses_despite_properties_the_schema_cannot_model() { - let summary: legacy_schema::ActionTestPlanRunSummaries = serde_json::from_value(json!({ - "failureSummaries": { "_values": [{ - "fileName": xc_string("/repo/Tests/SnapshotReproTests.swift"), - "attachments": { "_values": [{ - "userInfo": { "storage": { "_values": [{ - "_type": { "_name": "SortedKeyValueArrayPair" }, - "key": xc_string("Encoding"), - "value": xc_string("{ XCTImageEncodingCompressionQualityKey = 0.7; }") - }] } } - }] } - }] } - })) - .expect("a summary carrying attachment metadata must still deserialize"); - let failure_summary = &summary.failure_summaries.unwrap().values[0]; - assert_eq!( - FileCandidate::from_failure_summary(failure_summary, &identity()) - .first() - .map(|candidate| candidate.path.as_str().to_string()), - Some(String::from("/repo/Tests/SnapshotReproTests.swift")) - ); - } - - #[rstest] - #[case::scheme_and_fragment_stripped( - Some("file:///repo/Tests/Test.swift#EndingLineNumber=8"), - Some("/repo/Tests/Test.swift") - )] - #[case::spaces_encoded( - Some("file:///repo/Tests/My Test.swift"), - Some("/repo/Tests/My%20Test.swift") - )] - #[case::no_document_location(None, None)] - fn an_issue_summary_yields_a_cleaned_document_location( - #[case] url: Option<&str>, - #[case] expected: Option<&str>, - ) { - let summary = serde_json::from_value(json!({ - "documentLocationInCreatingWorkspace": { "url": url.map(xc_string) } - })) - .unwrap(); - let candidate = FileCandidate::from_issue_summary(&summary); - assert_eq!(candidate.as_ref().map(|c| c.path.as_str()), expected); - if let Some(candidate) = candidate { - assert_eq!(candidate.source, FileSource::DocumentLocation); - } - } -} diff --git a/xcresult/src/lib.rs b/xcresult/src/lib.rs index 5b5a30aa..1bc015eb 100644 --- a/xcresult/src/lib.rs +++ b/xcresult/src/lib.rs @@ -1,4 +1,6 @@ pub mod file_attribution; +pub mod lsp; +pub mod test_locations; pub mod types; pub mod xcresult; pub mod xcresult_legacy; diff --git a/xcresult/src/lsp.rs b/xcresult/src/lsp.rs new file mode 100644 index 00000000..2d80caff --- /dev/null +++ b/xcresult/src/lsp.rs @@ -0,0 +1,718 @@ +//! Just enough of the Language Server Protocol to ask a server what a file declares. +//! +//! Framing and JSON-RPC come from [`lsp_server`], and every method name and payload shape +//! from [`lsp_types`], so a request is named by its type rather than by a string literal. +//! +//! A request that times out leaves a reply in flight that would arrive after the next +//! request was sent. Replies are matched by id, so a late one is discarded rather than +//! misread — but the server has also shown it cannot keep up, so it is killed and the +//! caller restarts it instead of waiting on it again. +//! +//! # How source files are read +//! +//! **No file is ever held in memory, whole or in part beyond `CHUNK`.** `didOpen` has to +//! carry a file's entire text — the protocol takes it inline, and neither a path nor a +//! stream — so the naive shape of this is a `String` per file, which for one generated +//! source file is tens of megabytes resident and several copies of it before serialization +//! is done. +//! +//! Instead each file is read **twice, in `CHUNK`-sized pieces, and never retained**: +//! +//! 1. **The measuring pass.** `FileText` reads a chunk, hands it to serde_json's escaper +//! via [`Serializer::collect_str`], and the escaped bytes land in a `Counting` writer +//! over [`io::sink`] — counted, then dropped. This yields the exact `Content-Length` +//! while holding nothing, and is also where an unreadable or non-UTF-8 file is caught, +//! before anything has been announced to the server. +//! 2. **The emitting pass.** The header goes out, then the same chunked read runs again, +//! this time escaping into a [`BufWriter`] over the server's stdin. +//! +//! Two reads rather than one is the price of not buffering; the second usually comes from +//! the page cache. Both passes escape through serde_json, so the length announced cannot +//! disagree with the body written — and `send_did_open` tallies the second +//! pass anyway, because a file rewritten between the two would otherwise misframe the +//! stream. Peak memory per `didOpen` is therefore flat in file size, which +//! `a_file_larger_than_a_chunk_is_never_handed_over_whole` exists to keep true. +//! +//! Reading in fixed-size pieces splits multi-byte characters, and [`fmt::Write::write_str`] +//! takes only valid UTF-8, so `FileText::stream` carries the incomplete tail of a chunk +//! over into the next one. + +use std::{ + cell::Cell, + fmt, fs, + io::{self, BufReader, BufWriter, Read, Write}, + path::Path, + process::{Child, ChildStdin, Command, Stdio}, + str, + sync::mpsc::{Receiver, RecvTimeoutError, Sender, channel}, + thread, + time::{Duration, Instant}, +}; + +use lsp_server::{Message, Notification, Request, RequestId, Response}; +use lsp_types::{ + ClientCapabilities, DidCloseTextDocumentParams, DocumentSymbol, + DocumentSymbolClientCapabilities, DocumentSymbolParams, DocumentSymbolResponse, + InitializeParams, PartialResultParams, TextDocumentClientCapabilities, TextDocumentIdentifier, + Uri, WorkDoneProgressParams, + notification::{DidCloseTextDocument, DidOpenTextDocument, Initialized, Notification as _}, + request::{DocumentSymbolRequest, Initialize}, +}; +use serde::ser::{Serialize, SerializeStruct, Serializer}; + +/// How much of a file is read, and buffered towards the pipe, at a time. +/// +/// Nothing here ever holds a whole file: a `didOpen` for a 40 MB source file has the same +/// peak footprint as one for a 400 byte source file -- this buffer, plus the `BufWriter` of +/// the same size, plus at most three bytes of a character carried across a read boundary. +const CHUNK: usize = 64 * 1024; + +pub struct LanguageServer { + process: Child, + stdin: ChildStdin, + incoming: Receiver, + next_id: i32, + broken: bool, +} + +impl LanguageServer { + /// Start `program` and complete the LSP handshake against workspace `root`. + pub fn start( + program: &Path, + args: &[&str], + root: &Path, + timeout: Duration, + ) -> anyhow::Result { + let mut process = Command::new(program) + .args(args) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .spawn()?; + let stdin = process + .stdin + .take() + .ok_or_else(|| anyhow::anyhow!("language server has no stdin"))?; + let stdout = process + .stdout + .take() + .ok_or_else(|| anyhow::anyhow!("language server has no stdout"))?; + + let (sender, incoming) = channel(); + thread::spawn(move || read_messages(BufReader::new(stdout), &sender)); + + let mut server = Self { + process, + stdin, + incoming, + next_id: 1, + broken: false, + }; + let root_uri = file_uri(root)?; + server.request::( + #[allow(deprecated)] // `root_uri` is how sourcekit-lsp still finds the workspace. + InitializeParams { + process_id: Some(std::process::id()), + root_uri: Some(root_uri), + capabilities: ClientCapabilities { + text_document: Some(TextDocumentClientCapabilities { + document_symbol: Some(DocumentSymbolClientCapabilities { + hierarchical_document_symbol_support: Some(true), + ..Default::default() + }), + ..Default::default() + }), + ..Default::default() + }, + ..Default::default() + }, + timeout, + ); + server.notify::(lsp_types::InitializedParams {}); + if server.broken { + return Err(anyhow::anyhow!( + "language server did not complete initialize" + )); + } + Ok(server) + } + + /// The symbols `file_path` declares. The text is sent rather than left for the + /// server to read, so this answers even where build settings do not resolve. + /// + /// The file is read in chunks and never held: see this module's "How source files are + /// read", and `send_did_open` below. + pub fn document_symbols( + &mut self, + file_path: &Path, + language_id: &str, + timeout: Duration, + ) -> Option> { + let uri = file_uri(file_path).ok()?; + if let Err(error) = self.send_did_open(&uri, language_id, file_path) { + tracing::debug!("could not send {}: {}", file_path.display(), error); + return None; + } + let response = self + .request::( + DocumentSymbolParams { + text_document: TextDocumentIdentifier { uri: uri.clone() }, + work_done_progress_params: WorkDoneProgressParams::default(), + partial_result_params: PartialResultParams::default(), + }, + timeout, + ) + .flatten(); + self.notify::(DidCloseTextDocumentParams { + text_document: TextDocumentIdentifier { uri }, + }); + match response { + Some(DocumentSymbolResponse::Nested(symbols)) => Some(symbols), + // Only a server that ignored `hierarchicalDocumentSymbolSupport` answers flat, + // and without nesting there is nothing to tie a method to its type. + Some(DocumentSymbolResponse::Flat(_)) => { + tracing::debug!("{} answered without hierarchy", file_path.display()); + None + } + None => None, + } + } + + pub fn is_broken(&self) -> bool { + self.broken + } + + /// `didOpen` for `path`, framed and written here rather than through + /// [`Message::write`], which reaches the pipe only via a `to_string` of the whole + /// message — one full copy of the file on top of the one being read. + /// + /// `Content-Length` precedes the body and a pipe cannot be rewound, so the length has to + /// be known before any of the body goes out. That is what the two passes are for: the + /// first serializes into [`io::sink`] to measure, the second writes the header and + /// streams the body. serde_json does the escaping in both, so the count cannot disagree + /// with what is emitted. + /// + /// Reading twice means the two passes can disagree if the file is rewritten in between, + /// and a body that does not match the header it was announced with desynchronises the + /// stream for every message after it. So the second pass is tallied too, and a mismatch + /// abandons the server rather than corrupting the rest of the scan — the same response + /// this already has for one that stops answering. + fn send_did_open(&mut self, uri: &Uri, language_id: &str, path: &Path) -> io::Result<()> { + if self.broken { + return Err(io::Error::other("language server was already abandoned")); + } + let text = FileText::new(path); + let params = DidOpenParams { + uri, + language_id, + text: &text, + }; + let message = Envelope { + method: DidOpenTextDocument::METHOD, + params: ¶ms, + }; + + let mut measured = Counting { + inner: io::sink(), + count: 0, + }; + // A read or encoding failure surfaces here, before anything is written -- so an + // unreadable or non-UTF-8 file costs this file's symbols rather than the stream. + serde_json::to_writer(&mut measured, &message) + .map_err(|error| text.take_error().unwrap_or_else(|| io::Error::other(error)))?; + let length = measured.count; + + // Scoped so the borrow of `stdin` ends before the failure path needs `self`. + let count = { + let mut out = BufWriter::with_capacity(CHUNK, &mut self.stdin); + write!(out, "Content-Length: {length}\r\n\r\n")?; + let mut written = Counting { + inner: &mut out, + count: 0, + }; + let sent = serde_json::to_writer(&mut written, &message) + .map_err(|error| text.take_error().unwrap_or_else(|| io::Error::other(error))); + let count = written.count; + // Flushed before reporting a failure: the header is already on its way, so as + // much of the body as exists has to follow it for the length check to mean + // anything. + let flushed = out.flush(); + sent?; + flushed?; + count + }; + + if count != length { + self.abandon::<()>( + DidOpenTextDocument::METHOD, + "changed size while it was being sent", + ); + return Err(io::Error::other(format!( + "announced {length} bytes and sent {count}" + ))); + } + Ok(()) + } + + fn request( + &mut self, + params: R::Params, + timeout: Duration, + ) -> Option { + if self.broken { + return None; + } + let id = RequestId::from(self.next_id); + self.next_id += 1; + let params = serde_json::to_value(params).ok()?; + self.send(Message::Request(Request { + id: id.clone(), + method: R::METHOD.to_owned(), + params, + })); + + let deadline = Instant::now() + timeout; + loop { + let Some(remaining) = deadline.checked_duration_since(Instant::now()) else { + return self.abandon(R::METHOD, "timed out"); + }; + let message = match self.incoming.recv_timeout(remaining) { + Ok(message) => message, + Err(RecvTimeoutError::Timeout) => return self.abandon(R::METHOD, "timed out"), + Err(RecvTimeoutError::Disconnected) => return self.abandon(R::METHOD, "exited"), + }; + match message { + Message::Response(response) if response.id == id => { + return match response.response_result { + Ok(result) => serde_json::from_value(result).ok(), + Err(error) => { + tracing::debug!("language server refused {}: {:?}", R::METHOD, error); + None + } + }; + } + // sourcekit-lsp registers capabilities and asks for configuration during + // startup; a peer that never replies leaves those pending for its lifetime. + Message::Request(request) => { + self.send(Message::Response(Response::new_ok( + request.id, + serde_json::Value::Null, + ))); + } + // A reply to a request we already gave up on, or a diagnostic we ignore. + Message::Response(_) | Message::Notification(_) => {} + } + } + } + + fn notify(&mut self, params: N::Params) { + let Ok(params) = serde_json::to_value(params) else { + return; + }; + self.notify_value(N::METHOD, params); + } + + fn notify_value(&mut self, method: &str, params: serde_json::Value) { + if self.broken { + return; + } + self.send(Message::Notification(Notification { + method: method.to_owned(), + params, + })); + } + + fn send(&mut self, message: Message) { + if message.write(&mut self.stdin).is_err() || self.stdin.flush().is_err() { + self.abandon::<()>("write", "closed its input"); + } + } + + fn abandon(&mut self, method: &str, reason: &str) -> Option { + if !self.broken { + tracing::warn!( + "language server {} during {}; abandoning it", + reason, + method + ); + self.broken = true; + let _ = self.process.kill(); + } + None + } +} + +impl Drop for LanguageServer { + fn drop(&mut self) { + let _ = self.process.kill(); + let _ = self.process.wait(); + } +} + +fn read_messages(mut reader: R, sender: &Sender) { + while let Ok(Some(message)) = Message::read(&mut reader) { + if sender.send(message).is_err() { + return; + } + } +} + +/// A `file://` URI. A server that cannot parse the URI answers with no symbols rather +/// than an error, so a path with a space fails silently unless it is encoded — and +/// `lsp_types::Uri` is a bare RFC 3986 parser that will not encode one for us. +/// +/// The path is made absolute first, because `file://` takes an authority: a relative +/// `file://Tests/Foo.swift` parses with `Tests` as the *host* and loses a path component. +/// Only the URI is absolute — the caller keeps reporting the path it was given, which is +/// what codeowners are resolved against. +/// A file's contents as a JSON string value, read in chunks rather than held. +/// +/// [`Serializer::serialize_str`] wants the whole string contiguous, which is what forced the +/// file to be resident. [`Serializer::collect_str`] takes a [`fmt::Display`] instead, and +/// serde_json overrides it to push each fragment through its escaper straight into the +/// writer — so a `Display` that reads in chunks never materializes the file. +struct FileText<'a> { + path: &'a Path, + /// `Display::fmt` can only fail with a payload-free [`fmt::Error`], so the real cause is + /// stashed here. serde_json's own `collect_str` adapter does the same for writer errors. + error: Cell>, +} + +impl<'a> FileText<'a> { + fn new(path: &'a Path) -> Self { + Self { + path, + error: Cell::new(None), + } + } + + /// The stashed cause, if a pass failed. + fn take_error(&self) -> Option { + self.error.take() + } + + fn stream(&self, f: &mut fmt::Formatter<'_>) -> io::Result<()> { + let mut file = fs::File::open(self.path)?; + let mut buf = vec![0_u8; CHUNK]; + // Bytes at the front of `buf` held over from the last chunk: a read boundary can fall + // inside a multi-byte character, and `write_str` takes only valid UTF-8. + let mut carry = 0_usize; + loop { + let read = file.read(&mut buf[carry..])?; + if read == 0 { + break; + } + let filled = carry + read; + let valid = match str::from_utf8(&buf[..filled]) { + Ok(_) => filled, + // No `error_len` means the input merely stops mid-character, so the rest of + // it is in the next chunk. Anything else is a file we cannot send. + Err(error) if error.error_len().is_none() => error.valid_up_to(), + Err(error) => return Err(io::Error::new(io::ErrorKind::InvalidData, error)), + }; + let chunk = str::from_utf8(&buf[..valid]) + .map_err(|error| io::Error::new(io::ErrorKind::InvalidData, error))?; + f.write_str(chunk) + .map_err(|_| io::Error::other("the serializer rejected a fragment"))?; + buf.copy_within(valid..filled, 0); + carry = filled - valid; + } + if carry > 0 { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "file ends inside a multi-byte character", + )); + } + Ok(()) + } +} + +impl fmt::Display for FileText<'_> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self.stream(f) { + Ok(()) => Ok(()), + Err(error) => { + self.error.set(Some(error)); + Err(fmt::Error) + } + } + } +} + +/// `{"jsonrpc": "2.0", "method": ..., "params": ...}` — the framing [`Message::write`] would +/// add, rebuilt here because it reaches the pipe only through a fully buffered `to_string`. +struct Envelope<'a, P> { + method: &'a str, + params: &'a P, +} + +impl Serialize for Envelope<'_, P> { + fn serialize(&self, serializer: S) -> Result { + let mut envelope = serializer.serialize_struct("JsonRpc", 3)?; + envelope.serialize_field("jsonrpc", "2.0")?; + envelope.serialize_field("method", self.method)?; + envelope.serialize_field("params", self.params)?; + envelope.end() + } +} + +/// [`DidOpenTextDocumentParams`] with the text streamed instead of owned. +struct DidOpenParams<'a> { + uri: &'a Uri, + language_id: &'a str, + text: &'a FileText<'a>, +} + +impl Serialize for DidOpenParams<'_> { + fn serialize(&self, serializer: S) -> Result { + let mut params = serializer.serialize_struct("DidOpenTextDocumentParams", 1)?; + params.serialize_field("textDocument", &TextDocumentItemRef(self))?; + params.end() + } +} + +struct TextDocumentItemRef<'a>(&'a DidOpenParams<'a>); + +impl Serialize for TextDocumentItemRef<'_> { + fn serialize(&self, serializer: S) -> Result { + let mut item = serializer.serialize_struct("TextDocumentItem", 4)?; + item.serialize_field("uri", self.0.uri)?; + item.serialize_field("languageId", self.0.language_id)?; + item.serialize_field("version", &1)?; + item.serialize_field("text", &StreamedText(self.0.text))?; + item.end() + } +} + +struct StreamedText<'a>(&'a FileText<'a>); + +impl Serialize for StreamedText<'_> { + fn serialize(&self, serializer: S) -> Result { + serializer.collect_str(self.0) + } +} + +/// Tallies what it passes on, so the body's length can be measured in one pass and checked +/// against the header in the next. [`io::sink`] as the inner writer makes it count alone. +struct Counting { + inner: W, + count: usize, +} + +impl Write for Counting { + fn write(&mut self, buf: &[u8]) -> io::Result { + let written = self.inner.write(buf)?; + self.count += written; + Ok(written) + } + + fn flush(&mut self) -> io::Result<()> { + self.inner.flush() + } +} + +fn file_uri(path: &Path) -> anyhow::Result { + // Lexical, so a symlinked checkout is not rewritten to somewhere the caller never named. + let absolute = std::path::absolute(path) + .map_err(|e| anyhow::anyhow!("cannot resolve {}: {e}", path.display()))?; + let url = url::Url::from_file_path(&absolute) + .map_err(|_| anyhow::anyhow!("not a usable file path: {}", absolute.display()))?; + url.as_str() + .parse::() + .map_err(|e| anyhow::anyhow!("{} is not a usable URI: {e}", url.as_str())) +} + +#[cfg(test)] +mod tests { + use temp_testdir::TempDir; + + use super::*; + + /// Records what the streamed file is handed to the serializer in, without keeping it. + struct Fragments { + largest: usize, + total: usize, + text: String, + } + + impl fmt::Write for Fragments { + fn write_str(&mut self, fragment: &str) -> fmt::Result { + self.largest = self.largest.max(fragment.len()); + self.total += fragment.len(); + self.text.push_str(fragment); + Ok(()) + } + } + + fn fragments_of(path: &Path) -> Fragments { + let mut fragments = Fragments { + largest: 0, + total: 0, + text: String::new(), + }; + // `write!` builds the `Formatter` that `Display::fmt` writes into, so this drives + // exactly the path `collect_str` does. + fmt::Write::write_fmt(&mut fragments, format_args!("{}", FileText::new(path))) + .expect("the file streams"); + fragments + } + + // The whole point of the streaming payload. A file many chunks long must still reach the + // serializer a chunk at a time, or nothing has been gained over reading it in one go. + #[test] + fn a_file_larger_than_a_chunk_is_never_handed_over_whole() { + let dir = TempDir::default(); + let path = dir.join("Big.swift"); + let line = "// a line of source that is long enough to matter\n"; + let body = line.repeat((CHUNK * 3) / line.len()); + fs::write(&path, &body).unwrap(); + assert!(body.len() > CHUNK * 2, "the fixture has to span chunks"); + + let fragments = fragments_of(&path); + assert!( + fragments.largest <= CHUNK, + "a fragment of {} bytes means {} of the file was held at once", + fragments.largest, + fragments.largest + ); + assert_eq!(fragments.total, body.len(), "and all of it is sent"); + } + + // A read boundary lands wherever it lands, and `write_str` takes only valid UTF-8, so a + // character split across two reads has to be rejoined rather than dropped or mangled. + #[test] + fn a_character_split_across_two_reads_survives() { + let dir = TempDir::default(); + let path = dir.join("Accented.swift"); + // One byte short of a chunk, so the two-byte character straddles the boundary. + let body = format!("{}é tail", "a".repeat(CHUNK - 1)); + fs::write(&path, &body).unwrap(); + + assert_eq!(fragments_of(&path).text, body); + } + + // Reported rather than silently emitted as replacement characters, and -- because it is + // found on the measuring pass -- before any of it has been announced to the server. + #[test] + fn a_file_that_is_not_utf8_is_refused() { + let dir = TempDir::default(); + let path = dir.join("Latin1.swift"); + fs::write(&path, [b'/', b'/', 0xFF, b'\n']).unwrap(); + + let text = FileText::new(&path); + let mut sink = Fragments { + largest: 0, + total: 0, + text: String::new(), + }; + assert!(fmt::Write::write_fmt(&mut sink, format_args!("{text}")).is_err()); + assert_eq!( + text.take_error().map(|error| error.kind()), + Some(io::ErrorKind::InvalidData) + ); + } + + // The payload is ours now rather than `lsp_types`', so it has to keep describing the + // same document. A server given a malformed `didOpen` answers with no symbols rather + // than an error, so a drifted field name would look exactly like a checkout that + // declares nothing. Compared parsed, since field order carries no meaning in JSON. + #[test] + fn the_streamed_did_open_describes_the_same_document_as_the_typed_one() { + let dir = TempDir::default(); + let path = dir.join("MyTests.swift"); + let body = "class MyTests {\n\t\"quoted\" \\ é\n}\n"; + fs::write(&path, body).unwrap(); + let uri = file_uri(&path).unwrap(); + + let text = FileText::new(&path); + let streamed = serde_json::to_string(&DidOpenParams { + uri: &uri, + language_id: "swift", + text: &text, + }) + .unwrap(); + + let typed = serde_json::to_string(&lsp_types::DidOpenTextDocumentParams { + text_document: lsp_types::TextDocumentItem { + uri: uri.clone(), + language_id: String::from("swift"), + version: 1, + text: String::from(body), + }, + }) + .unwrap(); + + assert_eq!( + serde_json::from_str::(&streamed).unwrap(), + serde_json::from_str::(&typed).unwrap() + ); + } + + // The header is written before the body and a pipe cannot be rewound, so the measuring + // pass has to agree with the emitting one exactly -- a byte out either truncates the + // message or leaves the stream misframed for everything after it. + #[test] + fn the_measured_length_matches_what_is_emitted() { + let dir = TempDir::default(); + let path = dir.join("MyTests.swift"); + fs::write(&path, "class MyTests {\n\t\"q\" \\ é\u{7}\n}\n").unwrap(); + let uri = file_uri(&path).unwrap(); + + let text = FileText::new(&path); + let message = Envelope { + method: "textDocument/didOpen", + params: &DidOpenParams { + uri: &uri, + language_id: "swift", + text: &text, + }, + }; + + let mut measured = Counting { + inner: io::sink(), + count: 0, + }; + serde_json::to_writer(&mut measured, &message).unwrap(); + + let mut emitted = Counting { + inner: Vec::new(), + count: 0, + }; + serde_json::to_writer(&mut emitted, &message).unwrap(); + + assert_eq!(measured.count, emitted.count); + assert_eq!(measured.count, emitted.inner.len()); + } + + // A server that cannot parse the URI answers with no symbols rather than an error, so + // both of these fail silently in production if they regress. `lsp_types::Uri` will not + // encode for us and `file://` takes an authority, so neither is free. + #[test] + fn a_path_with_a_space_is_percent_encoded() { + let uri = file_uri(Path::new("/repo/Tests/My Test.swift")).unwrap(); + assert_eq!(uri.as_str(), "file:///repo/Tests/My%20Test.swift"); + } + + #[test] + fn a_hash_is_encoded_rather_than_starting_a_fragment() { + let uri = file_uri(Path::new("/repo/Tests/a#b.swift")).unwrap(); + assert_eq!(uri.as_str(), "file:///repo/Tests/a%23b.swift"); + } + + // A relative path would otherwise parse with its first component as the *host*, + // silently dropping it: `file://Tests/Foo.swift` is host `Tests`, path `/Foo.swift`. + #[test] + fn a_relative_path_becomes_an_absolute_uri() { + let uri = file_uri(Path::new("Tests/Foo.swift")).unwrap(); + assert!( + uri.as_str().starts_with("file:///"), + "expected an absolute file URI, got {}", + uri.as_str() + ); + assert!( + uri.as_str().ends_with("/Tests/Foo.swift"), + "expected the path to survive, got {}", + uri.as_str() + ); + } +} diff --git a/xcresult/src/main.rs b/xcresult/src/main.rs index 743fa487..14015013 100644 --- a/xcresult/src/main.rs +++ b/xcresult/src/main.rs @@ -1,9 +1,9 @@ -use std::{fs, io, path::PathBuf}; +use std::{fs, io, path::PathBuf, time::Duration}; use clap::Parser; use context::repo::RepoUrlParts; use tracing_subscriber::prelude::*; -use xcresult::xcresult::XCResult; +use xcresult::{test_locations::Limits, xcresult::XCResult}; #[derive(Debug, Parser)] pub struct Cli { @@ -21,6 +21,64 @@ pub struct Cli { pub output_file_path: Option, #[arg(long, required = false)] pub use_experimental_failure_summary: bool, + #[arg( + long, + env = constants::TRUNK_USE_EXPERIMENTAL_XCRESULT_TEST_LOCATIONS_ENV, + help = "Take each test's file from where a language server says it is declared in --repo-root, rather than from the failure that surfaced it. Reads the bundle with no legacy `xcresulttool get object` calls.", + action = clap::ArgAction::Set, + required = false, + require_equals = true, + num_args = 0..=1, + default_value = "false", + default_missing_value = "true", + conflicts_with = "use_experimental_failure_summary" + )] + pub use_experimental_xcresult_test_locations: bool, + #[arg( + long, + help = "Checkout to resolve test declarations in, defaults to the working directory.", + requires = "use_experimental_xcresult_test_locations" + )] + pub repo_root: Option, + /// Most source files to parse. Ranked so the likeliest declarations come first, and + /// parsing stops early once every test resolves. + #[arg( + long, + env = constants::TRUNK_XCRESULT_TEST_LOCATIONS_MAX_FILES_ENV, + default_value_t = Limits::default().max_files, + )] + pub xcresult_test_locations_max_files: usize, + /// Seconds to spend per language server. The clang server answers far slower per file + /// than the Swift one, so an Objective-C heavy repo wants this raised. + #[arg( + long, + env = constants::TRUNK_XCRESULT_TEST_LOCATIONS_BUDGET_SECS_ENV, + default_value_t = Limits::default().budget.as_secs(), + )] + pub xcresult_test_locations_budget_secs: u64, + /// Seconds to wait for a single language server reply before giving up on it. + #[arg( + long, + env = constants::TRUNK_XCRESULT_TEST_LOCATIONS_REQUEST_TIMEOUT_SECS_ENV, + default_value_t = Limits::default().request_timeout.as_secs(), + )] + pub xcresult_test_locations_request_timeout_secs: u64, + /// How many times to replace a language server that stops answering with a fresh one. + #[arg( + long, + env = constants::TRUNK_XCRESULT_TEST_LOCATIONS_RETRIES_ENV, + default_value_t = Limits::default().retries, + )] + pub xcresult_test_locations_retries: usize, + /// Largest source file to parse. One `didOpen` carries the whole file, so an outsized + /// generated one costs memory and usually a timed-out request for symbols it does not + /// declare. Skipped without being read. + #[arg( + long, + env = constants::TRUNK_XCRESULT_TEST_LOCATIONS_MAX_FILE_BYTES_ENV, + default_value_t = Limits::default().max_file_bytes, + )] + pub xcresult_test_locations_max_file_bytes: u64, } fn main() -> anyhow::Result<()> { @@ -34,16 +92,41 @@ fn main() -> anyhow::Result<()> { repo_url, output_file_path, use_experimental_failure_summary, + use_experimental_xcresult_test_locations, + repo_root, + xcresult_test_locations_max_files, + xcresult_test_locations_budget_secs, + xcresult_test_locations_request_timeout_secs, + xcresult_test_locations_retries, + xcresult_test_locations_max_file_bytes, } = Cli::parse(); let repo_url_parts = repo_url .and_then(|repo_url| RepoUrlParts::from_url(&repo_url).ok()) .unwrap_or_default(); - let xcresult = XCResult::new( - path, - org_url_slug.unwrap_or_default(), - repo_url_parts.repo_full_name(), - use_experimental_failure_summary, - )?; + let org_url_slug = org_url_slug.unwrap_or_default(); + let repo_full_name = repo_url_parts.repo_full_name(); + let xcresult = if use_experimental_xcresult_test_locations { + XCResult::new_with_declaration_locations( + path, + org_url_slug, + repo_full_name, + repo_root.unwrap_or_else(|| PathBuf::from(".")), + Limits { + max_files: xcresult_test_locations_max_files, + budget: Duration::from_secs(xcresult_test_locations_budget_secs), + request_timeout: Duration::from_secs(xcresult_test_locations_request_timeout_secs), + retries: xcresult_test_locations_retries, + max_file_bytes: xcresult_test_locations_max_file_bytes, + }, + )? + } else { + XCResult::new( + path, + org_url_slug, + repo_full_name, + use_experimental_failure_summary, + )? + }; let mut junits = xcresult.generate_junits(); let junit_count_and_first_junit = (junits.len(), junits.pop()); let junit = if let (1, Some(junit)) = junit_count_and_first_junit { diff --git a/xcresult/src/test_locations.rs b/xcresult/src/test_locations.rs new file mode 100644 index 00000000..75496dcc --- /dev/null +++ b/xcresult/src/test_locations.rs @@ -0,0 +1,1049 @@ +//! Where a test is *declared*, rather than where a failure surfaced — all an `.xcresult` +//! records (see [`crate::file_attribution`]). `documentSymbol` names the type containing +//! each method and, unlike `workspace/symbol`, needs no index and no build. + +use std::{ + collections::{HashMap, HashSet}, + path::{Path, PathBuf}, + time::{Duration, Instant}, +}; + +use ignore::{WalkBuilder, types::TypesBuilder}; +use lsp_types::{DocumentSymbol, SymbolKind}; + +use crate::{file_attribution::ReportedPath, lsp::LanguageServer, xcrun::find_program}; + +/// Kinds that can declare a test. +const METHOD_KINDS: [SymbolKind; 3] = [ + SymbolKind::METHOD, + SymbolKind::CONSTRUCTOR, + SymbolKind::FUNCTION, +]; +/// Kinds that can contain one. `INTERFACE` is how an Objective-C category arrives. +const CONTAINER_KINDS: [SymbolKind; 3] = + [SymbolKind::CLASS, SymbolKind::INTERFACE, SymbolKind::STRUCT]; + +const SWIFT_EXTENSIONS: [&str; 1] = ["swift"]; +const CLANG_EXTENSIONS: [&str; 5] = ["m", "mm", "c", "cc", "cpp"]; + +/// Directories holding something other than the repo's own code. Exact target ownership +/// would need a build log, and reading one costs the legacy call this path exists to avoid. +const SKIPPED_DIRECTORIES: [&str; 9] = [ + ".git", + ".build", + ".swiftpm", + "build", + "checkouts", + "Carthage", + "DerivedData", + "node_modules", + "Pods", +]; + +/// Every field is settable from the CLI, because the right value depends on the repo: the +/// clang server answers roughly an order of magnitude slower per file than the Swift one, +/// so an Objective-C heavy checkout needs more of all of them than these defaults give. +#[derive(Debug, Clone, Copy)] +pub struct Limits { + pub max_files: usize, + /// Spent per server kind rather than across both, so a large Swift tree cannot leave + /// the clang server with nothing left to parse Objective-C in. + pub budget: Duration, + pub request_timeout: Duration, + /// How many times a server that stops answering is replaced with a fresh one. + pub retries: usize, + /// Skipped above this, unread. `didOpen` carries the whole file, so a generated source + /// file costs its own size twice over and will usually exhaust `request_timeout` as + /// well — taking a retry and the rest of the budget with it, for one file's symbols. + pub max_file_bytes: u64, +} + +impl Default for Limits { + fn default() -> Self { + Self { + max_files: 2_000, + budget: Duration::from_secs(60), + request_timeout: Duration::from_secs(30), + retries: 1, + // Comfortably above hand-written source, and low enough that the generated files + // that reach megabytes -- which declare no tests -- are not read. + max_file_bytes: 2 * 1_024 * 1_024, + } + } +} + +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct TestKey { + suite: Option, + case: String, +} + +impl TestKey { + /// `Outer/Inner/case()` — only the innermost suite declares the method, and a top-level + /// swift-testing test has none. + pub fn from_node_identifier(node_identifier: &str) -> Self { + let mut components = node_identifier.rsplit('/'); + let case = components.next().unwrap_or_default(); + Self { + suite: components + .next() + .map(|suite| container_name(suite).to_string()), + case: normalized_case(case), + } + } +} + +impl TestKey { + /// `classname` is the target plus the dot-qualified suite path (`MyCLITests.Outer.Inner`), + /// collapsing to the bare target for a top-level `@Test func`, which declares no suite. + pub fn from_junit_classname(classname: &str, name: &str) -> Self { + let mut components = classname.rsplit('.'); + let innermost = components.next().unwrap_or_default(); + let has_suite = components.next().is_some(); + Self { + suite: has_suite.then(|| container_name(innermost).to_string()), + case: normalized_case(name), + } + } + + /// The first component, which tells same-named suites in different modules apart. + pub fn target_from_junit_classname(classname: &str) -> Option { + let target = classname.split('.').next()?; + (!target.is_empty()).then(|| target.to_string()) + } + + /// `test://com.apple.xcode////` — the second component is + /// the test bundle, which is the only thing distinguishing two same-named suites. + pub fn target_from_identifier_url(identifier_url: &str) -> Option { + let path = identifier_url.split("://").nth(1)?; + let target = path.split('/').nth(2)?; + (!target.is_empty()).then(|| percent_decoded(target)) + } +} + +fn percent_decoded(value: &str) -> String { + let bytes = value.as_bytes(); + let mut decoded = Vec::with_capacity(bytes.len()); + let mut index = 0; + while index < bytes.len() { + if bytes[index] == b'%' + && let Some(byte) = value + .get(index + 1..index + 3) + .and_then(|hex| u8::from_str_radix(hex, 16).ok()) + { + decoded.push(byte); + index += 3; + } else { + decoded.push(bytes[index]); + index += 1; + } + } + String::from_utf8(decoded).unwrap_or_else(|_| value.to_string()) +} + +/// Whether a path lies under a directory named for the target, which is how a candidate is +/// tied to the module that actually ran it. +fn is_in_target(file: &ReportedPath, target: &str) -> bool { + Path::new(file.as_str()) + .components() + .any(|component| component.as_os_str().to_string_lossy() == target) +} + +#[derive(Debug, Clone)] +pub struct DeclarationSite { + pub file: ReportedPath, + pub line: Option, +} + +#[derive(Debug, Default)] +pub struct TestLocationIndex { + declarations: HashMap, + /// Where each suite is declared, for a test that its own suite does not declare. + suites: HashMap, + targets: HashMap, +} + +impl TestLocationIndex { + pub fn resolve(repo_root: &Path, keys: &[(TestKey, Option)], limits: Limits) -> Self { + let targets = keys + .iter() + .filter_map(|(key, target)| target.clone().map(|target| (key.clone(), target))) + .collect::>(); + let keys = keys.iter().map(|(key, _)| key.clone()).collect::>(); + let suites = keys + .iter() + .filter_map(|key| key.suite.as_deref()) + .collect::>(); + let sources = scan_sources(repo_root, &suites, limits.max_files, limits.max_file_bytes); + let (swift, clang) = sources + .into_iter() + .partition::, _>(|path| has_extension(path, &SWIFT_EXTENSIONS)); + + let mut resolver = Resolver { + index: Self { + targets, + ..Self::default() + }, + unresolved: keys.clone(), + limits, + }; + resolver.parse(&swift, &SOURCEKIT_LSP, repo_root); + resolver.parse(&clang, &CLANGD, repo_root); + if !resolver.unresolved.is_empty() { + tracing::debug!( + "{} of {} test(s) have no declaration in the checkout", + resolver.unresolved.len(), + keys.len() + ); + } + resolver.index + } + + /// The file the test is written in, preferring the declaration of the method itself — + /// a suite split across extensions declares each test in its own file. + /// + /// A test run under a suite that does not declare it was inherited: a suite cannot run a + /// method it does not have, so the declaration is in some base class and that is the file + /// the test is written in. No inheritance graph is needed to reach that conclusion — the + /// test having run is the proof. + /// + /// One is needed to act on it whenever more than one class declares the case, though, + /// because an override is a declaration too: see [`Self::inherited_declaration`]. Where + /// nothing declares it, or several things do, the suite's own file stands in. + pub fn lookup(&self, key: &TestKey) -> Option<&DeclarationSite> { + if let Some(site) = self.method_declaration(key) { + return Some(site); + } + if let Some(site) = self.inherited_declaration(key) { + return Some(site); + } + match key.suite.as_ref() { + Some(suite) => self.suites.get(suite), + // A top-level swift-testing test has no suite, so the function is all there is. + None => None, + } + } + + /// The one declaration of `key`'s case under some other suite, which for a test that ran + /// is the base class it was inherited from. + /// + /// Only answers when there is exactly one candidate. Nothing here can tell an *ancestor* + /// that declares the method from an unrelated *sibling* that overrides it — an override + /// is a declaration of the same name, so by name alone the two are the same fact, and + /// separating them would need the inheritance graph, which is semantic and would cost a + /// build. So more than one candidate reports nothing and lets the suite's own file stand + /// in, rather than picking a file the test may well not be written in. + /// + /// Scoped to the target that ran it, since two modules can each declare a case of the + /// same name and those are never the same method. + fn inherited_declaration(&self, key: &TestKey) -> Option<&DeclarationSite> { + let target = self.targets.get(key); + let mut candidates: Vec<&DeclarationSite> = Vec::new(); + for (candidate, site) in &self.declarations { + if candidate.case != key.case || candidate.suite == key.suite { + continue; + } + if let Some(target) = target + && !is_in_target(&site.file, target) + { + continue; + } + // Two suites declaring it in one file is still one answer. + if !candidates.iter().any(|found| found.file == site.file) { + candidates.push(site); + } + } + + if candidates.len() > 1 { + // Sorted because the candidates come out of a `HashMap`, and a warning that + // names the same files in a different order every run is hard to trust. + let mut files = candidates + .iter() + .map(|site| site.file.as_str()) + .collect::>(); + files.sort_unstable(); + tracing::warn!( + "{} declares no {}, and {} places declare one -- any of them could be what it \ + inherited, so its own file is reported instead of guessing between {}", + key.suite.as_deref().unwrap_or("a top-level test"), + key.case, + files.len(), + files.join(", ") + ); + return None; + } + candidates.first().copied() + } + + /// Only the method's own declaration, which is what decides whether there is still + /// something worth parsing for. + /// + /// Resolution stops once nothing is left to find, so it cannot be driven by + /// [`Self::lookup`]: a suite is usually declared in the first file ranked for it, and + /// counting that as an answer would end the scan before the file the *method* is + /// declared in was ever read — collapsing every test to its suite's file. + fn method_declaration(&self, key: &TestKey) -> Option<&DeclarationSite> { + self.declarations.get(key) + } + + pub fn is_empty(&self) -> bool { + self.declarations.is_empty() + } + + fn record(&mut self, key: TestKey, candidate: DeclarationSite) { + match self.declarations.get(&key) { + None => { + self.declarations.insert(key, candidate); + } + Some(existing) => { + if let Some(target) = self.targets.get(&key) + && is_in_target(&candidate.file, target) + && !is_in_target(&existing.file, target) + { + tracing::debug!( + "preferring {} over {} for target {}", + candidate.file.as_str(), + existing.file.as_str(), + target + ); + self.declarations.insert(key, candidate); + } + } + } + } + + fn collect(&mut self, symbols: &[DocumentSymbol], file: &Path, container: Option<&str>) { + for symbol in symbols { + let site = || DeclarationSite { + file: ReportedPath::new(&file.to_string_lossy()), + line: declaration_line(symbol), + }; + if CONTAINER_KINDS.contains(&symbol.kind) { + // An Objective-C category is reported against the class it extends, which + // is already declared elsewhere, so the first declaration seen wins. + self.suites + .entry(container_name(&symbol.name).to_string()) + .or_insert_with(site); + } + if METHOD_KINDS.contains(&symbol.kind) { + let key = TestKey { + suite: container.map(|name| container_name(name).to_string()), + case: normalized_case(&symbol.name), + }; + self.record(key, site()); + } + if let Some(children) = symbol.children.as_deref() { + self.collect(children, file, Some(&symbol.name)); + } + } + } +} + +struct ServerKind { + program: &'static str, + args: &'static [&'static str], + language_id: &'static str, +} + +const SOURCEKIT_LSP: ServerKind = ServerKind { + program: "sourcekit-lsp", + args: &[], + language_id: "swift", +}; + +/// Background indexing is the work `documentSymbol` exists to avoid. +const CLANGD: ServerKind = ServerKind { + program: "clangd", + args: &["--background-index=false"], + language_id: "objective-c", +}; + +struct Resolver { + index: TestLocationIndex, + unresolved: Vec, + limits: Limits, +} + +impl Resolver { + /// Parse `files` with one `kind` of server, replacing it when it stops answering. + /// + /// A server that times out is killed rather than resynchronised, so the only way to + /// carry on is a fresh one. The file that broke it is skipped instead of retried: it + /// is the reason the last server died, and retrying it would spend the whole budget + /// re-earning the same timeout. + fn parse(&mut self, files: &[PathBuf], kind: &ServerKind, root: &Path) { + if files.is_empty() || self.unresolved.is_empty() { + return; + } + let Some(program) = find_program(kind.program) else { + tracing::warn!( + "{} not found; {} source file(s) left unparsed", + kind.program, + files.len() + ); + return; + }; + + let deadline = Instant::now() + self.limits.budget; + let mut remaining = files; + let mut parsed = 0; + for attempt in 0..=self.limits.retries { + if remaining.is_empty() || self.unresolved.is_empty() || Instant::now() >= deadline { + break; + } + if attempt > 0 { + tracing::warn!( + "{}: restarting it, {} file(s) left to parse", + kind.program, + remaining.len() + ); + } + let mut server = + match LanguageServer::start(&program, kind.args, root, self.limits.request_timeout) + { + Ok(server) => server, + Err(e) => { + tracing::warn!("failed to start {}: {}", kind.program, e); + return; + } + }; + + let mut consumed = 0; + for file in remaining { + consumed += 1; + if self.unresolved.is_empty() { + break; + } + if Instant::now() >= deadline { + tracing::warn!( + "{}: out of time after {} file(s), {} left unparsed", + kind.program, + parsed, + files.len() - parsed + ); + return; + } + // The file is read inside `document_symbols`, in chunks -- an unreadable or + // non-UTF-8 one is reported there and answers with no symbols, as it did + // when the read happened here. + let symbols = + server.document_symbols(file, kind.language_id, self.limits.request_timeout); + if server.is_broken() { + break; + } + let Some(symbols) = symbols else { + continue; + }; + parsed += 1; + self.index.collect(&symbols, file, None); + let index = &self.index; + self.unresolved + .retain(|key| index.method_declaration(key).is_none()); + } + remaining = &remaining[consumed.min(remaining.len())..]; + if !server.is_broken() { + break; + } + } + if !remaining.is_empty() && !self.unresolved.is_empty() { + tracing::warn!( + "{}: gave up with {} file(s) unparsed", + kind.program, + remaining.len() + ); + } + tracing::debug!( + "{}: parsed {} of {} file(s)", + kind.program, + parsed, + files.len() + ); + } +} + +/// LSP counts lines from zero; everything downstream counts from one. +fn declaration_line(symbol: &DocumentSymbol) -> Option { + let range = symbol.selection_range; + range.start.line.checked_add(1) +} + +/// Applied to identifier and symbol alike: Swift spells it `testExample()`, Objective-C +/// `-testExample`. +fn normalized_case(name: &str) -> String { + name.trim_start_matches(['+', '-']) + .trim_end_matches(['(', ')']) + .to_string() +} + +/// An Objective-C category comes back as `Suite(Category)`; identifiers name the class. +fn container_name(name: &str) -> &str { + name.split('(').next().unwrap_or(name) +} + +fn has_extension(path: &Path, extensions: &[&str]) -> bool { + path.extension() + .and_then(|extension| extension.to_str()) + .map(|extension| extensions.contains(&extension)) + .unwrap_or(false) +} + +/// Files most likely to pay off first: a suite is overwhelmingly declared in a file named +/// after it, and parsing stops once every test resolves. Symlinked directories are skipped. +/// +/// The extensions are registered explicitly rather than taken from `ignore`'s built-in +/// definitions, because those map `.h` to Objective-C and a header is not something the +/// clang server can answer `documentSymbol` for on its own. +/// +/// `.gitignore` already excludes most of [`SKIPPED_DIRECTORIES`] in a normal checkout, but +/// nothing guarantees it, so the list stays as an override on top. +fn scan_sources( + repo_root: &Path, + suites: &HashSet<&str>, + max_files: usize, + max_file_bytes: u64, +) -> Vec { + let mut types = TypesBuilder::new(); + for extension in SWIFT_EXTENSIONS.iter().chain(CLANG_EXTENSIONS.iter()) { + // `add` only fails on a malformed glob, and these are built from literals. + let _ = types.add("sources", &format!("*.{extension}")); + } + types.select("sources"); + let Ok(types) = types.build() else { + return Vec::new(); + }; + + let walker = WalkBuilder::new(repo_root) + .types(types) + .hidden(false) + .follow_links(false) + .filter_entry(|entry| { + entry.depth() == 0 + || !SKIPPED_DIRECTORIES.contains(&entry.file_name().to_string_lossy().as_ref()) + }) + .build(); + + // Counted rather than logged per file: a repo with a generated-sources directory would + // otherwise emit a warning for every one of them. + let mut oversized = 0_usize; + let mut found = walker + .flatten() + .filter(|entry| { + entry + .file_type() + .is_some_and(|file_type| file_type.is_file()) + }) + .filter(|entry| { + // The walker has already stat'd the entry, so the size costs nothing extra, and + // failing to read it here means the read would have failed anyway. + match entry.metadata() { + Ok(metadata) if metadata.len() > max_file_bytes => { + oversized += 1; + false + } + _ => true, + } + }) + .map(|entry| entry.into_path()) + .collect::>(); + if oversized > 0 { + tracing::warn!("{oversized} source file(s) skipped for exceeding {max_file_bytes} bytes"); + } + found.sort_by_cached_key(|path| (rank(path, suites), path.clone())); + found.truncate(max_files); + found +} + +fn rank(path: &Path, suites: &HashSet<&str>) -> u8 { + let Some(stem) = path + .file_stem() + .map(|stem| stem.to_string_lossy().to_string()) + else { + return 2; + }; + if suites.contains(stem.as_str()) { + 0 + } else if suites.iter().any(|suite| stem.contains(suite)) { + 1 + } else { + 2 + } +} + +#[cfg(test)] +mod tests { + use std::fs; + + use rstest::rstest; + use serde_json::{Value, json}; + use temp_testdir::TempDir; + + use super::*; + + const SWIFT_FILE: &str = "/repo/Tests/SnapshotReproTests.swift"; + + fn key(suite: Option<&str>, case: &str) -> TestKey { + TestKey { + suite: suite.map(String::from), + case: String::from(case), + } + } + + fn symbols(value: Value) -> Vec { + serde_json::from_value(value).unwrap() + } + + fn span(from: u64, to: u64) -> Value { + json!({ + "start": { "line": from, "character": 0 }, + "end": { "line": to, "character": 0 } + }) + } + + fn method(name: &str, line: u64) -> Value { + json!({ + "name": name, + "kind": SymbolKind::METHOD, + "range": span(line, line), + "selectionRange": span(line, line) + }) + } + + fn container(name: &str, kind: SymbolKind, lines: (u64, u64), children: Vec) -> Value { + json!({ + "name": name, + "kind": kind, + "range": span(lines.0, lines.1), + "selectionRange": span(lines.0, lines.0), + "children": children + }) + } + + fn indexed(file: &str, value: Value) -> TestLocationIndex { + let mut index = TestLocationIndex::default(); + index.collect(&symbols(value), Path::new(file), None); + index + } + + #[rstest] + #[case::swift_xctest( + "SnapshotReproTests/testExample()", + Some("SnapshotReproTests"), + "testExample" + )] + #[case::objc_has_no_parens( + "ObjcXCTestTests/testExample", + Some("ObjcXCTestTests"), + "testExample" + )] + #[case::top_level_swift_testing("failingSnapshot()", None, "failingSnapshot")] + #[case::only_the_innermost_suite_declares( + "OuterSuite/InnerSuite/testExample()", + Some("InnerSuite"), + "testExample" + )] + #[case::parameterized_keeps_its_labels( + "SnapshotReproTests/testExample(input:)", + Some("SnapshotReproTests"), + "testExample(input:" + )] + fn an_identifier_names_a_suite_and_a_case( + #[case] node_identifier: &str, + #[case] suite: Option<&str>, + #[case] case: &str, + ) { + assert_eq!( + TestKey::from_node_identifier(node_identifier), + key(suite, case) + ); + } + + // The identifier and the symbol are spelled differently in every language, so what + // matters is that normalizing both lands them on the same key. + #[rstest] + #[case::swift("SnapshotReproTests/testExample()", "testExample()")] + #[case::objc("ObjcXCTestTests/testExample", "-testExample")] + #[case::objc_class_method("ObjcXCTestTests/testExample", "+testExample")] + #[case::parameterized("Suite/testExample(input:)", "testExample(input:)")] + fn an_identifier_and_its_symbol_normalize_alike( + #[case] node_identifier: &str, + #[case] symbol_name: &str, + ) { + assert_eq!( + TestKey::from_node_identifier(node_identifier).case, + normalized_case(symbol_name) + ); + } + + // Resolution stops when nothing is left to find, so if the suite fallback counted as + // an answer the scan would end at the first file naming the suite and never read the + // one declaring the method. The two lookups have to disagree here. + #[test] + fn the_suite_fallback_does_not_count_as_a_resolved_declaration() { + let index = indexed( + SWIFT_FILE, + json!([container( + "SnapshotReproTests", + SymbolKind::CLASS, + (0, 2), + vec![] + )]), + ); + let key = key(Some("SnapshotReproTests"), "testExample"); + assert!( + index.method_declaration(&key).is_none(), + "the method is declared nowhere yet, so there is still work to do" + ); + assert!( + index.lookup(&key).is_some(), + "but the suite is known, so a file can still be reported for it" + ); + } + + // A suite cannot run a method it does not have, so a case arriving under a suite that + // declares no such method was inherited, and the one declaration of that name is the base + // class it came from. Nothing else in the checkout can be the answer. + #[test] + fn a_suite_that_does_not_declare_its_case_inherited_it() { + let index = indexed( + SWIFT_FILE, + json!([container( + "BaseTests", + SymbolKind::CLASS, + (0, 2), + vec![method("testExample()", 1)] + )]), + ); + assert_eq!( + index + .lookup(&key(Some("ChildTests"), "testExample")) + .map(|site| site.line), + Some(Some(2)), + "the inherited method resolves to the line it is written on" + ); + } + + // An override is its own declaration, so it answers before anything is inherited. + #[test] + fn an_override_keeps_its_own_declaration() { + let index = indexed( + SWIFT_FILE, + json!([ + container( + "BaseTests", + SymbolKind::CLASS, + (0, 2), + vec![method("testExample()", 1)] + ), + container( + "ChildTests", + SymbolKind::CLASS, + (3, 5), + vec![method("testExample()", 4)] + ) + ]), + ); + assert_eq!( + index + .lookup(&key(Some("ChildTests"), "testExample")) + .map(|site| site.line), + Some(Some(5)) + ); + } + + // The shape that matters in practice, and the reason this cannot be done by name: a + // sibling overriding the method declares it too. `ChildA` inherits from `Base` and + // `ChildB` overrides, so `testInherited` is declared in two files and neither the + // ancestor nor the sibling can be told apart from the case name -- which is what the + // `swift-test-xunit` fixture captures. + #[test] + fn a_sibling_override_makes_the_inherited_declaration_ambiguous() { + let mut index = indexed( + "/repo/Tests/MyCLITests/BaseTests.swift", + json!([container( + "BaseTests", + SymbolKind::CLASS, + (0, 2), + vec![method("testInherited()", 1)] + )]), + ); + index.collect( + &symbols(json!([container( + "ChildBTests", + SymbolKind::CLASS, + (0, 2), + vec![method("testInherited()", 1)] + )])), + Path::new("/repo/Tests/MyCLITests/ChildBTests.swift"), + None, + ); + index.suites.insert( + String::from("ChildATests"), + site("/repo/Tests/MyCLITests/ChildATests.swift"), + ); + let key = key(Some("ChildATests"), "testInherited"); + index + .targets + .insert(key.clone(), String::from("MyCLITests")); + + assert!( + index.inherited_declaration(&key).is_none(), + "the ancestor and the override are indistinguishable by name" + ); + assert_eq!( + index.lookup(&key).map(|site| site.file.as_str().to_owned()), + Some(String::from("/repo/Tests/MyCLITests/ChildATests.swift")), + "so the suite's own file stands in" + ); + } + + // Two unrelated suites declare the case in different files, so which one a third + // inherited it from is unknowable here. Guessing would report a file the test is not + // written in, so the suite's own file stands in instead. + #[test] + fn an_ambiguous_inheritance_declines_rather_than_guessing() { + let mut index = indexed( + SWIFT_FILE, + json!([container( + "OneTests", + SymbolKind::CLASS, + (0, 2), + vec![method("testExample()", 1)] + )]), + ); + index.collect( + &symbols(json!([container( + "TwoTests", + SymbolKind::CLASS, + (0, 2), + vec![method("testExample()", 1)] + )])), + Path::new("/repo/Tests/TwoTests.swift"), + None, + ); + index.suites.insert( + String::from("ThirdTests"), + site("/repo/Tests/ThirdTests.swift"), + ); + + assert_eq!( + index + .lookup(&key(Some("ThirdTests"), "testExample")) + .map(|site| site.file.as_str().to_owned()), + Some(String::from("/repo/Tests/ThirdTests.swift")) + ); + } + + // The same-named case in another module is not what this one inherited, so the target + // has to bound the search even when only one candidate exists. + #[test] + fn a_case_of_the_same_name_in_another_target_is_not_inherited() { + let mut index = indexed( + "/repo/Tests/OtherFeatureTests/OtherTests.swift", + json!([container( + "OtherTests", + SymbolKind::CLASS, + (0, 2), + vec![method("testExample()", 1)] + )]), + ); + let key = key(Some("ChildTests"), "testExample"); + index + .targets + .insert(key.clone(), String::from("MyFeatureTests")); + + assert!( + index.lookup(&key).is_none(), + "no declaration under MyFeatureTests, and no suite either" + ); + } + + #[test] + fn the_scan_ranks_suite_named_files_first_and_skips_vendored_directories() { + let root = TempDir::default(); + for relative in [ + "Sources/Alpha.swift", + "Tests/SnapshotReproTests.swift", + "Tests/SnapshotReproTestsHelper.swift", + "Pods/Vendored.swift", + ".build/checkouts/Dep/Dep.swift", + ] { + let path = root.join(relative); + fs::create_dir_all(path.parent().unwrap()).unwrap(); + fs::write(path, "").unwrap(); + } + let suites = HashSet::from(["SnapshotReproTests"]); + let scanned = scan_sources(root.as_ref(), &suites, 10, u64::MAX) + .iter() + .map(|path| path.file_name().unwrap().to_string_lossy().to_string()) + .collect::>(); + assert_eq!( + scanned, + vec![ + "SnapshotReproTests.swift", + "SnapshotReproTestsHelper.swift", + "Alpha.swift" + ] + ); + } + + #[test] + fn the_scan_stops_at_the_file_cap() { + let root = TempDir::default(); + for name in ["A.swift", "B.swift", "C.swift"] { + fs::write(root.join(name), "").unwrap(); + } + assert_eq!( + scan_sources(root.as_ref(), &HashSet::new(), 2, u64::MAX).len(), + 2 + ); + } + + // The size cap is on the scan rather than the read, so an outsized file never reaches + // `read_to_string` at all -- and the cap is a ceiling, not a rounding. + #[test] + fn the_scan_skips_a_file_over_the_byte_cap() { + let root = TempDir::default(); + fs::write(root.join("Small.swift"), "ab").unwrap(); + fs::write(root.join("Big.swift"), "abcd").unwrap(); + + let scanned = |max_file_bytes| { + scan_sources(root.as_ref(), &HashSet::new(), 10, max_file_bytes) + .iter() + .map(|path| path.file_name().unwrap().to_string_lossy().to_string()) + .collect::>() + }; + + assert_eq!(scanned(2), vec![String::from("Small.swift")]); + assert_eq!(scanned(4).len(), 2, "a file exactly at the cap is parsed"); + } + + fn site(file: &str) -> DeclarationSite { + DeclarationSite { + file: ReportedPath::new(file), + line: None, + } + } + + fn recorded(target: Option<&str>, files: [&str; 2]) -> String { + let test_key = key(Some("FooTests"), "testThing"); + let mut index = TestLocationIndex { + targets: target + .map(|target| HashMap::from([(test_key.clone(), String::from(target))])) + .unwrap_or_default(), + ..Default::default() + }; + for file in files { + index.record(test_key.clone(), site(file)); + } + index.lookup(&test_key).unwrap().file.as_str().to_owned() + } + + const IN_TARGET: &str = "/repo/Tests/AppTests/FooTests.swift"; + const OTHER_TARGET: &str = "/repo/Tests/LibTests/FooTests.swift"; + + // Two modules can declare the same `(suite, case)`, and the scan order that decides it is + // arbitrary, so the target the test actually ran under has to break the tie. + #[rstest] + #[case::target_scanned_second(Some("AppTests"), [OTHER_TARGET, IN_TARGET], IN_TARGET)] + #[case::target_scanned_first(Some("AppTests"), [IN_TARGET, OTHER_TARGET], IN_TARGET)] + #[case::no_target_keeps_the_first(None, [OTHER_TARGET, IN_TARGET], OTHER_TARGET)] + #[case::no_candidate_matches(Some("TestsNobodyHas"), [OTHER_TARGET, IN_TARGET], OTHER_TARGET)] + fn a_collision_resolves_to_the_target_that_ran_the_test( + #[case] target: Option<&str>, + #[case] files: [&str; 2], + #[case] expected: &str, + ) { + assert_eq!(recorded(target, files), expected); + } + + #[rstest] + #[case::top_level_function("MyCLITests", "helloworld()", None, "helloworld")] + #[case::in_a_suite("MyCLITests.AlphaSuite", "shared()", Some("AlphaSuite"), "shared")] + #[case::nested_suite("MyCLITests.AlphaSuite.Inner", "deep()", Some("Inner"), "deep")] + #[case::other_suite_same_case("MyCLITests.BetaSuite", "shared()", Some("BetaSuite"), "shared")] + #[case::parameterized( + "MyCLITests.ParamSuite", + "squares(n:)", + Some("ParamSuite"), + "squares(n:" + )] + #[case::objc_style( + "MyCLITests.LegacyXCTests", + "testOldStyle", + Some("LegacyXCTests"), + "testOldStyle" + )] + fn a_junit_classname_names_a_suite_and_a_case( + #[case] classname: &str, + #[case] name: &str, + #[case] suite: Option<&str>, + #[case] case: &str, + ) { + assert_eq!( + TestKey::from_junit_classname(classname, name), + key(suite, case) + ); + } + + #[rstest] + #[case::with_suite("MyCLITests.AlphaSuite", Some("MyCLITests"))] + #[case::bare_target("MyCLITests", Some("MyCLITests"))] + #[case::empty("", None)] + fn a_junit_classname_names_the_target(#[case] classname: &str, #[case] expected: Option<&str>) { + assert_eq!( + TestKey::target_from_junit_classname(classname).as_deref(), + expected + ); + } + + // The two build `suite` from different places — an identifier's second-to-last component + // versus a classname's innermost — so nothing else catches them drifting apart. + #[rstest] + #[case::top_level("helloworld()", "MyCLITests", "helloworld()")] + #[case::in_a_suite("AlphaSuite/shared()", "MyCLITests.AlphaSuite", "shared()")] + #[case::nested_suite( + "OuterSuite/InnerSuite/deep()", + "MyCLITests.OuterSuite.InnerSuite", + "deep()" + )] + #[case::parameterized("ParamSuite/squares(n:)", "MyCLITests.ParamSuite", "squares(n:)")] + #[case::no_argument_overload("OverloadSuite/check()", "MyCLITests.OverloadSuite", "check()")] + #[case::labelled_overload("OverloadSuite/check(a:)", "MyCLITests.OverloadSuite", "check(a:)")] + #[case::swift_xctest_method( + "LegacyXCTests/testOldStyle()", + "MyCLITests.LegacyXCTests", + "testOldStyle" + )] + #[case::objc_xctest_method( + "ObjcXCTestTests/testFailsInsideSharedHelper", + "ObjcXCTestTests.ObjcXCTestTests", + "testFailsInsideSharedHelper" + )] + fn an_xcresult_identifier_and_a_junit_classname_key_alike( + #[case] node_identifier: &str, + #[case] classname: &str, + #[case] name: &str, + ) { + assert_eq!( + TestKey::from_node_identifier(node_identifier), + TestKey::from_junit_classname(classname, name) + ); + } + + // Different fields, and the collision tie-break depends on them agreeing. + #[rstest] + #[case::in_a_suite( + "test://com.apple.xcode/MyCLI/MyCLITests/AlphaSuite/shared()", + "MyCLITests.AlphaSuite" + )] + #[case::top_level("test://com.apple.xcode/MyCLI/MyCLITests/helloworld()", "MyCLITests")] + fn an_xcresult_url_and_a_junit_classname_name_the_same_target( + #[case] identifier_url: &str, + #[case] classname: &str, + ) { + assert_eq!( + TestKey::target_from_identifier_url(identifier_url), + TestKey::target_from_junit_classname(classname) + ); + } +} diff --git a/xcresult/src/xcresult.rs b/xcresult/src/xcresult.rs index 18335f1e..84c3e5f2 100644 --- a/xcresult/src/xcresult.rs +++ b/xcresult/src/xcresult.rs @@ -1,24 +1,51 @@ use std::collections::HashMap; use std::str; +use std::sync::atomic::{AtomicUsize, Ordering}; use std::{fs, path::Path, time::Duration}; use chrono::{DateTime, Utc}; use quick_junit::{NonSuccessKind, Report, TestCase, TestCaseStatus, TestRerun, TestSuite}; +use crate::test_locations::{Limits, TestKey, TestLocationIndex}; use crate::types::{ SWIFT_DEFAULT_TEST_SUITE_NAME, schema::{TestNode, TestNodeType, TestResult, Tests}, }; use crate::xcresult_legacy::XCResultTestLegacy; -use crate::xcrun::{xcresulttool_get_object, xcresulttool_get_test_results_tests}; +use crate::xcrun::{ + xcresulttool_get_object, xcresulttool_get_test_results_summary, + xcresulttool_get_test_results_tests, +}; + +// `xcresulttool` migrates a bundle that predates `database.sqlite3` in place the first time +// it is read, so reading one writes into a directory we were only asked to read — and fails +// where it is not writable. Every bundle Xcode writes today already carries the file and is +// unaffected; copying every bundle to avoid the older case costs more than the case is worth. +// `tests/bundle_reading.rs` pins both halves of that. + +/// Where a test's file comes from — where a failure surfaced, or where the test is +/// declared — which also decides which `xcresulttool` calls the bundle is read with. +#[derive(Debug)] +pub enum FileAttribution { + FailureSummaries(HashMap), + Declarations(TestLocationIndex), +} -#[derive(Debug, Clone)] +/// Makes it visible how many tests the checkout could not account for. +#[derive(Debug, Default)] +struct AttributionCounts { + declared: AtomicUsize, + unresolved: AtomicUsize, +} + +#[derive(Debug)] pub struct XCResult { tests: Tests, org_url_slug: String, repo_full_name: String, - legacy_xcresult_tests: HashMap, + attribution: FileAttribution, test_run_started_at: Option>, + counts: AttributionCounts, } impl XCResult { @@ -95,15 +122,69 @@ impl XCResult { }; Ok(XCResult { tests: xcresulttool_get_test_results_tests(&absolute_path)?, - legacy_xcresult_tests, + attribution: FileAttribution::FailureSummaries(legacy_xcresult_tests), + org_url_slug, + repo_full_name, + test_run_started_at, + counts: AttributionCounts::default(), + }) + } + + /// Read the bundle without a single `get object --legacy` call, taking each test's file + /// from where it is declared in `repo_root`. Beyond attribution, that is what the flag + /// buys: the legacy per-test summary fetch is unbounded (48 GB peak on one timed-out + /// test) and nothing here can reach that object. + pub fn new_with_declaration_locations, U: AsRef>( + path: T, + org_url_slug: String, + repo_full_name: String, + repo_root: U, + limits: Limits, + ) -> anyhow::Result { + let absolute_path = fs::canonicalize(path.as_ref()).map_err(|e| { + anyhow::anyhow!( + "failed to get absolute path for {}: {}", + path.as_ref().display(), + e + ) + })?; + let tests = xcresulttool_get_test_results_tests(&absolute_path)?; + + let test_run_started_at = match xcresulttool_get_test_results_summary(&absolute_path) { + Ok(summary) => summary.start_time.and_then(|start_time| { + // This float's ULP exceeds a microsecond at epoch magnitudes, so anything + // below the millisecond the legacy date string also carries is noise. + DateTime::from_timestamp_millis((start_time * 1e3).round() as i64) + }), + Err(e) => { + tracing::warn!("Failed to get test run start time from xcresult: {}", e); + None + } + }; + + let mut keys = Vec::new(); + collect_test_keys(&tests.test_nodes, &mut keys); + let index = TestLocationIndex::resolve(repo_root.as_ref(), &keys, limits); + if index.is_empty() { + tracing::warn!( + "no test declarations found under {}; falling back to failure locations", + repo_root.as_ref().display() + ); + } + + Ok(XCResult { + tests, + attribution: FileAttribution::Declarations(index), org_url_slug, repo_full_name, test_run_started_at, + counts: AttributionCounts::default(), }) } pub fn generate_junits(&self) -> Vec { - self.tests + let reports: Vec = self + .tests .test_nodes .iter() .filter(|tn| matches!(tn.node_type, TestNodeType::TestPlan)) @@ -114,7 +195,15 @@ impl XCResult { )); report }) - .collect() + .collect(); + if matches!(self.attribution, FileAttribution::Declarations(_)) { + tracing::info!( + "xcresult test files: {} from a declaration, {} with no declaration found", + self.counts.declared.load(Ordering::Relaxed), + self.counts.unresolved.load(Ordering::Relaxed), + ); + } + reports } fn xcresult_test_bundles_and_suites_to_junit_test_suites( @@ -135,12 +224,7 @@ impl XCResult { ) } else if matches!(test_bundle_or_test_suite.node_type, TestNodeType::TestSuite) { let test_suite = test_bundle_or_test_suite; - vec![ - self.xcresult_test_suite_to_junit_test_suite( - test_suite, - Option::<&str>::None, - ), - ] + self.xcresult_test_suite_to_junit_test_suites(test_suite, None) } else { vec![] } @@ -153,11 +237,12 @@ impl XCResult { test_nodes: &[TestNode], bundle_name: Option, ) -> Vec { + let qualifier = bundle_name.as_ref().map(|bn| bn.as_ref()); let mut test_suites = test_nodes .iter() .filter(|tn| matches!(tn.node_type, TestNodeType::TestSuite)) - .map(|test_suite| { - self.xcresult_test_suite_to_junit_test_suite(test_suite, bundle_name.as_ref()) + .flat_map(|test_suite| { + self.xcresult_test_suite_to_junit_test_suites(test_suite, qualifier) }) .collect::>(); // test cases can be at the top level @@ -178,20 +263,31 @@ impl XCResult { test_suites } - fn xcresult_test_suite_to_junit_test_suite>( + /// A suite and, flattened after it, every suite nested inside it — JUnit has no nested + /// ``, and emitting only the outer one drops the tests the inner ones declare. + fn xcresult_test_suite_to_junit_test_suites( &self, xcresult_test_suite: &TestNode, - bundle_name: Option, - ) -> TestSuite { - let name = bundle_name - .as_ref() - .map(|bn| format!("{}.{}", bn.as_ref(), xcresult_test_suite.name)) + qualifier: Option<&str>, + ) -> Vec { + let name = qualifier + .map(|qualifier| format!("{}.{}", qualifier, xcresult_test_suite.name)) .unwrap_or_else(|| String::from(&xcresult_test_suite.name)); - let mut test_suite = TestSuite::new(name); + let mut test_suite = TestSuite::new(name.clone()); test_suite.add_test_cases( self.xcresult_test_cases_to_junit_test_cases(xcresult_test_suite.children.as_slice()), ); - test_suite + let mut test_suites = vec![test_suite]; + test_suites.extend( + xcresult_test_suite + .children + .iter() + .filter(|tn| matches!(tn.node_type, TestNodeType::TestSuite)) + .flat_map(|nested| { + self.xcresult_test_suite_to_junit_test_suites(nested, Some(&name)) + }), + ); + test_suites } fn xcresult_test_cases_to_junit_test_cases(&self, test_nodes: &[TestNode]) -> Vec { @@ -259,13 +355,11 @@ impl XCResult { test_case.set_timestamp(started_at); } - if let Some(node_identifier) = &xcresult_test_case.node_identifier { - let id = self.generate_id(node_identifier); + if let Some(id) = self.generate_id(xcresult_test_case) { test_case.extra.insert("id".into(), id.into()); - let file = self.find_test_case_file(node_identifier); - if let Some(file) = file { - test_case.extra.insert("file".into(), file.into()); - } + } + if let Some(file) = self.find_test_case_file(xcresult_test_case) { + test_case.extra.insert("file".into(), file.into()); } Some(test_case) @@ -326,35 +420,70 @@ impl XCResult { .and_then(|secs| Duration::try_from_secs_f64(secs).ok()) } - fn generate_id>(&self, raw_id: T) -> String { - let identifier_url = self - .legacy_xcresult_tests - .get(raw_id.as_ref()) - .map(|test| &test.identifier_url) - .map(|identifier_url| identifier_url.as_str()); - // join the org and repo name to the raw id and generate uuid v5 from it - uuid::Uuid::new_v5( - &uuid::Uuid::NAMESPACE_URL, - format!( - "{}#{}#{}", - &self.org_url_slug, - &self.repo_full_name, - identifier_url.unwrap_or(raw_id.as_ref()) + fn generate_id(&self, test_case: &TestNode) -> Option { + let node_identifier = test_case.node_identifier.as_deref()?; + let identifier_url = match &self.attribution { + FileAttribution::FailureSummaries(tests) => tests + .get(node_identifier) + .map(|test| test.identifier_url.as_str()), + // The legacy `identifierURL` under another name, so ids match across paths. + FileAttribution::Declarations(_) => test_case.node_identifier_url.as_deref(), + }; + Some( + uuid::Uuid::new_v5( + &uuid::Uuid::NAMESPACE_URL, + format!( + "{}#{}#{}", + &self.org_url_slug, + &self.repo_full_name, + identifier_url.unwrap_or(node_identifier) + ) + .as_bytes(), ) - .as_bytes(), + .to_string(), ) - .to_string() } - fn find_test_case_file>(&self, raw_id: T) -> Option { - if let Some(file) = self - .legacy_xcresult_tests - .get(raw_id.as_ref()) - .map(|test| &test.file) - .and_then(|file| file.as_ref()) + fn find_test_case_file(&self, test_case: &TestNode) -> Option { + let node_identifier = test_case.node_identifier.as_deref()?; + match &self.attribution { + FileAttribution::FailureSummaries(tests) => tests + .get(node_identifier) + .and_then(|test| test.file.clone()), + FileAttribution::Declarations(index) => { + if let Some(site) = index.lookup(&TestKey::from_node_identifier(node_identifier)) { + tracing::debug!( + "{} is declared at {}:{}", + node_identifier, + site.file.as_str(), + site.line.unwrap_or_default() + ); + self.counts.declared.fetch_add(1, Ordering::Relaxed); + return Some(site.file.as_str().to_owned()); + } + // Where a failure surfaced is not where the test is written, and reporting + // it hands the test to whoever owns that file. No file at all resolves no + // codeowners, which is recoverable; the wrong file is not. A test with no + // declaration to find is runtime-registered (Quick, `+testInvocations`). + self.counts.unresolved.fetch_add(1, Ordering::Relaxed); + tracing::debug!("no declaration in the checkout for {node_identifier}"); + None + } + } + } +} + +fn collect_test_keys(test_nodes: &[TestNode], keys: &mut Vec<(TestKey, Option)>) { + for test_node in test_nodes { + if matches!(test_node.node_type, TestNodeType::TestCase) + && let Some(node_identifier) = &test_node.node_identifier { - return Some(file.to_owned()); + let target = test_node + .node_identifier_url + .as_deref() + .and_then(TestKey::target_from_identifier_url); + keys.push((TestKey::from_node_identifier(node_identifier), target)); } - None + collect_test_keys(&test_node.children, keys); } } diff --git a/xcresult/src/xcrun.rs b/xcresult/src/xcrun.rs index 80286a22..ea5296b2 100644 --- a/xcresult/src/xcrun.rs +++ b/xcresult/src/xcrun.rs @@ -1,12 +1,94 @@ -use std::{ffi::OsStr, process::Command}; +use std::{ffi::OsStr, fs, path::Path, path::PathBuf, process::Command}; use lazy_static::lazy_static; +use serde::Deserialize; use crate::{ types::legacy_schema::{ActionTestPlanRunSummaries, ActionsInvocationRecord}, types::schema::Tests, }; +#[derive(Debug, Deserialize)] +pub struct TestResultsSummary { + /// Seconds since the Unix epoch. + #[serde(rename = "startTime")] + pub start_time: Option, +} + +pub fn xcresulttool_get_test_results_summary>( + path: T, +) -> anyhow::Result { + xcresulttool_min_version_check()?; + + let output = xcrun(&[ + "xcresulttool".as_ref(), + "get".as_ref(), + "test-results".as_ref(), + "summary".as_ref(), + "--path".as_ref(), + path.as_ref(), + ])?; + + serde_json::from_str::(&output) + .map_err(|e| anyhow::anyhow!("failed to parse json from xcresulttool output: {}", e)) +} + +/// `None` when `name` ships in neither Xcode nor the Command Line Tools. +/// Locate a developer tool. `xcrun` is the only way to find one inside an Xcode toolchain, +/// but on Linux the Swift toolchain puts `sourcekit-lsp` on `PATH` and there is no `xcrun`. +pub fn find_program(name: &str) -> Option { + if cfg!(target_os = "macos") + && let Some(path) = xcrun_find(name) + { + return Some(path); + } + which_program(name) +} + +fn which_program(name: &str) -> Option { + which_program_in(name, &std::env::var_os("PATH")?) +} + +fn which_program_in(name: &str, path_var: &OsStr) -> Option { + std::env::split_paths(path_var) + .map(|directory| directory.join(name)) + .find(|candidate| is_executable_file(candidate)) +} + +/// A non-executable file of the right name is not the program, and spawning it would fail +/// later with something far less obvious than "not found". +fn is_executable_file(path: &Path) -> bool { + let Ok(metadata) = fs::metadata(path) else { + return false; + }; + if !metadata.is_file() { + return false; + } + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + metadata.permissions().mode() & 0o111 != 0 + } + #[cfg(not(unix))] + true +} + +pub fn xcrun_find(name: &str) -> Option { + if !cfg!(target_os = "macos") { + return None; + } + let output = Command::new("xcrun").args(["--find", name]).output().ok()?; + if !output.status.success() { + return None; + } + let path = PathBuf::from(String::from_utf8(output.stdout).ok()?.trim()); + if path.as_os_str().is_empty() { + None + } else { + Some(path) + } +} + pub fn xcresulttool_get_test_results_tests>(path: T) -> anyhow::Result { xcresulttool_min_version_check()?; @@ -117,3 +199,43 @@ fn xcrun>(args: &[T]) -> anyhow::Result { let result = String::from_utf8(data)?; Ok(result) } + +#[cfg(test)] +mod tests { + use super::*; + + // The Linux Swift toolchain puts `sourcekit-lsp` on `PATH` with no `xcrun` to ask, so the + // fallback is the only way the declaration path can find a server there. + #[test] + fn a_program_on_path_is_found_without_xcrun() { + let temp_dir = tempfile::tempdir().unwrap(); + let elsewhere = tempfile::tempdir().unwrap(); + let program = temp_dir.path().join("pretend-lsp"); + fs::write(&program, b"#!/bin/sh\n").unwrap(); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + fs::set_permissions(&program, fs::Permissions::from_mode(0o755)).unwrap(); + } + let path_var = std::env::join_paths([elsewhere.path(), temp_dir.path()]).unwrap(); + + assert_eq!( + which_program_in("pretend-lsp", &path_var), + Some(program.clone()) + ); + assert_eq!(which_program_in("not-installed", &path_var), None); + } + + #[cfg(unix)] + #[test] + fn a_file_without_the_executable_bit_is_not_the_program() { + use std::os::unix::fs::PermissionsExt; + let temp_dir = tempfile::tempdir().unwrap(); + let program = temp_dir.path().join("pretend-lsp"); + fs::write(&program, b"not executable").unwrap(); + fs::set_permissions(&program, fs::Permissions::from_mode(0o644)).unwrap(); + let path_var = std::env::join_paths([temp_dir.path()]).unwrap(); + + assert_eq!(which_program_in("pretend-lsp", &path_var), None); + } +} diff --git a/xcresult/tests/bundle_reading.rs b/xcresult/tests/bundle_reading.rs new file mode 100644 index 00000000..e3dcdfa2 --- /dev/null +++ b/xcresult/tests/bundle_reading.rs @@ -0,0 +1,82 @@ +//! What reading a bundle does to it on disk, and what it needs from the filesystem. +//! +//! `xcresulttool` migrates a bundle that predates `database.sqlite3` in place the first +//! time it is read. Both halves of that are pinned here: the format Xcode writes today is +//! unaffected, and the older one is a known limitation rather than something we pay to +//! avoid on every upload. + +mod common; + +#[cfg(target_os = "macos")] +use common::{ORG_URL_SLUG, REPO_FULL_NAME, entries, set_writable, unpack_archive_to_temp_dir}; +#[cfg(target_os = "macos")] +use xcresult::xcresult::XCResult; + +// The case that matters now: a current bundle already carries `database.sqlite3`, so there +// is nothing to migrate and it is read where it lies. CI hands out artifact mounts without +// write access, so this has to hold without copying the bundle first. +#[cfg(target_os = "macos")] +#[test] +fn a_read_only_modern_bundle_is_readable_and_is_not_written_to() { + let temp_dir = unpack_archive_to_temp_dir("tests/data/test-inherited-test.xcresult.tar.gz"); + let bundle = temp_dir.as_ref().join("InheritedTest.xcresult"); + let before = entries(&bundle); + assert!( + before.iter().any(|entry| entry == "database.sqlite3"), + "the fixture must already be migrated for this to prove anything" + ); + + set_writable(&bundle, false); + let read_only_result = XCResult::new( + bundle.to_str().unwrap(), + ORG_URL_SLUG.clone(), + REPO_FULL_NAME.clone(), + false, + ) + .map(|xcresult| xcresult.generate_junits().len()); + set_writable(&bundle, true); + + assert_eq!( + read_only_result.map_err(|e| e.to_string()), + Ok(1), + "a read-only bundle in the current format must still be readable" + ); + pretty_assertions::assert_eq!( + entries(&bundle), + before, + "reading the bundle changed it on disk" + ); +} + +// The accepted limitation. A bundle predating `database.sqlite3` is migrated in place on +// first read, so a read-only one cannot be read at all. Copying every bundle to avoid this +// costs more than the case is worth, and no bundle Xcode writes today is in this format. +// If this ever starts passing, the migration behaviour changed and the note in +// `xcresult.rs` is stale. +#[cfg(target_os = "macos")] +#[test] +fn a_read_only_legacy_bundle_cannot_be_read() { + let temp_dir = unpack_archive_to_temp_dir("tests/data/test4.xcresult.tar.gz"); + let bundle = temp_dir.as_ref().join("test4.xcresult"); + assert!( + !entries(&bundle) + .iter() + .any(|entry| entry == "database.sqlite3"), + "the fixture must start un-migrated for this to prove anything" + ); + + set_writable(&bundle, false); + let read_only_result = XCResult::new( + bundle.to_str().unwrap(), + ORG_URL_SLUG.clone(), + REPO_FULL_NAME.clone(), + false, + ) + .map(|xcresult| xcresult.generate_junits().len()); + set_writable(&bundle, true); + + assert!( + read_only_result.is_err(), + "a read-only bundle in the older format is expected to fail the in-place migration" + ); +} diff --git a/xcresult/tests/common/mod.rs b/xcresult/tests/common/mod.rs new file mode 100644 index 00000000..72f6be4d --- /dev/null +++ b/xcresult/tests/common/mod.rs @@ -0,0 +1,250 @@ +//! Harness shared by the integration test binaries in this directory. +//! +//! This module is compiled into each of them separately, so anything one binary does not +//! call is dead code from that binary's point of view — hence the blanket allow. +//! +//! Each file under `tests/` is its own crate, so anything two of them need lives here +//! rather than being written twice. `tests/common/mod.rs` is a module rather than +//! `tests/common.rs`, which cargo would build and run as a third test binary. + +#![allow(dead_code)] + +use std::{fs::File, path::Path}; + +use context::repo::RepoUrlParts; +use flate2::read::GzDecoder; +use lazy_static::lazy_static; +use tar::Archive; +use temp_testdir::TempDir; +#[cfg(target_os = "macos")] +use xcresult::{test_locations::Limits, xcresult::XCResult}; + +/// 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`. +pub fn unpack_archive_to_temp_dir>(archive_file_path: T) -> TempDir { + let file = File::open(archive_file_path).unwrap(); + let decoder = GzDecoder::new(file); + let mut archive = Archive::new(decoder); + let temp_dir = TempDir::default(); + if let Err(e) = archive.unpack(temp_dir.as_ref()) { + panic!("failed to unpack data.tar.gz: {}", e); + } + temp_dir +} + +lazy_static! { + pub static ref ORG_URL_SLUG: String = String::from("trunk"); + pub static ref REPO_FULL_NAME: String = RepoUrlParts { + host: "github.com".to_string(), + owner: "trunk-io".to_string(), + name: "analytics-cli".to_string() + } + .repo_full_name(); +} + +/// Read a bundle the way `--use-experimental-xcresult-test-locations` does: each test's +/// file comes from where it is declared in `repo_root`, not from a failure. +#[cfg(target_os = "macos")] +pub fn declaration_report, U: AsRef>( + bundle_path: T, + repo_root: U, +) -> quick_junit::Report { + let xcresult = XCResult::new_with_declaration_locations( + bundle_path.as_ref().to_str().unwrap(), + ORG_URL_SLUG.clone(), + REPO_FULL_NAME.clone(), + repo_root.as_ref(), + Limits::default(), + ) + .expect("the declaration path reads the bundle"); + + let mut junits = xcresult.generate_junits(); + assert_eq!(junits.len(), 1); + junits.pop().unwrap() +} + +/// Case name -> reported file. Only safe where case names are unique across the bundle; +/// where two suites run a case of the same name, key on the suite as well. +#[cfg(target_os = "macos")] +pub fn declaration_files, U: AsRef>( + bundle_path: T, + repo_root: U, +) -> std::collections::HashMap { + declaration_report(bundle_path, repo_root) + .test_suites + .iter() + .flat_map(|test_suite| test_suite.test_cases.iter()) + .map(|test_case| { + let file = test_case + .extra + .iter() + .find(|(key, _)| key.as_str() == "file") + .map(|(_, value)| value.as_str().to_owned()) + .unwrap_or_default(); + (test_case.name.as_str().to_owned(), file) + }) + .collect() +} + +/// Assert the declaration flag changes a bundle's reported `file` and nothing else. +/// +/// Everything except `file` — suite and case names, ids, statuses, timestamps — has to +/// come out identical on both paths, because the flag is only meant to move the file. +/// `repo_root` of `None` means an empty checkout, where nothing resolves. +#[cfg(target_os = "macos")] +pub fn assert_the_declaration_flag_moves_only_the_file( + archive: &str, + bundle: &str, + repo_root: Option<&str>, +) { + /// Everything the flag must not change: which suites and cases exist, their ids, + /// statuses and timestamps. Returned field-wise so the guard below can look at the id + /// and timestamp themselves rather than at their rendering. + fn shape( + xcresult: &xcresult::xcresult::XCResult, + ) -> Vec<(String, String, String, String, String)> { + let mut junits = xcresult.generate_junits(); + assert_eq!(junits.len(), 1); + let junit = junits.pop().unwrap(); + let mut rows = Vec::new(); + for test_suite in &junit.test_suites { + for test_case in &test_suite.test_cases { + let id = test_case + .extra + .iter() + .find(|(key, _)| key.as_str() == "id") + .map(|(_, value)| value.as_str().to_owned()) + .unwrap_or_default(); + rows.push(( + test_suite.name.as_str().to_owned(), + test_case.name.as_str().to_owned(), + id, + format!("{:?}", test_case.status), + test_case + .timestamp + .map(|timestamp| timestamp.to_string()) + .unwrap_or_default(), + )); + } + } + rows + } + + let temp_dir = unpack_archive_to_temp_dir(archive); + let bundle_path = temp_dir.as_ref().join(bundle); + let path_str = bundle_path.to_str().unwrap(); + let empty_checkout = TempDir::default(); + let root: &Path = match repo_root { + Some(repo_root) => Path::new(repo_root), + None => empty_checkout.as_ref(), + }; + + let default = XCResult::new( + path_str, + ORG_URL_SLUG.clone(), + REPO_FULL_NAME.clone(), + false, + ) + .expect("the default path reads the bundle"); + let declarations = XCResult::new_with_declaration_locations( + path_str, + ORG_URL_SLUG.clone(), + REPO_FULL_NAME.clone(), + root, + Limits::default(), + ) + .expect("the declaration path reads the bundle"); + + let expected = shape(&default); + assert!(!expected.is_empty(), "the bundle must have test cases"); + // Without this the comparison passes vacuously: two paths that both emit an empty id + // or timestamp are equal. `nodeIdentifierURL` going missing would silently re-identify + // every xcresult test case in the product, and a `startTime` read against the wrong + // epoch would land every timestamp three decades off — both are quiet failures. + assert!( + expected + .iter() + .all(|(_, _, id, _, timestamp)| !id.is_empty() && !timestamp.is_empty()), + "the bundle must carry an id and a timestamp on every case for this to prove anything" + ); + pretty_assertions::assert_eq!(shape(&declarations), expected); + + for file in declaration_files(&bundle_path, root).values() { + assert!( + !["/.build/", "/checkouts/", "/DerivedData/"] + .iter() + .any(|segment| file.contains(segment)), + "the declaration path reported a vendored file: {file}" + ); + } +} + +/// Every path under `dir`, relative to it, sorted — so two listings can be compared to +/// prove a read left the directory alone. +pub fn entries(dir: &Path) -> Vec { + let mut found = Vec::new(); + let mut stack = vec![dir.to_path_buf()]; + while let Some(current) = stack.pop() { + for entry in std::fs::read_dir(¤t).unwrap() { + let path = entry.unwrap().path(); + found.push(path.strip_prefix(dir).unwrap().display().to_string()); + if path.is_dir() { + stack.push(path); + } + } + } + found.sort(); + found +} + +/// Make `dir` and everything under it writable or not, for proving a read does not need +/// write access — CI hands out artifact mounts that do not have it. +pub fn set_writable(dir: &Path, writable: bool) { + let mut stack = vec![dir.to_path_buf()]; + let mut all = vec![dir.to_path_buf()]; + while let Some(current) = stack.pop() { + for entry in std::fs::read_dir(¤t).unwrap() { + let path = entry.unwrap().path(); + if path.is_dir() { + stack.push(path.clone()); + } + all.push(path); + } + } + // Directories have to come last on the way down and first on the way back up. + all.sort(); + if !writable { + all.reverse(); + } + for path in all { + let mode = if writable { 0o755 } else { 0o555 }; + std::fs::set_permissions(&path, std::os::unix::fs::PermissionsExt::from_mode(mode)) + .unwrap(); + } +} + +// available: only the experimental path reads the per-test failure summary, and so +// the call stack, so it is the only one that can produce a `FileSource::TestFrame`. +// The legacy path sees `FileSource::DocumentLocation` alone. +#[cfg(target_os = "macos")] +pub fn assert_junit>( + bundle_path: T, + use_experimental_failure_summary: bool, + expected_junit_xml: &str, +) { + let path_str = bundle_path.as_ref().to_str().unwrap(); + let xcresult = XCResult::new( + path_str, + ORG_URL_SLUG.clone(), + REPO_FULL_NAME.clone(), + use_experimental_failure_summary, + ); + assert!(xcresult.is_ok()); + + let mut junits = xcresult.unwrap().generate_junits(); + assert_eq!(junits.len(), 1); + let junit = junits.pop().unwrap(); + let mut junit_writer: Vec = Vec::new(); + junit.serialize(&mut junit_writer).unwrap(); + pretty_assertions::assert_eq!(String::from_utf8(junit_writer).unwrap(), expected_junit_xml); +} diff --git a/xcresult/tests/data/swift-test-parity.xcresult.tar.gz b/xcresult/tests/data/swift-test-parity.xcresult.tar.gz new file mode 100644 index 00000000..274f2484 Binary files /dev/null and b/xcresult/tests/data/swift-test-parity.xcresult.tar.gz differ diff --git a/xcresult/tests/data/swift-test-xunit-xctest.junit.xml b/xcresult/tests/data/swift-test-xunit-xctest.junit.xml new file mode 100644 index 00000000..c243c073 --- /dev/null +++ b/xcresult/tests/data/swift-test-xunit-xctest.junit.xml @@ -0,0 +1,14 @@ + + + + + + + + + + + + + + diff --git a/xcresult/tests/data/swift-test-xunit.junit.xml b/xcresult/tests/data/swift-test-xunit.junit.xml new file mode 100644 index 00000000..edd16d67 --- /dev/null +++ b/xcresult/tests/data/swift-test-xunit.junit.xml @@ -0,0 +1,14 @@ + + + + + + + + + + + + + + diff --git a/xcresult/tests/data/test-inherited-test.xcresult.tar.gz b/xcresult/tests/data/test-inherited-test.xcresult.tar.gz new file mode 100644 index 00000000..9e38e38e Binary files /dev/null and b/xcresult/tests/data/test-inherited-test.xcresult.tar.gz differ diff --git a/xcresult/tests/data/test-nested-and-passing.junit.xml b/xcresult/tests/data/test-nested-and-passing.junit.xml new file mode 100644 index 00000000..9e251dee --- /dev/null +++ b/xcresult/tests/data/test-nested-and-passing.junit.xml @@ -0,0 +1,18 @@ + + + + + + + + + + + + + + + + + + diff --git a/xcresult/tests/data/test-nested-and-passing.xcresult.tar.gz b/xcresult/tests/data/test-nested-and-passing.xcresult.tar.gz new file mode 100644 index 00000000..d573da16 Binary files /dev/null and b/xcresult/tests/data/test-nested-and-passing.xcresult.tar.gz differ diff --git a/xcresult/tests/data/test-objc-category.xcresult.tar.gz b/xcresult/tests/data/test-objc-category.xcresult.tar.gz new file mode 100644 index 00000000..b37977fa Binary files /dev/null and b/xcresult/tests/data/test-objc-category.xcresult.tar.gz differ diff --git a/xcresult/tests/declaration_locations.rs b/xcresult/tests/declaration_locations.rs new file mode 100644 index 00000000..505bcffc --- /dev/null +++ b/xcresult/tests/declaration_locations.rs @@ -0,0 +1,345 @@ +//! Where the declaration path attributes a test whose own suite does not declare it. +//! +//! The rest of the declaration-path coverage lives in `xcresult.rs` alongside the +//! failure-summary tests it is compared against. These two are separate because their +//! fixtures exist only to pin *which* declaration is reported when more than one file +//! could plausibly answer. + +mod common; + +use common::unpack_archive_to_temp_dir; +#[cfg(target_os = "macos")] +use common::{ + assert_junit, assert_the_declaration_flag_moves_only_the_file, declaration_files, + declaration_report, +}; +use lazy_static::lazy_static; +use rstest::rstest; +use temp_testdir::TempDir; +use xcresult::test_locations::TestKey; + +lazy_static! { + static ref TEMP_DIR_TEST_NESTED_AND_PASSING: TempDir = + unpack_archive_to_temp_dir("tests/data/test-nested-and-passing.xcresult.tar.gz"); + static ref TEMP_DIR_TEST_INHERITED_TEST: TempDir = + unpack_archive_to_temp_dir("tests/data/test-inherited-test.xcresult.tar.gz"); +} + +// XCTest runs a base class's `test*` method again under every concrete subclass, so the +// same method arrives twice under two different suites. Neither arrival is written in +// `ConcreteTests.swift` — that file declares no test at all — so both report the base +// class's file, which is where the method they ran is actually written. +// +// A suite cannot run a method it does not have, so `ConcreteTests` having no declaration of +// its own is itself the proof that it inherited one, and the single declaration of that name +// in the checkout is where it came from. +#[cfg(target_os = "macos")] +#[test] +fn test_an_inherited_test_is_attributed_to_the_class_that_declares_it() { + let report = common::declaration_report( + TEMP_DIR_TEST_INHERITED_TEST + .as_ref() + .join("InheritedTest.xcresult"), + "tests/fixture-src/inherited-test", + ); + // Both suites run a case of the same name, so the suite has to be part of the key. + let files = report + .test_suites + .iter() + .flat_map(|test_suite| { + test_suite.test_cases.iter().map(move |test_case| { + ( + format!("{}/{}", test_suite.name.as_str(), test_case.name.as_str()), + test_case + .extra + .iter() + .find(|(key, _)| key.as_str() == "file") + .map(|(_, value)| value.as_str().to_owned()), + ) + }) + }) + .collect::>(); + + // Both arrivals of the one method, so both name the file it is declared in. That the + // suite differs and the file does not is the whole point. + for (suite, expected) in [ + ("BaseTests", "BaseTests.swift"), + ("ConcreteTests", "BaseTests.swift"), + ] { + let key = format!("InheritedTestTests.{suite}/testInheritedFails()"); + let file = files + .get(&key) + .unwrap_or_else(|| panic!("{key} is missing from the report (found {files:?})")) + .as_deref() + .unwrap_or_else(|| panic!("{key} got no file from its declaration")); + assert!( + file.ends_with(expected), + "expected {key} to be attributed to {expected}, got {file}" + ); + } +} + +#[rstest] +#[case::plain( + "test://com.apple.xcode/InRepoHelper/InRepoHelperTests/Suite/case()", + Some("InRepoHelperTests") +)] +#[case::percent_encoded( + "test://com.apple.xcode/swift%20testing/swift%20testing%20exampleTests/helloWorld()", + Some("swift testing exampleTests") +)] +#[case::too_short("test://com.apple.xcode/OnlyAScheme", None)] +#[case::not_a_url("Suite/case()", None)] +fn an_identifier_url_names_the_target(#[case] url: &str, #[case] expected: Option<&str>) { + assert_eq!( + TestKey::target_from_identifier_url(url).as_deref(), + expected + ); +} + +// Every one of these bundles names some other file in its failure summary — a vendored +// checkout, an in-repo helper, or nothing at all — so each case is a shape where the +// declaration is the only source that can name the file the test is written in. +#[cfg(target_os = "macos")] +#[rstest] +#[case::vendored_dependency( + "tests/data/test-dependency-raises-failure.xcresult.tar.gz", + "DependencyRaisesFailure.xcresult", + "tests/fixture-src/dependency-raises-failure", + &[("failsInsideDependency()", "DependencyRaisesFailureTests.swift")] +)] +#[case::in_repo_helper( + "tests/data/test-in-repo-helper-raises-failure.xcresult.tar.gz", + "InRepoHelperRaisesFailure.xcresult", + "tests/fixture-src/in-repo-helper-raises-failure", + &[("failsInsideHelper()", "InRepoHelperRaisesFailureTests.swift")] +)] +// Neither test reaches its own frame, so no failure summary can serve either of them: +// one crashes inside the dependency, the other is failed by a trait after its frame is +// gone. `data/test-crash-in-dependency.junit.xml` reports no file for both. +#[case::crashed_and_torn_down( + "tests/data/test-crash-in-dependency.xcresult.tar.gz", + "CrashInDependency.xcresult", + "tests/fixture-src/crash-in-dependency", + &[ + ("testCrashesInsideDependency()", "CrashInDependencyTests.swift"), + ("failsAfterItsOwnFrameIsGone()", "TeardownFailureTests.swift"), + ] +)] +#[case::objc_through_clangd( + "tests/data/test-objc-xctest.xcresult.tar.gz", + "ObjcXCTest.xcresult", + "tests/fixture-src/objc-xctest", + &[("testFailsInsideSharedHelper", "ObjcXCTestTests.m")] +)] +#[case::top_level_swift_testing_function( + "tests/data/test-toplevel-swift-testing.xcresult.tar.gz", + "ToplevelSwiftTesting.xcresult", + "tests/fixture-src/toplevel-swift-testing", + &[("failsInsideHelperWithoutASuite()", "ToplevelSwiftTestingTests.swift")] +)] +// A category's `documentSymbol` container is `ObjcCategoryTests(Extra)`, and the class's +// own file declares no tests at all — so unless that name is read back as the class it +// extends, the declaration is never matched and the file falls to the class's file. +#[case::declared_in_an_objc_category( + "tests/data/test-objc-category.xcresult.tar.gz", + "ObjcCategory.xcresult", + "tests/fixture-src/objc-category", + &[("testDeclaredInACategory", "ObjcCategoryTests+Extra.m")] +)] +fn test_a_declaration_names_the_file_the_test_is_written_in( + #[case] archive: &str, + #[case] bundle: &str, + #[case] repo_root: &str, + #[case] expected: &[(&str, &str)], +) { + let temp_dir = unpack_archive_to_temp_dir(archive); + let files = declaration_files(temp_dir.as_ref().join(bundle), repo_root); + for (name, suffix) in expected { + let file = files + .get(*name) + .unwrap_or_else(|| panic!("{name} is missing from the report (found {files:?})")); + assert!( + file.ends_with(suffix), + "expected {name} to resolve to {suffix}, got {file}" + ); + } +} + +// A checkout that declares none of the tests. Where a failure surfaced is not where the +// test is written, so with nothing to resolve the reported file is absent rather than the +// helper the failure came from — that path resolves the wrong codeowners. +#[cfg(target_os = "macos")] +#[test] +fn test_a_test_with_no_declaration_in_the_checkout_gets_no_file() { + let empty_checkout = TempDir::default(); + let report = common::declaration_report( + TEMP_DIR_TEST_INHERITED_TEST + .as_ref() + .join("InheritedTest.xcresult"), + empty_checkout.as_ref(), + ); + let cases = report + .test_suites + .iter() + .flat_map(|test_suite| test_suite.test_cases.iter()) + .collect::>(); + assert!( + !cases.is_empty(), + "the bundle must still emit its test cases" + ); + for test_case in cases { + assert_eq!( + test_case + .extra + .iter() + .find(|(key, _)| key.as_str() == "file") + .map(|(_, value)| value.as_str()), + None, + "{} was given a file with nothing declaring it", + test_case.name.as_str() + ); + } +} + +// Before the fix this bundle emitted tests="2" failures="0" — the inner suite's two tests +// and its failure all vanished. +#[cfg(target_os = "macos")] +#[rstest] +#[case::experimental_failure_summary(true)] +#[case::legacy_fallback(false)] +fn test_a_nested_suite_is_flattened_rather_than_dropped( + #[case] use_experimental_failure_summary: bool, +) { + assert_junit( + TEMP_DIR_TEST_NESTED_AND_PASSING + .as_ref() + .join("NestedAndPassing.xcresult"), + use_experimental_failure_summary, + include_str!("data/test-nested-and-passing.junit.xml"), + ); +} + +// Three of these four passed, so no failure summary names a file for any of them. The +// status is asserted too, or the fixture could drift to all-failing and still pass here. +#[cfg(target_os = "macos")] +#[test] +fn test_declaration_locations_give_a_passing_test_its_file() { + let report = declaration_report( + TEMP_DIR_TEST_NESTED_AND_PASSING + .as_ref() + .join("NestedAndPassing.xcresult"), + "tests/fixture-src/nested-and-passing", + ); + let cases: std::collections::HashMap = report + .test_suites + .iter() + .flat_map(|test_suite| test_suite.test_cases.iter()) + .map(|test_case| (test_case.name.as_str().to_owned(), test_case)) + .collect(); + + for (name, passed, expected) in [ + ("outerPasses()", true, "NestedAndPassingTests.swift"), + ("topLevelPasses()", true, "NestedAndPassingTests.swift"), + ("innerPasses()", true, "InnerSuite.swift"), + ("innerFails()", false, "InnerSuite.swift"), + ] { + let test_case = cases + .get(name) + .unwrap_or_else(|| panic!("{name} is missing from the report")); + assert_eq!( + matches!( + test_case.status, + quick_junit::TestCaseStatus::Success { .. } + ), + passed, + "{name} did not have the status the fixture was captured for" + ); + let file = test_case + .extra + .iter() + .find(|(key, _)| key.as_str() == "file") + .map(|(_, value)| value.as_str()) + .unwrap_or_else(|| panic!("{name} got no file from its declaration")); + assert!( + file.ends_with(expected), + "expected {name} to be declared in {expected}, got {file}" + ); + } +} + +// The flag is meant to move the `file` attribute and leave everything else alone, so every +// bundle the suite reads is run both ways and compared on all of it but that. Each case +// unpacks its own copy so another test cannot perturb the comparison. +#[cfg(target_os = "macos")] +#[rstest] +#[case::simple("tests/data/test1.xcresult.tar.gz", "test1.xcresult", None)] +#[case::complex("tests/data/test4.xcresult.tar.gz", "test4.xcresult", None)] +#[case::expected_failures( + "tests/data/test-ExpectedFailures.xcresult.tar.gz", + "test-ExpectedFailures.xcresult", + None +)] +#[case::swift_mix( + "tests/data/test-swift-mix.xcresult.tar.gz", + "test-swift-mix.xcresult", + None +)] +#[case::swift_without_test_suites( + "tests/data/test-swift-without-test-suites.xcresult.tar.gz", + "test-swift-without-test-suites.xcresult", + None +)] +#[case::swift_snapshot_testing( + "tests/data/test-swift-snapshot-testing.xcresult.tar.gz", + "SnapshotRepro.xcresult", + None +)] +#[case::dependency_raises_failure( + "tests/data/test-dependency-raises-failure.xcresult.tar.gz", + "DependencyRaisesFailure.xcresult", + Some("tests/fixture-src/dependency-raises-failure") +)] +#[case::in_repo_helper_raises_failure( + "tests/data/test-in-repo-helper-raises-failure.xcresult.tar.gz", + "InRepoHelperRaisesFailure.xcresult", + Some("tests/fixture-src/in-repo-helper-raises-failure") +)] +#[case::crash_in_dependency( + "tests/data/test-crash-in-dependency.xcresult.tar.gz", + "CrashInDependency.xcresult", + Some("tests/fixture-src/crash-in-dependency") +)] +#[case::objc_xctest( + "tests/data/test-objc-xctest.xcresult.tar.gz", + "ObjcXCTest.xcresult", + Some("tests/fixture-src/objc-xctest") +)] +#[case::toplevel_swift_testing( + "tests/data/test-toplevel-swift-testing.xcresult.tar.gz", + "ToplevelSwiftTesting.xcresult", + Some("tests/fixture-src/toplevel-swift-testing") +)] +#[case::nested_and_passing( + "tests/data/test-nested-and-passing.xcresult.tar.gz", + "NestedAndPassing.xcresult", + Some("tests/fixture-src/nested-and-passing") +)] +#[case::timestamps("tests/data/test-timestamp.xcresult.tar.gz", "test1.xcresult", None)] +#[case::inherited_test( + "tests/data/test-inherited-test.xcresult.tar.gz", + "InheritedTest.xcresult", + Some("tests/fixture-src/inherited-test") +)] +#[case::objc_category( + "tests/data/test-objc-category.xcresult.tar.gz", + "ObjcCategory.xcresult", + Some("tests/fixture-src/objc-category") +)] +fn test_the_declaration_flag_moves_the_file_and_nothing_else( + #[case] archive: &str, + #[case] bundle: &str, + #[case] repo_root: Option<&str>, +) { + assert_the_declaration_flag_moves_only_the_file(archive, bundle, repo_root); +} diff --git a/xcresult/tests/file_attribution.rs b/xcresult/tests/file_attribution.rs new file mode 100644 index 00000000..f7389fc4 --- /dev/null +++ b/xcresult/tests/file_attribution.rs @@ -0,0 +1,196 @@ +//! `file_attribution`'s public surface: which file a failure summary offers, in which +//! order, and how a path is normalised on the way out. + +use rstest::rstest; +use serde_json::{Value, json}; +use xcresult::file_attribution::{FileCandidate, FileSource, ReportedPath, TestIdentity}; +use xcresult::types::legacy_schema; + +const SUITE: &str = "SnapshotReproTests"; +const CASE: &str = "failingSnapshot()"; + +fn xc_string(value: &str) -> Value { + json!({ "_value": value }) +} + +fn failure_summary( + file_name: Option<&str>, + location: Option<&str>, + stack: &[(&str, &str)], +) -> legacy_schema::ActionTestFailureSummary { + serde_json::from_value(json!({ + "fileName": file_name.map(xc_string), + "sourceCodeContext": { + "location": { "filePath": location.map(xc_string) }, + "callStack": { "_values": stack.iter().map(|(symbol, path)| json!({ + "symbolInfo": { + "symbolName": xc_string(symbol), + "location": { "filePath": xc_string(path) } + } + })).collect::>() } + } + })) + .unwrap() +} + +fn identity() -> TestIdentity<'static> { + TestIdentity { + suite: Some(SUITE), + case: CASE, + } +} + +#[rstest] +#[case::spaces_are_encoded("/repo/Tests/My Test.swift", "/repo/Tests/My%20Test.swift")] +#[case::already_safe("/repo/Tests/Test.swift", "/repo/Tests/Test.swift")] +fn reported_path_normalizes_once(#[case] path: &str, #[case] expected: &str) { + assert_eq!(ReportedPath::new(path).as_str(), expected); +} + +#[rstest] +#[case::tuist_checkout("/repo/Tuist/.build/checkouts/Dep/Dep.swift", true)] +#[case::derived_data("/repo/DerivedData/SourcePackages/checkouts/Dep/Dep.swift", true)] +#[case::the_repos_own_code("/repo/Tests/SnapshotReproTests.swift", false)] +fn reported_path_recognizes_vendored_sources(#[case] path: &str, #[case] expected: bool) { + assert_eq!(ReportedPath::new(path).is_vendored_dependency(), expected); +} + +#[rstest] +#[case::swift_symbol("SnapshotReproTests.failingSnapshot()", true)] +#[case::objc_symbol("-[SnapshotReproTests failingSnapshot]", true)] +#[case::closure_inside_test("closure #1 in SnapshotReproTests.failingSnapshot()", true)] +#[case::helper_the_test_called("assertSnapshot(of:as:)", false)] +#[case::same_case_name_in_another_suite("OtherTests.failingSnapshot()", false)] +#[case::trait_that_invoked_the_test( + "closure #1 in _SnapshotsTestTrait.provideScope(for:testCase:performing:)", + false +)] +fn identity_recognizes_only_the_tests_own_frame(#[case] symbol: &str, #[case] expected: bool) { + assert_eq!(identity().is_named_by(symbol), expected); +} + +#[rstest] +#[case::top_level_swift_testing_function("failingSnapshot()", true)] +#[case::closure_inside_it("closure #1 in failingSnapshot()", true)] +#[case::suite_scoped_symbol("SnapshotReproTests.failingSnapshot()", false)] +fn a_suiteless_test_is_matched_by_its_bare_function(#[case] symbol: &str, #[case] expected: bool) { + let identity = TestIdentity { + suite: None, + case: CASE, + }; + assert_eq!(identity.is_named_by(symbol), expected); +} + +#[test] +fn candidates_are_offered_in_preference_order_and_keep_their_provenance() { + let summary = failure_summary( + Some("/repo/Tests/Raised.swift"), + Some("/repo/Tests/Location.swift"), + &[ + ("helper()", "/repo/Tests/Inner.swift"), + ( + "SnapshotReproTests.failingSnapshot()", + "/repo/Tests/Own.swift", + ), + ("framework()", "/repo/Tests/Outer.swift"), + ], + ); + assert_eq!( + FileCandidate::from_failure_summary(&summary, &identity()) + .iter() + .map(|candidate| (candidate.path.as_str(), candidate.source)) + .collect::>(), + vec![ + ("/repo/Tests/Own.swift", FileSource::TestFrame), + ("/repo/Tests/Raised.swift", FileSource::RaisedFrom), + ("/repo/Tests/Location.swift", FileSource::SourceCodeLocation), + // Frames run innermost first, so they are offered outermost first. + ("/repo/Tests/Outer.swift", FileSource::LastStackFrame), + ("/repo/Tests/Own.swift", FileSource::LastStackFrame), + ("/repo/Tests/Inner.swift", FileSource::LastStackFrame), + ] + ); +} + +#[test] +fn a_summary_offering_nothing_yields_no_candidates() { + let summary = failure_summary(None, None, &[]); + assert!(FileCandidate::from_failure_summary(&summary, &identity()).is_empty()); +} + +#[rstest] +#[case::other_languages_skipped( + &[("a", "/repo/Tests/Real.swift"), ("b", "/repo/Tests/Generated.cc"), ("c", "/repo/Readme.md")], + vec!["/repo/Tests/Real.swift"] +)] +#[case::nothing_usable(&[("a", "/repo/Tests/Generated.cc")], vec![])] +fn only_swift_and_objc_frames_are_offered( + #[case] stack: &[(&str, &str)], + #[case] expected: Vec<&str>, +) { + // With no `fileName` and no location, every candidate offered is a stack frame, + // so this reaches the same filtering through the public entry point. + let summary = failure_summary(None, None, stack); + assert_eq!( + FileCandidate::from_failure_summary(&summary, &identity()) + .iter() + .inspect(|candidate| assert_eq!(candidate.source, FileSource::LastStackFrame)) + .map(|candidate| candidate.path.as_str()) + .collect::>(), + expected + ); +} + +// Apple declares `SortedKeyValueArrayPair.value` as `SchemaSerializable`, a type +// the format description never defines, so the generator cannot model it and drops +// the property. The data still carries it, and an object that is both missing the +// property and declared exhaustive fails to deserialize — which silently disabled +// the whole experimental path for any bundle with test attachments. +#[test] +fn a_summary_parses_despite_properties_the_schema_cannot_model() { + let summary: legacy_schema::ActionTestPlanRunSummaries = serde_json::from_value(json!({ + "failureSummaries": { "_values": [{ + "fileName": xc_string("/repo/Tests/SnapshotReproTests.swift"), + "attachments": { "_values": [{ + "userInfo": { "storage": { "_values": [{ + "_type": { "_name": "SortedKeyValueArrayPair" }, + "key": xc_string("Encoding"), + "value": xc_string("{ XCTImageEncodingCompressionQualityKey = 0.7; }") + }] } } + }] } + }] } + })) + .expect("a summary carrying attachment metadata must still deserialize"); + let failure_summary = &summary.failure_summaries.unwrap().values[0]; + assert_eq!( + FileCandidate::from_failure_summary(failure_summary, &identity()) + .first() + .map(|candidate| candidate.path.as_str().to_string()), + Some(String::from("/repo/Tests/SnapshotReproTests.swift")) + ); +} + +#[rstest] +#[case::scheme_and_fragment_stripped( + Some("file:///repo/Tests/Test.swift#EndingLineNumber=8"), + Some("/repo/Tests/Test.swift") +)] +#[case::spaces_encoded( + Some("file:///repo/Tests/My Test.swift"), + Some("/repo/Tests/My%20Test.swift") +)] +#[case::no_document_location(None, None)] +fn an_issue_summary_yields_a_cleaned_document_location( + #[case] url: Option<&str>, + #[case] expected: Option<&str>, +) { + let summary = serde_json::from_value(json!({ + "documentLocationInCreatingWorkspace": { "url": url.map(xc_string) } + })) + .unwrap(); + let candidate = FileCandidate::from_issue_summary(&summary); + assert_eq!(candidate.as_ref().map(|c| c.path.as_str()), expected); + if let Some(candidate) = candidate { + assert_eq!(candidate.source, FileSource::DocumentLocation); + } +} diff --git a/xcresult/tests/fixture-src/README.md b/xcresult/tests/fixture-src/README.md index 8838bd97..388fe44e 100644 --- a/xcresult/tests/fixture-src/README.md +++ b/xcresult/tests/fixture-src/README.md @@ -62,15 +62,21 @@ table below, and only then save the new output over `../data/test-*.junit.xml`. ## What each scenario must exhibit `verify-failure-summaries.py` enforces the "captured shape" column and fails the -regeneration if a bundle stops reproducing it. - -| Scenario | Captured shape | Expected `file` (experimental) | Expected `file` (legacy) | -| ------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------ | ------------------------ | -| `dependency-raises-failure` | A swift-testing test calls a dependency helper that records the issue at its own `#filePath`. `fileName`, the source code context's location and the innermost frame are all under `SourcePackages/checkouts/`; only the test's own frame names the test file. | the test's own file | _(none)_ | -| `in-repo-helper-raises-failure` | Same, with the helper in the test target. Nothing rejects the helper's path, so the test's own frame has to win on its own merits. | the test's own file | the helper's file | -| `crash-in-dependency` | Two tests that never reach their own frame: one `fatalError`s inside the dependency (Xcode records a summary with no file at all), and one is failed by the dependency's `TestScoping` trait after its body returned (every file source is a checkout path). | _(none)_ | _(none)_ | -| `objc-xctest` | An Objective-C `XCTestCase` whose failure is raised by `XCTFail` in a shared category in another file, so the frame is symbolicated as `-[ObjcXCTestTests testFailsInsideSharedHelper]`. | the test's own file | _(none)_ | -| `toplevel-swift-testing` | A top-level `@Test func` with no suite, failed by a helper in another file, so the frame is the bare function name. | the test's own file | the helper's file | +regeneration if a bundle stops reproducing it. `nested-and-passing`, `inherited-test` +and `objc-category` are checked by `verify-test-structure.py` instead: their shape is +the result tree itself — which suite ran which test — and no failure summary +describes that. + +| Scenario | Captured shape | Expected `file` (experimental) | Expected `file` (legacy) | +| ------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------- | ---------------------------------------------------------- | +| `dependency-raises-failure` | A swift-testing test calls a dependency helper that records the issue at its own `#filePath`. `fileName`, the source code context's location and the innermost frame are all under `SourcePackages/checkouts/`; only the test's own frame names the test file. | the test's own file | _(none)_ | +| `in-repo-helper-raises-failure` | Same, with the helper in the test target. Nothing rejects the helper's path, so the test's own frame has to win on its own merits. | the test's own file | the helper's file | +| `crash-in-dependency` | Two tests that never reach their own frame: one `fatalError`s inside the dependency (Xcode records a summary with no file at all), and one is failed by the dependency's `TestScoping` trait after its body returned (every file source is a checkout path). | _(none)_ | _(none)_ | +| `objc-xctest` | An Objective-C `XCTestCase` whose failure is raised by `XCTFail` in a shared category in another file, so the frame is symbolicated as `-[ObjcXCTestTests testFailsInsideSharedHelper]`. | the test's own file | _(none)_ | +| `toplevel-swift-testing` | A top-level `@Test func` with no suite, failed by a helper in another file, so the frame is the bare function name. | the test's own file | the helper's file | +| `nested-and-passing` | A `@Suite` nested inside another `@Suite`, declared in a separate file, plus three passing tests. Before the flattening fix the inner suite was never visited, so its two tests — and its failure — vanished from the JUnit. Passing tests have no failure summary, so only a declaration can name their file. | the file each test is declared in | the failing test's own file _(none for the passing three)_ | +| `inherited-test` | One `test*` method declared only on a base class, so XCTest runs it twice — as `BaseTests/testInheritedFails` and again as `ConcreteTests/testInheritedFails`. Both raise the failure at the same line of `BaseTests.swift`, which is what makes the bundle discriminate: the concrete suite's own file is named nowhere in the result. | the file of the suite that ran it | `BaseTests.swift` for both | +| `objc-category` | An Objective-C test method declared in a category rather than in its class's own file. `documentSymbol` names the container `ObjcCategoryTests(Extra)`, and unless that is read back as the class it extends the declaration is never matched at all. | the category's file | _(none)_ | The two columns differ because only the experimental path reads the failure summary's call stack, which is the only source that can identify the test's own diff --git a/xcresult/tests/fixture-src/inherited-test/Package.swift b/xcresult/tests/fixture-src/inherited-test/Package.swift new file mode 100644 index 00000000..e5296046 --- /dev/null +++ b/xcresult/tests/fixture-src/inherited-test/Package.swift @@ -0,0 +1,10 @@ +// swift-tools-version: 6.0 +import PackageDescription + +let package = Package( + name: "InheritedTest", + platforms: [.macOS(.v13)], + targets: [ + .testTarget(name: "InheritedTestTests") + ] +) diff --git a/xcresult/tests/fixture-src/inherited-test/Tests/InheritedTestTests/BaseTests.swift b/xcresult/tests/fixture-src/inherited-test/Tests/InheritedTestTests/BaseTests.swift new file mode 100644 index 00000000..0a078fab --- /dev/null +++ b/xcresult/tests/fixture-src/inherited-test/Tests/InheritedTestTests/BaseTests.swift @@ -0,0 +1,10 @@ +import XCTest + +// XCTest runs every `test*` method it finds on a concrete subclass, including the ones +// only this base class declares. So `testInheritedFails` runs twice: once as +// `BaseTests/testInheritedFails`, and once as `ConcreteTests/testInheritedFails`. +class BaseTests: XCTestCase { + func testInheritedFails() { + XCTFail("declared on the base class, and reported against this file under either suite") + } +} diff --git a/xcresult/tests/fixture-src/inherited-test/Tests/InheritedTestTests/ConcreteTests.swift b/xcresult/tests/fixture-src/inherited-test/Tests/InheritedTestTests/ConcreteTests.swift new file mode 100644 index 00000000..507857de --- /dev/null +++ b/xcresult/tests/fixture-src/inherited-test/Tests/InheritedTestTests/ConcreteTests.swift @@ -0,0 +1,6 @@ +import XCTest + +// Declares no tests of its own, which is what makes it worth capturing: the method it runs +// is written nowhere in this file, so `ConcreteTests/testInheritedFails` has to report +// `BaseTests.swift`. Attributing it here would name a file the test does not appear in. +final class ConcreteTests: BaseTests {} diff --git a/xcresult/tests/fixture-src/nested-and-passing/Package.swift b/xcresult/tests/fixture-src/nested-and-passing/Package.swift new file mode 100644 index 00000000..afa461bf --- /dev/null +++ b/xcresult/tests/fixture-src/nested-and-passing/Package.swift @@ -0,0 +1,10 @@ +// swift-tools-version: 6.0 +import PackageDescription + +let package = Package( + name: "NestedAndPassing", + platforms: [.macOS(.v13)], + targets: [ + .testTarget(name: "NestedAndPassingTests") + ] +) diff --git a/xcresult/tests/fixture-src/nested-and-passing/Tests/NestedAndPassingTests/InnerSuite.swift b/xcresult/tests/fixture-src/nested-and-passing/Tests/NestedAndPassingTests/InnerSuite.swift new file mode 100644 index 00000000..552fc60d --- /dev/null +++ b/xcresult/tests/fixture-src/nested-and-passing/Tests/NestedAndPassingTests/InnerSuite.swift @@ -0,0 +1,18 @@ +import Testing + +/// Declared in its own file, and nested inside `OuterSuite`, so a suite that is a +/// child of another suite has to be visited and its file resolved independently. +extension OuterSuite { + @Suite + struct InnerSuite { + @Test + func innerPasses() { + #expect(true) + } + + @Test + func innerFails() { + #expect(Bool(false), "deliberate failure inside a nested suite") + } + } +} diff --git a/xcresult/tests/fixture-src/nested-and-passing/Tests/NestedAndPassingTests/NestedAndPassingTests.swift b/xcresult/tests/fixture-src/nested-and-passing/Tests/NestedAndPassingTests/NestedAndPassingTests.swift new file mode 100644 index 00000000..66602c35 --- /dev/null +++ b/xcresult/tests/fixture-src/nested-and-passing/Tests/NestedAndPassingTests/NestedAndPassingTests.swift @@ -0,0 +1,14 @@ +import Testing + +@Suite +struct OuterSuite { + @Test + func outerPasses() { + #expect(1 + 1 == 2) + } +} + +@Test +func topLevelPasses() { + #expect(true) +} diff --git a/xcresult/tests/fixture-src/objc-category/Package.swift b/xcresult/tests/fixture-src/objc-category/Package.swift new file mode 100644 index 00000000..5b50db2f --- /dev/null +++ b/xcresult/tests/fixture-src/objc-category/Package.swift @@ -0,0 +1,10 @@ +// swift-tools-version: 6.0 +import PackageDescription + +let package = Package( + name: "ObjcCategory", + platforms: [.macOS(.v13)], + targets: [ + .testTarget(name: "ObjcCategoryTests") + ] +) diff --git a/xcresult/tests/fixture-src/objc-category/Tests/ObjcCategoryTests/ObjcCategoryTests+Extra.m b/xcresult/tests/fixture-src/objc-category/Tests/ObjcCategoryTests/ObjcCategoryTests+Extra.m new file mode 100644 index 00000000..0291c4eb --- /dev/null +++ b/xcresult/tests/fixture-src/objc-category/Tests/ObjcCategoryTests/ObjcCategoryTests+Extra.m @@ -0,0 +1,20 @@ +#import + +#import "include/ObjcCategoryTestsExtra.h" + +// The include is spelled relative to this file rather than relying on the `-I include` +// the build passes: `documentSymbol` is answered with no build settings at all, and a +// header clangd cannot open leaves the container named `<>(Extra)` — the +// class name is simply gone, and the declaration can never be matched. +// +// A category adds the method to `ObjcCategoryTests`, so the run reports it as +// `ObjcCategoryTests/testDeclaredInACategory` while it is declared here. +// `documentSymbol` names the container `ObjcCategoryTests(Extra)`, which has to be read +// back as the class it extends for the declaration to be found at all. +@implementation ObjcCategoryTests (Extra) + +- (void)testDeclaredInACategory { + XCTFail(@"declared in an Objective-C category, not in the class's own file"); +} + +@end diff --git a/xcresult/tests/fixture-src/objc-category/Tests/ObjcCategoryTests/ObjcCategoryTests.m b/xcresult/tests/fixture-src/objc-category/Tests/ObjcCategoryTests/ObjcCategoryTests.m new file mode 100644 index 00000000..b69710a8 --- /dev/null +++ b/xcresult/tests/fixture-src/objc-category/Tests/ObjcCategoryTests/ObjcCategoryTests.m @@ -0,0 +1,5 @@ +#import "include/ObjcCategoryTestsExtra.h" + +// Declares no tests. Every test on this suite comes from the category in the other file. +@implementation ObjcCategoryTests +@end diff --git a/xcresult/tests/fixture-src/objc-category/Tests/ObjcCategoryTests/include/ObjcCategoryTestsExtra.h b/xcresult/tests/fixture-src/objc-category/Tests/ObjcCategoryTests/include/ObjcCategoryTestsExtra.h new file mode 100644 index 00000000..ad500245 --- /dev/null +++ b/xcresult/tests/fixture-src/objc-category/Tests/ObjcCategoryTests/include/ObjcCategoryTestsExtra.h @@ -0,0 +1,8 @@ +#import + +@interface ObjcCategoryTests : XCTestCase +@end + +@interface ObjcCategoryTests (Extra) +- (void)testDeclaredInACategory; +@end diff --git a/xcresult/tests/fixture-src/regenerate.sh b/xcresult/tests/fixture-src/regenerate.sh index b7a24583..0efd69fd 100755 --- a/xcresult/tests/fixture-src/regenerate.sh +++ b/xcresult/tests/fixture-src/regenerate.sh @@ -24,6 +24,9 @@ ALL_SCENARIOS=( crash-in-dependency objc-xctest toplevel-swift-testing + nested-and-passing + inherited-test + objc-category ) # scenario -> the package name, which is both the xcodebuild scheme prefix and the @@ -35,6 +38,9 @@ package_name() { crash-in-dependency) echo CrashInDependency ;; objc-xctest) echo ObjcXCTest ;; toplevel-swift-testing) echo ToplevelSwiftTesting ;; + nested-and-passing) echo NestedAndPassing ;; + inherited-test) echo InheritedTest ;; + objc-category) echo ObjcCategory ;; *) echo "unknown scenario: $1" >&2 exit 1 @@ -113,7 +119,12 @@ regenerate() { return 1 } - "${FIXTURE_SRC_DIR}/verify-failure-summaries.py" "${scenario}" "${dump}" + if [[ ${scenario} == nested-and-passing || ${scenario} == inherited-test || + ${scenario} == objc-category ]]; then + "${FIXTURE_SRC_DIR}/verify-test-structure.py" "${scenario}" "${bundle}" + else + "${FIXTURE_SRC_DIR}/verify-failure-summaries.py" "${scenario}" "${dump}" + fi tar -czf "${DATA_DIR}/test-${scenario}.xcresult.tar.gz" \ -C "${scenario_work_dir}" "${package}.xcresult" diff --git a/xcresult/tests/fixture-src/swift-test-xunit/Package.swift b/xcresult/tests/fixture-src/swift-test-xunit/Package.swift new file mode 100644 index 00000000..4dcb3cf4 --- /dev/null +++ b/xcresult/tests/fixture-src/swift-test-xunit/Package.swift @@ -0,0 +1,10 @@ +// swift-tools-version: 6.0 +import PackageDescription + +let package = Package( + name: "MyCLI", + targets: [ + .target(name: "MyCLI"), + .testTarget(name: "MyCLITests", dependencies: ["MyCLI"]), + ] +) diff --git a/xcresult/tests/fixture-src/swift-test-xunit/README.md b/xcresult/tests/fixture-src/swift-test-xunit/README.md new file mode 100644 index 00000000..1f5e16cf --- /dev/null +++ b/xcresult/tests/fixture-src/swift-test-xunit/README.md @@ -0,0 +1,103 @@ +# `swift test --xunit-output` fixture + +A SwiftPM package whose test target covers every shape `swift test --xunit-output` emits, +and the XML it produced, checked in as `../../data/swift-test-xunit.junit.xml`. + +Unlike the `.xcresult` scenarios next to this one, nothing here needs Xcode. `swift test` +produces no result bundle, `sourcekit-lsp` ships with the Swift toolchain on Linux, and the +xunit XML **contains no file path at all** — so a declaration is the only way to attribute +a test to a file on that platform. + +## Regenerating + +```sh +cp -R . /tmp/swift-test-xunit && cd /tmp/swift-test-xunit +swift test --parallel --xunit-output /tmp/out.xml +cp /tmp/out-swift-testing.xml ../../data/swift-test-xunit.junit.xml +cp /tmp/out.xml ../../data/swift-test-xunit-xctest.junit.xml +``` + +`--parallel` is **required**: without it `--xunit-output` emits nothing for XCTest, and only +the swift-testing file appears. + +Copy it out first: building in place leaves a `.build` directory inside the fixture, and the +declaration scan would then walk it (`.build` is in `SKIPPED_DIRECTORIES`, so it is ignored, +but it should not be committed either). + +Note that one run writes **two files**, and neither is named what you asked for in the +XCTest case being the only one at ``: + +| file | holds | +| -------------------------- | --------------------------------- | +| `-swift-testing.xml` | swift-testing (`@Test`, `@Suite`) | +| `` | XCTest (`XCTestCase` subclasses) | + +A project with both frameworks has to upload both. + +## What each shape proves + +| `classname` | `name` | declared in | why it is here | +| ----------------------------- | ---------------- | --------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------- | +| `MyCLITests` | `helloworld()` | `TopLevel.swift` | a top-level `@Test func` has no suite, so classname collapses to the bare target and only the suiteless lookup can find it | +| `MyCLITests.AlphaSuite` | `shared()` | `Suites.swift` | the ordinary case: innermost classname component is the declaring type | +| `MyCLITests.AlphaSuite.Inner` | `deep()` | `Suites.swift` | a nested suite is fully qualified, and only the innermost component declares the method | +| `MyCLITests.BetaSuite` | `shared()` | `BetaSuite.swift` | same case name as `AlphaSuite`'s in a **different file**, so collapsing the classname would make one borrow the other's file | +| `MyCLITests.ParamSuite` | `squares(n:)` | `Parameterized.swift` | a parameterised test keeps its argument labels and appears **once**, not once per argument, so the single entry is the declaration site | +| `MyCLITests.ParamSuite` | `pairs(s:flag:)` | `Parameterized.swift` | two labels, same shape | +| `MyCLITests.OverloadSuite` | `check()` | `OverloadA.swift` | the no-argument member of an overload set | +| `MyCLITests.OverloadSuite` | `check(a:)` | `OverloadA.swift` | differs from `check(b:)` **only by argument label** | +| `MyCLITests.OverloadSuite` | `check(b:)` | `OverloadB.swift` | declared in another file via an extension, so a normalisation that dropped labels would silently merge two distinct tests and give one the wrong file | + +## XCTest has no labels, so the class name carries the whole load + +XCTest test methods take no arguments, so there is nothing like `check(a:)` to separate two +of them — every one normalises to a bare method name. `BaseTests`, `ChildATests` and +`ChildBTests` all report a test called `testInherited`, and only the class name tells them +apart: + +| `classname` | `name` | declared in | why | +| ------------------------ | --------------- | ------------------- | --------------------------------------------------------------------------------------------------- | +| `MyCLITests.BaseTests` | `testInherited` | `BaseTests.swift` | declares it | +| `MyCLITests.ChildATests` | `testInherited` | `BaseTests.swift` | inherits it — resolved by walking `supertypes`, which the language server's superclass parse builds | +| `MyCLITests.ChildBTests` | `testInherited` | `ChildBTests.swift` | overrides it, so it is declared here and no chain walk is needed | + +That is the shape most XCTest suites actually have, and it is why `supertypes` exists. +Disabling the chain walk fails only the `inherited` case, which is the point. + +## XCTest needs `--parallel`, and needs no special handling + +`Legacy.swift` holds an `XCTestCase`, and it lands in `` rather than +`-swift-testing.xml`: + +```xml + +``` + +Which is the same `Module.Type` + method shape swift-testing uses, minus the `()` — so the +same parse resolves it, and `an_xctest_case_resolves_to_the_class_that_declares_it` proves +that against this package. + +Worth knowing: **without `--parallel` this file is not written at all**. The XCTest case runs +either way (the console reports `-[MyCLITests.LegacyXCTests testOldStyle] passed`), so a +project that omits `--parallel` silently uploads only its swift-testing results. + +## Parens and argument labels are part of a test's identity + +Both inputs report the same three overloads, and agree on how they spell them: + +| | `check()` | `check(a:)` | `check(b:)` | +| ------------------------- | ----------------------- | ------------------------- | ------------------------- | +| xcresult `nodeIdentifier` | `OverloadSuite/check()` | `OverloadSuite/check(a:)` | `OverloadSuite/check(b:)` | +| xunit `name` | `check()` | `check(a:)` | `check(b:)` | + +`normalized_case` trims only _trailing_ parens, so `check()` becomes `check` while +`check(a:)` becomes the unbalanced `check(a:`. Ugly 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_, not that it is tidy. Because labels survive it, the three +overloads key distinctly and resolve to their own declarations — including across files, +which `overloads_differing_only_by_argument_label_resolve_separately` pins. + +The one place the inputs genuinely disagree is an XCTest method: `xcodebuild` reports +`testOldStyle()` and `swift test` reports `testOldStyle`. Keying normalises that away so both +resolve to the same file, but the names differ in the uploaded JUnit — see +`the_two_inputs_spell_an_xctest_method_differently`. diff --git a/xcresult/tests/fixture-src/swift-test-xunit/Sources/MyCLI/MyCLI.swift b/xcresult/tests/fixture-src/swift-test-xunit/Sources/MyCLI/MyCLI.swift new file mode 100644 index 00000000..f233da78 --- /dev/null +++ b/xcresult/tests/fixture-src/swift-test-xunit/Sources/MyCLI/MyCLI.swift @@ -0,0 +1 @@ +public let answer = 42 diff --git a/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/BaseTests.swift b/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/BaseTests.swift new file mode 100644 index 00000000..e5b417db --- /dev/null +++ b/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/BaseTests.swift @@ -0,0 +1,5 @@ +import XCTest + +class BaseTests: XCTestCase { + func testInherited() { XCTAssertTrue(true) } +} diff --git a/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/BetaSuite.swift b/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/BetaSuite.swift new file mode 100644 index 00000000..2f9399da --- /dev/null +++ b/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/BetaSuite.swift @@ -0,0 +1,7 @@ +import Testing + +/// Declares a `shared()` too, in a different file, so the suite component of the JUnit +/// classname is the only thing that can tell the two apart. +@Suite struct BetaSuite { + @Test func shared() { #expect(true) } +} diff --git a/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/ChildATests.swift b/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/ChildATests.swift new file mode 100644 index 00000000..1dabe79c --- /dev/null +++ b/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/ChildATests.swift @@ -0,0 +1,4 @@ +import XCTest + +/// Inherits `testInherited` without redeclaring it, so its declaration is in BaseTests.swift. +final class ChildATests: BaseTests {} diff --git a/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/ChildBTests.swift b/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/ChildBTests.swift new file mode 100644 index 00000000..8c829185 --- /dev/null +++ b/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/ChildBTests.swift @@ -0,0 +1,6 @@ +import XCTest + +/// Overrides it, so its declaration is here. +final class ChildBTests: BaseTests { + override func testInherited() { XCTAssertTrue(true) } +} diff --git a/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/Legacy.swift b/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/Legacy.swift new file mode 100644 index 00000000..b3efd521 --- /dev/null +++ b/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/Legacy.swift @@ -0,0 +1,5 @@ +import XCTest + +final class LegacyXCTests: XCTestCase { + func testOldStyle() { XCTAssertTrue(true) } +} diff --git a/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/OverloadA.swift b/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/OverloadA.swift new file mode 100644 index 00000000..308ad010 --- /dev/null +++ b/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/OverloadA.swift @@ -0,0 +1,6 @@ +import Testing + +@Suite struct OverloadSuite { + @Test func check() {} + @Test(arguments: [1]) func check(a: Int) { _ = a } +} diff --git a/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/OverloadB.swift b/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/OverloadB.swift new file mode 100644 index 00000000..127bf93f --- /dev/null +++ b/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/OverloadB.swift @@ -0,0 +1,6 @@ +import Testing + +/// Same suite, different file, differing from `check(a:)` only by the argument label. +extension OverloadSuite { + @Test(arguments: [2]) func check(b: Int) { _ = b } +} diff --git a/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/Parameterized.swift b/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/Parameterized.swift new file mode 100644 index 00000000..4c314e1c --- /dev/null +++ b/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/Parameterized.swift @@ -0,0 +1,9 @@ +import Testing + +@Suite struct ParamSuite { + @Test(arguments: [1, 2, 3]) + func squares(n: Int) { #expect(n * n >= n) } + + @Test(arguments: ["a", "b"], [true, false]) + func pairs(s: String, flag: Bool) { #expect(!s.isEmpty || flag) } +} diff --git a/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/Suites.swift b/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/Suites.swift new file mode 100644 index 00000000..d74f05fc --- /dev/null +++ b/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/Suites.swift @@ -0,0 +1,9 @@ +import Testing + +@Suite struct AlphaSuite { + @Test func shared() { #expect(true) } + + @Suite struct Inner { + @Test func deep() { #expect(true) } + } +} diff --git a/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/TopLevel.swift b/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/TopLevel.swift new file mode 100644 index 00000000..d95f8e41 --- /dev/null +++ b/xcresult/tests/fixture-src/swift-test-xunit/Tests/MyCLITests/TopLevel.swift @@ -0,0 +1,5 @@ +import Testing + +@Test func helloworld() { + #expect(1 == 1) +} diff --git a/xcresult/tests/fixture-src/verify-test-structure.py b/xcresult/tests/fixture-src/verify-test-structure.py new file mode 100755 index 00000000..87d480f4 --- /dev/null +++ b/xcresult/tests/fixture-src/verify-test-structure.py @@ -0,0 +1,108 @@ +#!/usr/bin/env python3 +"""Assert that a captured .xcresult exhibits the *structure* it was captured for. + +The sibling `verify-failure-summaries.py` covers scenarios whose shape is a failure +raised somewhere other than the test. What these scenarios are captured for is the +result tree itself — a suite nested in a suite, a test that simply passed, or which +suite ran a test it does not declare — and no failure summary describes any of that. + + ./verify-test-structure.py +""" + +import json +import subprocess +import sys + +# `nested_suite` is an (outer, inner) pair; without one the flattening is unexercised. +SCENARIOS = { + "nested-and-passing": { + "nested_suite": ("OuterSuite", "InnerSuite"), + "passing": [ + "OuterSuite/outerPasses()", + "OuterSuite/InnerSuite/innerPasses()", + "topLevelPasses()", + ], + "failing": ["OuterSuite/InnerSuite/innerFails()"], + }, + # The same method has to arrive twice, under both suites, or there is no inherited + # test to attribute and the fixture proves nothing. + "inherited-test": { + "passing": [], + "failing": [ + "BaseTests/testInheritedFails()", + "ConcreteTests/testInheritedFails()", + ], + }, + # Declared in a category, so the run names a suite whose own file declares nothing. + "objc-category": { + "passing": [], + "failing": ["ObjcCategoryTests/testDeclaredInACategory"], + }, +} + + +def walk(node, parent, suites, cases): + kind = node.get("nodeType") + if kind == "Test Suite": + suites.append((parent, node.get("name"))) + parent = node.get("name") + elif kind == "Test Case": + cases.append(node) + for child in node.get("children", []) or []: + walk(child, parent, suites, cases) + + +def main(): + scenario, bundle = sys.argv[1], sys.argv[2] + expected = SCENARIOS[scenario] + tests = json.loads( + subprocess.run( + ["xcrun", "xcresulttool", "get", "test-results", "tests", "--path", bundle], + capture_output=True, + text=True, + check=True, + ).stdout + ) + + suites, cases = [], [] + for node in tests.get("testNodes", []): + walk(node, None, suites, cases) + by_identifier = {case.get("nodeIdentifier"): case for case in cases} + + failures = [] + if "nested_suite" in expected and expected["nested_suite"] not in suites: + failures.append( + f"expected a nested suite {expected['nested_suite']}, found {suites}" + ) + + for identifier in expected["passing"] + expected["failing"]: + case = by_identifier.get(identifier) + if case is None: + failures.append( + f"{identifier}: not in the bundle (found {sorted(by_identifier)})" + ) + continue + failed = case.get("result") == "Failed" + if identifier in expected["failing"] and not failed: + failures.append(f"{identifier}: expected it to have failed") + if identifier in expected["passing"] and failed: + failures.append(f"{identifier}: expected it to have passed") + # Falling back to `nodeIdentifier` would move every test case's id in the product. + if not case.get("nodeIdentifierURL"): + failures.append(f"{identifier}: no nodeIdentifierURL to derive an id from") + + if failures: + print(f"{scenario}: bundle does not exhibit its structure", file=sys.stderr) + for failure in failures: + print(f" - {failure}", file=sys.stderr) + sys.exit(1) + + print( + f"{scenario}: verified {len(expected['failing'])} failing and " + f"{len(expected['passing'])} passing test(s)" + + (" plus a nested suite" if "nested_suite" in expected else "") + ) + + +if __name__ == "__main__": + main() diff --git a/xcresult/tests/swift_test_xunit.rs b/xcresult/tests/swift_test_xunit.rs new file mode 100644 index 00000000..81b8735a --- /dev/null +++ b/xcresult/tests/swift_test_xunit.rs @@ -0,0 +1,322 @@ +//! `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. + +use std::{collections::HashMap, path::Path}; + +use rstest::rstest; +use xcresult::test_locations::{Limits, TestKey, TestLocationIndex}; +use xcresult::xcrun::find_program; + +const FIXTURE_ROOT: &str = "tests/fixture-src/swift-test-xunit"; +const XUNIT: &str = include_str!("data/swift-test-xunit.junit.xml"); +/// `--xunit-output` needs `--parallel` to emit XCTest results, and writes them to a separate +/// file from swift-testing's — which goes to `-swift-testing.xml`. +const XUNIT_XCTEST: &str = include_str!("data/swift-test-xunit-xctest.junit.xml"); + +/// Every case below resolves through a real `sourcekit-lsp` -- the fixture is all Swift, so +/// no `clangd` is involved. A missing server leaves the index empty rather than failing, so +/// without this the whole file fails on a machine that simply has no Swift toolchain. +/// +/// Skipping is only safe while something still runs these, so `REQUIRE_LANGUAGE_SERVER` turns +/// the skip back into a failure. CI sets it wherever it has just put the toolchain on `PATH`, +/// which is what stops the coverage from lapsing there unnoticed. +fn language_server_is_available() -> bool { + if find_program("sourcekit-lsp").is_some() { + return true; + } + // Empty counts as unset: a workflow computing this per-runner writes `""` for the ones it + // does not apply to, and `var_os` would otherwise read that as "required". + let required = std::env::var("REQUIRE_LANGUAGE_SERVER").is_ok_and(|value| !value.is_empty()); + assert!( + !required, + "sourcekit-lsp is not on PATH and REQUIRE_LANGUAGE_SERVER is set -- the toolchain this \ + job is supposed to provide is missing, so these tests would have silently skipped" + ); + eprintln!("skipping: sourcekit-lsp is not on PATH, so no declaration can be resolved"); + false +} + +fn testcases(xunit: &str) -> Vec<(String, String)> { + xunit + .lines() + .filter(|line| line.contains(" HashMap<(String, String), String> { + resolve_from(XUNIT) +} + +fn resolve_from(xunit: &str) -> HashMap<(String, String), String> { + let cases = testcases(xunit); + assert!(!cases.is_empty(), "the fixture must have testcases"); + + let keys = cases + .iter() + .map(|(classname, name)| { + ( + TestKey::from_junit_classname(classname, name), + TestKey::target_from_junit_classname(classname), + ) + }) + .collect::>(); + let index = TestLocationIndex::resolve(Path::new(FIXTURE_ROOT), &keys, Limits::default()); + + cases + .into_iter() + .filter_map(|(classname, name)| { + let key = TestKey::from_junit_classname(&classname, &name); + index + .lookup(&key) + .map(|site| ((classname, name), site.file.as_str().to_owned())) + }) + .collect() +} + +#[test] +fn every_swift_testing_case_resolves_to_the_file_it_is_declared_in() { + if !language_server_is_available() { + return; + } + let resolved = resolve(); + for (classname, name, expected) in [ + ("MyCLITests", "helloworld()", "TopLevel.swift"), + ("MyCLITests.AlphaSuite", "shared()", "Suites.swift"), + ("MyCLITests.AlphaSuite.Inner", "deep()", "Suites.swift"), + ("MyCLITests.BetaSuite", "shared()", "BetaSuite.swift"), + ( + "MyCLITests.ParamSuite", + "squares(n:)", + "Parameterized.swift", + ), + ( + "MyCLITests.ParamSuite", + "pairs(s:flag:)", + "Parameterized.swift", + ), + ] { + let key = (String::from(classname), String::from(name)); + let file = resolved + .get(&key) + .unwrap_or_else(|| panic!("{classname} {name} resolved to nothing")); + assert!( + file.ends_with(expected), + "expected {classname} {name} in {expected}, got {file}" + ); + } +} + +#[test] +fn overloads_differing_only_by_argument_label_resolve_separately() { + if !language_server_is_available() { + return; + } + let resolved = resolve(); + let file = |name: &str| { + resolved + .get(&(String::from("MyCLITests.OverloadSuite"), String::from(name))) + .unwrap_or_else(|| panic!("{name} resolved to nothing")) + .clone() + }; + let (a, b, none) = (file("check(a:)"), file("check(b:)"), file("check()")); + assert_ne!(a, b, "both labelled overloads resolved to {a}"); + assert!(a.ends_with("OverloadA.swift") && none.ends_with("OverloadA.swift")); + assert!(b.ends_with("OverloadB.swift")); +} + +// Collapsing the classname to its target would make one `shared()` borrow the other's file. +#[test] +fn two_suites_declaring_the_same_case_resolve_separately() { + if !language_server_is_available() { + return; + } + let resolved = resolve(); + let alpha = resolved + .get(&( + String::from("MyCLITests.AlphaSuite"), + String::from("shared()"), + )) + .expect("alpha resolved"); + let beta = resolved + .get(&( + String::from("MyCLITests.BetaSuite"), + String::from("shared()"), + )) + .expect("beta resolved"); + assert_ne!(alpha, beta, "both `shared()` cases resolved to {alpha}"); +} + +// XCTest goes to its own file and names a case `Module.Class` + a bare method, so it needs no +// separate handling — the innermost component is still the declaring type. +// +// `ChildATests` is the case that cannot be answered. It declares no `testInherited` of its +// own, so the method it ran is written elsewhere — but `testInherited` is declared twice in +// this target, by `BaseTests` (which it inherits from) and by `ChildBTests` (which merely +// overrides it). An override is a declaration of the same name, so by name alone the ancestor +// and the sibling are the same fact, and telling them apart needs an inheritance hierarchy, +// which is semantic and would cost a build to resolve. Reporting `BaseTests.swift` here would +// be a guess that happens to be right; reporting the suite's own file is the honest answer, +// and `inherited_declaration` warns when it declines. +// +// Where only one class declares the case there is nothing to guess between, and the base +// class's file is reported — `test_an_inherited_test_is_attributed_to_the_class_that_declares_it` +// covers that shape. +#[rstest] +#[case::declared("MyCLITests.BaseTests", "BaseTests.swift")] +#[case::inherited_but_ambiguous("MyCLITests.ChildATests", "ChildATests.swift")] +#[case::overridden("MyCLITests.ChildBTests", "ChildBTests.swift")] +fn an_xctest_method_resolves_to_its_declaration_unless_two_classes_declare_it( + #[case] classname: &str, + #[case] expected: &str, +) { + if !language_server_is_available() { + return; + } + let resolved = resolve_from(XUNIT_XCTEST); + let file = resolved + .get(&(String::from(classname), String::from("testInherited"))) + .unwrap_or_else(|| panic!("{classname} resolved to nothing")); + assert!(file.ends_with(expected), "expected {expected}, got {file}"); +} + +#[test] +fn an_xctest_case_resolves_to_the_class_that_declares_it() { + if !language_server_is_available() { + return; + } + let resolved = resolve_from(XUNIT_XCTEST); + let file = resolved + .get(&( + String::from("MyCLITests.LegacyXCTests"), + String::from("testOldStyle"), + )) + .expect("the XCTest case resolved"); + assert!(file.ends_with("Legacy.swift"), "got {file}"); +} + +/// One package captured both ways, which must land every test on the same file. +#[cfg(target_os = "macos")] +mod parity { + use std::{fs::File, path::PathBuf}; + + use flate2::read::GzDecoder; + use tar::Archive; + use temp_testdir::TempDir; + use xcresult::xcresult::XCResult; + + use super::*; + + fn xcresult_report() -> Vec { + let temp_dir = TempDir::default(); + Archive::new(GzDecoder::new( + File::open("tests/data/swift-test-parity.xcresult.tar.gz").unwrap(), + )) + .unpack(temp_dir.as_ref()) + .unwrap(); + let bundle: PathBuf = temp_dir.as_ref().join("MyCLI.xcresult"); + + let xcresult = XCResult::new_with_declaration_locations( + bundle.to_str().unwrap(), + String::from("trunk"), + String::from("github.com/trunk-io/analytics-cli"), + Path::new(FIXTURE_ROOT), + Limits::default(), + ) + .expect("the declaration path reads the bundle"); + + xcresult.generate_junits() + } + + fn without_parens(name: &str) -> String { + name.trim_end_matches(['(', ')']).to_owned() + } + + fn xcresult_raw_names() -> Vec { + xcresult_report() + .iter() + .flat_map(|report| report.test_suites.iter()) + .flat_map(|test_suite| test_suite.test_cases.iter()) + .map(|test_case| test_case.name.as_str().to_owned()) + .collect() + } + + /// Pairs rather than a map keyed by name: two suites here both declare `shared()`. + fn xcresult_pairs() -> Vec<(String, String)> { + let mut pairs = xcresult_report() + .iter() + .flat_map(|report| report.test_suites.iter()) + .flat_map(|test_suite| test_suite.test_cases.iter()) + .filter_map(|test_case| { + test_case + .extra + .iter() + .find(|(key, _)| key.as_str() == "file") + .map(|(_, value)| { + ( + without_parens(test_case.name.as_str()), + file_name(value.as_str()), + ) + }) + }) + .collect::>(); + pairs.sort(); + pairs + } + + fn xunit_pairs() -> Vec<(String, String)> { + let mut pairs = [XUNIT, XUNIT_XCTEST] + .into_iter() + .flat_map(|xunit| resolve_from(xunit).into_iter()) + .map(|((_, name), file)| (without_parens(&name), file_name(&file))) + .collect::>(); + pairs.sort(); + pairs + } + + fn file_name(path: &str) -> String { + Path::new(path) + .file_name() + .map(|name| name.to_string_lossy().into_owned()) + .unwrap_or_else(|| String::from(path)) + } + + #[test] + fn both_inputs_resolve_every_test_to_the_same_file() { + let from_xcresult = xcresult_pairs(); + assert!( + from_xcresult.len() >= 6, + "the bundle should carry every test, got {from_xcresult:?}" + ); + pretty_assertions::assert_eq!(xunit_pairs(), from_xcresult); + } + + // Upstream, not ours: `name` feeds `gen_info_id_base`, so the same test arriving through + // the two inputs does not land on one identity. Pinned so it cannot change unnoticed. + #[test] + fn the_two_inputs_spell_an_xctest_method_differently() { + assert!( + testcases(XUNIT_XCTEST) + .iter() + .any(|(_, name)| name == "testOldStyle"), + "the xunit spells it with parens" + ); + assert!( + xcresult_raw_names().contains(&String::from("testOldStyle()")), + "the bundle spells it {:?}", + xcresult_raw_names() + ); + } +} diff --git a/xcresult/tests/xcresult.rs b/xcresult/tests/xcresult.rs index 35397b5d..ca3ba62a 100644 --- a/xcresult/tests/xcresult.rs +++ b/xcresult/tests/xcresult.rs @@ -1,23 +1,14 @@ -use std::{fs::File, path::Path}; - -use context::repo::RepoUrlParts; -use flate2::read::GzDecoder; use lazy_static::lazy_static; +#[cfg(target_os = "macos")] use rstest::rstest; -use tar::Archive; use temp_testdir::TempDir; use xcresult::xcresult::XCResult; -fn unpack_archive_to_temp_dir>(archive_file_path: T) -> TempDir { - let file = File::open(archive_file_path).unwrap(); - let decoder = GzDecoder::new(file); - let mut archive = Archive::new(decoder); - let temp_dir = TempDir::default(); - if let Err(e) = archive.unpack(temp_dir.as_ref()) { - panic!("failed to unpack data.tar.gz: {}", e); - } - temp_dir -} +mod common; + +#[cfg(target_os = "macos")] +use common::assert_junit; +use common::{ORG_URL_SLUG, REPO_FULL_NAME, unpack_archive_to_temp_dir}; lazy_static! { static ref TEMP_DIR_TEST_1: TempDir = @@ -44,17 +35,12 @@ lazy_static! { unpack_archive_to_temp_dir("tests/data/test-objc-xctest.xcresult.tar.gz"); static ref TEMP_DIR_TEST_TOPLEVEL_SWIFT_TESTING: TempDir = unpack_archive_to_temp_dir("tests/data/test-toplevel-swift-testing.xcresult.tar.gz"); + static ref TEMP_DIR_TEST_NESTED_AND_PASSING: TempDir = + unpack_archive_to_temp_dir("tests/data/test-nested-and-passing.xcresult.tar.gz"); static ref TEMP_DIR_TEST_TIMESTAMP: TempDir = unpack_archive_to_temp_dir("tests/data/test-timestamp.xcresult.tar.gz"); static ref TEMP_DIR_TEST_VARIANT: TempDir = unpack_archive_to_temp_dir("tests/data/test-variant.xcresult.tar.gz"); - static ref ORG_URL_SLUG: String = String::from("trunk"); - static ref REPO_FULL_NAME: String = RepoUrlParts { - host: "github.com".to_string(), - owner: "trunk-io".to_string(), - name: "analytics-cli".to_string() - } - .repo_full_name(); } #[cfg(target_os = "macos")] @@ -261,31 +247,6 @@ fn test_swift_snapshot_testing_trait_failure_uses_assertion_file( // bundle must exhibit and how to regenerate it. // // The two cases expect different JUnit because they have different sources -// available: only the experimental path reads the per-test failure summary, and so -// the call stack, so it is the only one that can produce a `FileSource::TestFrame`. -// The legacy path sees `FileSource::DocumentLocation` alone. -#[cfg(target_os = "macos")] -fn assert_junit>( - bundle_path: T, - use_experimental_failure_summary: bool, - expected_junit_xml: &str, -) { - let path_str = bundle_path.as_ref().to_str().unwrap(); - let xcresult = XCResult::new( - path_str, - ORG_URL_SLUG.clone(), - REPO_FULL_NAME.clone(), - use_experimental_failure_summary, - ); - assert!(xcresult.is_ok()); - - let mut junits = xcresult.unwrap().generate_junits(); - assert_eq!(junits.len(), 1); - let junit = junits.pop().unwrap(); - let mut junit_writer: Vec = Vec::new(); - junit.serialize(&mut junit_writer).unwrap(); - pretty_assertions::assert_eq!(String::from_utf8(junit_writer).unwrap(), expected_junit_xml); -} // `FileSource::TestFrame` is the only usable source. The failure is recorded inside // the dependency, so `RaisedFrom`, `SourceCodeLocation` and the innermost