add strictness to tests

Exclude clang v7 and v8 on windows for PR review tests. Those versions do not have a consistent cross-platform output from clang-tidy.

Author: 2bndy5

.github/workflows/run-dev-tests.yml

@@ -75,23 +75,25 @@ jobs:
         run: sudo apt-get update
 
       - name: Install clang v7
+        if: matrix.os == 'Linux'
         uses: cpp-linter/cpp_linter_rs/install-clang-action@pr-reviews
         with:
           version: '7'
 
       - name: Collect Coverage for clang v7
-        working-directory: cpp-linter-lib
+        if: matrix.os == 'Linux'
         env:
           CLANG_VERSION: '7'
         run: just test
 
       - name: Install clang v8
+        if: matrix.os == 'Linux'
         uses: cpp-linter/cpp_linter_rs/install-clang-action@pr-reviews
         with:
           version: '8'
 
       - name: Collect Coverage for clang v8
-        working-directory: cpp-linter-lib
+        if: matrix.os == 'Linux'
         env:
           CLANG_VERSION: '8'
         run: just test
@@ -102,7 +104,6 @@ jobs:
           version: '9'
 
       - name: Collect Coverage for clang v9
-        working-directory: cpp-linter-lib
         env:
           CLANG_VERSION: '9'
         run: just test
@@ -113,7 +114,6 @@ jobs:
           version: '10'
 
       - name: Collect Coverage for clang v10
-        working-directory: cpp-linter-lib
         env:
           CLANG_VERSION: '10'
         run: just test
@@ -124,7 +124,6 @@ jobs:
           version: '11'
 
       - name: Collect Coverage for clang v11
-        working-directory: cpp-linter-lib
         env:
           CLANG_VERSION: '11'
         run: just test
@@ -135,7 +134,6 @@ jobs:
           version: '12'
 
       - name: Collect Coverage for clang v12
-        working-directory: cpp-linter-lib
         env:
           CLANG_VERSION: '12'
         run: just test
@@ -146,7 +144,6 @@ jobs:
           version: '13'
 
       - name: Collect Coverage for clang v13
-        working-directory: cpp-linter-lib
         env:
           CLANG_VERSION: '13'
         run: just test
@@ -157,7 +154,6 @@ jobs:
           version: '14'
 
       - name: Collect Coverage for clang v14
-        working-directory: cpp-linter-lib
         env:
           CLANG_VERSION: '14'
         run: just test
@@ -168,7 +164,6 @@ jobs:
           version: '15'
 
       - name: Collect Coverage for clang v15
-        working-directory: cpp-linter-lib
         env:
           CLANG_VERSION: '15'
         run: just test
@@ -179,7 +174,6 @@ jobs:
           version: '16'
 
       - name: Collect Coverage for clang v16
-        working-directory: cpp-linter-lib
         env:
           CLANG_VERSION: '16'
         run: just test
@@ -190,7 +184,6 @@ jobs:
           version: '17'
 
       - name: Collect Coverage for clang v17
-        working-directory: cpp-linter-lib
         env:
           CLANG_VERSION: '17'
         run: just test
@@ -201,13 +194,11 @@ jobs:
           version: '18'
 
       - name: Collect Coverage for clang v18
-        working-directory: cpp-linter-lib
         env:
           CLANG_VERSION: '18'
         run: just test --run-ignored=all
 
       - name: Generate Coverage HTML report
-        working-directory: cpp-linter-lib
         run: just pretty-cov
 
       - name: Upload coverage data
@@ -218,12 +209,11 @@ jobs:
 
       - name: Generate Coverage lcov report
         if: runner.os == 'Linux'
-        working-directory: cpp-linter-lib
         run: just lcov
 
       - uses: codecov/codecov-action@v4
         if: runner.os == 'Linux'
         with:
           token: ${{secrets.CODECOV_TOKEN}}
-          files: cpp-linter-lib/lcov.info
+          files: lcov.info
           fail_ci_if_error: true # optional (default = false)

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

@@ -77,6 +77,20 @@ impl Clone for Replacement {
     }
 }
 
