From 3a69010fcc9c0cb9feaf2a52691376a81f778360 Mon Sep 17 00:00:00 2001 From: Sofia Leon Date: Thu, 20 Feb 2020 10:22:09 -0800 Subject: [PATCH 1/2] fix QA bugs --- packages/merge-on-green/src/merge-logic.ts | 49 ++++++---------------- 1 file changed, 13 insertions(+), 36 deletions(-) diff --git a/packages/merge-on-green/src/merge-logic.ts b/packages/merge-on-green/src/merge-logic.ts index b014c11789c..0f855e1dab9 100644 --- a/packages/merge-on-green/src/merge-logic.ts +++ b/packages/merge-on-green/src/merge-logic.ts @@ -359,25 +359,6 @@ mergeOnGreen.getReviewsCompleted = async function getReviewsCompleted( } }; -mergeOnGreen.getReviewsRequested = async function getReviewsRequested( - owner: string, - repo: string, - pr: number, - github: GitHubAPI -): Promise { - try { - return ( - await github.pulls.listReviewRequests({ - owner, - repo, - pull_number: pr, - }) - ).data as ReviewsRequested; - } catch (err) { - return { users: [], teams: [] }; - } -}; - //this function cleans the reviews, since the listReviews method github provides returns a complete history of all comments added //and we just want the most recent for each reviewer mergeOnGreen.cleanReviews = function cleanReviews( @@ -407,10 +388,12 @@ mergeOnGreen.checkReviews = async function checkReviews( github: GitHubAPI ): Promise { console.info('=== checking required reviews ==='); - const [reviewsCompletedDirty, reviewsRequested] = await Promise.all([ - mergeOnGreen.getReviewsCompleted(owner, repo, pr, github), - mergeOnGreen.getReviewsRequested(owner, repo, pr, github), - ]); + const reviewsCompletedDirty = await mergeOnGreen.getReviewsCompleted( + owner, + repo, + pr, + github + ); let reviewsPassed = true; const reviewsCompleted = mergeOnGreen.cleanReviews(reviewsCompletedDirty); if (reviewsCompleted.length !== 0) { @@ -425,13 +408,6 @@ mergeOnGreen.checkReviews = async function checkReviews( console.log('No one has reviewed your PR'); return false; } - if ( - reviewsRequested.users.length !== 0 || - reviewsRequested.teams.length !== 0 - ) { - console.log('You have assigned reviewers that have not submitted a review'); - return false; - } return reviewsPassed; }; @@ -516,16 +492,17 @@ export async function mergeOnGreen( ); if (checkReview === true && checkStatus === true) { - console.log('Updating branch'); - await mergeOnGreen.updateBranch(owner, repo, pr, github); - console.log('Merging PR'); + let merged = false; try { + console.info(`attempt to merge ${owner}/${repo}`); await mergeOnGreen.merge(owner, repo, pr, github); - return true; + merged = true; } catch (err) { - console.log(err); - return false; + console.info(err); + console.log('Attempting to update branch'); + await mergeOnGreen.updateBranch(owner, repo, pr, github); } + return merged; } else if (state === 'stop') { console.log('Your PR timed out before its statuses & reviews passed'); await mergeOnGreen.commentOnPR(owner, repo, pr, github); From 99022990eb78a19786def7bd0cec55e2fb277a53 Mon Sep 17 00:00:00 2001 From: Sofia Leon Date: Thu, 20 Feb 2020 11:04:36 -0800 Subject: [PATCH 2/2] fix: tests --- .../merge-on-green/test/merge-on-green.ts | 39 +++++++------------ 1 file changed, 14 insertions(+), 25 deletions(-) diff --git a/packages/merge-on-green/test/merge-on-green.ts b/packages/merge-on-green/test/merge-on-green.ts index 7d315c8721b..b842648c84d 100644 --- a/packages/merge-on-green/test/merge-on-green.ts +++ b/packages/merge-on-green/test/merge-on-green.ts @@ -98,12 +98,6 @@ function getReviewsCompleted(response: Reviews[]) { .reply(200, response); } -function getReviewsRequested(response: ReviewRequests) { - return nock('https://api.github.com') - .get('/repos/testOwner/testRepo/pulls/1/requested_reviewers') - .reply(200, response); -} - function getLatestCommit(response: HeadSha[]) { return nock('https://api.github.com') .get('/repos/testOwner/testRepo/pulls/1/commits?per_page=100&page=1') @@ -134,6 +128,12 @@ function merge() { .reply(200); } +function mergeWithError() { + return nock('https://api.github.com') + .put('/repos/testOwner/testRepo/pulls/1/merge') + .reply(400); +} + function commentOnPR() { return nock('https://api.github.com') .post('/repos/testOwner/testRepo/issues/1/comments') @@ -185,7 +185,6 @@ describe('merge-on-green', () => { const scopes = [ getReviewsCompleted([{ user: { login: 'octocat' }, state: 'APPROVED' }]), - getReviewsRequested({ users: [], teams: [] }), getLatestCommit([{ sha: '6dcb09b5b57875f334f61aebed695e2e4193db5e' }]), getMogLabel([{ name: 'automerge' }]), getStatusi('6dcb09b5b57875f334f61aebed695e2e4193db5e', [ @@ -193,7 +192,6 @@ describe('merge-on-green', () => { ]), requiredChecksByLanguage({ content: requiredChecks.toString('base64') }), repoMap({ content: map.toString('base64') }), - updateBranch(), merge(), ]; @@ -229,7 +227,6 @@ describe('merge-on-green', () => { { user: { login: 'octocat' }, state: 'APPROVED' }, { user: { login: 'octokitten' }, state: 'CHANGES_REQUESTED' }, ]), - getReviewsRequested({ users: [], teams: [] }), getLatestCommit([{ sha: '6dcb09b5b57875f334f61aebed695e2e4193db5e' }]), getMogLabel([{ name: 'automerge' }]), getStatusi('6dcb09b5b57875f334f61aebed695e2e4193db5e', [ @@ -268,7 +265,6 @@ describe('merge-on-green', () => { const scopes = [ getReviewsCompleted([{ user: { login: 'octocat' }, state: 'APPROVED' }]), - getReviewsRequested({ users: [], teams: [] }), getLatestCommit([]), getMogLabel([{ name: 'automerge' }]), getStatusi('', [ @@ -307,7 +303,6 @@ describe('merge-on-green', () => { const scopes = [ getReviewsCompleted([{ user: { login: 'octocat' }, state: 'APPROVED' }]), - getReviewsRequested({ users: [], teams: [] }), getLatestCommit([{ sha: '6dcb09b5b57875f334f61aebed695e2e4193db5e' }]), getMogLabel([{ name: 'this is not the label you are looking for' }]), getStatusi('6dcb09b5b57875f334f61aebed695e2e4193db5e', [ @@ -346,7 +341,6 @@ describe('merge-on-green', () => { const scopes = [ getReviewsCompleted([{ user: { login: 'octocat' }, state: 'APPROVED' }]), - getReviewsRequested({ users: [], teams: [] }), getLatestCommit([{ sha: '6dcb09b5b57875f334f61aebed695e2e4193db5e' }]), getMogLabel([{ name: 'automerge' }]), getStatusi('6dcb09b5b57875f334f61aebed695e2e4193db5e', []), @@ -383,7 +377,6 @@ describe('merge-on-green', () => { const scopes = [ getReviewsCompleted([{ user: { login: 'octocat' }, state: 'APPROVED' }]), - getReviewsRequested({ users: [], teams: [] }), getLatestCommit([{ sha: '6dcb09b5b57875f334f61aebed695e2e4193db5e' }]), getMogLabel([{ name: 'automerge' }]), getStatusi('6dcb09b5b57875f334f61aebed695e2e4193db5e', [ @@ -422,7 +415,6 @@ describe('merge-on-green', () => { const scopes = [ getReviewsCompleted([{ user: { login: 'octocat' }, state: 'APPROVED' }]), - getReviewsRequested({ users: [], teams: [] }), getLatestCommit([{ sha: '6dcb09b5b57875f334f61aebed695e2e4193db5e' }]), getMogLabel([{ name: 'this is not the label you are looking for' }]), getStatusi('6dcb09b5b57875f334f61aebed695e2e4193db5e', [ @@ -462,7 +454,6 @@ describe('merge-on-green', () => { const scopes = [ getReviewsCompleted([{ user: { login: 'octocat' }, state: 'APPROVED' }]), - getReviewsRequested({ users: [], teams: [] }), getLatestCommit([{ sha: '6dcb09b5b57875f334f61aebed695e2e4193db5e' }]), getMogLabel([{ name: 'automerge' }]), getStatusi('6dcb09b5b57875f334f61aebed695e2e4193db5e', [ @@ -501,7 +492,6 @@ describe('merge-on-green', () => { const scopes = [ getReviewsCompleted([{ user: { login: 'octocat' }, state: 'APPROVED' }]), - getReviewsRequested({ users: [], teams: [] }), getLatestCommit([{ sha: '6dcb09b5b57875f334f61aebed695e2e4193db5e' }]), getMogLabel([{ name: 'automerge' }]), getStatusi('6dcb09b5b57875f334f61aebed695e2e4193db5e', []), @@ -515,7 +505,6 @@ describe('merge-on-green', () => { }), requiredChecksByLanguage({ content: requiredChecks.toString('base64') }), repoMap({ content: map.toString('base64') }), - updateBranch(), merge(), ]; @@ -548,7 +537,6 @@ describe('merge-on-green', () => { const scopes = [ getReviewsCompleted([]), - getReviewsRequested({ users: [], teams: [] }), getLatestCommit([{ sha: '6dcb09b5b57875f334f61aebed695e2e4193db5e' }]), getMogLabel([{ name: 'automerge' }]), getStatusi('6dcb09b5b57875f334f61aebed695e2e4193db5e', []), @@ -573,7 +561,7 @@ describe('merge-on-green', () => { scopes.forEach(s => s.done()); }); - it('fails if people are assigned to review but have not reviewed', async () => { + it('passes when special checks are passed', async () => { handler.listPRs = async () => { const watchPr: WatchPR[] = [ { @@ -593,20 +581,22 @@ describe('merge-on-green', () => { const scopes = [ getReviewsCompleted([{ user: { login: 'octocat' }, state: 'APPROVED' }]), - getReviewsRequested({ users: ['user'], teams: [] }), getLatestCommit([{ sha: '6dcb09b5b57875f334f61aebed695e2e4193db5e' }]), getMogLabel([{ name: 'automerge' }]), getStatusi('6dcb09b5b57875f334f61aebed695e2e4193db5e', []), + requiredChecksByLanguage({ + content: specialRequiredChecks.toString('base64'), + }), getRuns('6dcb09b5b57875f334f61aebed695e2e4193db5e', { check_runs: [ { - name: 'Kokoro - Test: Binary Compatibility', + name: 'Special Check', conclusion: 'success', }, ], }), - requiredChecksByLanguage({ content: requiredChecks.toString('base64') }), repoMap({ content: map.toString('base64') }), + merge(), ]; await probot.receive({ @@ -618,7 +608,7 @@ describe('merge-on-green', () => { scopes.forEach(s => s.done()); }); - it('passes when special checks are passed', async () => { + it('updates a branch if merge returns error', async () => { handler.listPRs = async () => { const watchPr: WatchPR[] = [ { @@ -638,7 +628,6 @@ describe('merge-on-green', () => { const scopes = [ getReviewsCompleted([{ user: { login: 'octocat' }, state: 'APPROVED' }]), - getReviewsRequested({ users: [], teams: [] }), getLatestCommit([{ sha: '6dcb09b5b57875f334f61aebed695e2e4193db5e' }]), getMogLabel([{ name: 'automerge' }]), getStatusi('6dcb09b5b57875f334f61aebed695e2e4193db5e', []), @@ -654,8 +643,8 @@ describe('merge-on-green', () => { ], }), repoMap({ content: map.toString('base64') }), + mergeWithError(), updateBranch(), - merge(), ]; await probot.receive({