Skip to content

Commit

Permalink
Treat 'no reports found' as a suboptimal parsing issue (#305)
Browse files Browse the repository at this point in the history 
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:~$
``
  • Loading branch information
@max-trunk
max-trunk authored Jan 17, 2025
1 parent f9e5618 commit dfcabc9
Show file tree
Hide file tree
Showing 11 changed files with 380 additions and 193 deletions.
2 changes: 1 addition & 1 deletion cli-tests/src/upload.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
17 changes: 17 additions & 0 deletions cli-tests/src/validate.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
234 changes: 142 additions & 92 deletions cli/src/validate_command.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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<String>,
#[arg(
Expand All @@ -39,7 +39,7 @@ pub struct ValidateArgs {
help = "Path to bazel build event protocol JSON file."
)]
bazel_bep_path: Option<String>,
#[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<String>,
Expand All @@ -49,7 +49,7 @@ pub async fn run_validate(validate_args: ValidateArgs) -> anyhow::Result<i32> {
let ValidateArgs {
junit_paths,
bazel_bep_path,
show_warnings,
show_warnings: _,
codeowners_path,
} = validate_args;

Expand All @@ -65,15 +65,17 @@ pub async fn run_validate(validate_args: ValidateArgs) -> anyhow::Result<i32> {
.map(JunitReportFileWithStatus::from)
.collect(),
};
validate(junit_file_paths, show_warnings, codeowners_path).await
validate(junit_file_paths, codeowners_path).await
}

type JunitFileToReportAndErrors = BTreeMap<String, (anyhow::Result<Report>, Vec<JunitParseError>)>;
type JunitFileToValidation = BTreeMap<String, anyhow::Result<JunitReportValidation>>;
type JunitFileToReportAndParseIssues =
BTreeMap<String, (anyhow::Result<Option<Report>>, Vec<JunitParseIssue>)>;
type JunitFileToReport = BTreeMap<String, Report>;
type JunitFileToParseIssues = BTreeMap<String, (anyhow::Result<()>, Vec<JunitParseIssue>)>;
type JunitFileToValidation = BTreeMap<String, JunitReportValidation>;

async fn validate(
junit_paths: Vec<JunitReportFileWithStatus>,
show_warnings: bool,
codeowners_path: Option<String>,
) -> anyhow::Result<i32> {
// scan files
Expand All @@ -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
};

Expand All @@ -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,
Expand All @@ -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
},
)
Expand All @@ -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(
Expand Down Expand Up @@ -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<JunitReportValidationFlatIssue> = 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<JunitReportValidationFlatIssue> = report_validation.all_issues_flat();

let num_validation_errors_str = if num_validation_errors > 0 {
num_validation_errors.to_string().red()
} else {
Expand All @@ -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!(
" {} - {}",
Expand Down Expand Up @@ -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,
Expand Down
Loading

0 comments on commit dfcabc9

Please sign in to comment.