+/// Get a string that summarizes the given `--style`
+pub fn summarize_style(style: &str) -> String {
+    if ["google", "chromium", "microsoft", "mozilla", "webkit"].contains(&style) {
+        // capitalize the first letter
+        let mut char_iter = style.chars();
+        let first_char = char_iter.next().unwrap();
+        first_char.to_uppercase().collect::<String>() + char_iter.as_str()
+    } else if style == "llvm" || style == "gnu" {
+        style.to_ascii_uppercase()
+    } else {
+        String::from("Custom")
+    }
+}
+
 /// Get a total count of clang-format advice from the given list of [FileObj]s.
 pub fn tally_format_advice(files: &[Arc<Mutex<FileObj>>]) -> u64 {
     let mut total = 0;
@@ -199,7 +213,7 @@ pub fn run_clang_format(
 
 #[cfg(test)]
 mod tests {
-    use super::{FormatAdvice, Replacement};
+    use super::{summarize_style, FormatAdvice, Replacement};
     use serde::Deserialize;
 
     #[test]
@@ -257,4 +271,23 @@ mod tests {
                 .unwrap();
         assert_eq!(expected, document);
     }
+
+    fn formalize_style(style: &str, expected: &str) {
+        assert_eq!(summarize_style(style), expected);
+    }
+
+    #[test]
+    fn formalize_llvm_style() {
+        formalize_style("llvm", "LLVM");
+    }
+
+    #[test]
+    fn formalize_google_style() {
+        formalize_style("google", "Google");
+    }
+
+    #[test]
+    fn formalize_custom_style() {
+        formalize_style("file", "Custom");
+    }
 }

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

@@ -258,7 +258,7 @@ impl ReviewComments {
                     .as_str(),
                 );
             }
-            if total > 0 {
+            if !self.full_patch[t as usize].is_empty() {
                 body.push_str(
                     format!(
                         "\n<details><summary>Click here for the full {tool_name} patch</summary>\n\n```diff\n{}```\n\n</details>\n",
@@ -267,7 +267,11 @@ impl ReviewComments {
                 );
             } else {
                 body.push_str(
-                    format!("\nNo concerns reported by {}. Great job! :tada:", tool_name).as_str(),
+                    format!(
+                        "\nNo concerns reported by {}. Great job! :tada:\n",
+                        tool_name
+                    )
+                    .as_str(),
                 )
             }
         }
@@ -350,6 +354,9 @@ pub trait MakeSuggestions {
                 .expect("Failed to convert patch buffer to string")
                 .as_str(),
         );
+        if summary_only {
+            return;
+        }
         for hunk_id in 0..hunks_total {
             let (hunk, line_count) = patch.hunk(hunk_id).expect("Failed to get hunk from patch");
             hunks_in_patch += 1;
@@ -359,51 +366,43 @@ pub trait MakeSuggestions {
             }
             let (start_line, end_line) = hunk_range.unwrap();
             let mut suggestion = String::new();
-            let suggestion_help = if !summary_only {
-                Some(self.get_suggestion_help(start_line, end_line))
-            } else {
-                None
-            };
+            let suggestion_help = self.get_suggestion_help(start_line, end_line);
             let mut removed = vec![];
             for line_index in 0..line_count {
                 let diff_line = patch
                     .line_in_hunk(hunk_id, line_index)
                     .expect("Failed to get line in a hunk");
                 let line = String::from_utf8(diff_line.content().to_owned())
                     .expect("Failed to convert line buffer to string");
-                if !summary_only {
-                    if ['+', ' '].contains(&diff_line.origin()) {
-                        suggestion.push_str(line.as_str());
-                    } else {
-                        removed.push(
-                            diff_line
-                                .old_lineno()
-                                .expect("Removed line has no line number?!"),
-                        );
-                    }
+                if ['+', ' '].contains(&diff_line.origin()) {
+                    suggestion.push_str(line.as_str());
+                } else {
+                    removed.push(
+                        diff_line
+                            .old_lineno()
+                            .expect("Removed line has no line number?!"),
+                    );
                 }
             }
-            if !summary_only {
-                if suggestion.is_empty() && !removed.is_empty() {
-                    suggestion.push_str(
-                        format!(
-                            "Please remove the line(s)\n- {}",
-                            removed
-                                .iter()
-                                .map(|l| l.to_string())
-                                .collect::<Vec<String>>()
-                                .join("\n- ")
-                        )
-                        .as_str(),
+            if suggestion.is_empty() && !removed.is_empty() {
+                suggestion.push_str(
+                    format!(
+                        "Please remove the line(s)\n- {}",
+                        removed
+                            .iter()
+                            .map(|l| l.to_string())
+                            .collect::<Vec<String>>()
+                            .join("\n- ")
                     )
-                } else {
-                    suggestion = format!("```suggestion\n{suggestion}```",);
-                }
+                    .as_str(),
+                )
+            } else {
+                suggestion = format!("```suggestion\n{suggestion}```",);
             }
             let comment = Suggestion {
                 line_start: start_line,
                 line_end: end_line,
-                suggestion: format!("{}\n{suggestion}", suggestion_help.unwrap_or_default()),
+                suggestion: format!("{suggestion_help}\n{suggestion}"),
                 path: file_name.clone(),
             };
             if !review_comments.is_comment_in_suggestions(&comment) {
@@ -463,4 +462,17 @@ mod tests {
             .to_string()
             .contains(TOOL_NAME)));
     }
+
+    #[test]
+    fn get_exe_by_invalid_path() {
+        let tool_exe = get_clang_tool_exe(TOOL_NAME, "non-existent-path");
+        assert!(tool_exe.is_err());
+    }
+
+    #[test]
+    fn get_exe_by_invalid_name() {
+        let clang_version = env::var("CLANG_VERSION").unwrap_or("16".to_string());
+        let tool_exe = get_clang_tool_exe("not-a-clang-tool", &clang_version);
+        assert!(tool_exe.is_err());
+    }
 }

cpp-linter-lib/src/cli/structs.rs

@@ -1,4 +1,4 @@
-use std::path::PathBuf;
+use std::{fmt::Display, path::PathBuf};
 
 use clap::ArgMatches;
 
@@ -27,6 +27,16 @@ impl LinesChangedOnly {
     }
 }
 
+impl Display for LinesChangedOnly {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            LinesChangedOnly::Off => write!(f, "false"),
+            LinesChangedOnly::Diff => write!(f, "diff"),
+            LinesChangedOnly::On => write!(f, "true"),
+        }
+    }
+}
+
 /// A structure to contain parsed CLI options.
 pub struct Cli {
     pub version: String,
@@ -134,6 +144,16 @@ impl ThreadComments {
     }
 }
 
+impl Display for ThreadComments {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            ThreadComments::On => write!(f, "true"),
+            ThreadComments::Off => write!(f, "false"),
+            ThreadComments::Update => write!(f, "update"),
+        }
+    }
+}
+
 /// A data structure to contain CLI options that relate to
 /// clang-tidy or clang-format arguments.
 #[derive(Debug, Clone, Default)]
@@ -227,7 +247,7 @@ impl Default for FeedbackInput {
 mod test {
     use crate::cli::get_arg_parser;
 
-    use super::Cli;
+    use super::{Cli, LinesChangedOnly, ThreadComments};
 
     #[test]
     fn parse_positional() {
@@ -239,4 +259,21 @@ mod test {
         assert!(not_ignored.contains(&String::from("file1.c")));
         assert!(not_ignored.contains(&String::from("file2.h")));
     }
+
+    #[test]
+    fn display_lines_changed_only_enum() {
+        let input = "diff".to_string();
+        assert_eq!(
+            LinesChangedOnly::from_string(&input),
+            LinesChangedOnly::Diff
+        );
+        assert_eq!(format!("{}", LinesChangedOnly::Diff), input);
+    }
+
+    #[test]
+    fn display_thread_comments_enum() {
+        let input = "false".to_string();
+        assert_eq!(ThreadComments::from_string(&input), ThreadComments::Off);
+        assert_eq!(format!("{}", ThreadComments::Off), input);
+    }
 }