bugfix: "the action skips PRs that is used to have request-for-change review" by @nafur

- bugfix: "the action skips PRs that is used to have `request-for-change` review"
- deduplicate reviews by reviewer
thanks @nafur

Author: zhiyelee

src/lib/github.js

@@ -115,12 +115,16 @@ export const getApprovalStatus = async (pullNumber) => {
     pull_number: pullNumber,
   });
 
+  let reviewers = new Set();
   let changesRequestedCount = 0;
   let approvalCount = 0;
 
-  reviewsData.forEach(({ state }) => {
+  reviewsData.reverse().forEach(({ state, user }) => {
+    if (reviewers.has(user.login)) return;
+    if (!['CHANGES_REQUESTED', 'APPROVED'].includes(state)) return;
     if (state === 'CHANGES_REQUESTED') changesRequestedCount += 1;
     if (state === 'APPROVED') approvalCount += 1;
+    reviewers.add(user.login);
   });
 
   return {

src/lib/github.test.js

@@ -243,7 +243,7 @@ describe('getApprovalStatus()', () => {
   });
 });
 
-describe('getAutoUpdateCanidate()', () => {
+describe('getAutoUpdateCandidate()', () => {
   const pullsList = require('../../test/fixtures/pulls_list.json');
   const reviewsList = require('../../test/fixtures/list_reviews.json');
   const prMetaData = require('../../test/fixtures/pr_metadata.json');
@@ -329,14 +329,43 @@ describe('getAutoUpdateCanidate()', () => {
     expect(mockedGet).toHaveBeenCalledTimes(0);
     expect(res).toBe(null);
   });
+  
+  test('PR with approvals from a single users will not be selected', async () => {
+    // 2 approvals, but from the same user
+    const reviews = {
+      ...reviewsList,
+      data: [
+        { ...reviewsList.data[0], state: 'APPROVED' },
+        { ...reviewsList.data[0], state: 'APPROVED' },
+      ],
+    };
+    const prList = [{ ...pullsList.data[0], auto_merge: {} }];
+    const mockedListReviews = jest.fn().mockResolvedValue(reviews);
+    const mockedGet = jest.fn();
+
+    github.getOctokit.mockReturnValue({
+      pulls: { listReviews: mockedListReviews, get: mockedGet },
+    });
+
+    const res = await gitLib.getAutoUpdateCandidate(prList);
+    expect(mockedListReviews).toHaveBeenCalled();
+    expect(utils.log).toHaveBeenCalledTimes(2);
+    expect(utils.printFailReason).toHaveBeenCalledTimes(1);
+    expect(utils.printFailReason).toHaveBeenCalledWith(
+      prList[0].number,
+      `approvalsCount: 1, requiredApprovalCount: ${requiredApprovalCount}, changesRequestedReviews: 0`,
+    );
+    expect(mockedGet).toHaveBeenCalledTimes(0);
+    expect(res).toBe(null);
+  });
 
   test('PR with mergeable !== true will not be selected', async () => {
     // has 2 approvals, not request for change review
     const reviews = {
       ...reviewsList,
       data: [
         { ...reviewsList.data[0], state: 'APPROVED' },
-        { ...reviewsList.data[0], state: 'APPROVED' },
+        { ...reviewsList.data[1], state: 'APPROVED' },
       ],
     };
     const prData = {
@@ -369,7 +398,7 @@ describe('getAutoUpdateCanidate()', () => {
       ...reviewsList,
       data: [
         { ...reviewsList.data[0], state: 'APPROVED' },
-        { ...reviewsList.data[0], state: 'APPROVED' },
+        { ...reviewsList.data[1], state: 'APPROVED' },
       ],
     };
     // pr mergeable: true, merge_state: clean
@@ -397,13 +426,13 @@ describe('getAutoUpdateCanidate()', () => {
     expect(res).toBe(null);
   });
 
-  test('PR with failed checks wonnt be selected', async () => {
+  test('PR with failed checks wont be selected', async () => {
     // has 2 approvals, no request for change review
     const reviews = {
       ...reviewsList,
       data: [
         { ...reviewsList.data[0], state: 'APPROVED' },
-        { ...reviewsList.data[0], state: 'APPROVED' },
+        { ...reviewsList.data[1], state: 'APPROVED' },
       ],
     };
     // pr mergeable: true, merge_state: clean
@@ -452,7 +481,7 @@ describe('getAutoUpdateCanidate()', () => {
       ...reviewsList,
       data: [
         { ...reviewsList.data[0], state: 'APPROVED' },
-        { ...reviewsList.data[0], state: 'APPROVED' },
+        { ...reviewsList.data[1], state: 'APPROVED' },
       ],
     };
     // pr mergeable: true, merge_state: clean
@@ -489,13 +518,60 @@ describe('getAutoUpdateCanidate()', () => {
     const res = await gitLib.getAutoUpdateCandidate(prList);
     expect(res).toBe(prList[0]);
   });
+  
+  test('Should ignore trailing comment reviews', async () => {
+    // has approvals, and some reviews at the end
+    const reviews = {
+      ...reviewsList,
+      data: [
+        { ...reviewsList.data[0], state: 'APPROVED' },
+        { ...reviewsList.data[1], state: 'APPROVED' },
+        { ...reviewsList.data[0], state: 'COMMENTED' },
+        { ...reviewsList.data[2], state: 'COMMENTED' },
+      ],
+    };
+    // pr mergeable: true, merge_state: clean
+    const prData = {
+      data: {
+        ...prMetaData.data,
+        ...{ mergeable: true, mergeable_state: 'behind' },
+      },
+    };
+
+    const check = checksList.data.check_runs[0];
+    const checks = {
+      ...checksList,
+      data: {
+        total_count: 2,
+        check_runs: [
+          { ...check, conclusion: 'success' },
+          { ...check, conclusion: 'success' },
+        ],
+      },
+    };
+
+    // has auto-merge PR
+    const prList = [{ ...pullsList.data[0], auto_merge: {} }];
+    const mockedListReviews = jest.fn().mockResolvedValue(reviews);
+    const mockedGet = jest.fn().mockResolvedValue(prData);
+    const mockedListForRef = jest.fn().mockResolvedValue(checks);
+
+    github.getOctokit.mockReturnValue({
+      pulls: { listReviews: mockedListReviews, get: mockedGet },
+      checks: { listForRef: mockedListForRef },
+    });
+
+    const res = await gitLib.getAutoUpdateCandidate(prList);
+    expect(res).toEqual(prList[0]);
+  });
+  
   test('Should return the first PR if it is all good', async () => {
     // has 2 approvals, no request for change review
     const reviews = {
       ...reviewsList,
       data: [
         { ...reviewsList.data[0], state: 'APPROVED' },
-        { ...reviewsList.data[0], state: 'APPROVED' },
+        { ...reviewsList.data[1], state: 'APPROVED' },
       ],
     };
     // pr mergeable: true, merge_state: clean
@@ -541,14 +617,14 @@ describe('getAutoUpdateCanidate()', () => {
       ...reviewsList,
       data: [
         { ...reviewsList.data[0], state: 'APPROVED' },
-        { ...reviewsList.data[0], state: 'CHANGES_REQUESTED' },
+        { ...reviewsList.data[1], state: 'CHANGES_REQUESTED' },
       ],
     };
     const reviewsSecond = {
       ...reviewsList,
       data: [
         { ...reviewsList.data[0], state: 'APPROVED' },
-        { ...reviewsList.data[0], state: 'APPROVED' },
+        { ...reviewsList.data[1], state: 'APPROVED' },
       ],
     };
     // pr mergeable: true, merge_state: clean