Skip to content

Commit fde6ce5

Browse files
2bndy52bndy5
authored
fix: properly parse xml with no replacements (#230)
Fixing XML parsing caused another problem to surface: Errors were ignored during capture of clang tools output. A side effect of that fix required some test changes about PR reviews (LGTM scenario). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Graceful handling when no formatting replacements are returned; avoids unexpected failures. * Lint review ignore rules separated for formatting vs. tidy, so each tool can ignore different file sets. * **Tests** * Added tests for parsing inputs with no replacements. * Tightened test assertions to fail fast on unexpected results. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent d8d252d commit fde6ce5

File tree

4 files changed

+51
-37
lines changed

4 files changed

+51
-37
lines changed

‎cpp-linter/src/clang_tools/clang_format.rs‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,10 @@ use crate::{
1818
common_fs::{FileObj, get_line_count_from_offset},
1919
};
2020

21-
#[derive(Debug, Clone, Deserialize, PartialEq, Eq)]
21+
#[derive(Debug, Clone, Deserialize, PartialEq, Eq, Default)]
2222
pub struct FormatAdvice {
2323
/// A list of [`Replacement`]s that clang-tidy wants to make.
24-
#[serde(rename(deserialize = "replacement"))]
24+
#[serde(rename(deserialize = "replacement"), default)]
2525
pub replacements: Vec<Replacement>,
2626

2727
pub patched: Option<Vec<u8>>,
@@ -151,10 +151,7 @@ pub fn run_clang_format(
151151
format!("Failed to parse XML output from clang-format for {file_name}")
152152
})?
153153
} else {
154-
FormatAdvice {
155-
replacements: vec![],
156-
patched: None,
157-
}
154+
FormatAdvice::default()
158155
};
159156
format_advice.patched = patched;
160157
if !format_advice.replacements.is_empty() {
@@ -196,6 +193,19 @@ mod tests {
196193
assert!(result.is_err());
197194
}
198195

196+
#[test]
197+
fn parse_xml_no_replacements() {
198+
let xml_raw = r#"<?xml version='1.0'?>
199+
<replacements xml:space='preserve' incomplete_format='false'>
200+
</replacements>"#
201+
.as_bytes()
202+
.to_vec();
203+
let expected = FormatAdvice::default();
204+
let xml = String::from_utf8(xml_raw).unwrap();
205+
let document = quick_xml::de::from_str::<FormatAdvice>(&xml).unwrap();
206+
assert_eq!(expected, document);
207+
}
208+
199209
#[test]
200210
fn parse_xml() {
201211
let xml_raw = r#"<?xml version='1.0'?>

‎cpp-linter/src/clang_tools/mod.rs‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -248,14 +248,15 @@ pub async fn capture_clang_tools_output(
248248
}
249249

250250
while let Some(output) = executors.join_next().await {
251-
if let Ok(out) = output? {
252-
let (file_name, logs) = out;
253-
rest_api_client.start_log_group(format!("Analyzing {}", file_name.to_string_lossy()));
254-
for (level, msg) in logs {
255-
log::log!(level, "{}", msg);
256-
}
257-
rest_api_client.end_log_group();
251+
// output?? acts as a fast-fail for any error encountered.
252+
// This includes any `spawn()` error and any `analyze_single_file()` error.
253+
// Any unresolved tasks are aborted and dropped when an error is returned here.
254+
let (file_name, logs) = output??;
255+
rest_api_client.start_log_group(format!("Analyzing {}", file_name.to_string_lossy()));
256+
for (level, msg) in logs {
257+
log::log!(level, "{}", msg);
258258
}
259+
rest_api_client.end_log_group();
259260
}
260261
Ok(clang_versions)
261262
}

‎cpp-linter/src/run.rs‎

Lines changed: 21 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -165,56 +165,57 @@ mod test {
165165
unsafe {
166166
env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests
167167
}
168-
let result = run_main(vec![
168+
run_main(vec![
169169
"cpp-linter".to_string(),
170170
"-l".to_string(),
171171
"false".to_string(),
172172
"--repo-root".to_string(),
173173
"tests".to_string(),
174174
"demo/demo.cpp".to_string(),
175175
])
176-
.await;
177-
assert!(result.is_ok());
176+
.await
177+
.unwrap();
178178
}
179179

180180
#[tokio::test]
181181
async fn version_command() {
182182
unsafe {
183183
env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests
184184
}
185-
let result = run_main(vec!["cpp-linter".to_string(), "version".to_string()]).await;
186-
assert!(result.is_ok());
185+
run_main(vec!["cpp-linter".to_string(), "version".to_string()])
186+
.await
187+
.unwrap();
187188
}
188189

189190
#[tokio::test]
190191
async fn force_debug_output() {
191192
unsafe {
192193
env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests
193194
}
194-
let result = run_main(vec![
195+
run_main(vec![
195196
"cpp-linter".to_string(),
196197
"-l".to_string(),
197198
"false".to_string(),
198199
"-v".to_string(),
199200
"-i=target|benches/libgit2".to_string(),
200201
])
201-
.await;
202-
assert!(result.is_ok());
202+
.await
203+
.unwrap();
203204
}
204205

205206
#[tokio::test]
206207
async fn no_version_input() {
207208
unsafe {
208209
env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests
209210
}
210-
let result = run_main(vec![
211+
run_main(vec![
211212
"cpp-linter".to_string(),
212213
"-l".to_string(),
213214
"false".to_string(),
214215
"-V".to_string(),
215216
])
216-
.await;
217-
assert!(result.is_ok());
217+
.await
218+
.unwrap();
218219
}
219220

220221
#[tokio::test]
@@ -223,14 +224,14 @@ mod test {
223224
env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests
224225
env::set_var("PRE_COMMIT", "1");
225226
}
226-
let result = run_main(vec![
227+
run_main(vec![
227228
"cpp-linter".to_string(),
228229
"--lines-changed-only".to_string(),
229230
"false".to_string(),
230231
"--ignore=target|benches/libgit2".to_string(),
231232
])
232-
.await;
233-
assert!(result.is_err());
233+
.await
234+
.unwrap_err();
234235
}
235236

236237
// Verifies that the system gracefully handles cases where all analysis is disabled.
@@ -240,29 +241,29 @@ mod test {
240241
unsafe {
241242
env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests
242243
}
243-
let result = run_main(vec![
244+
run_main(vec![
244245
"cpp-linter".to_string(),
245246
"-l".to_string(),
246247
"false".to_string(),
247248
"--style".to_string(),
248249
String::new(),
249250
"--tidy-checks=-*".to_string(),
250251
])
251-
.await;
252-
assert!(result.is_ok());
252+
.await
253+
.unwrap();
253254
}
254255

255256
#[tokio::test]
256257
async fn bad_repo_root() {
257258
unsafe {
258259
env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests
259260
}
260-
let result = run_main(vec![
261+
run_main(vec![
261262
"cpp-linter".to_string(),
262263
"--repo-root".to_string(),
263264
"some-non-existent-dir".to_string(),
264265
])
265-
.await;
266-
assert!(result.is_err());
266+
.await
267+
.unwrap_err();
267268
}
268269
}

‎cpp-linter/tests/reviews.rs‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -211,17 +211,19 @@ async fn setup(lib_root: &Path, test_params: &TestParams) {
211211
);
212212
}
213213

214-
let mut tool_ignore = "**/*.c".to_string();
214+
let mut format_ignore = "**/*.c".to_string();
215+
let mut tidy_ignore = format_ignore.clone();
215216
if test_params.force_lgtm {
216-
tool_ignore.push_str("|**/*.cpp|**/*.h");
217+
format_ignore.push_str("|**/*.cpp|**/*.h");
218+
tidy_ignore.push_str("|**/*.hpp");
217219
}
218220
let mut args = vec![
219221
"cpp-linter".to_string(),
220222
"-v=debug".to_string(),
221223
format!("-V={}", clang_version),
222224
format!("-l={}", test_params.lines_changed_only),
223-
format!("--ignore-tidy={}", tool_ignore),
224-
format!("--ignore-format={}", tool_ignore),
225+
format!("--ignore-tidy={}", tidy_ignore),
226+
format!("--ignore-format={}", format_ignore),
225227
format!("--tidy-review={}", test_params.tidy_review),
226228
format!("--format-review={}", test_params.format_review),
227229
format!("--passive-reviews={}", test_params.passive_reviews),

0 commit comments

Comments
 (0)