fix: Respect all comments in lastComment validator and comment action (#755)

Author: andekande

__fixtures__/unit/helper.js

@@ -207,8 +207,10 @@ module.exports = {
               resolve({ status: 204 })
             })
           },
-          listComments: () => {
-            return { data: (options.listComments) ? options.listComments : [] }
+          listComments: {
+            endpoint: {
+              merge: () => Promise.resolve({ data: (options.comments) ? options.comments : [] })
+            }
           },
           createComment: jest.fn().mockReturnValue(options.createComment || 'createComment call success'),
           deleteComment: jest.fn().mockReturnValue(options.deleteComment || 'deleteComment call success'),
@@ -253,5 +255,10 @@ module.exports = {
     context.probotContext.config = () => {
       return Promise.resolve(yaml.safeLoad(configString))
     }
+  },
+
+  flushPromises: () => {
+    // https://stackoverflow.com/questions/49405338/jest-test-promise-resolution-and-event-loop-tick
+    return new Promise(resolve => setImmediate(resolve))
   }
 }

__tests__/unit/actions/comment.test.js

@@ -33,6 +33,8 @@ test.each([
   }]
 
   await comment.afterValidate(context, settings, '', schedulerResult)
+  await Helper.flushPromises()
+
   expect(context.octokit.issues.createComment.mock.calls.length).toBe(1)
 })
 
@@ -49,6 +51,8 @@ test('check that comment created when afterValidate is called with proper parame
   }
 
   await comment.afterValidate(context, settings, '', result)
+  await Helper.flushPromises()
+
   expect(context.octokit.issues.createComment.mock.calls.length).toBe(1)
   expect(context.octokit.issues.createComment.mock.calls[0][0].body).toBe('Your run has returned the following status: pass')
 })
@@ -64,6 +68,8 @@ test('that comment is created three times when result contain three issues found
     }
   }]
   await comment.afterValidate(context, settings, '', schedulerResult)
+  await Helper.flushPromises()
+
   expect(context.octokit.issues.createComment.mock.calls.length).toBe(3)
 })
 
@@ -93,6 +99,8 @@ test('check that old comments from Mergeable are deleted if they exists', async
   }
 
   await comment.afterValidate(context, settings, '', result)
+  await Helper.flushPromises()
+
   expect(context.octokit.issues.deleteComment.mock.calls.length).toBe(1)
   expect(context.octokit.issues.deleteComment.mock.calls[0][0].comment_id).toBe('2')
 })
@@ -123,6 +131,8 @@ test('check that old comments checks toLowerCase of the Bot name', async () => {
   }
 
   await comment.afterValidate(context, settings, '', result)
+  await Helper.flushPromises()
+
   expect(context.octokit.issues.deleteComment.mock.calls.length).toBe(1)
   expect(context.octokit.issues.deleteComment.mock.calls[0][0].comment_id).toBe('2')
 })
@@ -156,6 +166,7 @@ test('error handling includes removing old error comments and creating new error
   }
 
   await comment.handleError(context, payload)
+
   expect(context.octokit.issues.deleteComment.mock.calls.length).toBe(1)
   expect(context.octokit.issues.deleteComment.mock.calls[0][0].comment_id).toBe('3')
   expect(context.octokit.issues.createComment.mock.calls[0][0].body).toBe(payload.body)
@@ -187,6 +198,7 @@ test('remove error comments only remove comments that includes "error" ', async
   const context = createMockContext(listComments)
 
   await comment.removeErrorComments(context, comment)
+
   expect(context.octokit.issues.deleteComment.mock.calls.length).toBe(1)
   expect(context.octokit.issues.deleteComment.mock.calls[0][0].comment_id).toBe('3')
 })
@@ -224,6 +236,7 @@ test('check that leave_old_comment option works', async () => {
   }
 
   await comment.afterValidate(context, settings, '', result)
+
   expect(context.octokit.issues.deleteComment.mock.calls.length).toBe(0)
 })
 
@@ -240,6 +253,7 @@ test('remove Error comment fail gracefully if payload does not exists', async ()
   }
 
   await comment.removeErrorComments(context)
+
   expect(context.octokit.issues.deleteComment.mock.calls.length).toBe(0)
 })
 
@@ -253,11 +267,13 @@ test('error handling includes removing old error comments and creating new error
   }
 
   await comment.afterValidate(context, settings, '', result)
+  await Helper.flushPromises()
+
   expect(context.octokit.issues.createComment.mock.calls[0][0].body).toBe('creator , do something!')
 })
 
