From 81ef3ab3a2f34d84b43ffec2b53171bec0a3b4ec Mon Sep 17 00:00:00 2001 From: Christian Millar Date: Mon, 2 Jun 2025 19:28:00 +0000 Subject: [PATCH 01/14] [TRUNK-14658] Add relative test file support Adds two pieces of logic: - Falls back to checking the test case classname for a path as well - Convert relative paths to absolute paths, browsing through non-hidden folders in the repository. These two cover the ways vitest differs from every other test suite we've encountered. --- Cargo.lock | 1 + cli-tests/src/upload.rs | 6 +- cli-tests/src/utils.rs | 22 +- cli-tests/src/validate.rs | 7 +- cli/src/context.rs | 3 +- cli/src/upload_command.rs | 1 + cli/src/validate_command.rs | 4 +- context-js/src/lib.rs | 6 +- context-py/src/lib.rs | 10 +- context/Cargo.toml | 1 + context/src/junit/bindings.rs | 21 +- context/src/junit/file_extractor.rs | 399 ++++++++++++++++++++++++++++ context/src/junit/mod.rs | 1 + context/src/junit/parser.rs | 26 +- context/src/junit/validator.rs | 15 +- context/tests/junit.rs | 38 +-- junit-mock/src/lib.rs | 27 +- junit-mock/src/main.rs | 3 +- 18 files changed, 515 insertions(+), 76 deletions(-) create mode 100644 context/src/junit/file_extractor.rs diff --git a/Cargo.lock b/Cargo.lock index 3ee11db1..783026a8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -987,6 +987,7 @@ dependencies = [ "tracing", "tsify-next", "uuid", + "walkdir", "wasm-bindgen", ] diff --git a/cli-tests/src/upload.rs b/cli-tests/src/upload.rs index a4d0dfa7..21257d58 100644 --- a/cli-tests/src/upload.rs +++ b/cli-tests/src/upload.rs @@ -18,7 +18,7 @@ use lazy_static::lazy_static; use predicates::prelude::*; use pretty_assertions::assert_eq; use prost::Message; -use tempfile::tempdir; +use tempfile::{tempdir, TempDir}; use test_utils::{ inputs::get_test_file_path, mock_server::{MockServerBuilder, RequestPayload, SharedMockServerState}, @@ -33,9 +33,9 @@ use crate::utils::{ // NOTE: must be multi threaded to start a mock server #[tokio::test(flavor = "multi_thread")] async fn upload_bundle() { - let temp_dir = tempdir().unwrap(); + let temp_dir = TempDir::with_prefix("not-hidden").unwrap(); generate_mock_git_repo(&temp_dir); - generate_mock_valid_junit_xmls(&temp_dir); + generate_mock_valid_junit_xmls(temp_dir.path()); generate_mock_codeowners(&temp_dir); let state = MockServerBuilder::new().spawn_mock_server().await; diff --git a/cli-tests/src/utils.rs b/cli-tests/src/utils.rs index d27fef29..9f921ab0 100644 --- a/cli-tests/src/utils.rs +++ b/cli-tests/src/utils.rs @@ -33,18 +33,19 @@ pub fn generate_mock_git_repo>(directory: T) { setup_repo_with_commit(directory).unwrap(); } -pub fn generate_mock_valid_junit_xmls>(directory: T) -> Vec { +pub fn generate_mock_valid_junit_xmls + Clone>(directory: T) -> Vec { let mut jm_options = junit_mock::Options::default(); jm_options.global.timestamp = Utc::now() .fixed_offset() .checked_sub_signed(TimeDelta::minutes(1)); let mut jm = JunitMock::new(junit_mock::Options::default()); - let reports = jm.generate_reports(); + let tmp_dir = Some(directory.clone()); + let reports = jm.generate_reports(&tmp_dir); jm.write_reports_to_file(directory.as_ref(), reports) .unwrap() } -pub fn generate_mock_bazel_bep>(directory: T) { +pub fn generate_mock_bazel_bep + std::fmt::Debug>(directory: T) { let mock_junits = generate_mock_valid_junit_xmls(&directory); let build_events: Vec = mock_junits @@ -78,33 +79,36 @@ pub fn generate_mock_bazel_bep>(directory: T) { file.write_all(outputs_contents.as_bytes()).unwrap(); } -pub fn generate_mock_invalid_junit_xmls>(directory: T) { +pub fn generate_mock_invalid_junit_xmls + Clone>(directory: T) { let mut jm_options = junit_mock::Options::default(); jm_options.test_suite.test_suite_names = Some(vec!["".to_string()]); jm_options.global.timestamp = Utc::now() .fixed_offset() .checked_sub_signed(TimeDelta::minutes(1)); let mut jm = JunitMock::new(jm_options); - let reports = jm.generate_reports(); + let tmp_dir = Some(directory.clone()); + let reports = jm.generate_reports(&tmp_dir); jm.write_reports_to_file(directory.as_ref(), reports) .unwrap(); } -pub fn generate_mock_suboptimal_junit_xmls>(directory: T) { +pub fn generate_mock_suboptimal_junit_xmls + Clone>(directory: T) { let mut jm_options = junit_mock::Options::default(); jm_options.global.timestamp = Utc::now() .fixed_offset() .checked_sub_signed(TimeDelta::hours(24)); let mut jm = JunitMock::new(jm_options); - let reports = jm.generate_reports(); + let tmp_dir = Some(directory.clone()); + let reports = jm.generate_reports(&tmp_dir); jm.write_reports_to_file(directory.as_ref(), reports) .unwrap(); } -pub fn generate_mock_missing_filepath_suboptimal_junit_xmls>(directory: T) { +pub fn generate_mock_missing_filepath_suboptimal_junit_xmls + Clone>(directory: T) { let jm_options = junit_mock::Options::default(); let mut jm = JunitMock::new(jm_options); - let mut reports = jm.generate_reports(); + let tmp_dir = Some(directory.clone()); + let mut reports = jm.generate_reports(&tmp_dir); for report in reports.iter_mut() { for testsuite in report.test_suites.iter_mut() { for test_case in testsuite.test_cases.iter_mut() { diff --git a/cli-tests/src/validate.rs b/cli-tests/src/validate.rs index 54bef20a..e508c014 100644 --- a/cli-tests/src/validate.rs +++ b/cli-tests/src/validate.rs @@ -1,10 +1,10 @@ use predicates::prelude::*; -use tempfile::tempdir; +use tempfile::{tempdir, TempDir}; use crate::{ command_builder::CommandBuilder, utils::{ - generate_mock_codeowners, generate_mock_invalid_junit_xmls, + generate_mock_codeowners, generate_mock_git_repo, generate_mock_invalid_junit_xmls, generate_mock_missing_filepath_suboptimal_junit_xmls, generate_mock_suboptimal_junit_xmls, generate_mock_valid_junit_xmls, write_junit_xml_to_dir, }, @@ -132,7 +132,7 @@ fn validate_invalid_xml() { #[test] fn validate_suboptimal_junits() { - let temp_dir = tempdir().unwrap(); + let temp_dir = TempDir::with_prefix("not-hidden").unwrap(); generate_mock_suboptimal_junit_xmls(&temp_dir); let assert = CommandBuilder::validate(temp_dir.path()) @@ -152,6 +152,7 @@ fn validate_suboptimal_junits() { #[test] fn validate_missing_filepath_suboptimal_junits() { let temp_dir = tempdir().unwrap(); + generate_mock_git_repo(&temp_dir); generate_mock_missing_filepath_suboptimal_junit_xmls(&temp_dir); generate_mock_codeowners(&temp_dir); diff --git a/cli/src/context.rs b/cli/src/context.rs index 770e2af5..3e33f94e 100644 --- a/cli/src/context.rs +++ b/cli/src/context.rs @@ -208,6 +208,7 @@ pub fn generate_internal_file( file_sets: &[FileSet], temp_dir: &TempDir, codeowners: Option<&CodeOwners>, + repo: &BundleRepo, ) -> anyhow::Result { let mut test_case_runs = Vec::new(); for file_set in file_sets { @@ -233,7 +234,7 @@ pub fn generate_internal_file( tracing::warn!("Failed to parse JUnit file {:?}: {:?}", file, e); continue; } - test_case_runs.extend(junit_parser.into_test_case_runs(codeowners)); + test_case_runs.extend(junit_parser.into_test_case_runs(codeowners, repo)); } } } diff --git a/cli/src/upload_command.rs b/cli/src/upload_command.rs index c3925cf5..28952f80 100644 --- a/cli/src/upload_command.rs +++ b/cli/src/upload_command.rs @@ -243,6 +243,7 @@ pub async fn run_upload( &meta.base_props.file_sets, &temp_dir, meta.base_props.codeowners.as_ref(), + &meta.base_props.repo, ); if let Ok(internal_bundled_file) = internal_bundled_file { meta.internal_bundled_file = Some(internal_bundled_file); diff --git a/cli/src/validate_command.rs b/cli/src/validate_command.rs index 105c29da..8a9c25cd 100644 --- a/cli/src/validate_command.rs +++ b/cli/src/validate_command.rs @@ -17,6 +17,7 @@ use context::{ JunitValidationLevel, }, }, + repo::BundleRepo, }; use quick_junit::Report; @@ -132,6 +133,7 @@ async fn validate( (parsed_reports, parse_issues) }, ); + let repo = BundleRepo::new(None, None, None, None, None, None, false).unwrap_or_default(); // print parse issues let (num_unparsable_reports, num_suboptimally_parsable_reports) = print_parse_issues(&parse_issues); @@ -139,7 +141,7 @@ async fn validate( // validate let report_validations: JunitFileToValidation = parsed_reports .into_iter() - .map(|(file, report)| (file, validate_report(&report))) + .map(|(file, report)| (file, validate_report(&report, &repo))) .collect(); // print validation results let (mut num_invalid_reports, mut num_suboptimal_reports) = diff --git a/context-js/src/lib.rs b/context-js/src/lib.rs index 9d6f725a..dcfef650 100644 --- a/context-js/src/lib.rs +++ b/context-js/src/lib.rs @@ -1,7 +1,10 @@ use std::{collections::HashMap, io::BufReader}; use bundle::{parse_meta_from_tarball as parse_tarball, VersionedBundle}; -use context::{env, junit, repo}; +use context::{ + env, junit, + repo::{self, BundleRepo}, +}; use futures::{future::Either, io::BufReader as BufReaderAsync, stream::TryStreamExt}; use js_sys::Uint8Array; use wasm_bindgen::prelude::*; @@ -92,6 +95,7 @@ pub fn junit_validate( ) -> junit::bindings::BindingsJunitReportValidation { junit::bindings::BindingsJunitReportValidation::from(junit::validator::validate( &report.clone().into(), + &BundleRepo::default(), )) } diff --git a/context-py/src/lib.rs b/context-py/src/lib.rs index 29d1a6bc..89c7a111 100644 --- a/context-py/src/lib.rs +++ b/context-py/src/lib.rs @@ -7,7 +7,10 @@ use bundle::{ use codeowners::{ associate_codeowners_multithreaded as associate_codeowners, BindingsOwners, CodeOwners, Owners, }; -use context::{env, junit, meta, repo}; +use context::{ + env, junit, meta, + repo::{self, BundleRepo}, +}; use prost::Message; use pyo3::{exceptions::PyTypeError, prelude::*}; use pyo3_stub_gen::{define_stub_info_gatherer, derive::gen_stub_pyfunction}; @@ -111,7 +114,10 @@ fn bin_parse(bin: Vec) -> PyResult> { fn junit_validate( report: junit::bindings::BindingsReport, ) -> junit::bindings::BindingsJunitReportValidation { - junit::bindings::BindingsJunitReportValidation::from(junit::validator::validate(&report.into())) + junit::bindings::BindingsJunitReportValidation::from(junit::validator::validate( + &report.into(), + &BundleRepo::default(), + )) } #[gen_stub_pyfunction] diff --git a/context/Cargo.toml b/context/Cargo.toml index 21b5de25..92ac8217 100644 --- a/context/Cargo.toml +++ b/context/Cargo.toml @@ -38,6 +38,7 @@ prost-wkt-types = { version = "0.5.1", features = ["vendored-protox"] } tracing = "0.1.41" prost = "0.12.6" codeowners = { version = "0.1.3", path = "../codeowners" } +walkdir = "2.5.0" [target.'cfg(target_os = "linux")'.dependencies] pyo3 = { version = "0.22.5", optional = true, features = [ diff --git a/context/src/junit/bindings.rs b/context/src/junit/bindings.rs index 4d6a5130..2250ae54 100644 --- a/context/src/junit/bindings.rs +++ b/context/src/junit/bindings.rs @@ -1059,8 +1059,11 @@ mod tests { #[test] fn parse_test_report_to_bindings() { use prost_wkt_types::Timestamp; + use tempfile::TempDir; - use crate::junit::validator::validate; + use crate::{junit::validator::validate, repo::BundleRepo}; + + let temp_dir = TempDir::with_prefix("not-hidden").unwrap(); let test_started_at = Timestamp { seconds: 1000, nanos: 0, @@ -1072,11 +1075,13 @@ mod tests { let codeowner1 = CodeOwner { name: "@user".into(), }; + let test_file = temp_dir.path().join("test_file"); + let file_str = String::from(test_file.as_os_str().to_str().unwrap()); let test1 = TestCaseRun { id: "test_id1".into(), name: "test_name".into(), classname: "test_classname".into(), - file: "test_file".into(), + file: file_str.clone(), parent_name: "test_parent_name1".into(), line: 1, status: TestCaseRunStatus::Success.into(), @@ -1092,7 +1097,7 @@ mod tests { id: "test_id2".into(), name: "test_name".into(), classname: "test_classname".into(), - file: "test_file".into(), + file: file_str, parent_name: "test_parent_name2".into(), line: 1, status: TestCaseRunStatus::Failure.into(), @@ -1180,7 +1185,7 @@ mod tests { assert_eq!(test_case2.codeowners.clone().unwrap().len(), 0); // verify that the test report is valid - let results = validate(&converted_bindings.clone().into()); + let results = validate(&converted_bindings.clone().into(), &BundleRepo::default()); assert_eq!(results.all_issues_flat().len(), 1); results .all_issues_flat() @@ -1216,17 +1221,19 @@ mod tests { #[cfg(feature = "bindings")] #[test] fn test_junit_conversion_paths() { + use crate::repo::BundleRepo; + let mut junit_parser = JunitParser::new(); let file_contents = r#" - + but was: ]]> - + "#; @@ -1234,7 +1241,7 @@ mod tests { assert!(parsed_results.is_ok()); // Get test case runs from parser - let test_case_runs = junit_parser.into_test_case_runs(None); + let test_case_runs = junit_parser.into_test_case_runs(None, &BundleRepo::default()); assert_eq!(test_case_runs.len(), 2); // Convert test case runs to bindings diff --git a/context/src/junit/file_extractor.rs b/context/src/junit/file_extractor.rs new file mode 100644 index 00000000..490a2f05 --- /dev/null +++ b/context/src/junit/file_extractor.rs @@ -0,0 +1,399 @@ +use std::fs; +use std::path::Path; + +use quick_junit::{TestCase, XmlString}; +use walkdir::{DirEntry, WalkDir}; + +use super::parser::extra_attrs; +use crate::repo::BundleRepo; + +fn not_hidden(entry: &DirEntry) -> bool { + entry + .file_name() + .to_str() + .map(|s| !s.starts_with(".")) + .unwrap_or(true) +} + +fn contains_test(path: &String, test_xml_name: &XmlString) -> bool { + let test_name = test_xml_name.as_str(); + // Frameworks like vitest handle tests with an it() call inside a describe() call by + // using "{describe_name} > {it_name}" as the test name, so we split on that in order + // to get substrings that we can search for. + let mut test_parts = test_name.split(" > "); + fs::read_to_string(Path::new(path)) + .ok() + .map(|text| { + let has_full_name = text.contains(test_name); + let has_name_splits = test_parts.all(|p| text.contains(p)); + if has_full_name || has_name_splits { + true + } else { + false + } + }) + .unwrap_or(false) +} + +fn file_containing_tests(file_paths: Vec, test_name: &XmlString) -> Option { + let mut matching_paths = file_paths + .iter() + .filter(|path| contains_test(*path, test_name)); + let first_match = matching_paths.next(); + let another_match = matching_paths.next(); + match (first_match, another_match) { + (None, _) => None, + (Some(only_match), None) => Some((*only_match).clone()), + (_, _) => None, + } +} + +// None if is not a file or file does not exist, Some(absolute path) if it does exist in the root +fn convert_to_absolute( + initial: &XmlString, + repo: &BundleRepo, + test_name: &XmlString, +) -> Option { + let initial_str = String::from(initial.as_str()); + let path = Path::new(&initial_str); + if path.is_absolute() { + println!("Path {:?} was absolute", path); + path.to_str().map(String::from) + } else if Path::new(&repo.repo_root).is_absolute() { + let mut walk = WalkDir::new(repo.repo_root.clone()) + .into_iter() + .filter_entry(not_hidden) + .filter_map(|result| { + if let Ok(entry) = result { + if entry.path().ends_with(path) { + entry.path().as_os_str().to_str().map(String::from).clone() + } else { + None + } + } else { + None + } + }); + let first_match = walk.next(); + let another_match = walk.next(); + match (first_match, another_match) { + (None, _) => None, + (Some(only_match), None) => Some(only_match), + (Some(first_match), Some(second_match)) => file_containing_tests( + vec![vec![first_match, second_match], walk.collect()].concat(), + test_name, + ), + } + } else { + None + } +} + +pub fn filename_for_test_case(test_case: &TestCase, repo: &BundleRepo) -> String { + println!( + "!!!!!!!!!!!!!!!!! File {:?} Filepath {:?} Classname {:?}", + test_case.extra.get(extra_attrs::FILE), + test_case.extra.get(extra_attrs::FILEPATH), + test_case.classname + ); + test_case + .extra + .get(extra_attrs::FILE) + .or(test_case.extra.get(extra_attrs::FILEPATH)) + .or(test_case.classname.as_ref()) + .map(|s| convert_to_absolute(s, repo, &test_case.name)) + .flatten() + .unwrap_or_default() +} + +#[cfg(test)] +mod tests { + use std::fs; + use std::path::PathBuf; + + use quick_junit::XmlString; + use tempfile::{tempdir, TempDir}; + + use super::*; + use crate::repo::RepoUrlParts; + + fn stringify(fp: PathBuf) -> String { + String::from(fp.as_os_str().to_str().unwrap()) + } + + fn bundle_repo(dir: &Path) -> BundleRepo { + BundleRepo { + repo: RepoUrlParts::default(), + repo_root: String::from(dir.as_os_str().to_str().unwrap()), + repo_url: String::from(""), + repo_head_sha: String::from(""), + repo_head_sha_short: None, + repo_head_branch: String::from(""), + repo_head_commit_epoch: 0, + repo_head_commit_message: String::from(""), + repo_head_author_name: String::from(""), + repo_head_author_email: String::from(""), + } + } + + #[test] + fn test_contains_test_if_unsplit_test_name_present() { + let temp_dir = tempdir().unwrap(); + let file_path = temp_dir.as_ref().join("match.txt"); + let text = r#" + describe("description", { + it("my_super_good_test", { + }) + }) + "#; + fs::write(file_path.clone(), text).unwrap(); + let actual = contains_test(&stringify(file_path), &XmlString::new("my_super_good_test")); + assert!(actual); + } + + #[test] + fn test_contains_test_if_split_test_name_present() { + let temp_dir = tempdir().unwrap(); + let file_path = temp_dir.as_ref().join("match.txt"); + let text = r#" + describe("description", { + it("my_super_good_test", { + }) + }) + "#; + fs::write(file_path.clone(), text).unwrap(); + let actual = contains_test( + &stringify(file_path), + &XmlString::new("description > my_super_good_test"), + ); + assert!(actual); + } + + #[test] + fn test_contains_test_if_part_of_split_test_name_present() { + let temp_dir = tempdir().unwrap(); + let file_path = temp_dir.as_ref().join("match.txt"); + let text = r#" + it("my_super_good_test", { + }) + "#; + fs::write(file_path.clone(), text).unwrap(); + let actual = contains_test( + &stringify(file_path), + &XmlString::new("description > my_super_good_test"), + ); + assert!(!actual); + } + + #[test] + fn test_contains_test_if_test_name_not_present() { + let temp_dir = tempdir().unwrap(); + let file_path = temp_dir.as_ref().join("match.txt"); + let text = r#" + describe("description", { + it("my_super_good_test", { + }) + }) + "#; + fs::write(file_path.clone(), text).unwrap(); + let actual = contains_test(&stringify(file_path), &XmlString::new("totally_different")); + assert!(!actual); + } + + #[test] + fn test_file_containing_test_if_none_contains_test() { + let temp_dir = tempdir().unwrap(); + let file_path_1 = temp_dir.as_ref().join("file_1.txt"); + let text_1 = r#"it("test_1")"#; + fs::write(file_path_1.clone(), text_1).unwrap(); + let file_path_2 = temp_dir.as_ref().join("file_2.txt"); + let text_2 = r#"it("test_2")"#; + fs::write(file_path_2.clone(), text_2).unwrap(); + let file_path_3 = temp_dir.as_ref().join("file_3.txt"); + let text_3 = r#"it("test_3")"#; + fs::write(file_path_3.clone(), text_3).unwrap(); + let actual = file_containing_tests( + vec![ + stringify(file_path_1), + stringify(file_path_2), + stringify(file_path_3), + ], + &XmlString::new("totally_different"), + ); + assert_eq!(actual, None); + } + + #[test] + fn test_file_containing_test_if_one_contains_test() { + let temp_dir = tempdir().unwrap(); + let file_path_1 = temp_dir.as_ref().join("file_1.txt"); + let text_1 = r#"it("test_1")"#; + fs::write(file_path_1.clone(), text_1).unwrap(); + let file_path_2 = temp_dir.as_ref().join("file_2.txt"); + let text_2 = r#"it("test_2")"#; + fs::write(file_path_2.clone(), text_2).unwrap(); + let file_path_3 = temp_dir.as_ref().join("file_3.txt"); + let text_3 = r#"it("test_3")"#; + fs::write(file_path_3.clone(), text_3).unwrap(); + let actual = file_containing_tests( + vec![ + stringify(file_path_1), + stringify(file_path_2.clone()), + stringify(file_path_3), + ], + &XmlString::new("test_2"), + ); + assert_eq!(actual, Some(stringify(file_path_2))); + } + + #[test] + fn test_file_containing_test_if_multiple_contain_test() { + let temp_dir = tempdir().unwrap(); + let file_path_1 = temp_dir.as_ref().join("file_1.txt"); + let text_1 = r#"it("common_test")"#; + fs::write(file_path_1.clone(), text_1).unwrap(); + let file_path_2 = temp_dir.as_ref().join("file_2.txt"); + let text_2 = r#"it("common_test")"#; + fs::write(file_path_2.clone(), text_2).unwrap(); + let file_path_3 = temp_dir.as_ref().join("file_3.txt"); + let text_3 = r#"it("test_3")"#; + fs::write(file_path_3.clone(), text_3).unwrap(); + let actual = file_containing_tests( + vec![ + stringify(file_path_1), + stringify(file_path_2), + stringify(file_path_3), + ], + &XmlString::new("common_test"), + ); + assert_eq!(actual, None); + } + + #[test] + fn test_convert_to_absolute_when_no_repo_root() { + let temp_dir = TempDir::with_prefix("not-hidden").unwrap(); + let file_path = temp_dir.as_ref().join("test.txt"); + let text = r#"it("test")"#; + fs::write(file_path.clone(), text).unwrap(); + let actual = convert_to_absolute( + &XmlString::new("test.txt"), + &BundleRepo::default(), + &XmlString::new("test"), + ); + assert_eq!(actual, None); + } + + #[test] + fn test_convert_to_absolute_when_already_absolute() { + let temp_dir = TempDir::with_prefix("not-hidden").unwrap(); + let file_path = temp_dir.as_ref().join("test.txt"); + let text = r#"it("test")"#; + fs::write(file_path.clone(), text).unwrap(); + let actual = convert_to_absolute( + &XmlString::new(stringify(file_path.clone())), + &bundle_repo(temp_dir.as_ref()), + &XmlString::new("test"), + ); + assert_eq!(actual, Some(stringify(file_path))); + } + + #[test] + fn test_convert_to_absolute_when_no_file_matches() { + let temp_dir = TempDir::with_prefix("not-hidden").unwrap(); + let file_path_1 = temp_dir.as_ref().join("file_1.txt"); + let text_1 = r#"it("test_1")"#; + fs::write(file_path_1.clone(), text_1).unwrap(); + let file_path_2 = temp_dir.as_ref().join("file_2.txt"); + let text_2 = r#"it("test_2")"#; + fs::write(file_path_2.clone(), text_2).unwrap(); + let file_path_3 = temp_dir.as_ref().join("file_3.txt"); + let text_3 = r#"it("test_3")"#; + fs::write(file_path_3.clone(), text_3).unwrap(); + let actual = convert_to_absolute( + &XmlString::new("not_a_test.txt"), + &bundle_repo(temp_dir.as_ref()), + &XmlString::new("test"), + ); + assert_eq!(actual, None); + } + + #[test] + fn test_convert_to_absolute_when_one_file_matches() { + let temp_dir = TempDir::with_prefix("not-hidden").unwrap(); + let inner_dir = "inner_dir"; + fs::create_dir(temp_dir.as_ref().join(inner_dir)).unwrap(); + let file_path_1 = temp_dir.as_ref().join(inner_dir).join("file_1.txt"); + let text_1 = r#"it("test_1")"#; + fs::write(file_path_1.clone(), text_1).unwrap(); + let file_path_2 = temp_dir.as_ref().join(inner_dir).join("file_2.txt"); + let text_2 = r#"it("test_2")"#; + fs::write(file_path_2.clone(), text_2).unwrap(); + let file_path_3 = temp_dir.as_ref().join(inner_dir).join("file_3.txt"); + let text_3 = r#"it("test_3")"#; + fs::write(file_path_3.clone(), text_3).unwrap(); + let actual = convert_to_absolute( + &XmlString::new("file_1.txt"), + &bundle_repo(temp_dir.as_ref()), + &XmlString::new("test"), + ); + assert_eq!(actual, Some(stringify(file_path_1))); + } + + #[test] + fn test_convert_to_absolute_when_many_files_match_and_none_contain() { + let temp_dir = TempDir::with_prefix("not-hidden").unwrap(); + let inner_dir = "inner_dir"; + let other_dir = "other_dir"; + fs::create_dir(temp_dir.as_ref().join(inner_dir)).unwrap(); + fs::create_dir(temp_dir.as_ref().join(other_dir)).unwrap(); + let file_path_1 = temp_dir.as_ref().join(inner_dir).join("file.txt"); + let text_1 = r#"it("test_1")"#; + fs::write(file_path_1.clone(), text_1).unwrap(); + let file_path_2 = temp_dir.as_ref().join(other_dir).join("file.txt"); + let text_2 = r#"it("test_2")"#; + fs::write(file_path_2.clone(), text_2).unwrap(); + let actual = convert_to_absolute( + &XmlString::new("file.txt"), + &bundle_repo(temp_dir.as_ref()), + &XmlString::new("totally_different"), + ); + assert_eq!(actual, None); + } + + #[test] + fn test_convert_to_absolute_when_many_files_match_and_one_contains() { + let temp_dir = TempDir::with_prefix("not-hidden").unwrap(); + let inner_dir = "inner_dir"; + let other_dir = "other_dir"; + fs::create_dir(temp_dir.as_ref().join(inner_dir)).unwrap(); + fs::create_dir(temp_dir.as_ref().join(other_dir)).unwrap(); + let file_path_1 = temp_dir.as_ref().join(inner_dir).join("file.txt"); + let text_1 = r#"it("test_1")"#; + fs::write(file_path_1.clone(), text_1).unwrap(); + let file_path_2 = temp_dir.as_ref().join(other_dir).join("file.txt"); + let text_2 = r#"it("test_2")"#; + fs::write(file_path_2.clone(), text_2).unwrap(); + let actual = convert_to_absolute( + &XmlString::new("file.txt"), + &bundle_repo(temp_dir.as_ref()), + &XmlString::new("test_1"), + ); + assert_eq!(actual, Some(stringify(file_path_1))); + } + + #[test] + fn test_convert_to_absolute_when_only_match_is_in_hidden_directory() { + let temp_dir = TempDir::with_prefix("not-hidden").unwrap(); + let hidden_dir = ".hidden"; + fs::create_dir(temp_dir.as_ref().join(hidden_dir)).unwrap(); + let file_path = temp_dir.as_ref().join(hidden_dir).join("test.txt"); + let text = r#"it("test")"#; + fs::write(file_path.clone(), text).unwrap(); + let actual = convert_to_absolute( + &XmlString::new("test.txt"), + &bundle_repo(temp_dir.as_ref()), + &XmlString::new("test"), + ); + assert_eq!(actual, None); + } +} diff --git a/context/src/junit/mod.rs b/context/src/junit/mod.rs index 23716c81..70457f9a 100644 --- a/context/src/junit/mod.rs +++ b/context/src/junit/mod.rs @@ -1,6 +1,7 @@ #[cfg(feature = "bindings")] pub mod bindings; mod date_parser; +mod file_extractor; pub mod junit_path; pub mod parser; pub mod validator; diff --git a/context/src/junit/parser.rs b/context/src/junit/parser.rs index 27e586c0..3d85cbd1 100644 --- a/context/src/junit/parser.rs +++ b/context/src/junit/parser.rs @@ -21,6 +21,7 @@ use thiserror::Error; use wasm_bindgen::prelude::*; use super::date_parser::JunitDateParser; +use crate::{junit::file_extractor::filename_for_test_case, repo::BundleRepo}; const TAG_REPORT: &[u8] = b"testsuites"; const TAG_TEST_SUITE: &[u8] = b"testsuite"; @@ -192,11 +193,16 @@ impl JunitParser { self.reports } - pub fn into_test_case_runs(self, codeowners: Option<&CodeOwners>) -> Vec { + pub fn into_test_case_runs( + self, + codeowners: Option<&CodeOwners>, + repo: &BundleRepo, + ) -> Vec { let mut test_case_runs = Vec::new(); for report in self.reports { for test_suite in report.test_suites { for test_case in test_suite.test_cases { + let file = filename_for_test_case(&test_case, repo); let mut test_case_run = TestCaseRun { name: test_case.name.into(), parent_name: test_suite.name.to_string(), @@ -259,12 +265,6 @@ impl JunitParser { TestCaseRunStatus::Failure.into() } }; - let file = test_case - .extra - .get(extra_attrs::FILE) - .or_else(|| test_case.extra.get(extra_attrs::FILEPATH)) - .map(|v| v.to_string()) - .unwrap_or_default(); if !file.is_empty() && codeowners.is_some() { let codeowners: Option> = codeowners .as_ref() @@ -854,7 +854,7 @@ mod tests { use prost_wkt_types::Timestamp; use proto::test_context::test_run::TestCaseRunStatus; - use crate::junit::parser::JunitParser; + use crate::{junit::parser::JunitParser, repo::BundleRepo}; #[test] fn test_into_test_case_runs() { let mut junit_parser = JunitParser::new(); @@ -862,12 +862,12 @@ mod tests { - + but was: ]]> - + @@ -875,7 +875,7 @@ mod tests { "#; let parsed_results = junit_parser.parse(BufReader::new(file_contents.as_bytes())); assert!(parsed_results.is_ok()); - let test_case_runs = junit_parser.into_test_case_runs(None); + let test_case_runs = junit_parser.into_test_case_runs(None, &BundleRepo::default()); assert_eq!(test_case_runs.len(), 2); let test_case_run1 = &test_case_runs[0]; assert_eq!(test_case_run1.name, "test_variant_truncation1"); @@ -886,7 +886,7 @@ mod tests { test_case_run1.status_output_message, "Expected: but was: " ); - assert_eq!(test_case_run1.file, "test.java"); + assert_eq!(test_case_run1.file, "/test.java"); assert_eq!(test_case_run1.attempt_number, 0); assert!(!test_case_run1.is_quarantined); assert_eq!( @@ -911,7 +911,7 @@ mod tests { assert_eq!(test_case_run2.classname, ""); assert_eq!(test_case_run2.status, TestCaseRunStatus::Failure as i32); assert_eq!(test_case_run2.status_output_message, "Test failed"); - assert_eq!(test_case_run2.file, "test.java"); + assert_eq!(test_case_run2.file, "/test.java"); assert_eq!(test_case_run2.attempt_number, 0); assert!(!test_case_run2.is_quarantined); assert_eq!( diff --git a/context/src/junit/validator.rs b/context/src/junit/validator.rs index 84029a48..b4d64006 100644 --- a/context/src/junit/validator.rs +++ b/context/src/junit/validator.rs @@ -10,7 +10,8 @@ use thiserror::Error; #[cfg(feature = "wasm")] use wasm_bindgen::prelude::*; -use super::parser::extra_attrs; +use crate::junit::file_extractor::filename_for_test_case; +use crate::repo::BundleRepo; use crate::string_safety::{validate_field_len, FieldLen}; pub const MAX_FIELD_LEN: usize = 1_000; @@ -63,7 +64,7 @@ impl Default for JunitValidationType { } } -pub fn validate(report: &Report) -> JunitReportValidation { +pub fn validate(report: &Report, repo: &BundleRepo) -> JunitReportValidation { let mut report_validation = JunitReportValidation::default(); for test_suite in report.test_suites.iter() { @@ -101,15 +102,7 @@ pub fn validate(report: &Report) -> JunitReportValidation { } }; - match validate_field_len::( - test_case - .extra - .get(extra_attrs::FILE) - .or(test_case.extra.get(extra_attrs::FILEPATH)) - .as_ref() - .map(|s| s.as_str()) - .unwrap_or_default(), - ) { + match validate_field_len::(filename_for_test_case(test_case, repo)) { FieldLen::Valid => (), FieldLen::TooShort(s) => { test_case_validation.add_issue(JunitValidationIssue::SubOptimal( diff --git a/context/tests/junit.rs b/context/tests/junit.rs index 85a834cc..bd499fbb 100644 --- a/context/tests/junit.rs +++ b/context/tests/junit.rs @@ -1,15 +1,18 @@ use std::{fs, io::BufReader, time::Duration}; use chrono::{NaiveTime, TimeDelta, Utc}; -use context::junit::{ - self, - parser::JunitParser, - validator::{ - JunitTestCaseValidationIssue, JunitTestCaseValidationIssueInvalid, - JunitTestCaseValidationIssueSubOptimal, JunitTestSuiteValidationIssue, - JunitTestSuiteValidationIssueInvalid, JunitTestSuiteValidationIssueSubOptimal, - JunitValidationIssue, JunitValidationLevel, +use context::{ + junit::{ + self, + parser::JunitParser, + validator::{ + JunitTestCaseValidationIssue, JunitTestCaseValidationIssueInvalid, + JunitTestCaseValidationIssueSubOptimal, JunitTestSuiteValidationIssue, + JunitTestSuiteValidationIssueInvalid, JunitTestSuiteValidationIssueSubOptimal, + JunitValidationIssue, JunitValidationLevel, + }, }, + repo::BundleRepo, }; use junit_mock::JunitMock; use quick_junit::Report; @@ -54,7 +57,8 @@ fn generate_mock_junit_reports( let mut jm = JunitMock::new(options); let seed = jm.get_seed(); - let reports = jm.generate_reports(); + let tmp_dir: Option = None; + let reports = jm.generate_reports(&tmp_dir); (seed, reports) } @@ -87,7 +91,7 @@ fn validate_test_suite_name_too_short() { test_suite.name = String::new().into(); } - let report_validation = junit::validator::validate(&generated_report); + let report_validation = junit::validator::validate(&generated_report, &BundleRepo::default()); assert_eq!( report_validation.max_level(), @@ -123,7 +127,7 @@ fn validate_test_case_name_too_short() { } } - let report_validation = junit::validator::validate(&generated_report); + let report_validation = junit::validator::validate(&generated_report, &BundleRepo::default()); assert_eq!( report_validation.max_level(), @@ -156,7 +160,7 @@ fn validate_test_suite_name_too_long() { test_suite.name = "a".repeat(junit::validator::MAX_FIELD_LEN + 1).into(); } - let report_validation = junit::validator::validate(&generated_report); + let report_validation = junit::validator::validate(&generated_report, &BundleRepo::default()); assert_eq!( report_validation.max_level(), @@ -192,7 +196,7 @@ fn validate_test_case_name_too_long() { } } - let report_validation = junit::validator::validate(&generated_report); + let report_validation = junit::validator::validate(&generated_report, &BundleRepo::default()); assert_eq!( report_validation.max_level(), @@ -230,7 +234,7 @@ fn validate_max_level() { } } - let report_validation = junit::validator::validate(&generated_report); + let report_validation = junit::validator::validate(&generated_report, &BundleRepo::default()); assert_eq!( report_validation.max_level(), @@ -305,7 +309,8 @@ fn parse_round_trip_and_validate_fuzzed() { for (index, generated_report) in generated_reports.iter().enumerate() { let serialized_generated_report = serialize_report(generated_report); let first_parsed_report = parse_report(&serialized_generated_report); - let report_validation = junit::validator::validate(&first_parsed_report); + let report_validation = + junit::validator::validate(&first_parsed_report, &BundleRepo::default()); assert_eq!( report_validation.max_level(), @@ -336,7 +341,8 @@ fn parse_round_trip_and_validate_fuzzed() { fn parse_without_testsuites_element() { let options = new_mock_junit_options(1, Some(1), Some(1), true); let mut jm = JunitMock::new(options); - let reports = jm.generate_reports(); + let tmp_dir: Option = None; + let reports = jm.generate_reports(&tmp_dir); let tempdir = TempDir::new().unwrap(); let xml_path = jm diff --git a/junit-mock/src/lib.rs b/junit-mock/src/lib.rs index 5ab74d3c..ba197e09 100644 --- a/junit-mock/src/lib.rs +++ b/junit-mock/src/lib.rs @@ -254,7 +254,7 @@ impl JunitMock { self.timestamp += duration; } - pub fn generate_reports(&mut self) -> Vec { + pub fn generate_reports>(&mut self, tmp_dir: &Option) -> Vec { self.timestamp = self .options .global @@ -280,7 +280,7 @@ impl JunitMock { let mut report = Report::new(report_name); report.set_timestamp(self.timestamp); self.total_duration = Duration::new(0, 0); - report.add_test_suites(self.generate_test_suites()); + report.add_test_suites(self.generate_test_suites(tmp_dir)); report.set_time(self.total_duration); let duration = self.fake_duration(self.options.report.report_duration_range.clone()); @@ -337,7 +337,7 @@ impl JunitMock { Ok(()) } - fn generate_test_suites(&mut self) -> Vec { + fn generate_test_suites>(&mut self, tmp_dir: &Option) -> Vec { self.options .test_suite .test_suite_names @@ -357,7 +357,7 @@ impl JunitMock { let mut test_suite = TestSuite::new(test_suite_name); test_suite.set_timestamp(self.timestamp); let last_duration = self.total_duration; - test_suite.add_test_cases(self.generate_test_cases()); + test_suite.add_test_cases(self.generate_test_cases(tmp_dir)); test_suite.set_time(self.total_duration - last_duration); if self.rand_bool(self.options.test_suite.test_suite_sys_out_percentage) { test_suite.set_system_out(self.fake_paragraphs()); @@ -370,7 +370,7 @@ impl JunitMock { .collect() } - fn generate_test_cases(&mut self) -> Vec { + fn generate_test_cases>(&mut self, tmp_dir: &Option) -> Vec { let classnames = self .options .test_case @@ -383,7 +383,7 @@ impl JunitMock { }) .unwrap_or_else(|| { (0..self.options.test_case.test_case_random_count) - .map(|_| fake::faker::filesystem::en::DirPath().fake_with_rng(&mut self.rng)) + .map(|_| fake::faker::lorem::en::Word().fake_with_rng(&mut self.rng)) .collect() }); @@ -411,8 +411,19 @@ impl JunitMock { let is_skipped = matches!(&test_case_status, TestCaseStatus::Skipped { .. }); let mut test_case = TestCase::new(test_case_name, test_case_status); - let file: String = - fake::faker::filesystem::en::FilePath().fake_with_rng(&mut self.rng); + let file: String = if let Some(parent_dir) = tmp_dir { + let path = parent_dir + .as_ref() + .join::(fake::faker::lorem::en::Word().fake_with_rng(&mut self.rng)) + .join::( + fake::faker::filesystem::en::FileName().fake_with_rng(&mut self.rng), + ); + std::fs::create_dir_all(path.clone().parent().unwrap()).unwrap(); + std::fs::File::create_new(path.clone()).unwrap(); + String::from(path.clone().as_os_str().to_str().unwrap()) + } else { + fake::faker::filesystem::en::FilePath().fake_with_rng(&mut self.rng) + }; test_case.extra.insert("file".into(), file.into()); test_case.set_classname(format!("{test_case_classname}/{test_case_name}")); test_case.set_assertions(self.rng.gen_range(1..10)); diff --git a/junit-mock/src/main.rs b/junit-mock/src/main.rs index d8c2fbd6..3d7dc617 100644 --- a/junit-mock/src/main.rs +++ b/junit-mock/src/main.rs @@ -20,7 +20,8 @@ fn main() -> Result<()> { let mut jm = JunitMock::new(options); println!("Using seed `{}` to generate random data.", jm.get_seed()); - let reports = jm.generate_reports(); + let tmp_dir: Option = None; + let reports = jm.generate_reports(&tmp_dir); jm.write_reports_to_file(directory, &reports)?; From 5b377e4edc377f0f9a680cebc8a0a79bbb90b4ee Mon Sep 17 00:00:00 2001 From: Christian Millar Date: Tue, 3 Jun 2025 16:21:45 +0000 Subject: [PATCH 02/14] Lint errors --- context/src/junit/file_extractor.rs | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/context/src/junit/file_extractor.rs b/context/src/junit/file_extractor.rs index 490a2f05..bc4dfc93 100644 --- a/context/src/junit/file_extractor.rs +++ b/context/src/junit/file_extractor.rs @@ -11,7 +11,7 @@ fn not_hidden(entry: &DirEntry) -> bool { entry .file_name() .to_str() - .map(|s| !s.starts_with(".")) + .map(|file_string| !file_string.starts_with(".")) .unwrap_or(true) } @@ -26,11 +26,7 @@ fn contains_test(path: &String, test_xml_name: &XmlString) -> bool { .map(|text| { let has_full_name = text.contains(test_name); let has_name_splits = test_parts.all(|p| text.contains(p)); - if has_full_name || has_name_splits { - true - } else { - false - } + has_full_name || has_name_splits }) .unwrap_or(false) } @@ -38,7 +34,7 @@ fn contains_test(path: &String, test_xml_name: &XmlString) -> bool { fn file_containing_tests(file_paths: Vec, test_name: &XmlString) -> Option { let mut matching_paths = file_paths .iter() - .filter(|path| contains_test(*path, test_name)); + .filter(|path| contains_test(path, test_name)); let first_match = matching_paths.next(); let another_match = matching_paths.next(); match (first_match, another_match) { @@ -57,7 +53,6 @@ fn convert_to_absolute( let initial_str = String::from(initial.as_str()); let path = Path::new(&initial_str); if path.is_absolute() { - println!("Path {:?} was absolute", path); path.to_str().map(String::from) } else if Path::new(&repo.repo_root).is_absolute() { let mut walk = WalkDir::new(repo.repo_root.clone()) @@ -90,19 +85,14 @@ fn convert_to_absolute( } pub fn filename_for_test_case(test_case: &TestCase, repo: &BundleRepo) -> String { - println!( - "!!!!!!!!!!!!!!!!! File {:?} Filepath {:?} Classname {:?}", - test_case.extra.get(extra_attrs::FILE), - test_case.extra.get(extra_attrs::FILEPATH), - test_case.classname - ); test_case .extra .get(extra_attrs::FILE) .or(test_case.extra.get(extra_attrs::FILEPATH)) .or(test_case.classname.as_ref()) - .map(|s| convert_to_absolute(s, repo, &test_case.name)) - .flatten() + .iter() + .flat_map(|s| convert_to_absolute(s, repo, &test_case.name)) + .next() .unwrap_or_default() } From 9487cfb2496ac4180bce1075d51d7e593f3101c4 Mon Sep 17 00:00:00 2001 From: Christian Millar Date: Wed, 4 Jun 2025 18:45:26 +0000 Subject: [PATCH 03/14] Fix failing tests --- junit-mock/src/lib.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/junit-mock/src/lib.rs b/junit-mock/src/lib.rs index ba197e09..b7f8464f 100644 --- a/junit-mock/src/lib.rs +++ b/junit-mock/src/lib.rs @@ -419,7 +419,9 @@ impl JunitMock { fake::faker::filesystem::en::FileName().fake_with_rng(&mut self.rng), ); std::fs::create_dir_all(path.clone().parent().unwrap()).unwrap(); - std::fs::File::create_new(path.clone()).unwrap(); + if !path.exists() { + std::fs::File::create_new(path.clone()).unwrap(); + } String::from(path.clone().as_os_str().to_str().unwrap()) } else { fake::faker::filesystem::en::FilePath().fake_with_rng(&mut self.rng) From 54ccca6e0078a22dddf7a889e97b0c63654ed314 Mon Sep 17 00:00:00 2001 From: Christian Millar Date: Thu, 5 Jun 2025 17:19:49 +0000 Subject: [PATCH 04/14] Relativise paths --- context/src/junit/file_extractor.rs | 63 +++++++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 3 deletions(-) diff --git a/context/src/junit/file_extractor.rs b/context/src/junit/file_extractor.rs index bc4dfc93..85f3d556 100644 --- a/context/src/junit/file_extractor.rs +++ b/context/src/junit/file_extractor.rs @@ -44,7 +44,10 @@ fn file_containing_tests(file_paths: Vec, test_name: &XmlString) -> Opti } } -// None if is not a file or file does not exist, Some(absolute path) if it does exist in the root +// None if is not a path in the root, +// Some(the path) if it's already absolute +// Some(absolute path) if it does exist in the root +// Some(relative_path) if we cannot walk the root and the relative path ends in an extension fn convert_to_absolute( initial: &XmlString, repo: &BundleRepo, @@ -52,9 +55,10 @@ fn convert_to_absolute( ) -> Option { let initial_str = String::from(initial.as_str()); let path = Path::new(&initial_str); + let repo_root_path = Path::new(&repo.repo_root); if path.is_absolute() { path.to_str().map(String::from) - } else if Path::new(&repo.repo_root).is_absolute() { + } else if repo_root_path.is_absolute() && repo_root_path.exists() { let mut walk = WalkDir::new(repo.repo_root.clone()) .into_iter() .filter_entry(not_hidden) @@ -79,11 +83,27 @@ fn convert_to_absolute( test_name, ), } + } else if path + .file_name() + .iter() + .flat_map(|os| os.to_str()) + .all(|name| name.contains(".")) + { + Some(initial_str) } else { None } } +// If the repo root is in the path, strips it out. If not, returns the original path. +fn relativise_to_root(base_path: String, repo: &BundleRepo) -> String { + let path = Path::new(&base_path); + match path.strip_prefix(repo.repo_root.clone()) { + Ok(relativised) => relativised.to_str().map(String::from).unwrap_or(base_path), + Err(_) => base_path, + } +} + pub fn filename_for_test_case(test_case: &TestCase, repo: &BundleRepo) -> String { test_case .extra @@ -92,6 +112,7 @@ pub fn filename_for_test_case(test_case: &TestCase, repo: &BundleRepo) -> String .or(test_case.classname.as_ref()) .iter() .flat_map(|s| convert_to_absolute(s, repo, &test_case.name)) + .map(|base_path| relativise_to_root(base_path, repo)) .next() .unwrap_or_default() } @@ -260,7 +281,7 @@ mod tests { } #[test] - fn test_convert_to_absolute_when_no_repo_root() { + fn test_convert_to_absolute_when_no_repo_root_but_given_file() { let temp_dir = TempDir::with_prefix("not-hidden").unwrap(); let file_path = temp_dir.as_ref().join("test.txt"); let text = r#"it("test")"#; @@ -270,6 +291,20 @@ mod tests { &BundleRepo::default(), &XmlString::new("test"), ); + assert_eq!(actual, Some(String::from("test.txt"))); + } + + #[test] + fn test_convert_to_absolute_when_no_repo_root_and_not_a_file() { + let temp_dir = TempDir::with_prefix("not-hidden").unwrap(); + let file_path = temp_dir.as_ref().join("test.txt"); + let text = r#"it("test")"#; + fs::write(file_path.clone(), text).unwrap(); + let actual = convert_to_absolute( + &XmlString::new("not_a_file"), + &BundleRepo::default(), + &XmlString::new("test"), + ); assert_eq!(actual, None); } @@ -386,4 +421,26 @@ mod tests { ); assert_eq!(actual, None); } + + #[test] + fn relativise_to_root_if_no_repo() { + let initial = String::from("/my/full/path.txt"); + let actual = relativise_to_root(initial.clone(), &BundleRepo::default()); + assert_eq!(actual, initial); + } + + #[test] + fn relativise_to_root_if_repo_not_a_prefix() { + let initial = String::from("/my/full/path.txt"); + let actual = + relativise_to_root(initial.clone(), &bundle_repo(Path::new("/something/else"))); + assert_eq!(actual, initial); + } + + #[test] + fn relativise_to_root_if_repo_is_a_prefix() { + let initial = String::from("/my/full/path.txt"); + let actual = relativise_to_root(initial.clone(), &bundle_repo(Path::new("/my/full"))); + assert_eq!(actual, String::from("path.txt")); + } } From 729b3e98c636b8dfe8dc610c6df08833a3b77470 Mon Sep 17 00:00:00 2001 From: Christian Millar Date: Thu, 5 Jun 2025 20:27:01 +0000 Subject: [PATCH 05/14] Test that uploaded file has the relative-to-root file listing --- cli-tests/src/upload.rs | 53 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/cli-tests/src/upload.rs b/cli-tests/src/upload.rs index 21257d58..9f355b9f 100644 --- a/cli-tests/src/upload.rs +++ b/cli-tests/src/upload.rs @@ -1,3 +1,4 @@ +use std::io::Write; use std::sync::{Arc, Mutex}; use std::{fs, io::BufReader}; @@ -1218,7 +1219,7 @@ enum CreateBundleResponse { } #[tokio::test(flavor = "multi_thread")] -async fn do_not_quarantines_tests_when_quarantine_disabled_set() { +async fn do_not_quarantine_tests_when_quarantine_disabled_set() { let temp_dir = tempdir().unwrap(); generate_mock_git_repo(&temp_dir); generate_mock_valid_junit_xmls(&temp_dir); @@ -1329,3 +1330,53 @@ async fn do_not_quarantines_tests_when_quarantine_disabled_set() { *QUARANTINE_CONFIG_RESPONSE.lock().unwrap() = QuarantineConfigResponse::Disabled; command.assert().failure(); } + +#[tokio::test(flavor = "multi_thread")] +async fn uploaded_file_contains_updated_test_files() { + let temp_dir = TempDir::with_prefix("not_hidden").unwrap(); + generate_mock_git_repo(&temp_dir); + + let inner_dir = temp_dir.path().join("inner_dir"); + fs::create_dir(inner_dir).unwrap(); + + let test_location = temp_dir.path().join("inner_dir").join("test_file.ts"); + let mut test_file = fs::File::create(test_location).unwrap(); + write!(test_file, r#"it("does stuff", x)"#).unwrap(); + + let junit_location = temp_dir.path().join("junit_file.xml"); + let mut junit_file = fs::File::create(junit_location).unwrap(); + write!(junit_file, r#" + + + + + + + + "#).unwrap(); + + let state = MockServerBuilder::new().spawn_mock_server().await; + let assert = CommandBuilder::upload(temp_dir.path(), state.host.clone()) + .junit_paths("junit_file.xml") + .command() + .assert() + .success(); + + let requests = state.requests.lock().unwrap().clone(); + assert_eq!(requests.len(), 3); + + let file_upload = assert_matches!(requests.get(1).unwrap(), RequestPayload::S3Upload(d) => d); + let file = fs::File::open(file_upload.join("meta.json")).unwrap(); + let reader = BufReader::new(file); + let bundle_meta: BundleMeta = serde_json::from_reader(reader).unwrap(); + let internal_bundled_file = bundle_meta.internal_bundled_file.as_ref().unwrap(); + let bin = fs::read(file_upload.join(&internal_bundled_file.path)).unwrap(); + let report = proto::test_context::test_run::TestResult::decode(&*bin).unwrap(); + + assert_eq!(report.test_case_runs.len(), 1); + let test_case_run = &report.test_case_runs.first().unwrap(); + assert_eq!(test_case_run.classname, "test_file.ts"); + assert_eq!(test_case_run.file, "inner_dir/test_file.ts"); + + println!("{assert}"); +} From d93a9d3459800c4f16b90bfa6175c2e8b80fa4fa Mon Sep 17 00:00:00 2001 From: Christian Millar Date: Fri, 6 Jun 2025 15:53:50 +0000 Subject: [PATCH 06/14] lint errors --- context-js/src/lib.rs | 5 ++++- context/src/junit/file_extractor.rs | 6 +++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/context-js/src/lib.rs b/context-js/src/lib.rs index 27c6b844..9de7a18b 100644 --- a/context-js/src/lib.rs +++ b/context-js/src/lib.rs @@ -4,7 +4,10 @@ use bundle::{ parse_internal_bin_from_tarball as parse_internal_bin, parse_meta_from_tarball as parse_tarball, VersionedBundle, }; -use context::{env, junit, repo::{self, BundleRepo}}; +use context::{ + env, junit, + repo::{self, BundleRepo}, +}; use futures::{future::Either, io::BufReader as BufReaderAsync, stream::TryStreamExt}; use js_sys::Uint8Array; use prost::Message; diff --git a/context/src/junit/file_extractor.rs b/context/src/junit/file_extractor.rs index 85f3d556..4dd0abb7 100644 --- a/context/src/junit/file_extractor.rs +++ b/context/src/junit/file_extractor.rs @@ -11,7 +11,7 @@ fn not_hidden(entry: &DirEntry) -> bool { entry .file_name() .to_str() - .map(|file_string| !file_string.starts_with(".")) + .map(|file_string| !file_string.starts_with('.')) .unwrap_or(true) } @@ -79,7 +79,7 @@ fn convert_to_absolute( (None, _) => None, (Some(only_match), None) => Some(only_match), (Some(first_match), Some(second_match)) => file_containing_tests( - vec![vec![first_match, second_match], walk.collect()].concat(), + [vec![first_match, second_match], walk.collect()].concat(), test_name, ), } @@ -87,7 +87,7 @@ fn convert_to_absolute( .file_name() .iter() .flat_map(|os| os.to_str()) - .all(|name| name.contains(".")) + .all(|name| name.contains('.')) { Some(initial_str) } else { From b1f26fbb0d0650083b81bf479de8d89aa484f717 Mon Sep 17 00:00:00 2001 From: Christian Millar Date: Fri, 6 Jun 2025 21:32:57 +0000 Subject: [PATCH 07/14] Revert "Relativise paths" This reverts commit 54ccca6e0078a22dddf7a889e97b0c63654ed314. --- cli-tests/src/upload.rs | 9 ++++- context/src/junit/file_extractor.rs | 53 ++--------------------------- 2 files changed, 10 insertions(+), 52 deletions(-) diff --git a/cli-tests/src/upload.rs b/cli-tests/src/upload.rs index 53466574..82d8a53f 100644 --- a/cli-tests/src/upload.rs +++ b/cli-tests/src/upload.rs @@ -1491,7 +1491,14 @@ async fn uploaded_file_contains_updated_test_files() { assert_eq!(report.test_case_runs.len(), 1); let test_case_run = &report.test_case_runs.first().unwrap(); assert_eq!(test_case_run.classname, "test_file.ts"); - assert_eq!(test_case_run.file, "inner_dir/test_file.ts"); + let expected_file = temp_dir + .path() + .join("inner_dir") + .join("test_file.ts") + .to_str() + .unwrap() + .to_string(); + assert_eq!(test_case_run.file, expected_file); println!("{assert}"); } diff --git a/context/src/junit/file_extractor.rs b/context/src/junit/file_extractor.rs index 4dd0abb7..0bf9f853 100644 --- a/context/src/junit/file_extractor.rs +++ b/context/src/junit/file_extractor.rs @@ -44,10 +44,7 @@ fn file_containing_tests(file_paths: Vec, test_name: &XmlString) -> Opti } } -// None if is not a path in the root, -// Some(the path) if it's already absolute -// Some(absolute path) if it does exist in the root -// Some(relative_path) if we cannot walk the root and the relative path ends in an extension +// None if is not a file or file does not exist, Some(absolute path) if it does exist in the root fn convert_to_absolute( initial: &XmlString, repo: &BundleRepo, @@ -95,15 +92,6 @@ fn convert_to_absolute( } } -// If the repo root is in the path, strips it out. If not, returns the original path. -fn relativise_to_root(base_path: String, repo: &BundleRepo) -> String { - let path = Path::new(&base_path); - match path.strip_prefix(repo.repo_root.clone()) { - Ok(relativised) => relativised.to_str().map(String::from).unwrap_or(base_path), - Err(_) => base_path, - } -} - pub fn filename_for_test_case(test_case: &TestCase, repo: &BundleRepo) -> String { test_case .extra @@ -112,7 +100,6 @@ pub fn filename_for_test_case(test_case: &TestCase, repo: &BundleRepo) -> String .or(test_case.classname.as_ref()) .iter() .flat_map(|s| convert_to_absolute(s, repo, &test_case.name)) - .map(|base_path| relativise_to_root(base_path, repo)) .next() .unwrap_or_default() } @@ -281,7 +268,7 @@ mod tests { } #[test] - fn test_convert_to_absolute_when_no_repo_root_but_given_file() { + fn test_convert_to_absolute_when_no_repo_root() { let temp_dir = TempDir::with_prefix("not-hidden").unwrap(); let file_path = temp_dir.as_ref().join("test.txt"); let text = r#"it("test")"#; @@ -294,20 +281,6 @@ mod tests { assert_eq!(actual, Some(String::from("test.txt"))); } - #[test] - fn test_convert_to_absolute_when_no_repo_root_and_not_a_file() { - let temp_dir = TempDir::with_prefix("not-hidden").unwrap(); - let file_path = temp_dir.as_ref().join("test.txt"); - let text = r#"it("test")"#; - fs::write(file_path.clone(), text).unwrap(); - let actual = convert_to_absolute( - &XmlString::new("not_a_file"), - &BundleRepo::default(), - &XmlString::new("test"), - ); - assert_eq!(actual, None); - } - #[test] fn test_convert_to_absolute_when_already_absolute() { let temp_dir = TempDir::with_prefix("not-hidden").unwrap(); @@ -421,26 +394,4 @@ mod tests { ); assert_eq!(actual, None); } - - #[test] - fn relativise_to_root_if_no_repo() { - let initial = String::from("/my/full/path.txt"); - let actual = relativise_to_root(initial.clone(), &BundleRepo::default()); - assert_eq!(actual, initial); - } - - #[test] - fn relativise_to_root_if_repo_not_a_prefix() { - let initial = String::from("/my/full/path.txt"); - let actual = - relativise_to_root(initial.clone(), &bundle_repo(Path::new("/something/else"))); - assert_eq!(actual, initial); - } - - #[test] - fn relativise_to_root_if_repo_is_a_prefix() { - let initial = String::from("/my/full/path.txt"); - let actual = relativise_to_root(initial.clone(), &bundle_repo(Path::new("/my/full"))); - assert_eq!(actual, String::from("path.txt")); - } } From c6f76179ecaec98faa47df3305aa74dd166e5337 Mon Sep 17 00:00:00 2001 From: Christian Millar Date: Mon, 9 Jun 2025 15:50:29 +0000 Subject: [PATCH 08/14] Test failure from merge --- context/tests/junit.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/context/tests/junit.rs b/context/tests/junit.rs index c939a8b3..382e4af1 100644 --- a/context/tests/junit.rs +++ b/context/tests/junit.rs @@ -306,7 +306,7 @@ fn validate_timestamps() { } } - let report_validation = junit::validator::validate(&generated_report); + let report_validation = junit::validator::validate(&generated_report, &BundleRepo::default()); assert_eq!( report_validation.max_level(), From 0f75674d98f996b4678a5c9eef4a2629747b466e Mon Sep 17 00:00:00 2001 From: Christian Millar Date: Mon, 9 Jun 2025 17:39:28 +0000 Subject: [PATCH 09/14] Printlns for the mac build --- .github/actions/run_tests/action.yaml | 2 +- cli-tests/src/upload.rs | 5 +++-- context/src/junit/file_extractor.rs | 1 + 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/.github/actions/run_tests/action.yaml b/.github/actions/run_tests/action.yaml index 9c654461..d6fe1496 100644 --- a/.github/actions/run_tests/action.yaml +++ b/.github/actions/run_tests/action.yaml @@ -24,4 +24,4 @@ runs: - name: Run tests shell: bash - run: cargo nextest run --features=bindings --workspace ${{ contains(inputs.target, 'musl') && '--exclude context-js --exclude context-py --exclude context_ruby' || '' }} --profile=ci + run: cargo nextest run --features=bindings --workspace ${{ contains(inputs.target, 'musl') && '--exclude context-js --exclude context-py --exclude context_ruby' || '' }} --profile=ci --nocapture diff --git a/cli-tests/src/upload.rs b/cli-tests/src/upload.rs index 82d8a53f..98c67b4c 100644 --- a/cli-tests/src/upload.rs +++ b/cli-tests/src/upload.rs @@ -1450,6 +1450,7 @@ async fn uses_passed_exit_code_if_unquarantined_tests_fail() { async fn uploaded_file_contains_updated_test_files() { let temp_dir = TempDir::with_prefix("not_hidden").unwrap(); generate_mock_git_repo(&temp_dir); + println!("Tmp dir path is {:?}", temp_dir); let inner_dir = temp_dir.path().join("inner_dir"); fs::create_dir(inner_dir).unwrap(); @@ -1488,6 +1489,8 @@ async fn uploaded_file_contains_updated_test_files() { let bin = fs::read(file_upload.join(&internal_bundled_file.path)).unwrap(); let report = proto::test_context::test_run::TestResult::decode(&*bin).unwrap(); + println!("{assert}"); + assert_eq!(report.test_case_runs.len(), 1); let test_case_run = &report.test_case_runs.first().unwrap(); assert_eq!(test_case_run.classname, "test_file.ts"); @@ -1499,6 +1502,4 @@ async fn uploaded_file_contains_updated_test_files() { .unwrap() .to_string(); assert_eq!(test_case_run.file, expected_file); - - println!("{assert}"); } diff --git a/context/src/junit/file_extractor.rs b/context/src/junit/file_extractor.rs index 0bf9f853..60bb2e84 100644 --- a/context/src/junit/file_extractor.rs +++ b/context/src/junit/file_extractor.rs @@ -53,6 +53,7 @@ fn convert_to_absolute( let initial_str = String::from(initial.as_str()); let path = Path::new(&initial_str); let repo_root_path = Path::new(&repo.repo_root); + println!("Had a repo_root of {:?}", repo.repo_root); if path.is_absolute() { path.to_str().map(String::from) } else if repo_root_path.is_absolute() && repo_root_path.exists() { From 319212fc3b18ce7fcd759f8b3776555c3f0c13e7 Mon Sep 17 00:00:00 2001 From: Christian Millar Date: Mon, 9 Jun 2025 19:07:09 +0000 Subject: [PATCH 10/14] seems that tempfile isn't returning a totally-absolute path in test --- cli-tests/src/upload.rs | 8 +++++++- context/src/junit/file_extractor.rs | 1 - 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/cli-tests/src/upload.rs b/cli-tests/src/upload.rs index 21648a8e..bd3297bd 100644 --- a/cli-tests/src/upload.rs +++ b/cli-tests/src/upload.rs @@ -1474,7 +1474,11 @@ async fn uses_passed_exit_code_if_unquarantined_tests_fail() { async fn uploaded_file_contains_updated_test_files() { let temp_dir = TempDir::with_prefix("not_hidden").unwrap(); generate_mock_git_repo(&temp_dir); - println!("Tmp dir path is {:?}", temp_dir); + println!( + "Tmp dir path is {:?}, canonicalized {:?}", + temp_dir, + temp_dir.canonicalize() + ); let inner_dir = temp_dir.path().join("inner_dir"); fs::create_dir(inner_dir).unwrap(); @@ -1520,6 +1524,8 @@ async fn uploaded_file_contains_updated_test_files() { assert_eq!(test_case_run.classname, "test_file.ts"); let expected_file = temp_dir .path() + .canonicalize() + .unwrap() .join("inner_dir") .join("test_file.ts") .to_str() diff --git a/context/src/junit/file_extractor.rs b/context/src/junit/file_extractor.rs index 60bb2e84..0bf9f853 100644 --- a/context/src/junit/file_extractor.rs +++ b/context/src/junit/file_extractor.rs @@ -53,7 +53,6 @@ fn convert_to_absolute( let initial_str = String::from(initial.as_str()); let path = Path::new(&initial_str); let repo_root_path = Path::new(&repo.repo_root); - println!("Had a repo_root of {:?}", repo.repo_root); if path.is_absolute() { path.to_str().map(String::from) } else if repo_root_path.is_absolute() && repo_root_path.exists() { From 9a92b82f107bfea33757be7a525bff7b8437e802 Mon Sep 17 00:00:00 2001 From: Christian Millar Date: Mon, 9 Jun 2025 19:28:20 +0000 Subject: [PATCH 11/14] Remove logging --- .github/actions/run_tests/action.yaml | 2 +- cli-tests/src/upload.rs | 9 ++------- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/.github/actions/run_tests/action.yaml b/.github/actions/run_tests/action.yaml index d6fe1496..9c654461 100644 --- a/.github/actions/run_tests/action.yaml +++ b/.github/actions/run_tests/action.yaml @@ -24,4 +24,4 @@ runs: - name: Run tests shell: bash - run: cargo nextest run --features=bindings --workspace ${{ contains(inputs.target, 'musl') && '--exclude context-js --exclude context-py --exclude context_ruby' || '' }} --profile=ci --nocapture + run: cargo nextest run --features=bindings --workspace ${{ contains(inputs.target, 'musl') && '--exclude context-js --exclude context-py --exclude context_ruby' || '' }} --profile=ci diff --git a/cli-tests/src/upload.rs b/cli-tests/src/upload.rs index bd3297bd..b05e2092 100644 --- a/cli-tests/src/upload.rs +++ b/cli-tests/src/upload.rs @@ -1474,11 +1474,6 @@ async fn uses_passed_exit_code_if_unquarantined_tests_fail() { async fn uploaded_file_contains_updated_test_files() { let temp_dir = TempDir::with_prefix("not_hidden").unwrap(); generate_mock_git_repo(&temp_dir); - println!( - "Tmp dir path is {:?}, canonicalized {:?}", - temp_dir, - temp_dir.canonicalize() - ); let inner_dir = temp_dir.path().join("inner_dir"); fs::create_dir(inner_dir).unwrap(); @@ -1517,8 +1512,6 @@ async fn uploaded_file_contains_updated_test_files() { let bin = fs::read(file_upload.join(&internal_bundled_file.path)).unwrap(); let report = proto::test_context::test_run::TestResult::decode(&*bin).unwrap(); - println!("{assert}"); - assert_eq!(report.test_case_runs.len(), 1); let test_case_run = &report.test_case_runs.first().unwrap(); assert_eq!(test_case_run.classname, "test_file.ts"); @@ -1532,4 +1525,6 @@ async fn uploaded_file_contains_updated_test_files() { .unwrap() .to_string(); assert_eq!(test_case_run.file, expected_file); + + println!("{assert}"); } From d86ee26098b37fd704779496e0410ec682a79d99 Mon Sep 17 00:00:00 2001 From: Christian Millar Date: Wed, 11 Jun 2025 19:51:15 +0000 Subject: [PATCH 12/14] Fix for failing tests out of merge, comments --- context/src/junit/bindings.rs | 10 ++++-- context/src/junit/parser.rs | 8 ++--- context/tests/junit.rs | 67 ++++++++++++++++++++++++----------- 3 files changed, 58 insertions(+), 27 deletions(-) diff --git a/context/src/junit/bindings.rs b/context/src/junit/bindings.rs index e3e60b1e..5d204fb8 100644 --- a/context/src/junit/bindings.rs +++ b/context/src/junit/bindings.rs @@ -1188,7 +1188,11 @@ mod tests { assert_eq!(test_case2.codeowners.clone().unwrap().len(), 0); // verify that the test report is valid - let results = validate(&converted_bindings.clone().into(), None, &BundleRepo::default()); + let results = validate( + &converted_bindings.clone().into(), + None, + &BundleRepo::default(), + ); assert_eq!(results.all_issues_flat().len(), 1); results .all_issues_flat() @@ -1231,12 +1235,12 @@ mod tests { - + but was: ]]> - + "#; diff --git a/context/src/junit/parser.rs b/context/src/junit/parser.rs index 3d85cbd1..15a6366f 100644 --- a/context/src/junit/parser.rs +++ b/context/src/junit/parser.rs @@ -862,12 +862,12 @@ mod tests { - + but was: ]]> - + @@ -886,7 +886,7 @@ mod tests { test_case_run1.status_output_message, "Expected: but was: " ); - assert_eq!(test_case_run1.file, "/test.java"); + assert_eq!(test_case_run1.file, "test.java"); assert_eq!(test_case_run1.attempt_number, 0); assert!(!test_case_run1.is_quarantined); assert_eq!( @@ -911,7 +911,7 @@ mod tests { assert_eq!(test_case_run2.classname, ""); assert_eq!(test_case_run2.status, TestCaseRunStatus::Failure as i32); assert_eq!(test_case_run2.status_output_message, "Test failed"); - assert_eq!(test_case_run2.file, "/test.java"); + assert_eq!(test_case_run2.file, "test.java"); assert_eq!(test_case_run2.attempt_number, 0); assert!(!test_case_run2.is_quarantined); assert_eq!( diff --git a/context/tests/junit.rs b/context/tests/junit.rs index 723890c5..0a9fab60 100644 --- a/context/tests/junit.rs +++ b/context/tests/junit.rs @@ -13,6 +13,7 @@ use context::{ JunitTestCaseValidationIssueSubOptimal, JunitTestSuiteValidationIssue, JunitTestSuiteValidationIssueInvalid, JunitTestSuiteValidationIssueSubOptimal, JunitValidationIssue, JunitValidationIssueType, JunitValidationLevel, + TestRunnerReportValidationIssue, TestRunnerReportValidationIssueSubOptimal, }, }, repo::BundleRepo, @@ -94,7 +95,8 @@ fn validate_test_suite_name_too_short() { test_suite.name = String::new().into(); } - let report_validation = junit::validator::validate(&generated_report, None, &BundleRepo::default()); + let report_validation = + junit::validator::validate(&generated_report, None, &BundleRepo::default()); assert_eq!( report_validation.max_level(), @@ -130,7 +132,8 @@ fn validate_test_case_name_too_short() { } } - let report_validation = junit::validator::validate(&generated_report, None, &BundleRepo::default()); + let report_validation = + junit::validator::validate(&generated_report, None, &BundleRepo::default()); assert_eq!( report_validation.max_level(), @@ -163,7 +166,8 @@ fn validate_test_suite_name_too_long() { test_suite.name = "a".repeat(junit::validator::MAX_FIELD_LEN + 1).into(); } - let report_validation = junit::validator::validate(&generated_report, None, &BundleRepo::default()); + let report_validation = + junit::validator::validate(&generated_report, None, &BundleRepo::default()); assert_eq!( report_validation.max_level(), @@ -199,7 +203,8 @@ fn validate_test_case_name_too_long() { } } - let report_validation = junit::validator::validate(&generated_report, None, &BundleRepo::default()); + let report_validation = + junit::validator::validate(&generated_report, None, &BundleRepo::default()); assert_eq!( report_validation.max_level(), @@ -237,7 +242,8 @@ fn validate_max_level() { } } - let report_validation = junit::validator::validate(&generated_report, None, &BundleRepo::default()); + let report_validation = + junit::validator::validate(&generated_report, None, &BundleRepo::default()); assert_eq!( report_validation.max_level(), @@ -308,7 +314,8 @@ fn validate_timestamps() { } } - let report_validation = junit::validator::validate(&generated_report, None, &BundleRepo::default()); + let report_validation = + junit::validator::validate(&generated_report, None, &BundleRepo::default()); assert_eq!( report_validation.max_level(), @@ -342,7 +349,8 @@ fn validate_test_runner_report_overrides_timestamp() { options.global.timestamp = Some(old_timestamp.fixed_offset()); let mut jm = JunitMock::new(options); let seed = jm.get_seed(); - let mut generated_reports = jm.generate_reports(); + let tmp_dir: Option = None; + let mut generated_reports = jm.generate_reports(&tmp_dir); let generated_report = generated_reports.pop().unwrap(); @@ -355,8 +363,11 @@ fn validate_test_runner_report_overrides_timestamp() { .checked_add_signed(TimeDelta::minutes(1)) .unwrap(), }; - let override_report_validation = - junit::validator::validate(&generated_report, Some(override_report)); + let override_report_validation = junit::validator::validate( + &generated_report, + Some(override_report), + &BundleRepo::default(), + ); pretty_assertions::assert_eq!( override_report_validation.all_issues(), &[ @@ -392,8 +403,11 @@ fn validate_test_runner_report_overrides_timestamp() { .checked_add_signed(TimeDelta::minutes(1)) .unwrap(), }; - let override_report_validation = - junit::validator::validate(&generated_report, Some(override_report)); + let override_report_validation = junit::validator::validate( + &generated_report, + Some(override_report), + &BundleRepo::default(), + ); pretty_assertions::assert_eq!( override_report_validation.all_issues(), &[ @@ -429,8 +443,11 @@ fn validate_test_runner_report_overrides_timestamp() { .checked_add_signed(TimeDelta::minutes(1)) .unwrap(), }; - let override_report_validation = - junit::validator::validate(&generated_report, Some(override_report)); + let override_report_validation = junit::validator::validate( + &generated_report, + Some(override_report), + &BundleRepo::default(), + ); pretty_assertions::assert_eq!( override_report_validation.all_issues(), &[ @@ -468,8 +485,11 @@ fn validate_test_runner_report_overrides_timestamp() { .checked_sub_signed(TimeDelta::minutes(1)) .unwrap(), }; - let override_report_validation = - junit::validator::validate(&generated_report, Some(override_report)); + let override_report_validation = junit::validator::validate( + &generated_report, + Some(override_report), + &BundleRepo::default(), + ); pretty_assertions::assert_eq!( override_report_validation.test_runner_report.issues(), &[TestRunnerReportValidationIssue::SubOptimal( @@ -491,8 +511,11 @@ fn validate_test_runner_report_overrides_timestamp() { .checked_add_signed(TimeDelta::minutes(1)) .unwrap(), }; - let override_report_validation = - junit::validator::validate(&generated_report, Some(override_report)); + let override_report_validation = junit::validator::validate( + &generated_report, + Some(override_report), + &BundleRepo::default(), + ); pretty_assertions::assert_eq!( override_report_validation.all_issues(), &[], @@ -502,7 +525,8 @@ fn validate_test_runner_report_overrides_timestamp() { } { - let report_validation = junit::validator::validate(&generated_report, None); + let report_validation = + junit::validator::validate(&generated_report, None, &BundleRepo::default()); pretty_assertions::assert_eq!( report_validation.all_issues(), &[JunitValidationIssueType::Report( @@ -540,8 +564,11 @@ fn validate_test_runner_report_overrides_timestamp() { test_case.timestamp = Some(test_case_timestamp); }); }); - let override_report_validation = - junit::validator::validate(&generated_report, Some(override_report)); + let override_report_validation = junit::validator::validate( + &generated_report, + Some(override_report), + &BundleRepo::default(), + ); pretty_assertions::assert_eq!( override_report_validation.all_issues(), &[ From bae24aef18910b4bfe3c182cbe7c6629170c5916 Mon Sep 17 00:00:00 2001 From: Christian Millar Date: Wed, 11 Jun 2025 21:12:29 +0000 Subject: [PATCH 13/14] fmt --- cli/src/validate_command.rs | 6 +++++- context-py/src/lib.rs | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/cli/src/validate_command.rs b/cli/src/validate_command.rs index 6d13e8a3..838e4324 100644 --- a/cli/src/validate_command.rs +++ b/cli/src/validate_command.rs @@ -152,7 +152,11 @@ async fn validate( .map(|(file, (report, test_runner_report))| { ( file, - validate_report(&report, test_runner_report.map(TestRunnerReport::from), &repo), + validate_report( + &report, + test_runner_report.map(TestRunnerReport::from), + &repo, + ), ) }) .collect(); diff --git a/context-py/src/lib.rs b/context-py/src/lib.rs index dcefe42a..b1837a4c 100644 --- a/context-py/src/lib.rs +++ b/context-py/src/lib.rs @@ -10,8 +10,8 @@ use codeowners::{ use context::{ env, junit::{self, junit_path::TestRunnerReport}, + meta, repo::{self, BundleRepo}, - meta }; use prost::Message; use pyo3::{exceptions::PyTypeError, prelude::*}; From 02b91f0d1d3663c259f54d9575c7f16122778ebb Mon Sep 17 00:00:00 2001 From: Christian Millar Date: Wed, 2 Jul 2025 19:51:28 +0000 Subject: [PATCH 14/14] Change to storing detected files in a detected_file field and using that for validation. Also changes the filename logic to just check that we are getting a file with an extension when we use classname, as it's overloaded with non-file uses by other test frameworks --- cli-tests/src/upload.rs | 3 ++- cli-tests/src/validate.rs | 2 +- context/src/junit/bindings.rs | 1 + context/src/junit/file_extractor.rs | 24 +++++++++++++++++++++++- context/src/junit/parser.rs | 9 +++++++-- context/src/junit/validator.rs | 6 ++++-- proto/proto/test_context.proto | 1 + 7 files changed, 39 insertions(+), 7 deletions(-) diff --git a/cli-tests/src/upload.rs b/cli-tests/src/upload.rs index c8c72aa6..498f2f3e 100644 --- a/cli-tests/src/upload.rs +++ b/cli-tests/src/upload.rs @@ -1680,7 +1680,8 @@ async fn uploaded_file_contains_updated_test_files() { .to_str() .unwrap() .to_string(); - assert_eq!(test_case_run.file, expected_file); + assert_eq!(test_case_run.file, String::from("test_file.ts")); + assert_eq!(test_case_run.detected_file, expected_file); println!("{assert}"); } diff --git a/cli-tests/src/validate.rs b/cli-tests/src/validate.rs index 91d831c2..501787df 100644 --- a/cli-tests/src/validate.rs +++ b/cli-tests/src/validate.rs @@ -1,9 +1,9 @@ use predicates::prelude::*; -use tempfile::{tempdir, TempDir}; use superconsole::{ style::{style, Color, Stylize}, Line, Span, }; +use tempfile::{tempdir, TempDir}; use crate::{ command_builder::CommandBuilder, diff --git a/context/src/junit/bindings.rs b/context/src/junit/bindings.rs index 1a559166..f78b1bc8 100644 --- a/context/src/junit/bindings.rs +++ b/context/src/junit/bindings.rs @@ -166,6 +166,7 @@ impl From for BindingsTestCase { attempt_number, is_quarantined, codeowners, + detected_file: _detected_file, }: TestCaseRun, ) -> Self { let started_at = started_at.unwrap_or_default(); diff --git a/context/src/junit/file_extractor.rs b/context/src/junit/file_extractor.rs index 0bf9f853..5e080b9e 100644 --- a/context/src/junit/file_extractor.rs +++ b/context/src/junit/file_extractor.rs @@ -92,7 +92,29 @@ fn convert_to_absolute( } } -pub fn filename_for_test_case(test_case: &TestCase, repo: &BundleRepo) -> String { +fn validate_as_filename(initial: &XmlString) -> Option { + let initial_str = String::from(initial.as_str()); + let path = Path::new(&initial_str); + if path.extension().is_some() { + Some(initial_str) + } else { + None + } +} + +pub fn filename_for_test_case(test_case: &TestCase) -> String { + test_case + .extra + .get(extra_attrs::FILE) + .or(test_case.extra.get(extra_attrs::FILEPATH)) + .or(test_case.classname.as_ref()) + .iter() + .flat_map(|s| validate_as_filename(s)) + .next() + .unwrap_or_default() +} + +pub fn detected_file_for_test_case(test_case: &TestCase, repo: &BundleRepo) -> String { test_case .extra .get(extra_attrs::FILE) diff --git a/context/src/junit/parser.rs b/context/src/junit/parser.rs index 15a6366f..b4da66f6 100644 --- a/context/src/junit/parser.rs +++ b/context/src/junit/parser.rs @@ -21,7 +21,10 @@ use thiserror::Error; use wasm_bindgen::prelude::*; use super::date_parser::JunitDateParser; -use crate::{junit::file_extractor::filename_for_test_case, repo::BundleRepo}; +use crate::{ + junit::file_extractor::{detected_file_for_test_case, filename_for_test_case}, + repo::BundleRepo, +}; const TAG_REPORT: &[u8] = b"testsuites"; const TAG_TEST_SUITE: &[u8] = b"testsuite"; @@ -202,7 +205,8 @@ impl JunitParser { for report in self.reports { for test_suite in report.test_suites { for test_case in test_suite.test_cases { - let file = filename_for_test_case(&test_case, repo); + let file = filename_for_test_case(&test_case); + let detected_file = detected_file_for_test_case(&test_case, repo); let mut test_case_run = TestCaseRun { name: test_case.name.into(), parent_name: test_suite.name.to_string(), @@ -277,6 +281,7 @@ impl JunitParser { } } test_case_run.file = file; + test_case_run.detected_file = detected_file; test_case_run.line = test_case .extra .get(extra_attrs::LINE) diff --git a/context/src/junit/validator.rs b/context/src/junit/validator.rs index 15e2fb09..b5e405c6 100644 --- a/context/src/junit/validator.rs +++ b/context/src/junit/validator.rs @@ -10,7 +10,7 @@ use thiserror::Error; #[cfg(feature = "wasm")] use wasm_bindgen::prelude::*; -use crate::junit::file_extractor::filename_for_test_case; +use crate::junit::file_extractor::detected_file_for_test_case; use crate::repo::BundleRepo; use crate::{ junit::junit_path::TestRunnerReport, @@ -128,7 +128,9 @@ pub fn validate( } }; - match validate_field_len::(filename_for_test_case(test_case, repo)) { + match validate_field_len::(detected_file_for_test_case( + test_case, repo, + )) { FieldLen::Valid => (), FieldLen::TooShort(s) => { test_case_validation.add_issue(JunitValidationIssue::SubOptimal( diff --git a/proto/proto/test_context.proto b/proto/proto/test_context.proto index 6930eb84..182b80b5 100644 --- a/proto/proto/test_context.proto +++ b/proto/proto/test_context.proto @@ -29,6 +29,7 @@ message TestCaseRun { string status_output_message = 11; bool is_quarantined = 12; repeated CodeOwner codeowners = 13; + string detected_file = 14; } message UploaderMetadata {