add tests; review error handling

Author: 2bndy5

cpp-linter/src/clang_tools/clang_format.rs

@@ -160,17 +160,13 @@ pub fn run_clang_format(
         .output()
         .with_context(|| format!("Failed to get replacements from clang-format: {file_name}"))?;
     if !output.stderr.is_empty() || !output.status.success() {
-        if let Ok(stderr) = String::from_utf8(output.stderr) {
-            logs.push((
-                log::Level::Debug,
-                format!("clang-format raised the follow errors:\n{}", stderr),
-            ));
-        } else {
-            logs.push((
-                log::Level::Error,
-                "stderr from clang-format was not UTF-8 encoded".to_string(),
-            ));
-        }
+        logs.push((
+            log::Level::Debug,
+            format!(
+                "clang-format raised the follow errors:\n{}",
+                String::from_utf8_lossy(&output.stderr)
+            ),
+        ));
     }
     if output.stdout.is_empty() {
         return Ok(logs);

cpp-linter/src/clang_tools/clang_tidy.rs

@@ -304,21 +304,17 @@ pub fn run_clang_tidy(
         log::Level::Debug,
         format!(
             "Output from clang-tidy:\n{}",
-            String::from_utf8(output.stdout.to_vec()).unwrap()
+            String::from_utf8_lossy(&output.stdout)
         ),
     ));
     if !output.stderr.is_empty() {
-        if let Ok(stderr) = String::from_utf8(output.stderr) {
-            logs.push((
-                log::Level::Debug,
-                format!("clang-tidy made the following summary:\n{}", stderr),
-            ));
-        } else {
-            logs.push((
-                log::Level::Error,
-                "clang-tidy stderr is not UTF-8 encoded".to_string(),
-            ));
-        }
+        logs.push((
+            log::Level::Debug,
+            format!(
+                "clang-tidy made the following summary:\n{}",
+                String::from_utf8_lossy(&output.stderr)
+            ),
+        ));
     }
     file.tidy_advice = parse_tidy_output(&output.stdout, &clang_params.database_json)?;
     if clang_params.tidy_review {

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

@@ -292,31 +292,31 @@ mod test {
         default::Default,
         env,
         io::Read,
-        path::PathBuf,
+        path::{Path, PathBuf},
         sync::{Arc, Mutex},
     };
 
-    use chrono::Utc;
-    use mockito::{Matcher, Server};
     use regex::Regex;
-    use reqwest::{Method, Url};
     use tempfile::{tempdir, NamedTempFile};
 
     use super::GithubApiClient;
     use crate::{
         clang_tools::capture_clang_tools_output,
         cli::{ClangParams, FeedbackInput, LinesChangedOnly},
-        common_fs::FileObj,
+        common_fs::{FileFilter, FileObj},
+        logger,
         rest_api::{RestApiClient, USER_OUTREACH},
     };
 
     // ************************* tests for step-summary and output variables
 
-    async fn create_comment(tidy_checks: &str, style: &str) -> (String, String) {
+    async fn create_comment(tidy_checks: &str, style: &str, fail_gh_out: bool) -> (String, String) {
         let tmp_dir = tempdir().unwrap();
         let rest_api_client = GithubApiClient::new().unwrap();
+        logger::init().unwrap();
         if env::var("ACTIONS_STEP_DEBUG").is_ok_and(|var| var == "true") {
             assert!(rest_api_client.debug_enabled);
+            log::set_max_level(log::LevelFilter::Debug);
         }
         let mut files = vec![Arc::new(Mutex::new(FileObj::new(PathBuf::from(
             "tests/demo/demo.cpp",
@@ -342,7 +342,14 @@ mod test {
         let mut step_summary_path = NamedTempFile::new_in(tmp_dir.path()).unwrap();
         env::set_var("GITHUB_STEP_SUMMARY", step_summary_path.path());
         let mut gh_out_path = NamedTempFile::new_in(tmp_dir.path()).unwrap();
-        env::set_var("GITHUB_OUTPUT", gh_out_path.path());
+        env::set_var(
+            "GITHUB_OUTPUT",
+            if fail_gh_out {
+                Path::new("not-a-file.txt")
+            } else {
+                gh_out_path.path()
+            },
+        );
         rest_api_client
             .post_feedback(&files, feedback_inputs)
             .await
@@ -354,13 +361,15 @@ mod test {
         assert!(&step_summary_content.contains(USER_OUTREACH));
         let mut gh_out_content = String::new();
         gh_out_path.read_to_string(&mut gh_out_content).unwrap();
-        assert!(gh_out_content.starts_with("checks-failed="));
+        if !fail_gh_out {
+            assert!(gh_out_content.starts_with("checks-failed="));
+        }
         (step_summary_content, gh_out_content)
     }
 
     #[tokio::test]
     async fn check_comment_concerns() {
-        let (comment, gh_out) = create_comment("readability-*", "file").await;
+        let (comment, gh_out) = create_comment("readability-*", "file", false).await;
         assert!(&comment.contains(":warning:\nSome files did not pass the configured checks!\n"));
         let fmt_pattern = Regex::new(r"format-checks-failed=(\d+)\n").unwrap();
         let tidy_pattern = Regex::new(r"tidy-checks-failed=(\d+)\n").unwrap();
@@ -380,61 +389,31 @@ mod test {
     #[tokio::test]
     async fn check_comment_lgtm() {
         env::set_var("ACTIONS_STEP_DEBUG", "true");
-        let (comment, gh_out) = create_comment("-*", "").await;
+        let (comment, gh_out) = create_comment("-*", "", false).await;
         assert!(&comment.contains(":heavy_check_mark:\nNo problems need attention."));
         assert_eq!(
             &gh_out,
             "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::new().unwrap();
-        let reset_timestamp = (Utc::now().timestamp() + 60).to_string();
-        let mock = server
-            .mock("GET", "/")
-            .match_body(Matcher::Any)
-            .expect_at_least(1)
-            .expect_at_most(5)
-            .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)
-                .unwrap();
-        GithubApiClient::send_api_request(
-            client.client.clone(),
-            request,
-            client.rate_limit_headers.clone(),
-            0,
-        )
-        .await
-        .unwrap();
-    }
-
     #[tokio::test]
-    #[ignore]
-    #[should_panic(expected = "REST API secondary rate limit exceeded")]
-    async fn secondary_rate_limit() {
-        simulate_rate_limit(true).await;
+    async fn fail_gh_output() {
+        env::set_var("ACTIONS_STEP_DEBUG", "true");
+        let (comment, gh_out) = create_comment("-*", "", true).await;
+        assert!(&comment.contains(":heavy_check_mark:\nNo problems need attention."));
+        assert_eq!(&gh_out, "");
     }
 
     #[tokio::test]
-    #[ignore]
-    #[should_panic(expected = "REST API rate limit exceeded!")]
-    async fn primary_rate_limit() {
-        simulate_rate_limit(false).await;
+    async fn fail_get_local_diff() {
+        env::set_var("CI", "false");
+        let tmp_dir = tempdir().unwrap();
+        env::set_current_dir(tmp_dir.path()).unwrap();
+        let rest_client = GithubApiClient::new().unwrap();
+        let files = rest_client
+            .get_list_of_changed_files(&FileFilter::new(&[], vec![]))
+            .await;
+        assert!(files.is_err())
     }
 }

cpp-linter/src/rest_api/github/specific_api.rs

@@ -30,9 +30,9 @@ impl GithubApiClient {
         let pull_request = {
             match event_name.as_str() {
                 "pull_request" => {
-                    let event_payload_path = env::var("GITHUB_EVENT_PATH").with_context(|| {
-                        "GITHUB_EVENT_NAME is set to 'pull_request', but GITHUB_EVENT_PATH is not set"
-                    })?;
+                    // GITHUB_*** env vars cannot be overwritten in CI runners on GitHub.
+                    let event_payload_path = env::var("GITHUB_EVENT_PATH")?;
+                    // event payload JSON file can be overwritten/removed in CI runners
                     let file_buf = &mut String::new();
                     OpenOptions::new()
                         .read(true)
@@ -51,16 +51,15 @@ impl GithubApiClient {
                 _ => None,
             }
         };
+        // GITHUB_*** env vars cannot be overwritten in CI runners on GitHub.
         let gh_api_url = env::var("GITHUB_API_URL").unwrap_or("https://api.github.com".to_string());
-        let api_url = Url::parse(gh_api_url.clone().as_str())
-            .with_context(|| format!("Failed to parse URL from GITHUB_API_URL: {}", gh_api_url))?;
+        let api_url = Url::parse(gh_api_url.as_str())?;
 
         Ok(GithubApiClient {
             client: Client::builder()
                 .default_headers(Self::make_headers()?)
                 .user_agent(USER_AGENT)
-                .build()
-                .with_context(|| "Failed to create a session client for REST API calls")?,
+                .build()?,
             pull_request,
             event_name,
             api_url,
@@ -87,6 +86,7 @@ impl GithubApiClient {
     /// Append step summary to CI workflow's summary page.
     pub fn post_step_summary(&self, comment: &String) {
         if let Ok(gh_out) = env::var("GITHUB_STEP_SUMMARY") {
+            // step summary MD file can be overwritten/removed in CI runners
             if let Ok(mut gh_out_file) = OpenOptions::new().append(true).open(gh_out) {
                 if let Err(e) = writeln!(gh_out_file, "\n{}\n", comment) {
                     log::error!("Could not write to GITHUB_STEP_SUMMARY file: {}", e);