From 4f5f9e6c36ed7abcafba6c65b9eed9df24c4c1fa Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Sun, 7 Dec 2025 14:40:15 -0800 Subject: [PATCH 1/4] fix: properly parse xml with no replacements --- cpp-linter/src/clang_tools/clang_format.rs | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/cpp-linter/src/clang_tools/clang_format.rs b/cpp-linter/src/clang_tools/clang_format.rs index dce92bb..33e2218 100644 --- a/cpp-linter/src/clang_tools/clang_format.rs +++ b/cpp-linter/src/clang_tools/clang_format.rs @@ -18,10 +18,10 @@ use crate::{ common_fs::{FileObj, get_line_count_from_offset}, }; -#[derive(Debug, Clone, Deserialize, PartialEq, Eq)] +#[derive(Debug, Clone, Deserialize, PartialEq, Eq, Default)] pub struct FormatAdvice { /// A list of [`Replacement`]s that clang-tidy wants to make. - #[serde(rename(deserialize = "replacement"))] + #[serde(rename(deserialize = "replacement"), default)] pub replacements: Vec, pub patched: Option>, @@ -196,6 +196,22 @@ mod tests { assert!(result.is_err()); } + #[test] + fn parse_xml_no_replacements() { + let xml_raw = r#" + +"# + .as_bytes() + .to_vec(); + let expected = FormatAdvice { + replacements: vec![], + patched: None, + }; + let xml = String::from_utf8(xml_raw).unwrap(); + let document = quick_xml::de::from_str::(&xml).unwrap(); + assert_eq!(expected, document); + } + #[test] fn parse_xml() { let xml_raw = r#" From e7615fd729f33186e62ca0549b1c60429e1c8230 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Thu, 11 Dec 2025 09:08:56 -0800 Subject: [PATCH 2/4] use `FormatAdvice::default()` where possible --- cpp-linter/src/clang_tools/clang_format.rs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/cpp-linter/src/clang_tools/clang_format.rs b/cpp-linter/src/clang_tools/clang_format.rs index 33e2218..b89d509 100644 --- a/cpp-linter/src/clang_tools/clang_format.rs +++ b/cpp-linter/src/clang_tools/clang_format.rs @@ -151,10 +151,7 @@ pub fn run_clang_format( format!("Failed to parse XML output from clang-format for {file_name}") })? } else { - FormatAdvice { - replacements: vec![], - patched: None, - } + FormatAdvice::default() }; format_advice.patched = patched; if !format_advice.replacements.is_empty() { @@ -203,10 +200,7 @@ mod tests { "# .as_bytes() .to_vec(); - let expected = FormatAdvice { - replacements: vec![], - patched: None, - }; + let expected = FormatAdvice::default(); let xml = String::from_utf8(xml_raw).unwrap(); let document = quick_xml::de::from_str::(&xml).unwrap(); assert_eq!(expected, document); From 68aff12b9eb588d281386d080f0d073e56e836e1 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Thu, 11 Dec 2025 10:43:19 -0800 Subject: [PATCH 3/4] fix: don't ignore errors When capturing clang tools output, any error encountered should cause fast-failure behavior. Also adjust run_main() tests so that erroneous output is more helpful.. --- cpp-linter/src/clang_tools/mod.rs | 15 +++++------ cpp-linter/src/run.rs | 41 ++++++++++++++++--------------- 2 files changed, 29 insertions(+), 27 deletions(-) diff --git a/cpp-linter/src/clang_tools/mod.rs b/cpp-linter/src/clang_tools/mod.rs index 1879ff6..096e8ad 100644 --- a/cpp-linter/src/clang_tools/mod.rs +++ b/cpp-linter/src/clang_tools/mod.rs @@ -248,14 +248,15 @@ pub async fn capture_clang_tools_output( } while let Some(output) = executors.join_next().await { - if let Ok(out) = output? { - let (file_name, logs) = out; - rest_api_client.start_log_group(format!("Analyzing {}", file_name.to_string_lossy())); - for (level, msg) in logs { - log::log!(level, "{}", msg); - } - rest_api_client.end_log_group(); + // output?? acts as a fast-fail for any error encountered. + // This includes any `spawn()` error and any `analyze_single_file()` error. + // Any unresolved tasks are aborted and dropped when an error is returned here. + let (file_name, logs) = output??; + rest_api_client.start_log_group(format!("Analyzing {}", file_name.to_string_lossy())); + for (level, msg) in logs { + log::log!(level, "{}", msg); } + rest_api_client.end_log_group(); } Ok(clang_versions) } diff --git a/cpp-linter/src/run.rs b/cpp-linter/src/run.rs index 88ef076..8db906e 100644 --- a/cpp-linter/src/run.rs +++ b/cpp-linter/src/run.rs @@ -165,7 +165,7 @@ mod test { unsafe { env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests } - let result = run_main(vec![ + run_main(vec![ "cpp-linter".to_string(), "-l".to_string(), "false".to_string(), @@ -173,8 +173,8 @@ mod test { "tests".to_string(), "demo/demo.cpp".to_string(), ]) - .await; - assert!(result.is_ok()); + .await + .unwrap(); } #[tokio::test] @@ -182,8 +182,9 @@ mod test { unsafe { env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests } - let result = run_main(vec!["cpp-linter".to_string(), "version".to_string()]).await; - assert!(result.is_ok()); + run_main(vec!["cpp-linter".to_string(), "version".to_string()]) + .await + .unwrap(); } #[tokio::test] @@ -191,15 +192,15 @@ mod test { unsafe { env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests } - let result = run_main(vec![ + run_main(vec![ "cpp-linter".to_string(), "-l".to_string(), "false".to_string(), "-v".to_string(), "-i=target|benches/libgit2".to_string(), ]) - .await; - assert!(result.is_ok()); + .await + .unwrap(); } #[tokio::test] @@ -207,14 +208,14 @@ mod test { unsafe { env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests } - let result = run_main(vec![ + run_main(vec![ "cpp-linter".to_string(), "-l".to_string(), "false".to_string(), "-V".to_string(), ]) - .await; - assert!(result.is_ok()); + .await + .unwrap(); } #[tokio::test] @@ -223,14 +224,14 @@ mod test { env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests env::set_var("PRE_COMMIT", "1"); } - let result = run_main(vec![ + run_main(vec![ "cpp-linter".to_string(), "--lines-changed-only".to_string(), "false".to_string(), "--ignore=target|benches/libgit2".to_string(), ]) - .await; - assert!(result.is_err()); + .await + .unwrap_err(); } // Verifies that the system gracefully handles cases where all analysis is disabled. @@ -240,7 +241,7 @@ mod test { unsafe { env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests } - let result = run_main(vec![ + run_main(vec![ "cpp-linter".to_string(), "-l".to_string(), "false".to_string(), @@ -248,8 +249,8 @@ mod test { String::new(), "--tidy-checks=-*".to_string(), ]) - .await; - assert!(result.is_ok()); + .await + .unwrap(); } #[tokio::test] @@ -257,12 +258,12 @@ mod test { unsafe { env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests } - let result = run_main(vec![ + run_main(vec![ "cpp-linter".to_string(), "--repo-root".to_string(), "some-non-existent-dir".to_string(), ]) - .await; - assert!(result.is_err()); + .await + .unwrap_err(); } } From 96dff79192e92ee764b6f299cf362b5d7c49b1dd Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Thu, 11 Dec 2025 10:44:11 -0800 Subject: [PATCH 4/4] ignore demo.hpp during LGTM tests about PR reviews --- cpp-linter/tests/reviews.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/cpp-linter/tests/reviews.rs b/cpp-linter/tests/reviews.rs index 339961c..21fa230 100644 --- a/cpp-linter/tests/reviews.rs +++ b/cpp-linter/tests/reviews.rs @@ -211,17 +211,19 @@ async fn setup(lib_root: &Path, test_params: &TestParams) { ); } - let mut tool_ignore = "**/*.c".to_string(); + let mut format_ignore = "**/*.c".to_string(); + let mut tidy_ignore = format_ignore.clone(); if test_params.force_lgtm { - tool_ignore.push_str("|**/*.cpp|**/*.h"); + format_ignore.push_str("|**/*.cpp|**/*.h"); + tidy_ignore.push_str("|**/*.hpp"); } let mut args = vec![ "cpp-linter".to_string(), "-v=debug".to_string(), format!("-V={}", clang_version), format!("-l={}", test_params.lines_changed_only), - format!("--ignore-tidy={}", tool_ignore), - format!("--ignore-format={}", tool_ignore), + format!("--ignore-tidy={}", tidy_ignore), + format!("--ignore-format={}", format_ignore), format!("--tidy-review={}", test_params.tidy_review), format!("--format-review={}", test_params.format_review), format!("--passive-reviews={}", test_params.passive_reviews),