[TRUNK-12978] v1 validate command #129

max-trunk · 2024-10-15T21:08:51Z

Adds a first iteration of a validate command, intended to allow users onboarding to flaky tests to verify they're producing valid JUnit xml files that we can accurately process, before they configure their CI jobs to start uploading to us.

Users pass a list of --junit-paths to validate. For each file found, validate will parse and validate using the parser and validator in this repo's context crate.

If any fatal parsing errors are encountered, validate will exit with a non-zero exit code.

If a file contains INVALID-level validation errors, validate considers this file to be 'invalid.' If a file contains no INVALID-level validation errors, and zero or more SUBOPTIMAL-level validation errors, validate considers this file to be 'valid.' If any 'invalid' files are encountered, validate exits with a non-zero exit code. If all files found are 'valid,' validate exits successfully and prints (eventually) a link to return to the onboarding flow of flaky tests.

Example invocation where fatal parsing error is encountered:

Example invocation where some files are invalid:

Example invocation where all files are valid, some with SUBOPTIMAL validation errors:

Example invocation where all files are valid, all with no SUBOPTIMAL validation errors:

Follow-ups:

show line numbers of where validation errors occur
add some more context to validation errors to better describe impact on flaky tests processing to users
use actual URL for flaky tests onboarding when that is known

trunk-staging-io · 2024-10-16T14:04:41Z

406 tests were run on cb493aaa. ✅ 406 Passed. View Full Report ↗︎

_settings

dfrankland · 2024-10-16T15:54:01Z

cli/src/main.rs


+use colored::{ColoredString, Colorize};
+


nit: can we group this with the rest of the imports for organization?

dfrankland · 2024-10-16T15:55:22Z

cli/src/main.rs

+    log::info!(
+        "Starting trunk-analytics-cli {} (git={}) rustc={}",
+        env!("CARGO_PKG_VERSION"),
+        env!("VERGEN_GIT_SHA"),
+        env!("VERGEN_RUSTC_SEMVER")
+    );


can we make a function that does this and reuse it across commands?

dfrankland · 2024-10-16T15:57:16Z

cli/src/main.rs

+            "  File set ({:?}): {}",
+            file_set.file_set_type,
+            file_set.glob
+        );
+        for file in &file_set.files {
+            log::info!("    {}", file.original_path_rel);
+        }


nit, optional: may want to use \t tab character for spacing

dfrankland · 2024-10-16T16:08:28Z

cli/src/scanner.rs

-            let original_path = path
+            let original_path_abs = path
+                .to_str()
+                .expect("failed to convert path to string")
+                .to_string();
+            let original_path_rel = path
+                .strip_prefix(repo_root)
+                .unwrap_or(&path)
                .to_str()
                .expect("failed to convert path to string")
                .to_string();


I know you're following the already existing pattern here, but I think we need to update this so it doesn't panic here. Instead of using expect we can use map_err + ? (try sigil)

Thanks! went with ok_or_else since I was working with an Option rather than a Result

dfrankland · 2024-10-16T16:10:22Z

cli/src/types.rs

+#[derive(Debug, Serialize, Deserialize, Clone)]
+pub struct WithFilePath<T> {
+    pub file_path: String,
+    pub wrapped: T,
+}


I think you probably don't need a named struct, but rather just a simple tuple:
https://doc.rust-lang.org/rust-by-example/primitives/tuples.html

dfrankland · 2024-10-16T16:13:58Z

cli/src/main.rs

+    let mut reports: Vec<WithFilePath<Report>> = Vec::new();
+    let mut parse_errors: Vec<WithFilePath<JunitParseError>> = Vec::new();
+    file_sets.iter().try_for_each(|file_set| {
+        file_set.files.iter().try_for_each(|bundled_file| {
+            let path = std::path::Path::new(&bundled_file.original_path);
+            let file = std::fs::File::open(path)?;
+            let file_buf_reader = BufReader::new(file);
+            let mut junit_parser = JunitParser::new();
+            junit_parser.parse(file_buf_reader).context(format!(
+                "Encountered unrecoverable error while parsing file: {}",
+                bundled_file.original_path_rel
+            ))?;
+            parse_errors.extend(junit_parser.errors().iter().map(|e| WithFilePath::<
+                JunitParseError,
+            > {
+                file_path: bundled_file.original_path_rel.clone(),
+                wrapped: *e,
+            }));
+            reports.extend(junit_parser.into_reports().iter().map(
+                |report| WithFilePath::<Report> {
+                    file_path: bundled_file.original_path_rel.clone(),
+                    wrapped: report.clone(),
+                },
+            ));
+            Ok::<(), anyhow::Error>(())
+        })?;
+        Ok::<(), anyhow::Error>(())
+    })?;


