diff --git a/.github/workflows/run-dev-tests.yml b/.github/workflows/run-dev-tests.yml index 2b59a67..688a381 100644 --- a/.github/workflows/run-dev-tests.yml +++ b/.github/workflows/run-dev-tests.yml @@ -80,25 +80,25 @@ jobs: run: sudo apt-get update - name: Install clang v7 - if: matrix.os == 'Linux' + if: runner.os == 'Linux' uses: cpp-linter/cpp_linter_rs/install-clang-action@main with: version: '7' - name: Collect Coverage for clang v7 - if: matrix.os == 'Linux' + if: runner.os == 'Linux' env: CLANG_VERSION: '7' run: just test - name: Install clang v8 - if: matrix.os == 'Linux' + if: runner.os == 'Linux' uses: cpp-linter/cpp_linter_rs/install-clang-action@main with: version: '8' - name: Collect Coverage for clang v8 - if: matrix.os == 'Linux' + if: runner.os == 'Linux' env: CLANG_VERSION: '8' run: just test diff --git a/cpp-linter-lib/src/rest_api/github_api.rs b/cpp-linter-lib/src/rest_api/github_api.rs index 57dc073..bd99209 100644 --- a/cpp-linter-lib/src/rest_api/github_api.rs +++ b/cpp-linter-lib/src/rest_api/github_api.rs @@ -192,20 +192,40 @@ impl RestApiClient for GithubApiClient { .unwrap(); let mut diff_header = HeaderMap::new(); diff_header.insert("Accept", "application/vnd.github.diff".parse().unwrap()); - let request = - Self::make_api_request(&self.client, url, Method::GET, None, Some(diff_header)); + let request = Self::make_api_request( + &self.client, + url.as_str(), + Method::GET, + None, + Some(diff_header), + ); let response = Self::send_api_request( self.client.clone(), request, - true, + false, self.rate_limit_headers.to_owned(), 0, ) - .await - .unwrap() - .text; - - parse_diff_from_buf(response.as_bytes(), file_filter) + .await; + match response { + Some(response) => { + if response.status.is_success() { + return parse_diff_from_buf(response.text.as_bytes(), file_filter); + } else { + let endpoint = if is_pr { + Url::parse(format!("{}/files", url.as_str()).as_str()) + .expect("failed to parse URL endpoint") + } else { + url + }; + self.get_changed_files_paginated(endpoint, file_filter) + .await + } + } + None => { + panic!("Failed to connect with GitHub server to get list of changed files.") + } + } } else { // get diff from libgit2 API let repo = open_repo(".") @@ -215,6 +235,54 @@ impl RestApiClient for GithubApiClient { } } + async fn get_changed_files_paginated( + &self, + url: Url, + file_filter: &FileFilter, + ) -> Vec { + let mut url = Some(Url::parse_with_params(url.as_str(), &[("page", "1")]).unwrap()); + let mut files = vec![]; + while let Some(ref endpoint) = url { + let request = + Self::make_api_request(&self.client, endpoint.as_str(), Method::GET, None, None); + let response = Self::send_api_request( + self.client.clone(), + request, + true, + self.rate_limit_headers.clone(), + 0, + ) + .await; + if let Some(response) = response { + url = Self::try_next_page(&response.headers); + let files_list = if self.event_name != "pull_request" { + let json_value: PushEventFiles = serde_json::from_str(&response.text) + .expect("Failed to deserialize list of changed files from json response"); + json_value.files + } else { + serde_json::from_str::>(&response.text).expect( + "Failed to deserialize list of file changes from Pull Request event.", + ) + }; + for file in files_list { + if let Some(patch) = file.patch { + let diff = format!( + "diff --git a/{old} b/{new}\n--- a/{old}\n+++ b/{new}\n{patch}", + old = file.previous_filename.unwrap_or(file.filename.clone()), + new = file.filename, + ); + if let Some(file_obj) = + parse_diff_from_buf(diff.as_bytes(), file_filter).first() + { + files.push(file_obj.to_owned()); + } + } + } + } + } + files + } + async fn post_feedback( &self, files: &[Arc>], @@ -671,6 +739,24 @@ impl From for ReviewDiffComment { } } +/// A structure for deserializing a single changed file in a CI event. +#[derive(Debug, Deserialize, PartialEq, Clone)] +struct GithubChangedFile { + /// The file's name (including relative path to repo root) + pub filename: String, + /// If renamed, this will be the file's old name as a [`Some`], otherwise [`None`]. + pub previous_filename: Option, + /// The individual patch that describes the file's changes. + pub patch: Option, +} + +/// A structure for deserializing a Push event's changed files. +#[derive(Debug, Deserialize, PartialEq, Clone)] +struct PushEventFiles { + /// The list of changed files. + pub files: Vec, +} + /// A structure for deserializing a comment from a response's json. #[derive(Debug, Deserialize, PartialEq, Clone)] struct PullRequestInfo { @@ -840,7 +926,7 @@ mod test { } let request = GithubApiClient::make_api_request(&client.client, url, Method::GET, None, None); - let _response = GithubApiClient::send_api_request( + GithubApiClient::send_api_request( client.client.clone(), request, true, diff --git a/cpp-linter-lib/src/rest_api/mod.rs b/cpp-linter-lib/src/rest_api/mod.rs index cd56cce..505bd5a 100644 --- a/cpp-linter-lib/src/rest_api/mod.rs +++ b/cpp-linter-lib/src/rest_api/mod.rs @@ -233,6 +233,16 @@ pub trait RestApiClient { file_filter: &FileFilter, ) -> impl Future>; + /// A way to get the list of changed files using REST API calls that employ a paginated response. + /// + /// This is a helper to [`RestApiClient::get_list_of_changed_files()`] but takes a formulated URL + /// endpoint based on the context of the triggering CI event. + fn get_changed_files_paginated( + &self, + url: Url, + file_filter: &FileFilter, + ) -> impl Future>; + /// Makes a comment in MarkDown syntax based on the concerns in `format_advice` and /// `tidy_advice` about the given set of `files`. /// diff --git a/cpp-linter-lib/tests/comments.rs b/cpp-linter-lib/tests/comments.rs index 46f8d7f..9970e7a 100644 --- a/cpp-linter-lib/tests/comments.rs +++ b/cpp-linter-lib/tests/comments.rs @@ -230,7 +230,7 @@ async fn setup(lib_root: &Path, test_params: &TestParams) { } async fn test_comment(test_params: &TestParams) { - let tmp_dir = create_test_space(); + let tmp_dir = create_test_space(true); let lib_root = env::current_dir().unwrap(); env::set_current_dir(tmp_dir.path()).unwrap(); setup(&lib_root, test_params).await; diff --git a/cpp-linter-lib/tests/common/mod.rs b/cpp-linter-lib/tests/common/mod.rs index da0e819..ae9ed3b 100644 --- a/cpp-linter-lib/tests/common/mod.rs +++ b/cpp-linter-lib/tests/common/mod.rs @@ -20,7 +20,7 @@ use tempfile::TempDir; /// /// The returned directory object will automatically delete the /// temporary folder when it is dropped out of scope. -pub fn create_test_space() -> TempDir { +pub fn create_test_space(setup_meson: bool) -> TempDir { let tmp = TempDir::new().unwrap(); fs::create_dir(tmp.path().join("src")).unwrap(); let src = fs::read_dir("tests/demo").unwrap(); @@ -32,6 +32,10 @@ pub fn create_test_space() -> TempDir { } } + if !setup_meson { + return tmp; + } + // generate compilation database with meson (& ninja) let test_dir = tmp.path().join("src"); let mut cmd = Command::new("meson"); diff --git a/cpp-linter-lib/tests/paginated_changed_files.rs b/cpp-linter-lib/tests/paginated_changed_files.rs new file mode 100644 index 0000000..f39bb36 --- /dev/null +++ b/cpp-linter-lib/tests/paginated_changed_files.rs @@ -0,0 +1,145 @@ +mod common; +use chrono::Utc; +use common::{create_test_space, mock_server}; +use mockito::Matcher; +use tempfile::{NamedTempFile, TempDir}; + +use cpp_linter_lib::{ + common_fs::FileFilter, + rest_api::{github_api::GithubApiClient, RestApiClient}, +}; +use std::{env, io::Write, path::Path}; + +#[derive(PartialEq)] +enum EventType { + Push, + PullRequest(u64), +} + +const REPO: &str = "cpp-linter/test-cpp-linter-action"; +const SHA: &str = "DEADBEEF"; +const TOKEN: &str = "123456"; +const RESET_RATE_LIMIT_HEADER: &str = "x-ratelimit-reset"; +const REMAINING_RATE_LIMIT_HEADER: &str = "x-ratelimit-remaining"; + +async fn get_paginated_changes(lib_root: &Path, event_type: EventType) { + env::set_var("GITHUB_REPOSITORY", REPO); + env::set_var("GITHUB_SHA", SHA); + env::set_var("GITHUB_TOKEN", TOKEN); + env::set_var("CI", "true"); + env::set_var( + "GITHUB_EVENT_NAME", + if event_type == EventType::Push { + "push" + } else { + "pull_request" + }, + ); + let tmp = TempDir::new().expect("Failed to create a temp dir for test"); + let mut event_payload = NamedTempFile::new_in(tmp.path()) + .expect("Failed to spawn a tmp file for test event payload"); + env::set_var("GITHUB_EVENT_PATH", event_payload.path()); + if let EventType::PullRequest(pr_number) = event_type { + event_payload + .write_all( + serde_json::json!({"number": pr_number}) + .to_string() + .as_bytes(), + ) + .expect("Failed to write data to test event payload file") + } + + let reset_timestamp = (Utc::now().timestamp() + 60).to_string(); + let asset_path = format!("{}/tests/paginated_changes", lib_root.to_str().unwrap()); + + let mut server = mock_server().await; + env::set_var("GITHUB_API_URL", server.url()); + env::set_current_dir(tmp.path()).unwrap(); + let gh_client = GithubApiClient::new(); + + let mut mocks = vec![]; + let diff_end_point = format!( + "/repos/{REPO}/{}", + if let EventType::PullRequest(pr) = event_type { + format!("pulls/{pr}") + } else { + format!("commits/{SHA}") + } + ); + mocks.push( + server + .mock("GET", diff_end_point.as_str()) + .match_header("Accept", "application/vnd.github.diff") + .match_header("Authorization", TOKEN) + .with_header(REMAINING_RATE_LIMIT_HEADER, "50") + .with_header(RESET_RATE_LIMIT_HEADER, reset_timestamp.as_str()) + .with_status(403) + .create(), + ); + let pg_end_point = if event_type == EventType::Push { + diff_end_point.clone() + } else { + format!("{diff_end_point}/files") + }; + for pg in 1..=2 { + let link = if pg == 1 { + format!("<{}{pg_end_point}?page=2>; rel=\"next\"", server.url()) + } else { + "".to_string() + }; + mocks.push( + server + .mock("GET", pg_end_point.as_str()) + .match_header("Accept", "application/vnd.github.raw+json") + .match_header("Authorization", TOKEN) + .match_query(Matcher::UrlEncoded("page".to_string(), pg.to_string())) + .with_header(REMAINING_RATE_LIMIT_HEADER, "50") + .with_header(RESET_RATE_LIMIT_HEADER, reset_timestamp.as_str()) + .with_body_from_file(format!( + "{asset_path}/{}_files_pg{pg}.json", + if event_type == EventType::Push { + "push" + } else { + "pull_request" + } + )) + .with_header("link", link.as_str()) + .create(), + ); + } + + let file_filter = FileFilter::new(&[], vec!["cpp".to_string(), "hpp".to_string()]); + let files = gh_client.get_list_of_changed_files(&file_filter).await; + assert_eq!(files.len(), 2); + for file in files { + assert!(["src/demo.cpp", "src/demo.hpp"].contains( + &file + .name + .as_path() + .to_str() + .expect("Failed to get file name from path") + )); + } + for mock in mocks { + mock.assert(); + } +} + +async fn test_get_changes(event_type: EventType) { + let tmp_dir = create_test_space(false); + let lib_root = env::current_dir().unwrap(); + env::set_current_dir(tmp_dir.path()).unwrap(); + get_paginated_changes(&lib_root, event_type).await; + env::set_current_dir(lib_root.as_path()).unwrap(); + drop(tmp_dir); +} + +#[tokio::test] +async fn get_push_files_paginated() { + test_get_changes(EventType::Push).await +} + +#[tokio::test] +async fn get_pr_files_paginated() { + test_get_changes(EventType::PullRequest(42)).await +} diff --git a/cpp-linter-lib/tests/paginated_changes/pull_request_files_pg1.json b/cpp-linter-lib/tests/paginated_changes/pull_request_files_pg1.json new file mode 100644 index 0000000..5a70fd9 --- /dev/null +++ b/cpp-linter-lib/tests/paginated_changes/pull_request_files_pg1.json @@ -0,0 +1,27 @@ +[ + { + "sha": "52501fa1dc96d6bc6f8a155816df041b1de975d9", + "filename": ".github/workflows/cpp-lint-package.yml", + "status": "modified", + "additions": 9, + "deletions": 5, + "changes": 14, + "blob_url": "https://github.com/cpp-linter/test-cpp-linter-action/blob/635a9c57bdcca07b99ddef52c2640337c50280b1/.github%2Fworkflows%2Fcpp-lint-package.yml", + "raw_url": "https://github.com/cpp-linter/test-cpp-linter-action/raw/635a9c57bdcca07b99ddef52c2640337c50280b1/.github%2Fworkflows%2Fcpp-lint-package.yml", + "contents_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/contents/.github%2Fworkflows%2Fcpp-lint-package.yml?ref=635a9c57bdcca07b99ddef52c2640337c50280b1", + "patch": "@@ -7,16 +7,17 @@ on:\n description: 'which branch to test'\n default: 'main'\n required: true\n+ pull_request:\n \n jobs:\n cpp-linter:\n runs-on: windows-latest\n \n strategy:\n matrix:\n- clang-version: ['7', '8', '9','10', '11', '12', '13', '14', '15', '16', '17']\n+ clang-version: ['10', '11', '12', '13', '14', '15', '16', '17']\n repo: ['cpp-linter/cpp-linter']\n- branch: ['${{ inputs.branch }}']\n+ branch: ['pr-review-suggestions']\n fail-fast: false\n \n steps:\n@@ -62,10 +63,13 @@ jobs:\n -i=build \n -p=build \n -V=${{ runner.temp }}/llvm \n- -f=false \n --extra-arg=\"-std=c++14 -Wall\" \n- --thread-comments=${{ matrix.clang-version == '12' }} \n- -a=${{ matrix.clang-version == '12' }}\n+ --file-annotations=false\n+ --lines-changed-only=false\n+ --extension=h,c\n+ --thread-comments=${{ matrix.clang-version == '16' }} \n+ --tidy-review=${{ matrix.clang-version == '16' }}\n+ --format-review=${{ matrix.clang-version == '16' }}\n \n - name: Fail fast?!\n if: steps.linter.outputs.checks-failed > 0" + }, + { + "sha": "1bf553e06e4b7c6c9a9be5da4845acbdeb04f6a5", + "filename": "src/demo.cpp", + "previous_filename": "src/demo.c", + "status": "modified", + "additions": 11, + "deletions": 10, + "changes": 21, + "blob_url": "https://github.com/cpp-linter/test-cpp-linter-action/blob/635a9c57bdcca07b99ddef52c2640337c50280b1/src%2Fdemo.cpp", + "raw_url": "https://github.com/cpp-linter/test-cpp-linter-action/raw/635a9c57bdcca07b99ddef52c2640337c50280b1/src%2Fdemo.cpp", + "contents_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/contents/src%2Fdemo.cpp?ref=635a9c57bdcca07b99ddef52c2640337c50280b1", + "patch": "@@ -1,17 +1,18 @@\n /** This is a very ugly test code (doomed to fail linting) */\n #include \"demo.hpp\"\n-#include \n-#include \n+#include \n \n-// using size_t from cstddef\n-size_t dummyFunc(size_t i) { return i; }\n \n-int main()\n-{\n- for (;;)\n- break;\n+\n+\n+int main(){\n+\n+ for (;;) break;\n+\n \n printf(\"Hello world!\\n\");\n \n- return 0;\n-}\n+\n+\n+\n+ return 0;}" + } +] diff --git a/cpp-linter-lib/tests/paginated_changes/pull_request_files_pg2.json b/cpp-linter-lib/tests/paginated_changes/pull_request_files_pg2.json new file mode 100644 index 0000000..987a239 --- /dev/null +++ b/cpp-linter-lib/tests/paginated_changes/pull_request_files_pg2.json @@ -0,0 +1,14 @@ +[ + { + "sha": "f93d0122ae2e3c1952c795837d71c432036b55eb", + "filename": "src/demo.hpp", + "status": "modified", + "additions": 3, + "deletions": 8, + "changes": 11, + "blob_url": "https://github.com/cpp-linter/test-cpp-linter-action/blob/635a9c57bdcca07b99ddef52c2640337c50280b1/src%2Fdemo.hpp", + "raw_url": "https://github.com/cpp-linter/test-cpp-linter-action/raw/635a9c57bdcca07b99ddef52c2640337c50280b1/src%2Fdemo.hpp", + "contents_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/contents/src%2Fdemo.hpp?ref=635a9c57bdcca07b99ddef52c2640337c50280b1", + "patch": "@@ -5,12 +5,10 @@\n class Dummy {\n char* useless;\n int numb;\n+ Dummy() :numb(0), useless(\"\\0\"){}\n \n public:\n- void *not_usefull(char *str){\n- useless = str;\n- return 0;\n- }\n+ void *not_useful(char *str){useless = str;}\n };\n \n \n@@ -28,14 +26,11 @@ class Dummy {\n \n \n \n-\n-\n-\n-\n \n \n struct LongDiff\n {\n+\n long diff;\n \n };" + } +] diff --git a/cpp-linter-lib/tests/paginated_changes/push_files_pg1.json b/cpp-linter-lib/tests/paginated_changes/push_files_pg1.json new file mode 100644 index 0000000..8022a1e --- /dev/null +++ b/cpp-linter-lib/tests/paginated_changes/push_files_pg1.json @@ -0,0 +1,29 @@ +{ + "files": [ + { + "sha": "52501fa1dc96d6bc6f8a155816df041b1de975d9", + "filename": ".github/workflows/cpp-lint-package.yml", + "status": "modified", + "additions": 9, + "deletions": 5, + "changes": 14, + "blob_url": "https://github.com/cpp-linter/test-cpp-linter-action/blob/635a9c57bdcca07b99ddef52c2640337c50280b1/.github%2Fworkflows%2Fcpp-lint-package.yml", + "raw_url": "https://github.com/cpp-linter/test-cpp-linter-action/raw/635a9c57bdcca07b99ddef52c2640337c50280b1/.github%2Fworkflows%2Fcpp-lint-package.yml", + "contents_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/contents/.github%2Fworkflows%2Fcpp-lint-package.yml?ref=635a9c57bdcca07b99ddef52c2640337c50280b1", + "patch": "@@ -7,16 +7,17 @@ on:\n description: 'which branch to test'\n default: 'main'\n required: true\n+ pull_request:\n \n jobs:\n cpp-linter:\n runs-on: windows-latest\n \n strategy:\n matrix:\n- clang-version: ['7', '8', '9','10', '11', '12', '13', '14', '15', '16', '17']\n+ clang-version: ['10', '11', '12', '13', '14', '15', '16', '17']\n repo: ['cpp-linter/cpp-linter']\n- branch: ['${{ inputs.branch }}']\n+ branch: ['pr-review-suggestions']\n fail-fast: false\n \n steps:\n@@ -62,10 +63,13 @@ jobs:\n -i=build \n -p=build \n -V=${{ runner.temp }}/llvm \n- -f=false \n --extra-arg=\"-std=c++14 -Wall\" \n- --thread-comments=${{ matrix.clang-version == '12' }} \n- -a=${{ matrix.clang-version == '12' }}\n+ --file-annotations=false\n+ --lines-changed-only=false\n+ --extension=h,c\n+ --thread-comments=${{ matrix.clang-version == '16' }} \n+ --tidy-review=${{ matrix.clang-version == '16' }}\n+ --format-review=${{ matrix.clang-version == '16' }}\n \n - name: Fail fast?!\n if: steps.linter.outputs.checks-failed > 0" + }, + { + "sha": "1bf553e06e4b7c6c9a9be5da4845acbdeb04f6a5", + "filename": "src/demo.cpp", + "previous_filename": "src/demo.c", + "status": "modified", + "additions": 11, + "deletions": 10, + "changes": 21, + "blob_url": "https://github.com/cpp-linter/test-cpp-linter-action/blob/635a9c57bdcca07b99ddef52c2640337c50280b1/src%2Fdemo.cpp", + "raw_url": "https://github.com/cpp-linter/test-cpp-linter-action/raw/635a9c57bdcca07b99ddef52c2640337c50280b1/src%2Fdemo.cpp", + "contents_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/contents/src%2Fdemo.cpp?ref=635a9c57bdcca07b99ddef52c2640337c50280b1", + "patch": "@@ -1,17 +1,18 @@\n /** This is a very ugly test code (doomed to fail linting) */\n #include \"demo.hpp\"\n-#include \n-#include \n+#include \n \n-// using size_t from cstddef\n-size_t dummyFunc(size_t i) { return i; }\n \n-int main()\n-{\n- for (;;)\n- break;\n+\n+\n+int main(){\n+\n+ for (;;) break;\n+\n \n printf(\"Hello world!\\n\");\n \n- return 0;\n-}\n+\n+\n+\n+ return 0;}" + } + ] +} diff --git a/cpp-linter-lib/tests/paginated_changes/push_files_pg2.json b/cpp-linter-lib/tests/paginated_changes/push_files_pg2.json new file mode 100644 index 0000000..573f532 --- /dev/null +++ b/cpp-linter-lib/tests/paginated_changes/push_files_pg2.json @@ -0,0 +1,16 @@ +{ + "files": [ + { + "sha": "f93d0122ae2e3c1952c795837d71c432036b55eb", + "filename": "src/demo.hpp", + "status": "modified", + "additions": 3, + "deletions": 8, + "changes": 11, + "blob_url": "https://github.com/cpp-linter/test-cpp-linter-action/blob/635a9c57bdcca07b99ddef52c2640337c50280b1/src%2Fdemo.hpp", + "raw_url": "https://github.com/cpp-linter/test-cpp-linter-action/raw/635a9c57bdcca07b99ddef52c2640337c50280b1/src%2Fdemo.hpp", + "contents_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/contents/src%2Fdemo.hpp?ref=635a9c57bdcca07b99ddef52c2640337c50280b1", + "patch": "@@ -5,12 +5,10 @@\n class Dummy {\n char* useless;\n int numb;\n+ Dummy() :numb(0), useless(\"\\0\"){}\n \n public:\n- void *not_usefull(char *str){\n- useless = str;\n- return 0;\n- }\n+ void *not_useful(char *str){useless = str;}\n };\n \n \n@@ -28,14 +26,11 @@ class Dummy {\n \n \n \n-\n-\n-\n-\n \n \n struct LongDiff\n {\n+\n long diff;\n \n };" + } + ] +} diff --git a/cpp-linter-lib/tests/reviews.rs b/cpp-linter-lib/tests/reviews.rs index bbd9f54..4f27018 100644 --- a/cpp-linter-lib/tests/reviews.rs +++ b/cpp-linter-lib/tests/reviews.rs @@ -210,7 +210,7 @@ async fn setup(lib_root: &Path, test_params: &TestParams) { } async fn test_review(test_params: &TestParams) { - let tmp_dir = create_test_space(); + let tmp_dir = create_test_space(true); let lib_root = env::current_dir().unwrap(); env::set_current_dir(tmp_dir.path()).unwrap(); setup(&lib_root, test_params).await;