From c1751e2deaf218bc61ccc500ece3c64a2cc78cf8 Mon Sep 17 00:00:00 2001 From: Jan Hesse Date: Thu, 20 Jun 2024 22:50:09 +0200 Subject: [PATCH] fix: Respect all comments in lastComment validator and comment action (#755) --- __fixtures__/unit/helper.js | 11 +++++-- __tests__/unit/actions/comment.test.js | 20 ++++++++++-- __tests__/unit/github/api.test.js | 32 ++++++++++++++++--- __tests__/unit/validators/approvals.test.js | 2 -- __tests__/unit/validators/lastComment.test.js | 2 +- docs/changelog.rst | 1 + lib/actions/comment.js | 2 +- lib/github/api.js | 9 ++++-- lib/validators/lastComment.js | 2 +- 9 files changed, 65 insertions(+), 16 deletions(-) diff --git a/__fixtures__/unit/helper.js b/__fixtures__/unit/helper.js index 081bb10e..6bcc283d 100644 --- a/__fixtures__/unit/helper.js +++ b/__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)) } } diff --git a/__tests__/unit/actions/comment.test.js b/__tests__/unit/actions/comment.test.js index 3871005b..fb82cb57 100644 --- a/__tests__/unit/actions/comment.test.js +++ b/__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() diff --git a/__tests__/unit/github/api.test.js b/__tests__/unit/github/api.test.js index bd83bf2d..910f872f 100644 --- a/__tests__/unit/github/api.test.js +++ b/__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('valdis@github.com') }) + 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 }) diff --git a/__tests__/unit/validators/approvals.test.js b/__tests__/unit/validators/approvals.test.js index 3aa5b85e..54389e04 100644 --- a/__tests__/unit/validators/approvals.test.js +++ b/__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 () => { diff --git a/__tests__/unit/validators/lastComment.test.js b/__tests__/unit/validators/lastComment.test.js index ead8c2d6..f2178299 100644 --- a/__tests__/unit/validators/lastComment.test.js +++ b/__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 }] }) } diff --git a/docs/changelog.rst b/docs/changelog.rst index 50640b04..d3d5a16b 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -1,5 +1,6 @@ CHANGELOG ===================================== +| June 20, 2024: fix: Respect all comments in lastComment validator and comment action `#755 `_ | June 12, 2024: feat: Support `issue_comment` event as trigger for actions `#754 `_ | June 10, 2024: fix: Docker image not working `#753 `_ | June 10, 2024: feat: publish multi arch docker image to dockerhub `#751 `_ diff --git a/lib/actions/comment.js b/lib/actions/comment.js index ea89f03f..3417039c 100644 --- a/lib/actions/comment.js +++ b/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) => { diff --git a/lib/github/api.js b/lib/github/api.js index 8cce4b6d..7acabe88 100644 --- a/lib/github/api.js +++ b/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) diff --git a/lib/validators/lastComment.js b/lib/validators/lastComment.js index ef8547f7..5cbd6e46 100644 --- a/lib/validators/lastComment.js +++ b/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,