From dfcabc943346cd359e3599dfbecafd1a93088174 Mon Sep 17 00:00:00 2001 From: max-trunk Date: Fri, 17 Jan 2025 11:19:54 -0500 Subject: [PATCH] Treat 'no reports found' as a suboptimal parsing issue (#305) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Part of [TRUNK-14095](https://linear.app/trunk/issue/TRUNK-14095/qa-bug-no-reports-found-message-on-uploads-page) The goal here is to not flag an invalid-level data quality error to the user in the uploads view when a bundle contains one or more empty XML files. See ticket ^ and relevant slack [thread](https://trunk-io.slack.com/archives/C07PYM8K0PQ/p1736980140133479). However, I think we should still track that a bundle contained an empty XML file - so I decided to relegate it's error level to suboptimal (which currently won't show up in the uploads view). So, This PR introduces the concept of 'invalid' & 'suboptimal' junit _parsing_ issues; similar to the paradigm we have for validation issues. A new `BindingsParseResult` struct is introduced such that a Python or JS client can access any invalid / suboptimal parsing issues encountered during junit parsing _in addition_ to the parsed report. This also tangentially cleans up the `validate` CLI command to ensure its output is in line with how we're validating junits in our systems. Example output: ``` max@max-cloudtop:~$ ./src/analytics-cli/target/debug/trunk-analytics-cli validate --junit-paths="test.xml,test2.xml,test3.xml,test4.xml" 2025-01-16T19:54:37 [INFO] - Starting trunk flakytests 0.0.0 (git=714a55e17fa8e1f3c3ab4b480771109b7ac3eadc) rustc=1.80.0-nightly Validating the following 4 files: File set matching test.xml: test.xml File set matching test2.xml: test2.xml File set matching test3.xml: test3.xml File set matching test4.xml: test4.xml test.xml - 0 validation errors, 1 validation warnings OPTIONAL - no reports found test2.xml - 1 validation errors INVALID - multiple reports found test3.xml - 1 test suites, 33 test cases, 0 validation errors, 1 validation warnings OPTIONAL - report has stale (> 1 hour(s)) timestamps test4.xml - 1 test suites, 1 test cases, 0 validation errors, 1 validation warnings OPTIONAL - report has stale (> 1 hour(s)) timestamps 3 files are valid, 1 files are not valid, 3 files have validation warnings ❌ Checking for codeowners file... OPTIONAL - No codeowners file found. max@max-cloudtop:~$ `` --- cli-tests/src/upload.rs | 2 +- cli-tests/src/validate.rs | 17 ++ cli/src/validate_command.rs | 234 ++++++++++++-------- context-js/src/lib.rs | 32 +-- context-js/tests/parse_and_validate.test.ts | 18 +- context-py/src/lib.rs | 56 ++--- context-py/tests/test_junit_parse.py | 36 +-- context-py/tests/test_junit_validate.py | 20 +- context/src/junit/bindings.rs | 17 +- context/src/junit/parser.rs | 139 ++++++++++-- context/tests/junit.rs | 2 +- 11 files changed, 380 insertions(+), 193 deletions(-) diff --git a/cli-tests/src/upload.rs b/cli-tests/src/upload.rs index 2c47f3d8..146dd270 100644 --- a/cli-tests/src/upload.rs +++ b/cli-tests/src/upload.rs @@ -239,7 +239,7 @@ async fn upload_bundle_using_bep() { // Uploaded file is a junit, even when using BEP let mut junit_parser = JunitParser::new(); assert!(junit_parser.parse(junit_reader).is_ok()); - assert!(junit_parser.errors().is_empty()); + assert!(junit_parser.issues().is_empty()); let mut bazel_bep_parser = BazelBepParser::new(tar_extract_directory.join("bazel_bep.json")); let parse_result = bazel_bep_parser.parse().ok().unwrap(); diff --git a/cli-tests/src/validate.rs b/cli-tests/src/validate.rs index 40100fa8..6071e9ce 100644 --- a/cli-tests/src/validate.rs +++ b/cli-tests/src/validate.rs @@ -92,6 +92,23 @@ fn validate_invalid_junits_no_codeowners() { println!("{assert}"); } +#[test] +fn validate_empty_xml() { + let temp_dir = tempdir().unwrap(); + let empty_xml = ""; + write_junit_xml_to_dir(empty_xml, &temp_dir); + + let assert = Command::new(CARGO_RUN.path()) + .current_dir(&temp_dir) + .args(["validate", "--junit-paths", "./*"]) + .assert() + .success() + .stdout(predicate::str::contains("1 validation warning")) + .stdout(predicate::str::contains("OPTIONAL - no reports found")); + + println!("{assert}"); +} + #[test] fn validate_invalid_xml() { let temp_dir = tempdir().unwrap(); diff --git a/cli/src/validate_command.rs b/cli/src/validate_command.rs index f7b252aa..51e7b163 100644 --- a/cli/src/validate_command.rs +++ b/cli/src/validate_command.rs @@ -10,7 +10,7 @@ use context::{ bazel_bep::parser::BazelBepParser, junit::{ junit_path::JunitReportFileWithStatus, - parser::{JunitParseError, JunitParser}, + parser::{JunitParseIssue, JunitParseIssueLevel, JunitParser}, validator::{ validate as validate_report, JunitReportValidation, JunitReportValidationFlatIssue, JunitReportValidationIssueSubOptimal, JunitValidationIssue, JunitValidationIssueType, @@ -30,7 +30,7 @@ pub struct ValidateArgs { conflicts_with = "bazel_bep_path", value_delimiter = ',', value_parser = clap::builder::NonEmptyStringValueParser::new(), - help = "Comma-separated list of glob paths to junit files." + help = "Comma-separated list of glob paths to junit files.", )] junit_paths: Vec, #[arg( @@ -39,7 +39,7 @@ pub struct ValidateArgs { help = "Path to bazel build event protocol JSON file." )] bazel_bep_path: Option, - #[arg(long, help = "Show warning-level log messages in output.")] + #[arg(long, help = "Show warning-level log messages in output.", hide = true)] show_warnings: bool, #[arg(long, help = "Value to override CODEOWNERS file or directory path.")] pub codeowners_path: Option, @@ -49,7 +49,7 @@ pub async fn run_validate(validate_args: ValidateArgs) -> anyhow::Result { let ValidateArgs { junit_paths, bazel_bep_path, - show_warnings, + show_warnings: _, codeowners_path, } = validate_args; @@ -65,15 +65,17 @@ pub async fn run_validate(validate_args: ValidateArgs) -> anyhow::Result { .map(JunitReportFileWithStatus::from) .collect(), }; - validate(junit_file_paths, show_warnings, codeowners_path).await + validate(junit_file_paths, codeowners_path).await } -type JunitFileToReportAndErrors = BTreeMap, Vec)>; -type JunitFileToValidation = BTreeMap>; +type JunitFileToReportAndParseIssues = + BTreeMap>, Vec)>; +type JunitFileToReport = BTreeMap; +type JunitFileToParseIssues = BTreeMap, Vec)>; +type JunitFileToValidation = BTreeMap; async fn validate( junit_paths: Vec, - show_warnings: bool, codeowners_path: Option, ) -> anyhow::Result { // scan files @@ -93,36 +95,49 @@ async fn validate( } print_matched_files(&file_set_builder); - // parse and validate + // parse let parse_results = parse_file_sets(file_set_builder.file_sets()); - if show_warnings { - print_parse_errors(&parse_results); - } - let report_validations: JunitFileToValidation = parse_results - .into_iter() - .map(|parse_result| { - ( - parse_result.0, - match parse_result.1 .0 { - Ok(report) => Ok(validate_report(&report)), - Err(e) => Err(e), + let num_reports = parse_results.len(); + let (parsed_reports, parse_issues) = parse_results.into_iter().fold( + (JunitFileToReport::new(), JunitFileToParseIssues::new()), + |(mut parsed_reports, mut parse_issues), (file, (parse_result, issues))| { + match parse_result { + Ok(report) => match report { + Some(report) => { + parsed_reports.insert(file, report); + } + None => { + parse_issues.insert(file, (Ok(()), issues)); + } }, - ) - }) + Err(e) => { + parse_issues.insert(file, (Err(e), Vec::new())); + } + } + (parsed_reports, parse_issues) + }, + ); + // print parse issues + let (num_unparsable_reports, num_suboptimally_parsable_reports) = + print_parse_issues(&parse_issues); + + // validate + let report_validations: JunitFileToValidation = parsed_reports + .into_iter() + .map(|(file, report)| (file, validate_report(&report))) .collect(); + // print validation results + let (mut num_invalid_reports, mut num_suboptimal_reports) = + print_validation_issues(&report_validations); - // print results - let (num_invalid_reports, num_suboptimal_reports) = - print_validation_errors(&report_validations); + // print summary + num_invalid_reports += num_unparsable_reports; + num_suboptimal_reports += num_suboptimally_parsable_reports; let exit = if num_invalid_reports == 0 { - print_summary_success(report_validations.len(), num_suboptimal_reports); + print_summary_success(num_reports, num_suboptimal_reports); EXIT_SUCCESS } else { - print_summary_failure( - report_validations.len(), - num_invalid_reports, - num_suboptimal_reports, - ); + print_summary_failure(num_reports, num_invalid_reports, num_suboptimal_reports); EXIT_FAILURE }; @@ -133,10 +148,10 @@ async fn validate( Ok(exit) } -fn parse_file_sets(file_sets: &[FileSet]) -> JunitFileToReportAndErrors { +fn parse_file_sets(file_sets: &[FileSet]) -> JunitFileToReportAndParseIssues { file_sets.iter().flat_map(|file_set| &file_set.files).fold( - JunitFileToReportAndErrors::new(), - |mut parse_results, bundled_file| -> JunitFileToReportAndErrors { + JunitFileToReportAndParseIssues::new(), + |mut parse_results, bundled_file| -> JunitFileToReportAndParseIssues { let path = std::path::Path::new(&bundled_file.original_path); let file = match std::fs::File::open(path) { Ok(file) => file, @@ -159,14 +174,21 @@ fn parse_file_sets(file_sets: &[FileSet]) -> JunitFileToReportAndErrors { return parse_results; } - let parse_errors = junit_parser.errors().to_vec(); - for report in junit_parser.into_reports() { + let parse_issues = junit_parser.issues().to_vec(); + let mut parsed_reports = junit_parser.into_reports(); + if parsed_reports.len() != 1 { parse_results.insert( bundled_file.get_print_path().to_string(), - (Ok(report), parse_errors.clone()), + (Ok(None), parse_issues), ); + return parse_results; } + parse_results.insert( + bundled_file.get_print_path().to_string(), + (Ok(Some(parsed_reports.remove(0))), Vec::new()), + ); + parse_results }, ) @@ -185,34 +207,85 @@ fn print_matched_files(file_set_builder: &FileSetBuilder) { } } -fn print_parse_errors(parse_results: &JunitFileToReportAndErrors) { - let num_parse_errors = parse_results - .iter() - .fold(0, |mut num_parse_errors, parse_result| { - num_parse_errors += parse_result.1 .1.len(); - num_parse_errors - }); +fn print_parse_issues(parse_issues: &JunitFileToParseIssues) -> (usize, usize) { + let mut num_unparsable_reports: usize = 0; + let mut num_suboptimally_parsable_reports: usize = 0; + for (i, (file, (parse_result, parse_issues))) in parse_issues.iter().enumerate() { + if i == 0 { + println!(); + } - if num_parse_errors == 0 { - return; - } + let (fatal_parse_error, issues, num_parse_errors, num_parse_warnings) = + if let Err(e) = parse_result { + (Some(e), &Vec::new(), 1, 0) + } else { + let (num_parse_errors, num_parse_warnings) = + parse_issues.iter().fold((0, 0), |mut acc, issue| { + match JunitParseIssueLevel::from(issue) { + JunitParseIssueLevel::Invalid => { + acc.0 += 1; + } + JunitParseIssueLevel::SubOptimal => { + acc.1 += 1; + } + _ => (), + } + acc + }); + (None, parse_issues, num_parse_errors, num_parse_warnings) + }; - println!( - "\nEncountered the following {} non-fatal errors while parsing files:", - num_parse_errors.to_string().yellow() - ); + let num_parse_errors_str = if num_parse_errors > 0 { + num_parse_errors.to_string().red() + } else { + num_parse_errors.to_string().green() + }; + let num_parse_warnings_str = if num_parse_warnings > 0 { + format!( + ", {} validation warnings", + num_parse_warnings.to_string().yellow() + ) + } else { + String::from("") + }; + println!( + "{} - {} validation errors{}", + file, num_parse_errors_str, num_parse_warnings_str, + ); - for parse_result in parse_results { - if parse_result.1 .1.is_empty() { - continue; + if let Some(parse_error) = fatal_parse_error { + println!( + " {} - {}", + print_parse_issue_level(JunitParseIssueLevel::Invalid), + parse_error, + ); } - println!(" File: {}", parse_result.0); + for issue in issues { + println!( + " {} - {}", + print_parse_issue_level(JunitParseIssueLevel::from(issue)), + issue, + ); + } - for parse_error in &parse_result.1 .1 { - println!(" {}", parse_error); + if num_parse_errors > 0 { + num_unparsable_reports += 1; + } + if num_parse_warnings > 0 { + num_suboptimally_parsable_reports += 1; } } + + (num_unparsable_reports, num_suboptimally_parsable_reports) +} + +fn print_parse_issue_level(level: JunitParseIssueLevel) -> ColoredString { + match level { + JunitParseIssueLevel::SubOptimal => "OPTIONAL".yellow(), + JunitParseIssueLevel::Invalid => "INVALID".red(), + JunitParseIssueLevel::Valid => "VALID".green(), + } } fn print_summary_failure( @@ -255,34 +328,20 @@ fn print_summary_success(num_reports: usize, num_suboptimal_reports: usize) { ); } -fn print_validation_errors(report_validations: &JunitFileToValidation) -> (usize, usize) { - println!(); +fn print_validation_issues(report_validations: &JunitFileToValidation) -> (usize, usize) { let mut num_invalid_reports: usize = 0; let mut num_suboptimal_reports: usize = 0; - for report_validation in report_validations { - let mut num_test_suites = 0; - let mut num_test_cases = 0; - let num_validation_errors: usize; - let mut num_validation_warnings = 0; - let mut report_parse_error: Option<&anyhow::Error> = None; - let mut all_issues: Vec = Vec::new(); - - match report_validation.1 { - Ok(validation) => { - num_test_suites = validation.test_suites().len(); - num_test_cases = validation.test_cases().len(); - - num_validation_errors = validation.num_invalid_issues(); - num_validation_warnings = validation.num_suboptimal_issues(); - - all_issues = validation.all_issues_flat(); - } - Err(e) => { - report_parse_error = Some(e); - num_validation_errors = 1; - } + for (i, (file, report_validation)) in report_validations.iter().enumerate() { + if i == 0 { + println!(); } + let num_test_suites = report_validation.test_suites().len(); + let num_test_cases = report_validation.test_cases().len(); + let num_validation_errors = report_validation.num_invalid_issues(); + let num_validation_warnings = report_validation.num_suboptimal_issues(); + let all_issues: Vec = report_validation.all_issues_flat(); + let num_validation_errors_str = if num_validation_errors > 0 { num_validation_errors.to_string().red() } else { @@ -298,21 +357,13 @@ fn print_validation_errors(report_validations: &JunitFileToValidation) -> (usize }; println!( "{} - {} test suites, {} test cases, {} validation errors{}", - report_validation.0, + file, num_test_suites, num_test_cases, num_validation_errors_str, num_validation_warnings_str, ); - if let Some(parse_error) = report_parse_error { - println!( - " {} - {}", - print_validation_level(JunitValidationLevel::Invalid), - parse_error, - ); - } - for issue in all_issues { println!( " {} - {}", @@ -355,8 +406,7 @@ fn print_codeowners_validation( let has_test_cases_without_matching_codeowners_paths = report_validations .iter() - .filter_map(|(_, report_validation)| report_validation.as_ref().ok()) - .flat_map(|report_validation| report_validation.all_issues()) + .flat_map(|(_, report_validation)| report_validation.all_issues()) .any(|issue| { matches!( issue, diff --git a/context-js/src/lib.rs b/context-js/src/lib.rs index 1472589d..7ebf03a1 100644 --- a/context-js/src/lib.rs +++ b/context-js/src/lib.rs @@ -45,23 +45,25 @@ pub fn env_validate(ci_info: &env::parser::CIInfo) -> env::validator::EnvValidat } #[wasm_bindgen] -pub fn junit_parse(xml: Vec) -> Result, JsError> { +pub fn junit_parse(xml: Vec) -> Result { let mut junit_parser = junit::parser::JunitParser::new(); - if junit_parser.parse(BufReader::new(&xml[..])).is_err() { - let error_message = junit_parser - .errors() - .into_iter() - .map(|e| e.to_string()) - .collect::>() - .join("\n"); - return Err(JsError::new(&error_message)); - } + junit_parser + .parse(BufReader::new(&xml[..])) + .map_err(|e| JsError::new(&e.to_string()))?; - Ok(junit_parser - .into_reports() - .into_iter() - .map(junit::bindings::BindingsReport::from) - .collect()) + let issues_flat = junit_parser.issues_flat(); + let mut parsed_reports = junit_parser.into_reports(); + + let report = if let (1, Some(parsed_report)) = (parsed_reports.len(), parsed_reports.pop()) { + Some(junit::bindings::BindingsReport::from(parsed_report)) + } else { + None + }; + + Ok(junit::bindings::BindingsParseResult { + report, + issues: issues_flat, + }) } #[wasm_bindgen] diff --git a/context-js/tests/parse_and_validate.test.ts b/context-js/tests/parse_and_validate.test.ts index 8da1d491..c616058a 100644 --- a/context-js/tests/parse_and_validate.test.ts +++ b/context-js/tests/parse_and_validate.test.ts @@ -75,8 +75,10 @@ describe("context-js", () => { `; - let report = junit_parse(Buffer.from(validJunitXml, "utf-8")); - let junitReportValidation = junit_validate(report[0]); + let parse_result = junit_parse(Buffer.from(validJunitXml, "utf-8")); + let report = parse_result.report; + + let junitReportValidation = junit_validate(report); expect(junitReportValidation.max_level()).toBe(JunitValidationLevel.Valid); @@ -91,8 +93,10 @@ describe("context-js", () => { `; - report = junit_parse(Buffer.from(suboptimalJunitXml, "utf-8")); - junitReportValidation = junit_validate(report[0]); + parse_result = junit_parse(Buffer.from(suboptimalJunitXml, "utf-8")); + report = parse_result.report; + + junitReportValidation = junit_validate(report); expect(junitReportValidation.max_level()).toBe( JunitValidationLevel.SubOptimal, @@ -116,8 +120,10 @@ describe("context-js", () => { `; - report = junit_parse(Buffer.from(nestedJunitXml, "utf-8")); - junitReportValidation = junit_validate(report[0]); + parse_result = junit_parse(Buffer.from(nestedJunitXml, "utf-8")); + report = parse_result.report; + + junitReportValidation = junit_validate(report); expect(junitReportValidation.max_level()).toBe(JunitValidationLevel.Valid); }); diff --git a/context-py/src/lib.rs b/context-py/src/lib.rs index a037362f..b91fa45c 100644 --- a/context-py/src/lib.rs +++ b/context-py/src/lib.rs @@ -57,30 +57,37 @@ fn ci_platform_to_string(ci_platform: env::parser::CIPlatform) -> String { #[gen_stub_pyfunction] #[pyfunction] -fn junit_parse(xml: Vec) -> PyResult> { +fn junit_parse(xml: Vec) -> PyResult { let mut junit_parser = junit::parser::JunitParser::new(); if let Err(e) = junit_parser.parse(BufReader::new(&xml[..])) { - let collected_errors = collect_parse_errors(&junit_parser); - if !collected_errors.is_empty() { - return Err(PyTypeError::new_err(format!( - "{}\n{}", - e.to_string(), - collected_errors - ))); - } return Err(PyTypeError::new_err(e.to_string())); } - let collected_errors = collect_parse_errors(&junit_parser); - if !collected_errors.is_empty() { - return Err(PyTypeError::new_err(collected_errors)); - } + let issues_flat = junit_parser.issues_flat(); + let mut parsed_reports = junit_parser.into_reports(); - Ok(junit_parser - .into_reports() - .into_iter() - .map(junit::bindings::BindingsReport::from) - .collect()) + let report = if let (1, Some(parsed_report)) = (parsed_reports.len(), parsed_reports.pop()) { + Some(junit::bindings::BindingsReport::from(parsed_report)) + } else { + None + }; + + Ok(junit::bindings::BindingsParseResult { + report, + issues: issues_flat, + }) +} + +#[gen_stub_pyfunction] +#[pyfunction] +fn junit_parse_issue_level_to_string( + junit_parse_issue_level: junit::parser::JunitParseIssueLevel, +) -> String { + match junit_parse_issue_level { + junit::parser::JunitParseIssueLevel::Valid => "VALID".to_string(), + junit::parser::JunitParseIssueLevel::SubOptimal => "SUBOPTIMAL".to_string(), + junit::parser::JunitParseIssueLevel::Invalid => "INVALID".to_string(), + } } #[gen_stub_pyfunction] @@ -328,6 +335,7 @@ fn context_py(m: &Bound<'_, PyModule>) -> PyResult<()> { m.add_function(wrap_pyfunction!(branch_class_to_string, m)?)?; m.add_function(wrap_pyfunction!(ci_platform_to_string, m)?)?; + m.add_class::()?; m.add_class::()?; m.add_class::()?; m.add_class::()?; @@ -335,11 +343,14 @@ fn context_py(m: &Bound<'_, PyModule>) -> PyResult<()> { m.add_class::()?; m.add_class::()?; m.add_class::()?; + m.add_class::()?; + m.add_class::()?; m.add_class::()?; m.add_class::()?; m.add_class::()?; m.add_function(wrap_pyfunction!(junit_parse, m)?)?; m.add_function(wrap_pyfunction!(bin_parse, m)?)?; + m.add_function(wrap_pyfunction!(junit_parse_issue_level_to_string, m)?)?; m.add_function(wrap_pyfunction!(junit_validate, m)?)?; m.add_function(wrap_pyfunction!(junit_validation_level_to_string, m)?)?; m.add_function(wrap_pyfunction!(junit_validation_type_to_string, m)?)?; @@ -367,12 +378,3 @@ fn context_py(m: &Bound<'_, PyModule>) -> PyResult<()> { Ok(()) } - -fn collect_parse_errors(parser: &junit::parser::JunitParser) -> String { - parser - .errors() - .into_iter() - .map(|e| e.to_string()) - .collect::>() - .join("\n") -} diff --git a/context-py/tests/test_junit_parse.py b/context-py/tests/test_junit_parse.py index 44062604..30957f26 100644 --- a/context-py/tests/test_junit_parse.py +++ b/context-py/tests/test_junit_parse.py @@ -1,9 +1,9 @@ def test_junit_parse_valid(): - import typing as PT from datetime import datetime, timezone from context_py import ( BindingsNonSuccessKind, + BindingsParseResult, BindingsReport, BindingsTestCaseStatusStatus, junit_parse, @@ -29,10 +29,11 @@ def test_junit_parse_valid(): """ - reports: PT.List[BindingsReport] = junit_parse(str.encode(valid_junit_xml)) + parse_result: BindingsParseResult = junit_parse(str.encode(valid_junit_xml)) + assert len(parse_result.issues) == 0 - assert len(reports) == 1 - report = reports[0] + report: BindingsReport | None = parse_result.report + assert report is not None assert len(report.test_suites) == 1 test_suite = report.test_suites[0] @@ -58,16 +59,17 @@ def test_junit_parse_valid(): assert test_case.status.non_success.description == "FAILURE BODY" -def test_junit_parse_non_xml(): - from context_py import junit_parse - from pytest import raises +def test_junit_parse_no_reports(): + from context_py import JunitParseIssueLevel, junit_parse simple_string = "no reports here!" - with raises(Exception) as excinfo: - _ = junit_parse(str.encode(simple_string)) + parse_result = junit_parse(str.encode(simple_string)) - assert str(excinfo.value) == "no reports found" + assert parse_result.report is None + assert len(parse_result.issues) == 1 + assert parse_result.issues[0].level == JunitParseIssueLevel.SubOptimal + assert parse_result.issues[0].error_message == "no reports found" def test_junit_parse_broken_xml(): @@ -86,10 +88,13 @@ def test_junit_parse_broken_xml(): def test_junit_parse_nested_testsuites(): - import typing as PT from datetime import datetime, timezone - from context_py import BindingsReport, BindingsTestCaseStatusStatus, junit_parse + from context_py import ( + BindingsParseResult, + BindingsTestCaseStatusStatus, + junit_parse, + ) valid_timestamp = datetime.now().astimezone(timezone.utc).isoformat() @@ -106,10 +111,11 @@ def test_junit_parse_nested_testsuites(): """ - reports: PT.List[BindingsReport] = junit_parse(str.encode(nested_testsuites_xml)) + parse_result: BindingsParseResult = junit_parse(str.encode(nested_testsuites_xml)) + assert len(parse_result.issues) == 0 - assert len(reports) == 1 - report = reports[0] + report = parse_result.report + assert report is not None assert len(report.test_suites) == 1 test_suite = report.test_suites[0] diff --git a/context-py/tests/test_junit_validate.py b/context-py/tests/test_junit_validate.py index daf4c362..ffa5079b 100644 --- a/context-py/tests/test_junit_validate.py +++ b/context-py/tests/test_junit_validate.py @@ -1,9 +1,8 @@ def test_junit_validate_valid(): - import typing as PT from datetime import datetime, timezone from context_py import ( - BindingsReport, + BindingsParseResult, JunitValidationLevel, junit_parse, junit_validate, @@ -26,10 +25,9 @@ def test_junit_validate_valid(): """ - reports: PT.List[BindingsReport] = junit_parse(str.encode(valid_junit_xml)) - - assert len(reports) == 1 - report = reports[0] + parse_result: BindingsParseResult = junit_parse(str.encode(valid_junit_xml)) + report = parse_result.report + assert report is not None junit_report_validation = junit_validate(report) @@ -47,11 +45,10 @@ def test_junit_validate_valid(): def test_junit_validate_suboptimal(): - import typing as PT from datetime import datetime, timedelta, timezone from context_py import ( - BindingsReport, + BindingsParseResult, JunitValidationLevel, JunitValidationType, junit_parse, @@ -73,10 +70,9 @@ def test_junit_validate_suboptimal(): """ - reports: PT.List[BindingsReport] = junit_parse(str.encode(suboptimal_junit_xml)) - - assert len(reports) == 1 - report = reports[0] + parse_result: BindingsParseResult = junit_parse(str.encode(suboptimal_junit_xml)) + report = parse_result.report + assert report is not None junit_report_validation = junit_validate(report) diff --git a/context/src/junit/bindings.rs b/context/src/junit/bindings.rs index 0f720a54..7ae5587a 100644 --- a/context/src/junit/bindings.rs +++ b/context/src/junit/bindings.rs @@ -12,11 +12,22 @@ use quick_junit::{ #[cfg(feature = "wasm")] use wasm_bindgen::prelude::*; -use super::validator::{ - JunitReportValidation, JunitReportValidationFlatIssue, JunitTestSuiteValidation, - JunitValidationLevel, JunitValidationType, +use super::{ + parser::JunitParseFlatIssue, + validator::{ + JunitReportValidation, JunitReportValidationFlatIssue, JunitTestSuiteValidation, + JunitValidationLevel, JunitValidationType, + }, }; +#[cfg_attr(feature = "pyo3", gen_stub_pyclass, pyclass(get_all))] +#[cfg_attr(feature = "wasm", wasm_bindgen(getter_with_clone))] +#[derive(Clone, Debug)] +pub struct BindingsParseResult { + pub report: Option, + pub issues: Vec, +} + #[cfg_attr(feature = "pyo3", gen_stub_pyclass, pyclass(get_all))] #[cfg_attr(feature = "wasm", wasm_bindgen(getter_with_clone))] #[derive(Clone, Debug)] diff --git a/context/src/junit/parser.rs b/context/src/junit/parser.rs index 5059c6e8..c96bc108 100644 --- a/context/src/junit/parser.rs +++ b/context/src/junit/parser.rs @@ -1,11 +1,21 @@ -use std::{io::BufRead, mem}; +use std::{ + fmt::{Display, Formatter, Result}, + io::BufRead, + mem, +}; +#[cfg(feature = "pyo3")] +use pyo3::prelude::*; +#[cfg(feature = "pyo3")] +use pyo3_stub_gen::derive::{gen_stub_pyclass, gen_stub_pyclass_enum}; use quick_junit::{NonSuccessKind, Report, TestCase, TestCaseStatus, TestRerun, TestSuite}; use quick_xml::{ events::{BytesStart, BytesText, Event}, Reader, }; use thiserror::Error; +#[cfg(feature = "wasm")] +use wasm_bindgen::prelude::*; use super::date_parser::JunitDateParser; @@ -30,10 +40,53 @@ pub mod extra_attrs { pub const ID: &str = "id"; } +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum JunitParseIssue { + SubOptimal(JunitParseIssueSubOptimal), + Invalid(JunitParseIssueInvalid), +} + +impl Display for JunitParseIssue { + fn fmt(&self, f: &mut Formatter) -> Result { + match self { + JunitParseIssue::SubOptimal(so) => write!(f, "{}", so), + JunitParseIssue::Invalid(i) => write!(f, "{}", i), + } + } +} + +#[cfg_attr(feature = "pyo3", gen_stub_pyclass_enum, pyclass(eq, eq_int))] +#[cfg_attr(feature = "wasm", wasm_bindgen)] +#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)] +pub enum JunitParseIssueLevel { + Valid = 0, + SubOptimal = 1, + Invalid = 2, +} + +impl Default for JunitParseIssueLevel { + fn default() -> Self { + Self::Valid + } +} + +impl From<&JunitParseIssue> for JunitParseIssueLevel { + fn from(value: &JunitParseIssue) -> Self { + match value { + JunitParseIssue::SubOptimal(..) => JunitParseIssueLevel::SubOptimal, + JunitParseIssue::Invalid(..) => JunitParseIssueLevel::Invalid, + } + } +} + #[derive(Error, Debug, Copy, Clone, PartialEq, Eq)] -pub enum JunitParseError { +pub enum JunitParseIssueSubOptimal { #[error("no reports found")] ReportNotFound, +} + +#[derive(Error, Debug, Copy, Clone, PartialEq, Eq)] +pub enum JunitParseIssueInvalid { #[error("multiple reports found")] ReportMultipleFound, #[error("report end tag found without start tag")] @@ -54,6 +107,14 @@ pub enum JunitParseError { TestRerunTestCaseNotFound, } +#[cfg_attr(feature = "pyo3", gen_stub_pyclass, pyclass(get_all))] +#[cfg_attr(feature = "wasm", wasm_bindgen(getter_with_clone))] +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct JunitParseFlatIssue { + pub level: JunitParseIssueLevel, + pub error_message: String, +} + #[derive(Debug, Clone)] enum Text { SystemOut(Option), @@ -71,7 +132,7 @@ enum CurrentReportState { #[derive(Debug, Clone)] pub struct JunitParser { date_parser: JunitDateParser, - errors: Vec, + issues: Vec, reports: Vec, current_report: Report, current_report_state: CurrentReportState, @@ -82,11 +143,17 @@ pub struct JunitParser { current_text: Option, } +impl Default for JunitParser { + fn default() -> Self { + Self::new() + } +} + impl JunitParser { pub fn new() -> Self { Self { date_parser: Default::default(), - errors: Default::default(), + issues: Default::default(), reports: Default::default(), current_report: Report::new(""), current_report_state: CurrentReportState::Default, @@ -98,8 +165,19 @@ impl JunitParser { } } - pub fn errors(&self) -> &Vec { - &self.errors + pub fn issues(&self) -> &Vec { + &self.issues + } + + pub fn issues_flat(&self) -> Vec { + return self + .issues() + .iter() + .map(|i| JunitParseFlatIssue { + level: JunitParseIssueLevel::from(i), + error_message: i.to_string(), + }) + .collect(); } pub fn reports(&self) -> &Vec { @@ -126,11 +204,15 @@ impl JunitParser { } match self.reports.len() { - 0 => self.errors.push(JunitParseError::ReportNotFound), + 0 => self.issues.push(JunitParseIssue::SubOptimal( + JunitParseIssueSubOptimal::ReportNotFound, + )), 1 => { // There should only be 1 report per JUnit.xml file } - _ => self.errors.push(JunitParseError::ReportMultipleFound), + _ => self.issues.push(JunitParseIssue::Invalid( + JunitParseIssueInvalid::ReportMultipleFound, + )), }; Ok(()) @@ -222,11 +304,11 @@ impl JunitParser { fn match_text(&mut self, e: &BytesText) { if self.current_text.is_some() { - self.set_text_value(&e); + self.set_text_value(e); } else if self.current_test_rerun.is_some() { - self.set_test_rerun_description(&e); + self.set_test_rerun_description(e); } else { - self.set_test_case_status_description(&e); + self.set_test_case_status_description(e); } } @@ -260,7 +342,9 @@ impl JunitParser { fn close_report(&mut self) { if self.current_report_state != CurrentReportState::Opened { - self.errors.push(JunitParseError::ReportStartTagNotFound); + self.issues.push(JunitParseIssue::Invalid( + JunitParseIssueInvalid::ReportStartTagNotFound, + )); return; } @@ -331,14 +415,18 @@ impl JunitParser { } self.current_report.add_test_suite(test_suite); } else { - self.errors.push(JunitParseError::TestSuiteStartTagNotFound); + self.issues.push(JunitParseIssue::Invalid( + JunitParseIssueInvalid::TestSuiteStartTagNotFound, + )); } } fn open_test_case(&mut self, e: &BytesStart) { let test_case_name = parse_attr::name(e).unwrap_or_default(); if test_case_name.is_empty() { - self.errors.push(JunitParseError::TestCaseName); + self.issues.push(JunitParseIssue::Invalid( + JunitParseIssueInvalid::TestCaseName, + )); }; let mut test_case = TestCase::new(test_case_name, TestCaseStatus::success()); @@ -388,10 +476,14 @@ impl JunitParser { if let Some(test_case) = self.current_test_case.take() { test_suite.add_test_case(test_case); } else { - self.errors.push(JunitParseError::TestCaseStartTagNotFound); + self.issues.push(JunitParseIssue::Invalid( + JunitParseIssueInvalid::TestCaseStartTagNotFound, + )); } } else { - self.errors.push(JunitParseError::TestCaseTestSuiteNotFound); + self.issues.push(JunitParseIssue::Invalid( + JunitParseIssueInvalid::TestCaseTestSuiteNotFound, + )); } } @@ -422,8 +514,9 @@ impl JunitParser { test_case.status = test_case_status; } else { - self.errors - .push(JunitParseError::TestCaseStatusTestCaseNotFound); + self.issues.push(JunitParseIssue::Invalid( + JunitParseIssueInvalid::TestCaseStatusTestCaseNotFound, + )); } } @@ -476,10 +569,14 @@ impl JunitParser { if let Some(test_rerun) = self.current_test_rerun.take() { test_case.status.add_rerun(test_rerun); } else { - self.errors.push(JunitParseError::TestRerunStartTagNotFound); + self.issues.push(JunitParseIssue::Invalid( + JunitParseIssueInvalid::TestRerunStartTagNotFound, + )); } } else { - self.errors.push(JunitParseError::TestRerunTestCaseNotFound); + self.issues.push(JunitParseIssue::Invalid( + JunitParseIssueInvalid::TestRerunTestCaseNotFound, + )); } } @@ -634,7 +731,7 @@ mod unescape_and_truncate { .map(|b| safe_truncate_cow::(b)) } - fn safe_truncate_cow<'a, const MAX_LEN: usize>(value: Cow<'a, str>) -> Cow<'a, str> { + fn safe_truncate_cow(value: Cow<'_, str>) -> Cow<'_, str> { match value { Cow::Borrowed(b) => Cow::Borrowed(safe_truncate_str::(b)), Cow::Owned(b) => Cow::Owned(String::from(safe_truncate_str::(b.as_str()))), diff --git a/context/tests/junit.rs b/context/tests/junit.rs index e10649f4..5f059231 100644 --- a/context/tests/junit.rs +++ b/context/tests/junit.rs @@ -72,7 +72,7 @@ fn parse_report>(serialized_report: T) -> Report { .parse(BufReader::new(&serialized_report.as_ref()[..])) .unwrap(); - assert_eq!(junit_parser.errors(), &[]); + assert_eq!(junit_parser.issues(), &[]); let mut parsed_reports = junit_parser.into_reports(); assert_eq!(parsed_reports.len(), 1);