nit: this is more idiomatically a fold operation. for_each is typically for side-effects

dfrankland · 2024-10-16T16:16:28Z

cli/src/main.rs

@@ -511,10 +537,222 @@ async fn run_test(test_args: TestArgs) -> anyhow::Result<i32> {
    Ok(exit_code)
 }

+async fn run_validate(validate_args: ValidateArgs) -> anyhow::Result<i32> {


This file is getting unwieldy in length. Can we split out a file for this command and we'll split out the other commands into files later?

Also, I'd love for you to split out functions for each logical part of the command that you run so that it's easy to understand the flow

Alrighty, lmk if what I've got now is a bit easier on the eyes

TylerJang27

LGTM!

TylerJang27 · 2024-10-16T23:27:54Z

cli/src/validate.rs

+) -> (usize, usize) {
+    log::info!("");
+    let mut num_invalid_reports: usize = 0;
+    let mut num_optionally_invalid_reports: usize = 0;


nit: rename to suboptimal for consistency with the underlying types/enums
(and rename elsewhere)

TylerJang27 · 2024-10-16T23:31:53Z

cli-tests/src/validate.rs

+
+    println!("{assert}");
+}
+


nit: would also be nice to have a test for suboptimal junits

dfrankland · 2024-10-17T17:37:39Z

