fix: transform cpp-linter API into git-bot-feedback API

add a test that reuses a previous PR review comment.

This involves a new asset. I checked the expected result as a full PR review JSON payload.

all this just to cover 8 lines; glad I did it because it fixes a bug when transforming a cpp-linter suggestion into a review comment (per the differences in API between cpp-linter and git-bot-feedback).

EDIT: switch to diagnostic commonly present despite different versions of clang.

Author: 2bndy5

cpp-linter/src/clang_tools/mod.rs

@@ -197,7 +197,11 @@ pub struct Suggestion {
 impl Suggestion {
     pub(crate) fn as_review_comment(&self) -> ReviewComment {
         ReviewComment {
-            line_start: Some(self.line_start),
+            line_start: if self.line_start == self.line_end {
+                None
+            } else {
+                Some(self.line_start)
+            },
             line_end: self.line_end,
             comment: self.suggestion.clone(),
             path: self.path.clone(),

cpp-linter/tests/reviews.rs

@@ -21,6 +21,8 @@ const MOCK_ASSETS_PATH: &str = "tests/reviews_test_assets/";
 const RESET_RATE_LIMIT_HEADER: &str = "x-ratelimit-reset";
 const REMAINING_RATE_LIMIT_HEADER: &str = "x-ratelimit-remaining";
 
+const QUERY_REVIEW_THREADS: &str = r#"(?s).*"query":"query.*reviewThreads.*"#;
+
 struct TestParams {
     pub lines_changed_only: LinesChangedOnly,
     pub tidy_review: bool,
@@ -36,6 +38,7 @@ struct TestParams {
     pub fail_posting: bool,
     pub bad_pr_info: bool,
     pub bad_existing_reviews: bool,
+    pub reused_review: bool,
 }
 
 impl Default for TestParams {
@@ -55,6 +58,7 @@ impl Default for TestParams {
             fail_posting: false,
             bad_pr_info: false,
             bad_existing_reviews: false,
+            reused_review: false,
         }
     }
 }
@@ -146,19 +150,31 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) {
         && test_params.pr_state != "closed"
         && !test_params.bad_pr_info
     {
-        mocks.push(
-            server
-                .mock(
-                    "PUT",
-                    format!("{reviews_endpoint}/1807607546/dismissals").as_str(),
-                )
-                .match_body(r#"{"event":"DISMISS","message":"outdated review"}"#)
-                .match_header("Authorization", format!("token {TOKEN}").as_str())
-                .with_header(REMAINING_RATE_LIMIT_HEADER, "50")
-                .with_header(RESET_RATE_LIMIT_HEADER, reset_timestamp.as_str())
-                .with_status(if test_params.fail_dismissal { 403 } else { 200 })
-                .create(),
-        );
+        if test_params.reused_review {
+            // simulate reuse of existing review
+            mocks.push(
+                server
+                    .mock("POST", "/graphql")
+                    .match_body(Matcher::Regex(QUERY_REVIEW_THREADS.to_string()))
+                    .with_body_from_file(format!("{asset_path}existing_review_threads.json"))
+                    .create(),
+            );
+        } else {
+            // simulate dismissal because not reusing existing review
+            mocks.push(
+                server
+                    .mock(
+                        "PUT",
+                        format!("{reviews_endpoint}/1807607546/dismissals").as_str(),
+                    )
+                    .match_body(r#"{"event":"DISMISS","message":"outdated review"}"#)
+                    .match_header("Authorization", format!("token {TOKEN}").as_str())
+                    .with_header(REMAINING_RATE_LIMIT_HEADER, "50")
+                    .with_header(RESET_RATE_LIMIT_HEADER, reset_timestamp.as_str())
+                    .with_status(if test_params.fail_dismissal { 403 } else { 200 })
+                    .create(),
+            );
+        }
     }
 
     let lgtm_allowed = !test_params.force_lgtm || !test_params.no_lgtm;
@@ -175,7 +191,9 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) {
             "REQUEST_CHANGES"
         };
         let tool_summary = {
-            if !(test_params.format_review || test_params.tidy_review)
+            if test_params.reused_review {
+                "suggestions were duplicates of previous reviews"
+            } else if !(test_params.format_review || test_params.tidy_review)
                 || test_params.lines_changed_only == LinesChangedOnly::On
             {
                 ""
@@ -303,6 +321,15 @@ async fn changed_lines() {
     test_review(&TestParams::default()).await;
 }
 
+#[tokio::test]
+async fn changed_lines_reused_review() {
+    test_review(&TestParams {
+        reused_review: true,
+        ..Default::default()
+    })
+    .await;
+}
+
 #[tokio::test]
 async fn all_lines_passive() {
     test_review(&TestParams {

cpp-linter/tests/reviews_test_assets/existing_review_threads.json

@@ -0,0 +1,42 @@
+{
+  "data": {
+    "repository": {
+      "pullRequest": {
+        "reviewThreads": {
+          "nodes": [
+            {
+              "id": "do-not-hide-this-thread",
+              "isResolved": false,
+              "isCollapsed": false,
+              "comments": {
+                "nodes": [
+                  {
+                    "id": "reuse-this-review-comment",
+                    "body": "<!-- cpp linter action -->\n### clang-tidy suggestion\n\n```suggestion\n    Dummy() : numb(0), useless(\"\\0\") {}\n```\n",
+                    "path": "src/demo.hpp",
+                    "line": 7,
+                    "startLine": null,
+                    "originalLine": 7,
+                    "originalStartLine": null,
+                    "pullRequestReview": {
+                      "id": "PRR_kwDOFY2uzM5rveb6",
+                      "isMinimized": false
+                    }
+                  }
+                ],
+                "pageInfo": {
+                  "endCursor": "Y3Vyc29yOnYyOpK0MjAyNC0xMi0yMFQxODoyNTo0OVrOcOgYZA==",
+                  "hasNextPage": false
+                }
+              }
+            }
+          ],
+          "pageInfo": {
+            "endCursor": "pg2",
+            "hasNextPage": false
+          }
+        }
+      }
+    }
+  }
+}

cpp-linter/tests/reviews_test_assets/full_review_result-v16.json

@@ -0,0 +1,43 @@
+{
+    "event": "REQUEST_CHANGES",
+    "body": "<!-- cpp linter action -->\n## Cpp-linter Review\n### Used clang-format v16.0.0\n### Used clang-tidy v16.0.0\n\nOnly 8 out of 9 concerns fit within this pull request's diff.\n\n1 suggestions were duplicates of previous reviews.\n\n<details><summary>Click here for a patch of fixes outside the diff</summary><p>\n\n```diff\n--- a/src/demo.hpp\n+++ b/src/demo.hpp\n-\n-\n-\n-\n-\n-\n-\n-\n-\n-\n-\n-\n-\n-\n-\n-\n-\n```\n\n</p></details>\n\n\nHave any feedback or feature suggestions? [Share it here.](https://github.com/cpp-linter/cpp-linter-action/issues)",
+    "comments": [
+        {
+            "body": "<!-- cpp linter action -->\n### clang-tidy suggestion\n\n```suggestion\n  for (;;)\n    break;\n```\n",
+            "line": 6,
+            "path": "src/demo.cpp"
+        },
+        {
+            "body": "<!-- cpp linter action -->\n### clang-tidy suggestion\n\n```suggestion\n  printf(\"Hello world!\\n\");\n```\n",
+            "line": 9,
+            "path": "src/demo.cpp"
+        },
+        {
+            "body": "<!-- cpp linter action -->\n### clang-tidy suggestion\n\n```suggestion\n  return 0;\n}\n```\n",
+            "line": 17,
+            "start_line": 7,
+            "path": "src/demo.cpp"
+        },
+        {
+            "body": "<!-- cpp linter action -->\n### clang-tidy suggestion\n\n```suggestion\n    Dummy() : useless(\"\\0\"), numb(0) {}\n```\n",
+            "line": 7,
+            "path": "src/demo.hpp"
+        },
+        {
+            "body": "<!-- cpp linter action -->\n### clang-tidy suggestion\n\n```suggestion\n  public:\n    auto not_useful(char *str) -> void * { useless = str; }\n```\n",
+            "line": 10,
+            "start_line": 9,
+            "path": "src/demo.hpp"
+        },
+        {
+            "body": "<!-- cpp linter action -->\n### clang-tidy diagnostic\n**src/demo.hpp:8:31** error: [clang-diagnostic-writable-strings]\n\n> ISO C++11 does not allow conversion from string literal to 'char *'\n\n```hpp\n    Dummy() :numb(0), useless(\"\\0\"){}\n                              ^\n```\n",
+            "line": 8,
+            "path": "src/demo.hpp"
+        },
+        {
+            "body": "<!-- cpp linter action -->\n### clang-tidy diagnostic\n**src/demo.hpp:11:47** error: [clang-diagnostic-return-type]\n\n> non-void function does not return a value\n\n```hpp\n    void *not_useful(char *str){useless = str;}\n                                              ^\n```\n",
+            "line": 11,
+            "path": "src/demo.hpp"
+        }
+    ]
+}

cpp-linter/tests/reviews_test_assets/full_review_result-v18.json

@@ -0,0 +1,43 @@
+{
+    "event": "REQUEST_CHANGES",
+    "body": "<!-- cpp linter action -->\n## Cpp-linter Review\n### Used clang-format v18.1.8\n### Used clang-tidy v18.1.8\n\n\nHave any feedback or feature suggestions? [Share it here.](https://github.com/cpp-linter/cpp-linter-action/issues)",
+    "comments": [
+        {
+            "body": "<!-- cpp linter action -->\n### clang-tidy suggestion\n\n```suggestion\n    Dummy() : numb(0), useless(\"\\0\") {}\n```\n",
+            "line": 7,
+            "path": "src/demo.hpp"
+        },
+        {
+            "body": "<!-- cpp linter action -->\n### clang-tidy suggestion\n\n```suggestion\n  public:\n    auto not_useful(char *str) -> void * { useless = str; }\n```\n",
+            "line": 10,
+            "start_line": 9,
+            "path": "src/demo.hpp"
+        },
+        {
+            "body": "<!-- cpp linter action -->\n### clang-tidy suggestion\n\n```suggestion\n#include <cstdio>\n```\n",
+            "line": 2,
+            "path": "src/demo.cpp"
+        },
+        {
+            "body": "<!-- cpp linter action -->\n### clang-tidy suggestion\n\n```suggestion\nauto main() -> int {\n```\n",
+            "line": 4,
+            "path": "src/demo.cpp"
+        },
+        {
+            "body": "<!-- cpp linter action -->\n### clang-tidy suggestion\n\n```suggestion\n  for (;;) {\n    break;\n  }\n```\n",
+            "line": 6,
+            "path": "src/demo.cpp"
+        },
+        {
+            "body": "<!-- cpp linter action -->\n### clang-tidy diagnostic(s)\n- statement should be inside braces [[readability-braces-around-statements](https://clang.llvm.org/extra/clang-tidy/checks/readability/braces-around-statements.html)]\n\n```suggestion\n  printf(\"Hello world!\\n\");\n```\n",
+            "line": 10,
+            "path": "src/demo.cpp"
+        },
+        {
+            "body": "<!-- cpp linter action -->\n### clang-tidy diagnostic(s)\n- use a trailing return type for this function [[modernize-use-trailing-return-type](https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-trailing-return-type.html)]\n- statement should be inside braces [[readability-braces-around-statements](https://clang.llvm.org/extra/clang-tidy/checks/readability/braces-around-statements.html)]\n\n```suggestion\n  return 0;\n}\n```\n",
+            "line": 17,
+            "start_line": 7,
+            "path": "src/demo.cpp"
+        }
+    ]
+}