fix parsing of compilation database; add rate limit tests

Author: 2bndy5

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

@@ -18,19 +18,19 @@ use crate::{
     common_fs::{normalize_path, FileObj},
 };
 
-/// Used to deserialize a JSON compilation database
-#[derive(Deserialize, Debug, Clone)]
-pub struct CompilationDatabase {
-    /// A list of [`CompilationUnit`]
-    units: Vec<CompilationUnit>,
-}
+// /// Used to deserialize a JSON compilation database
+// #[derive(Deserialize, Debug, Clone)]
+// pub struct CompilationDatabase {
+//     /// A list of [`CompilationUnit`]
+//     units: Vec<CompilationUnit>,
+// }
 
 /// Used to deserialize a json compilation database's translation unit.
 ///
 /// The only purpose this serves is to normalize relative paths for build systems that
 /// use/need relative paths (ie ninja).
 #[derive(Deserialize, Debug, Clone)]
-struct CompilationUnit {
+pub struct CompilationUnit {
     /// The directory of the build environment
     directory: String,
 
@@ -75,15 +75,14 @@ pub struct TidyNotification {
 
 impl TidyNotification {
     pub fn diagnostic_link(&self) -> String {
-        let ret_val = if let Some((category, name)) = self.diagnostic.split_once('-') {
-            format!(
-                "[{}](https://clang.llvm.org/extra/clang-tidy/checks/{category}/{name}.html)",
-                self.diagnostic
-            )
-        } else {
-            self.diagnostic.clone()
-        };
-        ret_val
+        if self.diagnostic.starts_with("clang-diagnostic") {
+            return self.diagnostic.clone();
+        }
+        let (category, name) = self.diagnostic.split_once('-').unwrap();
+        format!(
+            "[{}](https://clang.llvm.org/extra/clang-tidy/checks/{category}/{name}.html)",
+            self.diagnostic
+        )
     }
 }
 
@@ -100,7 +99,7 @@ pub struct TidyAdvice {
 /// in the notifications.
 fn parse_tidy_output(
     tidy_stdout: &[u8],
-    database_json: &Option<CompilationDatabase>,
+    database_json: &Option<Vec<CompilationUnit>>,
 ) -> Option<TidyAdvice> {
     let note_header = Regex::new(r"^(.+):(\d+):(\d+):\s(\w+):(.*)\[([a-zA-Z\d\-\.]+)\]$").unwrap();
     let mut notification = None;
@@ -114,29 +113,29 @@ fn parse_tidy_output(
 
             // normalize the filename path and try to make it relative to the repo root
             let mut filename = PathBuf::from(&captured[1]);
-            if filename.is_relative() {
-                // if database was given try to use that first
-                if let Some(db_json) = &database_json {
-                    let mut found_unit = false;
-                    for unit in &db_json.units {
-                        if unit.file == captured[0] {
-                            filename =
-                                normalize_path(&PathBuf::from_iter([&unit.directory, &unit.file]));
-                            found_unit = true;
-                            break;
-                        }
+            // if database was given try to use that first
+            if let Some(db_json) = &database_json {
+                let mut found_unit = false;
+                for unit in db_json {
+                    let unit_path =
+                        PathBuf::from_iter([unit.directory.as_str(), unit.file.as_str()]);
+                    if unit_path == filename {
+                        filename =
+                            normalize_path(&PathBuf::from_iter([&unit.directory, &unit.file]));
+                        found_unit = true;
+                        break;
                     }
-                    if !found_unit {
-                        // file was not a named unit in the database;
-                        // try to normalize path as if relative to working directory.
-                        // NOTE: This shouldn't happen with a properly formed JSON database
-                        filename = normalize_path(&PathBuf::from_iter([&cur_dir, &filename]));
-                    }
-                } else {
-                    // still need to normalize the relative path despite missing database info.
-                    // let's assume the file is relative to current working directory.
+                }
+                if !found_unit {
+                    // file was not a named unit in the database;
+                    // try to normalize path as if relative to working directory.
+                    // NOTE: This shouldn't happen with a properly formed JSON database
                     filename = normalize_path(&PathBuf::from_iter([&cur_dir, &filename]));
                 }
+            } else {
+                // still need to normalize the relative path despite missing database info.
+                // let's assume the file is relative to current working directory.
+                filename = normalize_path(&PathBuf::from_iter([&cur_dir, &filename]));
             }
             assert!(filename.is_absolute());
             if filename.is_absolute() && filename.starts_with(&cur_dir) {
@@ -198,7 +197,7 @@ pub fn run_clang_tidy(
     lines_changed_only: &LinesChangedOnly,
     database: &Option<PathBuf>,
     extra_args: &Option<Vec<String>>,
-    database_json: &Option<CompilationDatabase>,
+    database_json: &Option<Vec<CompilationUnit>>,
 ) -> Vec<(log::Level, std::string::String)> {
     let mut logs = vec![];
     let mut file = file.lock().unwrap();

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

@@ -24,7 +24,7 @@ use crate::{
 pub mod clang_format;
 use clang_format::run_clang_format;
 pub mod clang_tidy;
-use clang_tidy::{run_clang_tidy, CompilationDatabase};
+use clang_tidy::{run_clang_tidy, CompilationUnit};
 
 /// Fetch the path to a clang tool by `name` (ie `"clang-tidy"` or `"clang-format"`) and
 /// `version`.
@@ -182,12 +182,12 @@ pub async fn capture_clang_tools_output(
 
     // parse database (if provided) to match filenames when parsing clang-tidy's stdout
     if let Some(db_path) = &clang_params.database {
-        if let Ok(db_str) = fs::read(db_path) {
+        if let Ok(db_str) = fs::read(db_path.join("compile_commands.json")) {
             clang_params.database_json = Some(
-                serde_json::from_str::<CompilationDatabase>(
+                serde_json::from_str::<Vec<CompilationUnit>>(
                     String::from_utf8(db_str).unwrap().as_str(),
                 )
-                .unwrap(),
+                .expect("Failed to parse compile_commands.json"),
             )
         }
     };

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

@@ -3,7 +3,7 @@ use std::path::PathBuf;
 use clap::ArgMatches;
 
 use super::convert_extra_arg_val;
-use crate::{clang_tools::clang_tidy::CompilationDatabase, common_fs::FileFilter};
+use crate::{clang_tools::clang_tidy::CompilationUnit, common_fs::FileFilter};
 
 /// An enum to describe `--lines-changed-only` CLI option's behavior.
 #[derive(PartialEq, Clone, Debug)]
@@ -139,7 +139,7 @@ pub struct ClangParams {
     pub lines_changed_only: LinesChangedOnly,
     pub database: Option<PathBuf>,
     pub extra_args: Option<Vec<String>>,
-    pub database_json: Option<CompilationDatabase>,
+    pub database_json: Option<Vec<CompilationUnit>>,
     pub style: String,
     pub clang_tidy_command: Option<PathBuf>,
     pub clang_format_command: Option<PathBuf>,

cpp-linter-lib/src/rest_api/github_api.rs

@@ -361,7 +361,6 @@ impl GithubApiClient {
         #[allow(clippy::nonminimal_bool)] // an inaccurate assessment
         if (is_lgtm && !no_lgtm) || !is_lgtm {
             let payload = HashMap::from([("body", comment.to_owned())]);
-            #[cfg(not(test))]
             log::debug!("payload body:\n{:?}", payload);
             let req_meth = if comment_url.is_some() {
                 Method::PATCH
@@ -529,7 +528,10 @@ mod test {
         sync::{Arc, Mutex},
     };
 
+    use chrono::Utc;
+    use mockito::{Matcher, Server};
     use regex::Regex;
+    use reqwest::{Method, Url};
     use tempfile::{tempdir, NamedTempFile};
 
     use super::GithubApiClient;
@@ -619,4 +621,49 @@ mod test {
             "checks-failed=0\nformat-checks-failed=0\ntidy-checks-failed=0\n"
         );
     }
+
+    async fn simulate_rate_limit(secondary: bool) {
+        let mut server = Server::new_async().await;
+        let url = Url::parse(server.url().as_str()).unwrap();
+        env::set_var("GITHUB_API_URL", server.url());
+        let client = GithubApiClient::default();
+        let reset_timestamp = (Utc::now().timestamp() + 60).to_string();
+        let mock = server
+            .mock("GET", "/")
+            .match_query(Matcher::Any)
+            .with_status(429)
+            .with_header(
+                &client.rate_limit_headers.remaining,
+                if secondary { "1" } else { "0" },
+            )
+            .with_header(&client.rate_limit_headers.reset, &reset_timestamp);
+        if secondary {
+            mock.with_header(&client.rate_limit_headers.retry, "0")
+                .create();
+        } else {
+            mock.create();
+        }
+        let request =
+            GithubApiClient::make_api_request(&client.client, url, Method::GET, None, None);
+        let _response = GithubApiClient::send_api_request(
+            client.client.clone(),
+            request,
+            true,
+            client.rate_limit_headers.clone(),
+            0,
+        )
+        .await;
+    }
+
+    #[tokio::test]
+    #[should_panic(expected = "REST API secondary rate limit exceeded")]
+    async fn secondary_rate_limit() {
+        simulate_rate_limit(true).await;
+    }
+
+    #[tokio::test]
+    #[should_panic(expected = "REST API rate limit exceeded!")]
+    async fn primary_rate_limit() {
+        simulate_rate_limit(false).await;
+    }
 }

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

@@ -131,24 +131,24 @@ pub trait RestApiClient {
                             .expect("Failed to extract remaining attempts about rate limit")
                             .parse::<i64>()
                             .expect("Failed to parse i64 from remaining attempts about rate limit");
-                        let reset = DateTime::from_timestamp(
-                            response
-                                .headers()
-                                .get(&rate_limit_headers.reset)
-                                .expect("response headers does not include a reset timestamp")
-                                .to_str()
-                                .expect("Failed to extract reset info about rate limit")
-                                .parse::<i64>()
-                                .expect("Failed to parse i64 from reset time about rate limit"),
-                            0,
-                        )
-                        .expect("rate limit reset UTC timestamp is an invalid");
                         if remaining <= 0 {
+                            let reset = DateTime::from_timestamp(
+                                response
+                                    .headers()
+                                    .get(&rate_limit_headers.reset)
+                                    .expect("response headers does not include a reset timestamp")
+                                    .to_str()
+                                    .expect("Failed to extract reset info about rate limit")
+                                    .parse::<i64>()
+                                    .expect("Failed to parse i64 from reset time about rate limit"),
+                                0,
+                            )
+                            .expect("rate limit reset UTC timestamp is an invalid");
                             panic!("REST API rate limit exceeded! Resets at {}", reset);
                         }
 
                         // check if secondary rate limit is violated; backoff and try again.
-                        if retries >= 5 {
+                        if retries > 4 {
                             panic!("REST API secondary rate limit exceeded");
                         }
                         if let Some(retry) = response.headers().get(&rate_limit_headers.retry) {

cpp-linter-lib/tests/comments.rs

@@ -1,7 +1,14 @@
 use chrono::Utc;
 use cpp_linter_lib::run::run_main;
 use mockito::{Matcher, Server, ServerGuard};
-use std::{env, fmt::Display, fs, io::Write, path::Path, process::Command};
+use std::{
+    env,
+    fmt::Display,
+    fs,
+    io::{self, BufRead, Write},
+    path::Path,
+    process::Command,
+};
 use tempfile::{NamedTempFile, TempDir};
 
 const SHA: &str = "8d68756375e0483c7ac2b4d6bbbece420dbbb495";
@@ -155,6 +162,8 @@ async fn setup(
         "--ignore-format=src/some source.c".to_string(),
         format!("--thread-comments={thread_comments}"),
         format!("--no-lgtm={no_lgtm}"),
+        "-p=build".to_string(),
+        "-i=build".to_string(),
     ];
     if is_lgtm {
         args.push("-e=c".to_string());
@@ -176,9 +185,17 @@ fn create_test_space() -> TempDir {
     }
 
     // generate compilation database with meson (& ninja)
-    let test_dir = tmp.path().to_str().unwrap();
+    let test_dir = tmp.path().join("src");
     let mut cmd = Command::new("meson");
-    cmd.args(["init", "-C", test_dir]);
+    cmd.args([
+        "init",
+        "-C",
+        test_dir.to_str().unwrap(),
+        "--name",
+        "demo",
+        "demo.cpp",
+        "demo.hpp",
+    ]);
     let output = cmd.output().expect("Failed to run 'meson init'");
     println!(
         "meson init stdout:\n{}",
@@ -190,7 +207,7 @@ fn create_test_space() -> TempDir {
         "setup",
         "--backend=ninja",
         meson_build_dir.as_path().to_str().unwrap(),
-        test_dir,
+        test_dir.to_str().unwrap(),
     ]);
     let output = cmd
         .output()
@@ -199,6 +216,11 @@ fn create_test_space() -> TempDir {
         "meson setup stdout:\n{}",
         String::from_utf8(output.stdout.to_vec()).unwrap()
     );
+    let db = fs::File::open(meson_build_dir.join("compile_commands.json"))
+        .expect("Failed to open compilation database");
+    for line in io::BufReader::new(db).lines().map_while(Result::ok) {
+        println!("{line}");
+    }
     tmp
 }
 

justfile

@@ -20,21 +20,20 @@ py-dev:
 # run the test suite
 [group("code coverage")]
 test:
-    cargo llvm-cov --no-report --tests \
+    cargo llvm-cov --no-report \
     nextest --manifest-path cpp-linter-lib/Cargo.toml \
-    #--success-output=final \
     --lib --color always
 
 # generate and open pretty coverage report
 [group("code coverage")]
 pretty-cov *args='':
-    cargo llvm-cov report --json --output-path coverage.json
+    cargo llvm-cov report --json --output-path coverage.json --ignore-filename-regex main
     llvm-cov-pretty coverage.json {{ args }}
 
 # generate and open detailed coverage report
 [group("code coverage")]
 llvm-cov *args='':
-    cargo llvm-cov report --html {{ args }}
+    cargo llvm-cov report --html --ignore-filename-regex main {{ args }}
 
 # This is useful for IDE gutter indicators of line coverage.
 # See Coverage Gutters ext in VSCode.