-const createMockContext = (listComments, eventName = undefined, event = undefined) => {
-  const context = Helper.mockContext({ listComments, eventName, event })
+const createMockContext = (comments, eventName = undefined, event = undefined) => {
+  const context = Helper.mockContext({ comments, eventName, event })
 
   context.octokit.issues.createComment = jest.fn()
   context.octokit.issues.deleteComment = jest.fn()

__tests__/unit/github/api.test.js

@@ -9,6 +9,12 @@ describe('listFiles', () => {
     expect(res[0]).toEqual({ filename: 'abc.js', additions: 0, deletions: 0, changes: 0, status: 'modified' })
   })
 
+  test('that pagination is used', async () => {
+    const context = Helper.mockContext()
+    await GithubAPI.listFiles(context)
+    expect(context.octokit.paginate.mock.calls.length).toBe(1)
+  })
+
   test('that error are re-thrown', async () => {
     const context = Helper.mockContext()
     context.octokit.pulls.listFiles.endpoint.merge = jest.fn().mockRejectedValue({ status: 402 })
@@ -259,14 +265,20 @@ describe('createComment', () => {
 
 describe('listComments', () => {
   test('return correct data if no error', async () => {
-    const res = await GithubAPI.listComments(Helper.mockContext({ listComments: [{ user: { login: 'mergeable[bot]' } }, { user: { login: 'userA' } }] }))
-    expect(res.data.length).toEqual(2)
-    expect(res.data[0].user.login).toEqual('mergeable[bot]')
+    const res = await GithubAPI.listComments(Helper.mockContext({ comments: [{ user: { login: 'mergeable[bot]' } }, { user: { login: 'userA' } }] }))
+    expect(res.length).toEqual(2)
+    expect(res[0].user.login).toEqual('mergeable[bot]')
+  })
+
+  test('that pagination is used', async () => {
+    const context = Helper.mockContext()
+    await GithubAPI.listComments(context)
+    expect(context.octokit.paginate.mock.calls.length).toBe(1)
   })
 
   test('that error are re-thrown', async () => {
     const context = Helper.mockContext()
-    context.octokit.issues.listComments = jest.fn().mockRejectedValue({ status: 402 })
+    context.octokit.issues.listComments.endpoint.merge = jest.fn().mockRejectedValue({ status: 402 })
 
     try {
       await GithubAPI.listComments(context)
@@ -680,6 +692,12 @@ describe('listReviews', () => {
     expect(res).toEqual(reviews)
   })
 
+  test('that pagination is used', async () => {
+    const context = Helper.mockContext()
+    await GithubAPI.listReviews(context)
+    expect(context.octokit.paginate.mock.calls.length).toBe(1)
+  })
+
   test('that error are re-thrown', async () => {
     const context = Helper.mockContext()
     context.octokit.pulls.listReviews.endpoint.merge = jest.fn().mockRejectedValue({ status: 402 })
@@ -725,6 +743,12 @@ describe('listCommits', () => {
     expect(res[0].committer.email).toEqual('[email protected]')
   })
 
+  test('that pagination is used', async () => {
+    const context = Helper.mockContext()
+    await GithubAPI.listCommits(context)
+    expect(context.octokit.paginate.mock.calls.length).toBe(1)
+  })
+
   test('that error are NOT re-thrown', async () => {
     const context = Helper.mockContext()
     context.octokit.pulls.listCommits.endpoint.merge = jest.fn().mockRejectedValue({ status: 402 })

__tests__/unit/validators/approvals.test.js

@@ -317,8 +317,6 @@ test('validate correctly with reviews more than 30.', async () => {
   })
   expect(validation.validations[0].details.input).toEqual(['user2', 'user1'])
   expect(validation.status).toBe('pass')
-  // ensure paginate was called
-  expect(context.octokit.paginate.mock.calls.length).toBe(1)
 })
 
 test('pr creator is removed from required reviewer list', async () => {

__tests__/unit/validators/lastComment.test.js

@@ -122,5 +122,5 @@ test('complex Logic test', async () => {
 })
 
 function createMockContext (comments) {
-  return Helper.mockContext({ listComments: Array.isArray(comments) ? comments.map(comment => ({ body: comment })) : [{ body: comments }] })
+  return Helper.mockContext({ comments: Array.isArray(comments) ? comments.map(comment => ({ body: comment })) : [{ body: comments }] })
 }

docs/changelog.rst

@@ -1,5 +1,6 @@
 CHANGELOG
 =====================================
+| June 20, 2024: fix: Respect all comments in lastComment validator and comment action `#755 <https://github.com/mergeability/mergeable/pull/755>`_
 | June 12, 2024: feat: Support `issue_comment` event as trigger for actions `#754 <https://github.com/mergeability/mergeable/pull/754>`_
 | June 10, 2024: fix: Docker image not working `#753 <https://github.com/mergeability/mergeable/pull/753>`_
 | June 10, 2024: feat: publish multi arch docker image to dockerhub `#751 <https://github.com/mergeability/mergeable/pull/751>`_

lib/actions/comment.js

@@ -14,7 +14,7 @@ const fetchCommentsByMergeable = async (context, issueNumber, actionObj) => {
   const comments = await actionObj.githubAPI.listComments(context, issueNumber)
 
   const botName = process.env.APP_NAME ? process.env.APP_NAME : 'Mergeable'
-  return comments.data.filter(comment => comment.user.login.toLowerCase() === `${botName.toLowerCase()}[bot]`)
+  return comments.filter(comment => comment.user.login.toLowerCase() === `${botName.toLowerCase()}[bot]`)
 }
 
 const deleteOldComments = async (context, oldComments, actionObj) => {

lib/github/api.js

@@ -166,7 +166,7 @@ class GithubAPI {
     debugLog(context, callFn)
 
     try {
-      return context.octokit.issues.createComment(
+      return await context.octokit.issues.createComment(
         context.repo({ issue_number: issueNumber, body })
       )
     } catch (err) {
@@ -180,8 +180,11 @@ class GithubAPI {
     debugLog(context, callFn)
 
     try {
-      return context.octokit.issues.listComments(
-        context.repo({ issue_number: issueNumber })
+      return await context.octokit.paginate(
+        context.octokit.issues.listComments.endpoint.merge(
+          context.repo({ issue_number: issueNumber })
+        ),
+        res => res.data
       )
     } catch (err) {
       return checkCommonError(err, context, callFn)

lib/validators/lastComment.js

@@ -24,7 +24,7 @@ class LastComment extends Validator {
   }
 
   async validate (context, validationSettings) {
-    const comments = (await this.githubAPI.listComments(context, this.getPayload(context).number)).data
+    const comments = await this.githubAPI.listComments(context, this.getPayload(context).number)
 
     return this.processOptions(
       validationSettings,