From 4fc1372e913135517f50574c948ace5f985d37fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B8ren=20Louv-Jansen?= Date: Fri, 15 Jul 2022 01:12:11 +0200 Subject: [PATCH 1/4] Add another case to auto merge --- .../autoMergeNowOrLater.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/lib/cherrypickAndCreateTargetPullRequest/autoMergeNowOrLater.ts b/src/lib/cherrypickAndCreateTargetPullRequest/autoMergeNowOrLater.ts index f3c4df44..23772be9 100644 --- a/src/lib/cherrypickAndCreateTargetPullRequest/autoMergeNowOrLater.ts +++ b/src/lib/cherrypickAndCreateTargetPullRequest/autoMergeNowOrLater.ts @@ -28,15 +28,16 @@ export async function autoMergeNowOrLater( } logger.info( - `Auto merge: Could not enable auto merge for PR "#${pullNumber}" due to ${e.message}` + `Auto merge: Failed to enable auto merge for PR "#${pullNumber}" due to ${e.message}` ); const isMissingStatusChecks = e.githubResponse.data.errors?.some( (e) => e.type === 'UNPROCESSABLE' && - e.message.includes( + (e.message.includes( 'Branch does not have required protected branch rules' - ) + ) || + e.message.includes('Pull request is in clean status')) ); // if auto merge cannot be enabled due to missing status checks, the PR should be merged immediately @@ -44,10 +45,12 @@ export async function autoMergeNowOrLater( throw e; } + logger.info('Auto merge: Attempting to merge immeidately'); + try { await mergePullRequest(options, pullNumber); spinner.text = - 'Auto-merge: Pull request was merged without waiting for status checks'; + 'Auto-merge: Pull request was merged immediately without waiting for status checks'; } catch (e) { if (!(e instanceof Error)) { throw new Error(`Unknown error: ${e}`); From 287f1a19bf5d8e8e6a4d6bf48f1ca5720eea604f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B8ren=20Louv-Jansen?= Date: Sat, 16 Jul 2022 00:23:06 +0200 Subject: [PATCH 2/4] Improve test --- .../autoMergeNowOrLater.ts | 3 +- ...nablePullRequestAutoMerge.mutation.test.ts | 86 ++++++++++++++++++- 2 files changed, 83 insertions(+), 6 deletions(-) diff --git a/src/lib/cherrypickAndCreateTargetPullRequest/autoMergeNowOrLater.ts b/src/lib/cherrypickAndCreateTargetPullRequest/autoMergeNowOrLater.ts index 23772be9..a2040fd0 100644 --- a/src/lib/cherrypickAndCreateTargetPullRequest/autoMergeNowOrLater.ts +++ b/src/lib/cherrypickAndCreateTargetPullRequest/autoMergeNowOrLater.ts @@ -49,8 +49,7 @@ export async function autoMergeNowOrLater( try { await mergePullRequest(options, pullNumber); - spinner.text = - 'Auto-merge: Pull request was merged immediately without waiting for status checks'; + spinner.text = 'Auto-merge: Pull request was merged immediately'; } catch (e) { if (!(e instanceof Error)) { throw new Error(`Unknown error: ${e}`); diff --git a/src/lib/github/v4/enablePullRequestAutoMerge.mutation.test.ts b/src/lib/github/v4/enablePullRequestAutoMerge.mutation.test.ts index a43ed222..76a1224f 100644 --- a/src/lib/github/v4/enablePullRequestAutoMerge.mutation.test.ts +++ b/src/lib/github/v4/enablePullRequestAutoMerge.mutation.test.ts @@ -94,7 +94,7 @@ async function addCommit(octokit: Octokit) { } describe('enablePullRequestAutoMerge', () => { - describe('create a PR for 7.x which has status checks', () => { + describe('create a PR and enable auto-merge against "approvals-required-branch"', () => { let pullNumber: number; let branchName: string; let octokit: Octokit; @@ -115,7 +115,11 @@ describe('enablePullRequestAutoMerge', () => { const sha = await addCommit(octokit); await createBranch(octokit, branchName, sha); - pullNumber = await createPr(options, branchName, '7.x'); + pullNumber = await createPr( + options, + branchName, + 'approvals-required-branch' + ); }); // cleanup @@ -192,7 +196,66 @@ describe('enablePullRequestAutoMerge', () => { }); }); - describe('create a PR for 6.x which does not have status checks', () => { + describe('create a PR and attempt to enable auto-merge against "no-checks-required-branch"', () => { + let pullNumber: number; + let branchName: string; + let octokit: Octokit; + let options: ValidConfigOptions; + + beforeAll(async () => { + const randomString = crypto.randomBytes(4).toString('hex'); + branchName = `test-${randomString}`; + + options = { + accessToken, + repoOwner: TEST_REPO_OWNER, + repoName: TEST_REPO_NAME, + } as ValidConfigOptions; + + octokit = new Octokit({ auth: accessToken }); + await resetReference(octokit); + + const sha = await addCommit(octokit); + await createBranch(octokit, branchName, sha); + pullNumber = await createPr( + options, + branchName, + 'no-checks-required-branch' + ); + }); + + // cleanup + afterAll(async () => { + await closePr(octokit, pullNumber); + await deleteBranch(octokit, branchName); + await resetReference(octokit); + }); + + it('should not be able to enable auto-merge', async () => { + await expect(() => + enablePullRequestAutoMerge( + { ...options, autoMergeMethod: 'merge' }, + pullNumber + ) + ).rejects.toThrowErrorMatchingInlineSnapshot( + `"[\\"Pull request Pull request is in clean status\\"] (Github API v4)"` + ); + }); + + it('should not be able to enable auto-merge via merge', async () => { + await expect( + async () => + await enablePullRequestAutoMerge( + { ...options, autoMergeMethod: 'merge' }, + pullNumber + ) + ).rejects.toThrowErrorMatchingInlineSnapshot( + `"[\\"Pull request Pull request is in clean status\\"] (Github API v4)"` + ); + }); + }); + + describe('create a PR and attempt to enable auto-merge against "status-checks-required-branch"', () => { let pullNumber: number; let branchName: string; let octokit: Octokit; @@ -213,7 +276,11 @@ describe('enablePullRequestAutoMerge', () => { const sha = await addCommit(octokit); await createBranch(octokit, branchName, sha); - pullNumber = await createPr(options, branchName, '6.x'); + pullNumber = await createPr( + options, + branchName, + 'status-checks-required-branch' + ); }); // cleanup @@ -223,6 +290,17 @@ describe('enablePullRequestAutoMerge', () => { await resetReference(octokit); }); + it('should not be able to enable auto-merge', async () => { + await expect(() => + enablePullRequestAutoMerge( + { ...options, autoMergeMethod: 'merge' }, + pullNumber + ) + ).rejects.toThrowErrorMatchingInlineSnapshot( + `"[\\"Pull request Pull request is in clean status\\"] (Github API v4)"` + ); + }); + it('should not be able to enable auto-merge via merge', async () => { await expect( async () => From a39a3bf448803a5d996b19534419f25556d3cb53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B8ren=20Louv-Jansen?= Date: Sun, 17 Jul 2022 09:21:34 +0200 Subject: [PATCH 3/4] Minor changes to test --- ...nablePullRequestAutoMerge.mutation.test.ts | 84 ++++++++++++------- 1 file changed, 54 insertions(+), 30 deletions(-) diff --git a/src/lib/github/v4/enablePullRequestAutoMerge.mutation.test.ts b/src/lib/github/v4/enablePullRequestAutoMerge.mutation.test.ts index 76a1224f..de7496b5 100644 --- a/src/lib/github/v4/enablePullRequestAutoMerge.mutation.test.ts +++ b/src/lib/github/v4/enablePullRequestAutoMerge.mutation.test.ts @@ -29,7 +29,13 @@ function resetReference(octokit: Octokit) { }); } -async function closePr(octokit: Octokit, pullNumber: number) { +async function closePr({ + octokit, + pullNumber, +}: { + octokit: Octokit; + pullNumber: number; +}) { await octokit.pulls.update({ owner: TEST_REPO_OWNER, repo: TEST_REPO_NAME, @@ -38,14 +44,18 @@ async function closePr(octokit: Octokit, pullNumber: number) { }); } -async function createPr( - options: ValidConfigOptions, - branchName: string, - baseBranch: string -) { +async function createPr({ + options, + headBranch, + baseBranch, +}: { + options: ValidConfigOptions; + headBranch: string; + baseBranch: string; +}) { const prPayload: PullRequestPayload = { base: baseBranch, - head: branchName, + head: headBranch, body: 'testing...', owner: TEST_REPO_OWNER, repo: TEST_REPO_NAME, @@ -56,7 +66,13 @@ async function createPr( return number; } -async function deleteBranch(octokit: Octokit, branchName: string) { +async function deleteBranch({ + octokit, + branchName, +}: { + octokit: Octokit; + branchName: string; +}) { await octokit.git.deleteRef({ owner: TEST_REPO_OWNER, repo: TEST_REPO_NAME, @@ -64,7 +80,15 @@ async function deleteBranch(octokit: Octokit, branchName: string) { }); } -async function createBranch(octokit: Octokit, branchName: string, sha: string) { +async function createBranch({ + octokit, + branchName, + sha, +}: { + octokit: Octokit; + branchName: string; + sha: string; +}) { await octokit.git.createRef({ owner: TEST_REPO_OWNER, repo: TEST_REPO_NAME, @@ -114,18 +138,18 @@ describe('enablePullRequestAutoMerge', () => { await resetReference(octokit); const sha = await addCommit(octokit); - await createBranch(octokit, branchName, sha); - pullNumber = await createPr( + await createBranch({ octokit, branchName, sha }); + pullNumber = await createPr({ options, - branchName, - 'approvals-required-branch' - ); + headBranch: branchName, + baseBranch: 'approvals-required-branch', + }); }); // cleanup afterAll(async () => { - await closePr(octokit, pullNumber); - await deleteBranch(octokit, branchName); + await closePr({ octokit, pullNumber }); + await deleteBranch({ octokit, branchName }); await resetReference(octokit); }); @@ -216,18 +240,18 @@ describe('enablePullRequestAutoMerge', () => { await resetReference(octokit); const sha = await addCommit(octokit); - await createBranch(octokit, branchName, sha); - pullNumber = await createPr( + await createBranch({ octokit, branchName, sha }); + pullNumber = await createPr({ options, - branchName, - 'no-checks-required-branch' - ); + headBranch: branchName, + baseBranch: 'no-checks-required-branch', + }); }); // cleanup afterAll(async () => { - await closePr(octokit, pullNumber); - await deleteBranch(octokit, branchName); + await closePr({ octokit, pullNumber }); + await deleteBranch({ octokit, branchName }); await resetReference(octokit); }); @@ -275,18 +299,18 @@ describe('enablePullRequestAutoMerge', () => { await resetReference(octokit); const sha = await addCommit(octokit); - await createBranch(octokit, branchName, sha); - pullNumber = await createPr( + await createBranch({ octokit, branchName, sha }); + pullNumber = await createPr({ options, - branchName, - 'status-checks-required-branch' - ); + headBranch: branchName, + baseBranch: 'status-checks-required-branch', + }); }); // cleanup afterAll(async () => { - await closePr(octokit, pullNumber); - await deleteBranch(octokit, branchName); + await closePr({ octokit, pullNumber }); + await deleteBranch({ octokit, branchName }); await resetReference(octokit); }); From c95ac1fd6fcbb2b68e2883105b0ee990a4b44800 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B8ren=20Louv-Jansen?= Date: Mon, 18 Jul 2022 13:29:36 +0200 Subject: [PATCH 4/4] Improve tests --- .../autoMergeNowOrLater.ts | 37 ++----- ...nablePullRequestAutoMerge.mutation.test.ts | 101 ++++++++++-------- .../github/v4/enablePullRequestAutoMerge.ts | 15 ++- 3 files changed, 79 insertions(+), 74 deletions(-) diff --git a/src/lib/cherrypickAndCreateTargetPullRequest/autoMergeNowOrLater.ts b/src/lib/cherrypickAndCreateTargetPullRequest/autoMergeNowOrLater.ts index a2040fd0..11398a60 100644 --- a/src/lib/cherrypickAndCreateTargetPullRequest/autoMergeNowOrLater.ts +++ b/src/lib/cherrypickAndCreateTargetPullRequest/autoMergeNowOrLater.ts @@ -1,7 +1,10 @@ import { ValidConfigOptions } from '../../options/options'; import { mergePullRequest } from '../github/v3/mergePullRequest'; import { GithubV4Exception } from '../github/v4/apiRequestV4'; -import { enablePullRequestAutoMerge } from '../github/v4/enablePullRequestAutoMerge'; +import { + enablePullRequestAutoMerge, + parseGithubError, +} from '../github/v4/enablePullRequestAutoMerge'; import { logger } from '../logger'; import { ora } from '../ora'; @@ -31,37 +34,15 @@ export async function autoMergeNowOrLater( `Auto merge: Failed to enable auto merge for PR "#${pullNumber}" due to ${e.message}` ); - const isMissingStatusChecks = e.githubResponse.data.errors?.some( - (e) => - e.type === 'UNPROCESSABLE' && - (e.message.includes( - 'Branch does not have required protected branch rules' - ) || - e.message.includes('Pull request is in clean status')) - ); - - // if auto merge cannot be enabled due to missing status checks, the PR should be merged immediately + const { isMissingStatusChecks } = parseGithubError(e); if (!isMissingStatusChecks) { throw e; } - logger.info('Auto merge: Attempting to merge immeidately'); - - try { - await mergePullRequest(options, pullNumber); - spinner.text = 'Auto-merge: Pull request was merged immediately'; - } catch (e) { - if (!(e instanceof Error)) { - throw new Error(`Unknown error: ${e}`); - } - - logger.error( - `Auto merge: Could not merge PR "#${pullNumber}" immediately due to ${e.message}`, - e - ); - - throw e; - } + // if auto merge cannot be enabled due to missing status checks, the PR should be merged immediately + logger.info('Auto merge: Attempting to merge immediately'); + await mergePullRequest(options, pullNumber); + spinner.text = 'Auto-merge: Pull request was merged immediately'; } spinner.succeed(); diff --git a/src/lib/github/v4/enablePullRequestAutoMerge.mutation.test.ts b/src/lib/github/v4/enablePullRequestAutoMerge.mutation.test.ts index de7496b5..d9b5d96d 100644 --- a/src/lib/github/v4/enablePullRequestAutoMerge.mutation.test.ts +++ b/src/lib/github/v4/enablePullRequestAutoMerge.mutation.test.ts @@ -3,8 +3,12 @@ import { Octokit } from '@octokit/rest'; import { ValidConfigOptions } from '../../../options/options'; import { getDevAccessToken } from '../../../test/private/getDevAccessToken'; import { createPullRequest, PullRequestPayload } from '../v3/createPullRequest'; +import { GithubV4Exception } from './apiRequestV4'; import { disablePullRequestAutoMerge } from './disablePullRequestAutoMerge'; -import { enablePullRequestAutoMerge } from './enablePullRequestAutoMerge'; +import { + enablePullRequestAutoMerge, + parseGithubError, +} from './enablePullRequestAutoMerge'; import { fetchPullRequestAutoMergeMethod } from './fetchPullRequestAutoMergeMethod'; // The test repo requires auto-merge being enabled in options, as well as all merge types enabled (merge, squash, rebase) @@ -182,14 +186,24 @@ describe('enablePullRequestAutoMerge', () => { expect(autoMergeMethod).toBe('MERGE'); }); - it('should not enable auto-merge via rebase because it is disallowed', async () => { - await expect( - async () => - await enablePullRequestAutoMerge( - { ...options, autoMergeMethod: 'rebase' }, - pullNumber - ) - ).rejects.toThrowErrorMatchingInlineSnapshot( + it('should fail when enabling auto-merge via rebase because it is disallowed', async () => { + let errorMessage; + let isMissingStatusChecks; + + try { + await enablePullRequestAutoMerge( + { ...options, autoMergeMethod: 'rebase' }, + pullNumber + ); + } catch (e) { + const err = e as GithubV4Exception; + const res = parseGithubError(err); + errorMessage = err.message; + isMissingStatusChecks = res.isMissingStatusChecks; + } + + expect(isMissingStatusChecks).toBe(false); + expect(errorMessage).toMatchInlineSnapshot( `"[\\"Merge method rebase merging is not allowed on this repository\\"] (Github API v4)"` ); @@ -220,7 +234,7 @@ describe('enablePullRequestAutoMerge', () => { }); }); - describe('create a PR and attempt to enable auto-merge against "no-checks-required-branch"', () => { + describe('when creating a PR against "no-checks-required-branch"', () => { let pullNumber: number; let branchName: string; let octokit: Octokit; @@ -255,31 +269,29 @@ describe('enablePullRequestAutoMerge', () => { await resetReference(octokit); }); - it('should not be able to enable auto-merge', async () => { - await expect(() => - enablePullRequestAutoMerge( + it('should not be possible to enable auto-merge', async () => { + let isMissingStatusChecks; + let errorMessage; + try { + await enablePullRequestAutoMerge( { ...options, autoMergeMethod: 'merge' }, pullNumber - ) - ).rejects.toThrowErrorMatchingInlineSnapshot( - `"[\\"Pull request Pull request is in clean status\\"] (Github API v4)"` - ); - }); - - it('should not be able to enable auto-merge via merge', async () => { - await expect( - async () => - await enablePullRequestAutoMerge( - { ...options, autoMergeMethod: 'merge' }, - pullNumber - ) - ).rejects.toThrowErrorMatchingInlineSnapshot( + ); + } catch (e) { + const err = e as GithubV4Exception; + const res = parseGithubError(err); + errorMessage = err.message; + isMissingStatusChecks = res.isMissingStatusChecks; + } + + expect(errorMessage).toMatchInlineSnapshot( `"[\\"Pull request Pull request is in clean status\\"] (Github API v4)"` ); + expect(isMissingStatusChecks).toBe(true); }); }); - describe('create a PR and attempt to enable auto-merge against "status-checks-required-branch"', () => { + describe('when createing a PR against "status-checks-required-branch"', () => { let pullNumber: number; let branchName: string; let octokit: Octokit; @@ -314,27 +326,26 @@ describe('enablePullRequestAutoMerge', () => { await resetReference(octokit); }); - it('should not be able to enable auto-merge', async () => { - await expect(() => - enablePullRequestAutoMerge( + it('should not be possible to enable auto-merge', async () => { + let isMissingStatusChecks; + let errorMessage; + + try { + await enablePullRequestAutoMerge( { ...options, autoMergeMethod: 'merge' }, pullNumber - ) - ).rejects.toThrowErrorMatchingInlineSnapshot( - `"[\\"Pull request Pull request is in clean status\\"] (Github API v4)"` - ); - }); - - it('should not be able to enable auto-merge via merge', async () => { - await expect( - async () => - await enablePullRequestAutoMerge( - { ...options, autoMergeMethod: 'merge' }, - pullNumber - ) - ).rejects.toThrowErrorMatchingInlineSnapshot( + ); + } catch (e) { + const err = e as GithubV4Exception; + const res = parseGithubError(err); + errorMessage = err.message; + isMissingStatusChecks = res.isMissingStatusChecks; + } + + expect(errorMessage).toMatchInlineSnapshot( `"[\\"Pull request Pull request is in clean status\\"] (Github API v4)"` ); + expect(isMissingStatusChecks).toBe(true); }); }); }); diff --git a/src/lib/github/v4/enablePullRequestAutoMerge.ts b/src/lib/github/v4/enablePullRequestAutoMerge.ts index 8665a01c..269da456 100644 --- a/src/lib/github/v4/enablePullRequestAutoMerge.ts +++ b/src/lib/github/v4/enablePullRequestAutoMerge.ts @@ -1,7 +1,7 @@ import gql from 'graphql-tag'; import { ValidConfigOptions } from '../../../options/options'; import { fetchPullRequestId } from './FetchPullRequestId'; -import { apiRequestV4 } from './apiRequestV4'; +import { apiRequestV4, GithubV4Exception } from './apiRequestV4'; interface Response { enablePullRequestAutoMerge: { pullRequest?: { number: number } }; @@ -49,3 +49,16 @@ export async function enablePullRequestAutoMerge( return res.enablePullRequestAutoMerge.pullRequest?.number; } + +export function parseGithubError(e: GithubV4Exception) { + const isMissingStatusChecks = e.githubResponse.data.errors?.some( + (e) => + e.type === 'UNPROCESSABLE' && + (e.message.includes( + 'Branch does not have required protected branch rules' + ) || + e.message.includes('Pull request is in clean status')) + ); + + return { isMissingStatusChecks }; +}