cli/src/main.rs

 async fn run(cli: Cli) -> anyhow::Result<i32> {
    match cli.command {
        Commands::Upload(upload_args) => run_upload(upload_args, None, None, None, None).await,
        Commands::Test(test_args) => run_test(test_args).await,
+        Commands::Validate(validate_args) => run_validate(validate_args).await,


Totally optional, but we can get rid of the run_validate indirection here:

Suggested change

Commands::Validate(validate_args) => run_validate(validate_args).await,

Commands::Validate(validate_args) => {

let ValidateArgs {

junit_paths,

show_warnings,

} = validate_args;

print_cli_start_info();

validate(junit_paths, show_warnings).await

},

IMO, this gets us closer to splitting concerns in a way that prevents repeated code and inconsistencies

dfrankland · 2024-10-17T17:40:11Z

cli/src/validate.rs

+            Vec::<(Report, String)>::new(),          // Vec<(Report, file path)>
+            Vec::<(JunitParseError, String)>::new(), // Vec<(JunitParseError, file path)>


Accidental comments?

dfrankland · 2024-10-17T17:50:58Z

cli/src/validate.rs

+
+fn parse_file_sets(
+    file_sets: Vec<FileSet>,
+) -> anyhow::Result<(Vec<(Report, String)>, Vec<(JunitParseError, String)>)> {


You can create a type alias for these more complex types and compose them together nicely for readability
https://doc.rust-lang.org/reference/items/type-aliases.html

For example:

type ParsedReportAndFilePaths = Vec<(Report, String)>; type JunitParseErrorsAndFilePaths = Vec<(JunitParseError, String)>;

which would update your return type to:

anyhow::Result<(ParsedReportAndFilePaths, JunitParseErrorsAndFilePaths)>

Btw, it occurs to me that we could use a BTreeMap here though, no? Something like

type JunitFileToReportAndErrors = BTreeMap<String, (Vec<Report>, Vec<JunitParseError>)>;

dfrankland · 2024-10-17T17:53:31Z

cli/src/validate.rs

+            Ok::<(Vec<(Report, String)>, Vec<(JunitParseError, String)>), anyhow::Error>(
+                file_sets_parse_results,
+            )


If you set a return type of the closure you won't have to annotate the type of Result here:

e.g.

|mut file_sets_parse_results, file_set| -> anyhow::Result<(Vec<(Report, String)>, Vec<(JunitParseError, String)>)> { // ... Ok(file_sets_parse_results) }

dfrankland · 2024-10-17T17:54:06Z

cli/src/validate.rs

+fn print_matched_files(file_sets: &[FileSet], file_counter: FileSetCounter) {
+    log::info!("");
+    log::info!(
+        "Validating the following {} files:",
+        file_counter.get_count()
+    );
+    for file_set in file_sets {
+        log::info!("  File set matching {}:", file_set.glob);
+        for file in &file_set.files {
+            log::info!("\t{}", file.original_path_rel);
+        }
+    }
+}
+
+fn print_parse_errors(parse_errors: Vec<(JunitParseError, String)>) {
+    log::info!("");
+    log::warn!(
+        "Encountered the following {} non-fatal errors while parsing files:",
+        parse_errors.len().to_string().yellow()
+    );
+
+    let mut current_file_original_path = parse_errors[0].1.clone();
+    log::warn!("  File: {}", current_file_original_path);
+
+    for error in parse_errors {
+        if error.1 != current_file_original_path {
+            current_file_original_path = error.1;
+            log::warn!("  File: {}", current_file_original_path);
+        }
+
+        log::warn!("\t{}", error.0);
+    }
+}


This is so much nicer! Thank you 🙏

dfrankland · 2024-10-17T17:54:38Z

cli/src/validate.rs

+    log::info!("");
+    log::info!(


Consider using \n for making newlines

max-trunk added 3 commits October 15, 2024 20:49

validate v1

3ea4e6e

common cli-tests utils

1e21476

undo original_path -> original_path_abs

7254a19

print file path for parsing error

939f395

max-trunk requested review from dfrankland, katchao, pv72895, TylerJang27 and tenesttang-trunk October 16, 2024 14:40

dfrankland reviewed Oct 16, 2024

View reviewed changes

max-trunk added 2 commits October 16, 2024 20:52

pr feedback

53afb87

rm comment

f907369

TylerJang27 approved these changes Oct 16, 2024

View reviewed changes

pr feedback; shannon feedback

963e8c9

dfrankland approved these changes Oct 17, 2024

View reviewed changes

max-trunk added 2 commits October 17, 2024 18:26

merge main

cb9fcd1

more pr feedback

2199e83

max-trunk merged commit 2fea36d into main Oct 17, 2024
11 checks passed

max-trunk mentioned this pull request Oct 18, 2024

[TRUNK-13072] Add num_files, num_tests, repo_head_sha_short to meta.json #131

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TRUNK-12978] v1 validate command #129

[TRUNK-12978] v1 validate command #129

max-trunk commented Oct 15, 2024 •

edited

Loading

trunk-staging-io bot commented Oct 16, 2024 •

edited

Loading

dfrankland Oct 16, 2024

dfrankland Oct 16, 2024

dfrankland Oct 16, 2024

dfrankland Oct 16, 2024

max-trunk Oct 16, 2024

dfrankland Oct 16, 2024

dfrankland Oct 16, 2024

dfrankland Oct 16, 2024

max-trunk Oct 16, 2024

max-trunk Oct 16, 2024

TylerJang27 left a comment

TylerJang27 Oct 16, 2024

TylerJang27 Oct 16, 2024

dfrankland Oct 17, 2024

dfrankland Oct 17, 2024

dfrankland Oct 17, 2024

dfrankland Oct 17, 2024

dfrankland Oct 17, 2024

dfrankland Oct 17, 2024

-        Commands::Validate(validate_args) => run_validate(validate_args).await,
+        Commands::Validate(validate_args) => {
+            let ValidateArgs {
+                junit_paths,
+                show_warnings,
+            } = validate_args;
+            print_cli_start_info();
+            validate(junit_paths, show_warnings).await
+        },

		Vec::<(Report, String)>::new(), // Vec<(Report, file path)>
		Vec::<(JunitParseError, String)>::new(), // Vec<(JunitParseError, file path)>

[TRUNK-12978] v1 validate command #129

[TRUNK-12978] v1 validate command #129

Conversation

max-trunk commented Oct 15, 2024 • edited Loading

trunk-staging-io bot commented Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerJang27 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

max-trunk commented Oct 15, 2024 •

edited

Loading

trunk-staging-io bot commented Oct 16, 2024 •

edited

Loading