From e686145618c166805c5e4a251c87275e060ce5ed Mon Sep 17 00:00:00 2001 From: Ricardo Souza Date: Thu, 30 Jul 2020 20:34:09 +0200 Subject: [PATCH 01/17] feat: add retry when trying to merge --- cSpell.json | 1 + src/common/merge.ts | 59 +++++++++++++++++---- src/eventHandlers/checkSuite/index.spec.ts | 16 +++--- src/eventHandlers/checkSuite/index.ts | 10 ++-- src/eventHandlers/pullRequest/index.spec.ts | 6 +-- src/eventHandlers/pullRequest/index.ts | 7 ++- src/eventHandlers/push/index.spec.ts | 14 ++--- src/eventHandlers/push/index.ts | 10 ++-- src/index.ts | 12 +++-- 9 files changed, 96 insertions(+), 39 deletions(-) diff --git a/cSpell.json b/cSpell.json index 4514d1d811..5fcbe9de5e 100644 --- a/cSpell.json +++ b/cSpell.json @@ -25,6 +25,7 @@ "language": "en-US", "version": "0.1", "words": [ + "backoff", "codeowners", "commitlint", "coverallsapp", diff --git a/src/common/merge.ts b/src/common/merge.ts index dd47a548a1..82495ef0e3 100644 --- a/src/common/merge.ts +++ b/src/common/merge.ts @@ -7,30 +7,53 @@ import { import { parseInputMergeMethod } from '../utilities/inputParsers'; import { logDebug, logInfo } from '../utilities/log'; +export interface PullRequestDetails { + commitHeadline: string; + pullRequestId: string; + reviewEdge: { node: { state: string } } | undefined; +} + +const EXPONENTIAL_BACKOFF = 2; +const WAIT_TIME_SECONDS = 1000; + +const delay = async (duration: number): Promise => { + return new Promise((resolve: () => void): void => { + setTimeout((): void => { + resolve(); + }, duration); + }); +}; + /** * Approves and merges a given Pull Request. */ export const merge = async ( octokit: ReturnType, - { - commitHeadline, - pullRequestId, - reviewEdge, - }: { - commitHeadline: string; - pullRequestId: string; - reviewEdge: { node: { state: string } } | undefined; - }, + pullRequestDetails: PullRequestDetails, ): Promise => { const mergeMethod = parseInputMergeMethod(); + const { commitHeadline, pullRequestId, reviewEdge } = pullRequestDetails; + const mutation = reviewEdge === undefined ? approveAndMergePullRequestMutation(mergeMethod) : mergePullRequestMutation(mergeMethod); + await octokit.graphql(mutation, { commitHeadline, pullRequestId }); +}; + +export const mergeWithRetry = async ( + octokit: ReturnType, + details: { + numberOfRetries: number; + trial: number; + } & PullRequestDetails, +): Promise => { + const { trial, numberOfRetries } = details; + try { - await octokit.graphql(mutation, { commitHeadline, pullRequestId }); + await merge(octokit, details); } catch (error) { logInfo( 'An error ocurred while merging the Pull Request. This is usually ' + @@ -40,5 +63,21 @@ export const merge = async ( ); /* eslint-disable-next-line @typescript-eslint/no-base-to-string */ logDebug(`Original error: ${(error as Error).toString()}.`); + + if (trial <= numberOfRetries) { + const nextRetryIn = trial ** EXPONENTIAL_BACKOFF * WAIT_TIME_SECONDS; + + logInfo(`Retrying in ${nextRetryIn.toString()}...`); + + await delay(nextRetryIn); + + await mergeWithRetry(octokit, { + ...details, + numberOfRetries, + trial: trial + 1, + }); + } else { + throw error; + } } }; diff --git a/src/eventHandlers/checkSuite/index.spec.ts b/src/eventHandlers/checkSuite/index.spec.ts index 61123a52a6..0be965a0df 100644 --- a/src/eventHandlers/checkSuite/index.spec.ts +++ b/src/eventHandlers/checkSuite/index.spec.ts @@ -61,7 +61,7 @@ describe('check Suite event handler', (): void => { }); nock('https://api.github.com').post('/graphql').reply(OK); - await checkSuiteHandle(octokit, 'dependabot-preview[bot]'); + await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3); expect(warningSpy).not.toHaveBeenCalled(); }); @@ -116,7 +116,7 @@ describe('check Suite event handler', (): void => { }) .reply(OK); - await checkSuiteHandle(octokit, 'dependabot-preview[bot]'); + await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3); }); it('does not approve pull requests that are not mergeable', async (): Promise< @@ -160,7 +160,7 @@ describe('check Suite event handler', (): void => { }, }); - await checkSuiteHandle(octokit, 'dependabot-preview[bot]'); + await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3); expect(infoSpy).toHaveBeenCalledWith( 'Pull request is not in a mergeable state: CONFLICTING.', @@ -208,7 +208,7 @@ describe('check Suite event handler', (): void => { }, }); - await checkSuiteHandle(octokit, 'dependabot-preview[bot]'); + await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3); expect(infoSpy).toHaveBeenCalledWith('Pull request is already merged.'); }); @@ -254,7 +254,7 @@ describe('check Suite event handler', (): void => { }, }); - await checkSuiteHandle(octokit, 'dependabot-preview[bot]'); + await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3); expect(infoSpy).toHaveBeenCalledWith( 'Pull request cannot be merged cleanly. Current state: UNKNOWN.', @@ -302,7 +302,7 @@ describe('check Suite event handler', (): void => { }, }); - await checkSuiteHandle(octokit, 'dependabot-preview[bot]'); + await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3); expect(infoSpy).toHaveBeenCalledWith('Pull request is not open: CLOSED.'); }); @@ -312,7 +312,7 @@ describe('check Suite event handler', (): void => { > => { expect.assertions(1); - await checkSuiteHandle(octokit, 'some-other-login'); + await checkSuiteHandle(octokit, 'some-other-login', 3); expect(infoSpy).toHaveBeenCalledWith( 'Pull request created by dependabot-preview[bot], not some-other-login, skipping.', @@ -334,7 +334,7 @@ describe('check Suite event handler', (): void => { }, }); - await checkSuiteHandle(octokit, 'dependabot-preview[bot]'); + await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3); expect(warningSpy).toHaveBeenCalled(); }); diff --git a/src/eventHandlers/checkSuite/index.ts b/src/eventHandlers/checkSuite/index.ts index fede3acd65..74a99c2935 100644 --- a/src/eventHandlers/checkSuite/index.ts +++ b/src/eventHandlers/checkSuite/index.ts @@ -2,7 +2,7 @@ import { context, getOctokit } from '@actions/github'; -import { merge } from '../../common/merge'; +import { mergeWithRetry } from '../../common/merge'; import { findPullRequestInfo as findPullRequestInformation } from '../../graphql/queries'; import { MergeableState, @@ -91,6 +91,7 @@ const getPullRequestInformation = async ( const tryMerge = async ( octokit: ReturnType, + numberOfRetries: number, { commitMessageHeadline, mergeStateStatus, @@ -122,10 +123,12 @@ const tryMerge = async ( } else if (pullRequestState !== 'OPEN') { logInfo(`Pull request is not open: ${pullRequestState}.`); } else { - await merge(octokit, { + await mergeWithRetry(octokit, { commitHeadline: commitMessageHeadline, + numberOfRetries, pullRequestId, reviewEdge: reviewEdges[0], + trial: 1, }); } }; @@ -133,6 +136,7 @@ const tryMerge = async ( export const checkSuiteHandle = async ( octokit: ReturnType, gitHubLogin: string, + numberOfReties: number, ): Promise => { const pullRequests = context.payload.check_suite.pull_requests as Array<{ number: number; @@ -168,7 +172,7 @@ export const checkSuiteHandle = async ( )}.`, ); - await tryMerge(octokit, pullRequestInformation); + await tryMerge(octokit, numberOfReties, pullRequestInformation); } } }; diff --git a/src/eventHandlers/pullRequest/index.spec.ts b/src/eventHandlers/pullRequest/index.spec.ts index 62e50d61c5..ceccc3f275 100644 --- a/src/eventHandlers/pullRequest/index.spec.ts +++ b/src/eventHandlers/pullRequest/index.spec.ts @@ -65,7 +65,7 @@ describe('pull request event handler', (): void => { }); nock('https://api.github.com').post('/graphql').reply(OK); - await pullRequestHandle(octokit, 'dependabot-preview[bot]'); + await pullRequestHandle(octokit, 'dependabot-preview[bot]', 3); expect(warningSpy).not.toHaveBeenCalled(); }); @@ -77,7 +77,7 @@ describe('pull request event handler', (): void => { data: null, }); - await pullRequestHandle(octokit, 'dependabot-preview[bot]'); + await pullRequestHandle(octokit, 'dependabot-preview[bot]', 3); }); it('does not approve an already approved pull request', async (): Promise< @@ -129,6 +129,6 @@ describe('pull request event handler', (): void => { }) .reply(OK); - await pullRequestHandle(octokit, 'dependabot-preview[bot]'); + await pullRequestHandle(octokit, 'dependabot-preview[bot]', 3); }); }); diff --git a/src/eventHandlers/pullRequest/index.ts b/src/eventHandlers/pullRequest/index.ts index 8383b58a5a..056a6737cc 100644 --- a/src/eventHandlers/pullRequest/index.ts +++ b/src/eventHandlers/pullRequest/index.ts @@ -1,6 +1,6 @@ import { context, getOctokit } from '@actions/github'; -import { merge } from '../../common/merge'; +import { mergeWithRetry } from '../../common/merge'; import { findPullRequestLastApprovedReview } from '../../graphql/queries'; import { ReviewEdges } from '../../types'; import { logInfo, logWarning } from '../../utilities/log'; @@ -52,6 +52,7 @@ const getPullRequestInformation = async ( export const pullRequestHandle = async ( octokit: ReturnType, gitHubLogin: string, + numberOfRetries: number, ): Promise => { const { repository, pull_request: pullRequest } = context.payload; @@ -86,10 +87,12 @@ export const pullRequestHandle = async ( )}.`, ); - await merge(octokit, { + await mergeWithRetry(octokit, { commitHeadline: pullRequest.title, + numberOfRetries, pullRequestId: pullRequest.node_id, reviewEdge: pullRequestInformation.reviewEdges[0], + trial: 1, }); } }; diff --git a/src/eventHandlers/push/index.spec.ts b/src/eventHandlers/push/index.spec.ts index ceb31e1c21..a434a8127e 100644 --- a/src/eventHandlers/push/index.spec.ts +++ b/src/eventHandlers/push/index.spec.ts @@ -59,7 +59,7 @@ describe('push event handler', (): void => { }); nock('https://api.github.com').post('/graphql').reply(OK); - await pushHandle(octokit, 'dependabot-preview[bot]'); + await pushHandle(octokit, 'dependabot-preview[bot]', 3); expect(warningSpy).not.toHaveBeenCalled(); }); @@ -106,7 +106,7 @@ describe('push event handler', (): void => { }) .reply(OK); - await pushHandle(octokit, 'dependabot-preview[bot]'); + await pushHandle(octokit, 'dependabot-preview[bot]', 3); }); it('does not approve pull requests that are not mergeable', async (): Promise< @@ -142,7 +142,7 @@ describe('push event handler', (): void => { }, }); - await pushHandle(octokit, 'dependabot-preview[bot]'); + await pushHandle(octokit, 'dependabot-preview[bot]', 3); expect(infoSpy).toHaveBeenCalledWith( 'Pull request is not in a mergeable state: CONFLICTING.', @@ -182,7 +182,7 @@ describe('push event handler', (): void => { }, }); - await pushHandle(octokit, 'dependabot-preview[bot]'); + await pushHandle(octokit, 'dependabot-preview[bot]', 3); expect(infoSpy).toHaveBeenCalledWith('Pull request is already merged.'); }); @@ -220,7 +220,7 @@ describe('push event handler', (): void => { }, }); - await pushHandle(octokit, 'dependabot-preview[bot]'); + await pushHandle(octokit, 'dependabot-preview[bot]', 3); expect(infoSpy).toHaveBeenCalledWith('Pull request is not open: CLOSED.'); }); @@ -230,7 +230,7 @@ describe('push event handler', (): void => { > => { expect.assertions(1); - await pushHandle(octokit, 'some-other-login'); + await pushHandle(octokit, 'some-other-login', 3); expect(infoSpy).toHaveBeenCalledWith( 'Pull request created by dependabot-preview[bot], not some-other-login, skipping.', @@ -254,7 +254,7 @@ describe('push event handler', (): void => { }, }); - await pushHandle(octokit, 'dependabot-preview[bot]'); + await pushHandle(octokit, 'dependabot-preview[bot]', 3); expect(warningSpy).toHaveBeenCalled(); }); diff --git a/src/eventHandlers/push/index.ts b/src/eventHandlers/push/index.ts index ea3d9efbf5..b407914657 100644 --- a/src/eventHandlers/push/index.ts +++ b/src/eventHandlers/push/index.ts @@ -1,6 +1,6 @@ import { context, getOctokit } from '@actions/github'; -import { merge } from '../../common/merge'; +import { mergeWithRetry } from '../../common/merge'; import { findPullRequestInfoAndReviews as findPullRequestInformationAndReviews } from '../../graphql/queries'; import { CommitMessageHeadlineGroup, @@ -78,6 +78,7 @@ const getPullRequestInformation = async ( const tryMerge = async ( octokit: ReturnType, + numberOfRetries: number, { commitMessageHeadline, mergeableState, @@ -94,10 +95,12 @@ const tryMerge = async ( } else if (pullRequestState !== 'OPEN') { logInfo(`Pull request is not open: ${pullRequestState}.`); } else { - await merge(octokit, { + await mergeWithRetry(octokit, { commitHeadline: commitMessageHeadline, + numberOfRetries, pullRequestId, reviewEdge: reviewEdges[0], + trial: 1, }); } }; @@ -105,6 +108,7 @@ const tryMerge = async ( export const pushHandle = async ( octokit: ReturnType, gitHubLogin: string, + numberOfRetries: number, ): Promise => { if (context.payload.pusher.name !== gitHubLogin) { logInfo( @@ -131,7 +135,7 @@ export const pushHandle = async ( )}.`, ); - await tryMerge(octokit, { + await tryMerge(octokit, numberOfRetries, { ...pullRequestInformation, commitMessageHeadline: getCommitMessageHeadline(), }); diff --git a/src/index.ts b/src/index.ts index c9c4d3500c..34b72296ff 100644 --- a/src/index.ts +++ b/src/index.ts @@ -8,8 +8,14 @@ import { } from './eventHandlers'; import { logInfo, logWarning } from './utilities/log'; +const DEFAULT_NUMBER_OF_RETRIES = 3; + const GITHUB_TOKEN = getInput('GITHUB_TOKEN'); const GITHUB_LOGIN = getInput('GITHUB_LOGIN'); +const NUMBER_OF_RETRIES = + getInput('NUMBER_OF_RETRIES') === undefined + ? DEFAULT_NUMBER_OF_RETRIES + : parseInt(getInput('NUMBER_OF_RETRIES'), 10); const octokit = getOctokit(GITHUB_TOKEN); @@ -18,11 +24,11 @@ const main = async (): Promise => { switch (context.eventName) { case 'check_suite': - return checkSuiteHandle(octokit, GITHUB_LOGIN); + return checkSuiteHandle(octokit, GITHUB_LOGIN, NUMBER_OF_RETRIES); case 'pull_request': - return pullRequestHandle(octokit, GITHUB_LOGIN); + return pullRequestHandle(octokit, GITHUB_LOGIN, NUMBER_OF_RETRIES); case 'push': - return pushHandle(octokit, GITHUB_LOGIN); + return pushHandle(octokit, GITHUB_LOGIN, NUMBER_OF_RETRIES); default: logWarning(`Unknown event ${context.eventName}, skipping.`); } From 8ed68295c9e0c7afe9f33275a33e7e8a086682e7 Mon Sep 17 00:00:00 2001 From: Ricardo Souza Date: Tue, 4 Aug 2020 11:26:38 +0200 Subject: [PATCH 02/17] chore: rename variable name --- src/common/merge.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/common/merge.ts b/src/common/merge.ts index 82495ef0e3..ad5f67c5e5 100644 --- a/src/common/merge.ts +++ b/src/common/merge.ts @@ -14,7 +14,7 @@ export interface PullRequestDetails { } const EXPONENTIAL_BACKOFF = 2; -const WAIT_TIME_SECONDS = 1000; +const DEFAULT_WAIT_TIME = 1000; const delay = async (duration: number): Promise => { return new Promise((resolve: () => void): void => { @@ -65,7 +65,7 @@ export const mergeWithRetry = async ( logDebug(`Original error: ${(error as Error).toString()}.`); if (trial <= numberOfRetries) { - const nextRetryIn = trial ** EXPONENTIAL_BACKOFF * WAIT_TIME_SECONDS; + const nextRetryIn = trial ** EXPONENTIAL_BACKOFF * DEFAULT_WAIT_TIME; logInfo(`Retrying in ${nextRetryIn.toString()}...`); From 9289fbb535adc4a70c36a3530bc106aa9f141849 Mon Sep 17 00:00:00 2001 From: Ricardo Souza Date: Tue, 4 Aug 2020 11:28:02 +0200 Subject: [PATCH 03/17] test: update tests --- src/eventHandlers/checkSuite/index.spec.ts | 62 +++++++++++++++++++++ src/eventHandlers/pullRequest/index.spec.ts | 61 ++++++++++++++++++++ src/eventHandlers/push/index.spec.ts | 54 ++++++++++++++++++ 3 files changed, 177 insertions(+) diff --git a/src/eventHandlers/checkSuite/index.spec.ts b/src/eventHandlers/checkSuite/index.spec.ts index 0be965a0df..e27b40ecc6 100644 --- a/src/eventHandlers/checkSuite/index.spec.ts +++ b/src/eventHandlers/checkSuite/index.spec.ts @@ -7,8 +7,10 @@ import { getOctokit } from '@actions/github'; import { OK } from 'http-status-codes'; import * as nock from 'nock'; +import * as merge from '../../common/merge'; import { mergePullRequestMutation } from '../../graphql/mutations'; import { AllowedMergeMethods } from '../../utilities/inputParsers'; +import * as log from '../../utilities/log'; import { checkSuiteHandle } from '.'; /* cspell:disable-next-line */ @@ -338,4 +340,64 @@ describe('check Suite event handler', (): void => { expect(warningSpy).toHaveBeenCalled(); }); + + it('retries up to two times before failing', async (): Promise => { + expect.assertions(6); + + nock('https://api.github.com') + .post('/graphql') + .reply(OK, { + data: { + repository: { + pullRequest: { + commits: { + edges: [ + { + node: { + commit: { + messageHeadline: COMMIT_HEADLINE, + }, + }, + }, + ], + }, + id: PULL_REQUEST_ID, + mergeStateStatus: 'CLEAN', + mergeable: 'MERGEABLE', + merged: false, + reviews: { + edges: [ + { + node: { + state: 'APPROVED', + }, + }, + ], + }, + state: 'OPEN', + }, + }, + }, + }); + + const mergeSpy = jest + .spyOn(merge, 'merge') + .mockImplementation() + .mockRejectedValue(new Error('Error when merging')); + const logDebugSpy = jest.spyOn(log, 'logDebug'); + const logInfoSpy = jest.spyOn(log, 'logInfo'); + + try { + await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 2); + } catch (error) { + expect(error).toBeInstanceOf(Error); + expect(error.message).toStrictEqual('Error when merging'); + expect(mergeSpy).toHaveBeenCalledTimes(3); + expect(logDebugSpy).toHaveBeenCalledTimes(3); + expect(logInfoSpy.mock.calls[1][0]).toStrictEqual( + 'An error ocurred while merging the Pull Request. This is usually caused by the base branch being out of sync with the target branch. In this case, the base branch must be rebased. Some tools, such as Dependabot, do that automatically.', + ); + expect(logInfoSpy.mock.calls[2][0]).toStrictEqual('Retrying in 1000...'); + } + }, 10000); }); diff --git a/src/eventHandlers/pullRequest/index.spec.ts b/src/eventHandlers/pullRequest/index.spec.ts index ceccc3f275..4ba9b5ad8d 100644 --- a/src/eventHandlers/pullRequest/index.spec.ts +++ b/src/eventHandlers/pullRequest/index.spec.ts @@ -7,8 +7,10 @@ import { getOctokit } from '@actions/github'; import { OK } from 'http-status-codes'; import * as nock from 'nock'; +import * as merge from '../../common/merge'; import { mergePullRequestMutation } from '../../graphql/mutations'; import { AllowedMergeMethods } from '../../utilities/inputParsers'; +import * as log from '../../utilities/log'; import { pullRequestHandle } from '.'; /* cspell:disable-next-line */ @@ -131,4 +133,63 @@ describe('pull request event handler', (): void => { await pullRequestHandle(octokit, 'dependabot-preview[bot]', 3); }); + + it('retries up to two times before failing', async (): Promise => { + expect.assertions(6); + + nock('https://api.github.com') + .post('/graphql') + .reply(OK, { + data: { + repository: { + pullRequest: { + commits: { + edges: [ + { + node: { + commit: { + message: COMMIT_HEADLINE, + }, + }, + }, + ], + }, + id: PULL_REQUEST_ID, + mergeable: 'MERGEABLE', + merged: false, + reviews: { + edges: [ + { + node: { + state: 'APPROVED', + }, + }, + ], + }, + state: 'OPEN', + }, + }, + }, + }); + + const mergeSpy = jest + .spyOn(merge, 'merge') + .mockImplementation() + .mockRejectedValue(new Error('Error when merging')); + const logDebugSpy = jest.spyOn(log, 'logDebug'); + const logInfoSpy = jest.spyOn(log, 'logInfo'); + + try { + await pullRequestHandle(octokit, 'dependabot-preview[bot]', 2); + } catch (error) { + expect(error).toBeInstanceOf(Error); + expect(error.message).toStrictEqual('Error when merging'); + expect(mergeSpy).toHaveBeenCalledTimes(3); + expect(logDebugSpy).toHaveBeenCalledTimes(3); + expect(logInfoSpy.mock.calls[1][0]).toStrictEqual( + 'An error ocurred while merging the Pull Request. This is usually caused by the base branch being out of sync with the target branch. In this case, the base branch must be rebased. Some tools, such as Dependabot, do that automatically.', + ); + expect(logInfoSpy.mock.calls[2][0]).toStrictEqual('Retrying in 1000...'); + } + }, 10000); }); diff --git a/src/eventHandlers/push/index.spec.ts b/src/eventHandlers/push/index.spec.ts index a434a8127e..a369501cc5 100644 --- a/src/eventHandlers/push/index.spec.ts +++ b/src/eventHandlers/push/index.spec.ts @@ -7,8 +7,10 @@ import { getOctokit } from '@actions/github'; import { OK } from 'http-status-codes'; import * as nock from 'nock'; +import * as merge from '../../common/merge'; import { mergePullRequestMutation } from '../../graphql/mutations'; import { AllowedMergeMethods } from '../../utilities/inputParsers'; +import * as log from '../../utilities/log'; import { pushHandle } from '.'; /* cspell:disable-next-line */ @@ -258,4 +260,56 @@ describe('push event handler', (): void => { expect(warningSpy).toHaveBeenCalled(); }); + + it('retries up to two times before failing', async (): Promise => { + expect.assertions(6); + + nock('https://api.github.com') + .post('/graphql') + .reply(OK, { + data: { + repository: { + pullRequests: { + nodes: [ + { + id: PULL_REQUEST_ID, + mergeable: 'MERGEABLE', + merged: false, + reviews: { + edges: [ + { + node: { + state: 'APPROVED', + }, + }, + ], + }, + state: 'OPEN', + }, + ], + }, + }, + }, + }); + + const mergeSpy = jest + .spyOn(merge, 'merge') + .mockImplementation() + .mockRejectedValue(new Error('Error when merging')); + const logDebugSpy = jest.spyOn(log, 'logDebug'); + const logInfoSpy = jest.spyOn(log, 'logInfo'); + + try { + await pushHandle(octokit, 'dependabot-preview[bot]', 2); + } catch (error) { + expect(error).toBeInstanceOf(Error); + expect(error.message).toStrictEqual('Error when merging'); + expect(mergeSpy).toHaveBeenCalledTimes(3); + expect(logDebugSpy).toHaveBeenCalledTimes(3); + expect(logInfoSpy.mock.calls[1][0]).toStrictEqual( + 'An error ocurred while merging the Pull Request. This is usually caused by the base branch being out of sync with the target branch. In this case, the base branch must be rebased. Some tools, such as Dependabot, do that automatically.', + ); + expect(logInfoSpy.mock.calls[2][0]).toStrictEqual('Retrying in 1000...'); + } + }, 10000); }); From c97bfc0c629b225c961f19138e7e07da53fe2601 Mon Sep 17 00:00:00 2001 From: Ricardo Souza Date: Tue, 4 Aug 2020 14:34:23 +0200 Subject: [PATCH 04/17] docs: add entry in readme file --- README.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/README.md b/README.md index 10b846c692..416e48871d 100644 --- a/README.md +++ b/README.md @@ -142,6 +142,24 @@ steps: MERGE_METHOD: MERGE ``` +### Number of retries + +In case the merge action fails, by default it will automatically be retried up +to three times using an exponential backoff strategy. This means, the first +retry will happen 1 second after the first failure, while the second will happen +4 seconds after the previous, the third 9 seconds, and so on. + +It's possible to configure the number of retries by providing a value for +`NUMBER_OF_RETRIES` (by default, the value is `3`). + +```yaml +steps: + - name: Merge me! + uses: ridedott/merge-me-action@master + with: + NUMBER_OF_RETRIES: 2 +``` + ## Getting Started These instructions will get you a copy of the project up and running on your From d1fdb79d59df1a3e026987f5548550a625e23f8f Mon Sep 17 00:00:00 2001 From: Ricardo Souza Date: Wed, 5 Aug 2020 11:05:41 +0200 Subject: [PATCH 05/17] chore: swap definition --- src/common/merge.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/common/merge.ts b/src/common/merge.ts index ad5f67c5e5..7f8934a01e 100644 --- a/src/common/merge.ts +++ b/src/common/merge.ts @@ -45,10 +45,10 @@ export const merge = async ( export const mergeWithRetry = async ( octokit: ReturnType, - details: { + details: PullRequestDetails & { numberOfRetries: number; trial: number; - } & PullRequestDetails, + }, ): Promise => { const { trial, numberOfRetries } = details; From 29742be82787b40fe9192a9a9308d6d91c9269cf Mon Sep 17 00:00:00 2001 From: Ricardo Souza Date: Wed, 5 Aug 2020 11:17:35 +0200 Subject: [PATCH 06/17] chore: rename 'trial' to 'retryCount' --- src/common/merge.ts | 10 +++++----- src/eventHandlers/checkSuite/index.ts | 2 +- src/eventHandlers/pullRequest/index.ts | 2 +- src/eventHandlers/push/index.ts | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/common/merge.ts b/src/common/merge.ts index 7f8934a01e..e355b9e67b 100644 --- a/src/common/merge.ts +++ b/src/common/merge.ts @@ -47,10 +47,10 @@ export const mergeWithRetry = async ( octokit: ReturnType, details: PullRequestDetails & { numberOfRetries: number; - trial: number; + retryCount: number; }, ): Promise => { - const { trial, numberOfRetries } = details; + const { retryCount, numberOfRetries } = details; try { await merge(octokit, details); @@ -64,8 +64,8 @@ export const mergeWithRetry = async ( /* eslint-disable-next-line @typescript-eslint/no-base-to-string */ logDebug(`Original error: ${(error as Error).toString()}.`); - if (trial <= numberOfRetries) { - const nextRetryIn = trial ** EXPONENTIAL_BACKOFF * DEFAULT_WAIT_TIME; + if (retryCount <= numberOfRetries) { + const nextRetryIn = retryCount ** EXPONENTIAL_BACKOFF * DEFAULT_WAIT_TIME; logInfo(`Retrying in ${nextRetryIn.toString()}...`); @@ -74,7 +74,7 @@ export const mergeWithRetry = async ( await mergeWithRetry(octokit, { ...details, numberOfRetries, - trial: trial + 1, + retryCount: retryCount + 1, }); } else { throw error; diff --git a/src/eventHandlers/checkSuite/index.ts b/src/eventHandlers/checkSuite/index.ts index 74a99c2935..b970a6f14f 100644 --- a/src/eventHandlers/checkSuite/index.ts +++ b/src/eventHandlers/checkSuite/index.ts @@ -127,8 +127,8 @@ const tryMerge = async ( commitHeadline: commitMessageHeadline, numberOfRetries, pullRequestId, + retryCount: 1, reviewEdge: reviewEdges[0], - trial: 1, }); } }; diff --git a/src/eventHandlers/pullRequest/index.ts b/src/eventHandlers/pullRequest/index.ts index 056a6737cc..6a7ee93c67 100644 --- a/src/eventHandlers/pullRequest/index.ts +++ b/src/eventHandlers/pullRequest/index.ts @@ -91,8 +91,8 @@ export const pullRequestHandle = async ( commitHeadline: pullRequest.title, numberOfRetries, pullRequestId: pullRequest.node_id, + retryCount: 1, reviewEdge: pullRequestInformation.reviewEdges[0], - trial: 1, }); } }; diff --git a/src/eventHandlers/push/index.ts b/src/eventHandlers/push/index.ts index b407914657..4409e64b3a 100644 --- a/src/eventHandlers/push/index.ts +++ b/src/eventHandlers/push/index.ts @@ -99,8 +99,8 @@ const tryMerge = async ( commitHeadline: commitMessageHeadline, numberOfRetries, pullRequestId, + retryCount: 1, reviewEdge: reviewEdges[0], - trial: 1, }); } }; From 7f874f6fc1df00bb91ce477b1c57a7572161833e Mon Sep 17 00:00:00 2001 From: Ricardo Souza Date: Wed, 5 Aug 2020 11:21:12 +0200 Subject: [PATCH 07/17] chore: rename `numberOfRetries` to `maximumRetries` --- README.md | 4 ++-- src/common/merge.ts | 8 ++++---- src/eventHandlers/checkSuite/index.ts | 4 ++-- src/eventHandlers/pullRequest/index.ts | 4 ++-- src/eventHandlers/push/index.ts | 8 ++++---- src/index.ts | 16 ++++++++-------- 6 files changed, 22 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 416e48871d..f6115fa404 100644 --- a/README.md +++ b/README.md @@ -150,14 +150,14 @@ retry will happen 1 second after the first failure, while the second will happen 4 seconds after the previous, the third 9 seconds, and so on. It's possible to configure the number of retries by providing a value for -`NUMBER_OF_RETRIES` (by default, the value is `3`). +`MAXIMUM_RETRIES` (by default, the value is `3`). ```yaml steps: - name: Merge me! uses: ridedott/merge-me-action@master with: - NUMBER_OF_RETRIES: 2 + MAXIMUM_RETRIES: 2 ``` ## Getting Started diff --git a/src/common/merge.ts b/src/common/merge.ts index e355b9e67b..c18fb38aec 100644 --- a/src/common/merge.ts +++ b/src/common/merge.ts @@ -46,11 +46,11 @@ export const merge = async ( export const mergeWithRetry = async ( octokit: ReturnType, details: PullRequestDetails & { - numberOfRetries: number; + maximumRetries: number; retryCount: number; }, ): Promise => { - const { retryCount, numberOfRetries } = details; + const { retryCount, maximumRetries } = details; try { await merge(octokit, details); @@ -64,7 +64,7 @@ export const mergeWithRetry = async ( /* eslint-disable-next-line @typescript-eslint/no-base-to-string */ logDebug(`Original error: ${(error as Error).toString()}.`); - if (retryCount <= numberOfRetries) { + if (retryCount <= maximumRetries) { const nextRetryIn = retryCount ** EXPONENTIAL_BACKOFF * DEFAULT_WAIT_TIME; logInfo(`Retrying in ${nextRetryIn.toString()}...`); @@ -73,7 +73,7 @@ export const mergeWithRetry = async ( await mergeWithRetry(octokit, { ...details, - numberOfRetries, + maximumRetries, retryCount: retryCount + 1, }); } else { diff --git a/src/eventHandlers/checkSuite/index.ts b/src/eventHandlers/checkSuite/index.ts index b970a6f14f..3ce938986a 100644 --- a/src/eventHandlers/checkSuite/index.ts +++ b/src/eventHandlers/checkSuite/index.ts @@ -91,7 +91,7 @@ const getPullRequestInformation = async ( const tryMerge = async ( octokit: ReturnType, - numberOfRetries: number, + maximumRetries: number, { commitMessageHeadline, mergeStateStatus, @@ -125,7 +125,7 @@ const tryMerge = async ( } else { await mergeWithRetry(octokit, { commitHeadline: commitMessageHeadline, - numberOfRetries, + maximumRetries, pullRequestId, retryCount: 1, reviewEdge: reviewEdges[0], diff --git a/src/eventHandlers/pullRequest/index.ts b/src/eventHandlers/pullRequest/index.ts index 6a7ee93c67..c62b86f297 100644 --- a/src/eventHandlers/pullRequest/index.ts +++ b/src/eventHandlers/pullRequest/index.ts @@ -52,7 +52,7 @@ const getPullRequestInformation = async ( export const pullRequestHandle = async ( octokit: ReturnType, gitHubLogin: string, - numberOfRetries: number, + maximumRetries: number, ): Promise => { const { repository, pull_request: pullRequest } = context.payload; @@ -89,7 +89,7 @@ export const pullRequestHandle = async ( await mergeWithRetry(octokit, { commitHeadline: pullRequest.title, - numberOfRetries, + maximumRetries, pullRequestId: pullRequest.node_id, retryCount: 1, reviewEdge: pullRequestInformation.reviewEdges[0], diff --git a/src/eventHandlers/push/index.ts b/src/eventHandlers/push/index.ts index 4409e64b3a..1d0eb6bd16 100644 --- a/src/eventHandlers/push/index.ts +++ b/src/eventHandlers/push/index.ts @@ -78,7 +78,7 @@ const getPullRequestInformation = async ( const tryMerge = async ( octokit: ReturnType, - numberOfRetries: number, + maximumRetries: number, { commitMessageHeadline, mergeableState, @@ -97,7 +97,7 @@ const tryMerge = async ( } else { await mergeWithRetry(octokit, { commitHeadline: commitMessageHeadline, - numberOfRetries, + maximumRetries, pullRequestId, retryCount: 1, reviewEdge: reviewEdges[0], @@ -108,7 +108,7 @@ const tryMerge = async ( export const pushHandle = async ( octokit: ReturnType, gitHubLogin: string, - numberOfRetries: number, + maximumRetries: number, ): Promise => { if (context.payload.pusher.name !== gitHubLogin) { logInfo( @@ -135,7 +135,7 @@ export const pushHandle = async ( )}.`, ); - await tryMerge(octokit, numberOfRetries, { + await tryMerge(octokit, maximumRetries, { ...pullRequestInformation, commitMessageHeadline: getCommitMessageHeadline(), }); diff --git a/src/index.ts b/src/index.ts index 34b72296ff..5549eed984 100644 --- a/src/index.ts +++ b/src/index.ts @@ -8,14 +8,14 @@ import { } from './eventHandlers'; import { logInfo, logWarning } from './utilities/log'; -const DEFAULT_NUMBER_OF_RETRIES = 3; +const DEFAULT_MAXIMUM_RETRIES = 3; const GITHUB_TOKEN = getInput('GITHUB_TOKEN'); const GITHUB_LOGIN = getInput('GITHUB_LOGIN'); -const NUMBER_OF_RETRIES = - getInput('NUMBER_OF_RETRIES') === undefined - ? DEFAULT_NUMBER_OF_RETRIES - : parseInt(getInput('NUMBER_OF_RETRIES'), 10); +const MAXIMUM_RETRIES = + getInput('MAXIMUM_RETRIES') === undefined + ? DEFAULT_MAXIMUM_RETRIES + : parseInt(getInput('MAXIMUM_RETRIES'), 10); const octokit = getOctokit(GITHUB_TOKEN); @@ -24,11 +24,11 @@ const main = async (): Promise => { switch (context.eventName) { case 'check_suite': - return checkSuiteHandle(octokit, GITHUB_LOGIN, NUMBER_OF_RETRIES); + return checkSuiteHandle(octokit, GITHUB_LOGIN, MAXIMUM_RETRIES); case 'pull_request': - return pullRequestHandle(octokit, GITHUB_LOGIN, NUMBER_OF_RETRIES); + return pullRequestHandle(octokit, GITHUB_LOGIN, MAXIMUM_RETRIES); case 'push': - return pushHandle(octokit, GITHUB_LOGIN, NUMBER_OF_RETRIES); + return pushHandle(octokit, GITHUB_LOGIN, MAXIMUM_RETRIES); default: logWarning(`Unknown event ${context.eventName}, skipping.`); } From e849d1028e6e46bfd4e0269b92d0552bb6b06e4e Mon Sep 17 00:00:00 2001 From: Ricardo Souza Date: Wed, 5 Aug 2020 16:37:50 +0200 Subject: [PATCH 08/17] test: add check on the retrying log messages (to ensure the backoff happens in the correct time) --- src/eventHandlers/checkSuite/index.spec.ts | 3 ++- src/eventHandlers/pullRequest/index.spec.ts | 3 ++- src/eventHandlers/push/index.spec.ts | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/eventHandlers/checkSuite/index.spec.ts b/src/eventHandlers/checkSuite/index.spec.ts index e27b40ecc6..0bdf619988 100644 --- a/src/eventHandlers/checkSuite/index.spec.ts +++ b/src/eventHandlers/checkSuite/index.spec.ts @@ -342,7 +342,7 @@ describe('check Suite event handler', (): void => { }); it('retries up to two times before failing', async (): Promise => { - expect.assertions(6); + expect.assertions(7); nock('https://api.github.com') .post('/graphql') @@ -398,6 +398,7 @@ describe('check Suite event handler', (): void => { 'An error ocurred while merging the Pull Request. This is usually caused by the base branch being out of sync with the target branch. In this case, the base branch must be rebased. Some tools, such as Dependabot, do that automatically.', ); expect(logInfoSpy.mock.calls[2][0]).toStrictEqual('Retrying in 1000...'); + expect(logInfoSpy.mock.calls[4][0]).toStrictEqual('Retrying in 4000...'); } }, 10000); }); diff --git a/src/eventHandlers/pullRequest/index.spec.ts b/src/eventHandlers/pullRequest/index.spec.ts index 4ba9b5ad8d..eb5c60edf5 100644 --- a/src/eventHandlers/pullRequest/index.spec.ts +++ b/src/eventHandlers/pullRequest/index.spec.ts @@ -135,7 +135,7 @@ describe('pull request event handler', (): void => { }); it('retries up to two times before failing', async (): Promise => { - expect.assertions(6); + expect.assertions(7); nock('https://api.github.com') .post('/graphql') @@ -190,6 +190,7 @@ describe('pull request event handler', (): void => { 'An error ocurred while merging the Pull Request. This is usually caused by the base branch being out of sync with the target branch. In this case, the base branch must be rebased. Some tools, such as Dependabot, do that automatically.', ); expect(logInfoSpy.mock.calls[2][0]).toStrictEqual('Retrying in 1000...'); + expect(logInfoSpy.mock.calls[4][0]).toStrictEqual('Retrying in 4000...'); } }, 10000); }); diff --git a/src/eventHandlers/push/index.spec.ts b/src/eventHandlers/push/index.spec.ts index a369501cc5..7ab8d268e6 100644 --- a/src/eventHandlers/push/index.spec.ts +++ b/src/eventHandlers/push/index.spec.ts @@ -262,7 +262,7 @@ describe('push event handler', (): void => { }); it('retries up to two times before failing', async (): Promise => { - expect.assertions(6); + expect.assertions(7); nock('https://api.github.com') .post('/graphql') @@ -310,6 +310,7 @@ describe('push event handler', (): void => { 'An error ocurred while merging the Pull Request. This is usually caused by the base branch being out of sync with the target branch. In this case, the base branch must be rebased. Some tools, such as Dependabot, do that automatically.', ); expect(logInfoSpy.mock.calls[2][0]).toStrictEqual('Retrying in 1000...'); + expect(logInfoSpy.mock.calls[4][0]).toStrictEqual('Retrying in 4000...'); } }, 10000); }); From e28bc570e715801e4bece323301d72f8553901ff Mon Sep 17 00:00:00 2001 From: Ricardo Souza Date: Mon, 10 Aug 2020 10:34:23 +0200 Subject: [PATCH 09/17] test: mock endpoint instead of `merge` function; removes `export` frun `merge` function --- src/common/merge.ts | 2 +- src/eventHandlers/checkSuite/index.spec.ts | 12 ++++++------ src/eventHandlers/pullRequest/index.spec.ts | 12 ++++++------ src/eventHandlers/push/index.spec.ts | 12 ++++++------ 4 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/common/merge.ts b/src/common/merge.ts index c18fb38aec..ae3d288015 100644 --- a/src/common/merge.ts +++ b/src/common/merge.ts @@ -27,7 +27,7 @@ const delay = async (duration: number): Promise => { /** * Approves and merges a given Pull Request. */ -export const merge = async ( +const merge = async ( octokit: ReturnType, pullRequestDetails: PullRequestDetails, ): Promise => { diff --git a/src/eventHandlers/checkSuite/index.spec.ts b/src/eventHandlers/checkSuite/index.spec.ts index 0bdf619988..0e4f99d7b6 100644 --- a/src/eventHandlers/checkSuite/index.spec.ts +++ b/src/eventHandlers/checkSuite/index.spec.ts @@ -378,12 +378,12 @@ describe('check Suite event handler', (): void => { }, }, }, - }); + }) + .post('/graphql') + .times(3) + .reply(403, 'Error when merging'); - const mergeSpy = jest - .spyOn(merge, 'merge') - .mockImplementation() - .mockRejectedValue(new Error('Error when merging')); + const mergeWithRetrySpy = jest.spyOn(merge, 'mergeWithRetry'); const logDebugSpy = jest.spyOn(log, 'logDebug'); const logInfoSpy = jest.spyOn(log, 'logInfo'); @@ -392,7 +392,7 @@ describe('check Suite event handler', (): void => { } catch (error) { expect(error).toBeInstanceOf(Error); expect(error.message).toStrictEqual('Error when merging'); - expect(mergeSpy).toHaveBeenCalledTimes(3); + expect(mergeWithRetrySpy).toHaveBeenCalledTimes(3); expect(logDebugSpy).toHaveBeenCalledTimes(3); expect(logInfoSpy.mock.calls[1][0]).toStrictEqual( 'An error ocurred while merging the Pull Request. This is usually caused by the base branch being out of sync with the target branch. In this case, the base branch must be rebased. Some tools, such as Dependabot, do that automatically.', diff --git a/src/eventHandlers/pullRequest/index.spec.ts b/src/eventHandlers/pullRequest/index.spec.ts index eb5c60edf5..a1f0d4640e 100644 --- a/src/eventHandlers/pullRequest/index.spec.ts +++ b/src/eventHandlers/pullRequest/index.spec.ts @@ -170,12 +170,12 @@ describe('pull request event handler', (): void => { }, }, }, - }); + }) + .post('/graphql') + .times(3) + .reply(403, 'Error when merging'); - const mergeSpy = jest - .spyOn(merge, 'merge') - .mockImplementation() - .mockRejectedValue(new Error('Error when merging')); + const mergeWithRetrySpy = jest.spyOn(merge, 'mergeWithRetry'); const logDebugSpy = jest.spyOn(log, 'logDebug'); const logInfoSpy = jest.spyOn(log, 'logInfo'); @@ -184,7 +184,7 @@ describe('pull request event handler', (): void => { } catch (error) { expect(error).toBeInstanceOf(Error); expect(error.message).toStrictEqual('Error when merging'); - expect(mergeSpy).toHaveBeenCalledTimes(3); + expect(mergeWithRetrySpy).toHaveBeenCalledTimes(3); expect(logDebugSpy).toHaveBeenCalledTimes(3); expect(logInfoSpy.mock.calls[1][0]).toStrictEqual( 'An error ocurred while merging the Pull Request. This is usually caused by the base branch being out of sync with the target branch. In this case, the base branch must be rebased. Some tools, such as Dependabot, do that automatically.', diff --git a/src/eventHandlers/push/index.spec.ts b/src/eventHandlers/push/index.spec.ts index 7ab8d268e6..682c8a7079 100644 --- a/src/eventHandlers/push/index.spec.ts +++ b/src/eventHandlers/push/index.spec.ts @@ -290,12 +290,12 @@ describe('push event handler', (): void => { }, }, }, - }); + }) + .post('/graphql') + .times(3) + .reply(403, 'Error when merging'); - const mergeSpy = jest - .spyOn(merge, 'merge') - .mockImplementation() - .mockRejectedValue(new Error('Error when merging')); + const mergeWithRetrySpy = jest.spyOn(merge, 'mergeWithRetry'); const logDebugSpy = jest.spyOn(log, 'logDebug'); const logInfoSpy = jest.spyOn(log, 'logInfo'); @@ -304,7 +304,7 @@ describe('push event handler', (): void => { } catch (error) { expect(error).toBeInstanceOf(Error); expect(error.message).toStrictEqual('Error when merging'); - expect(mergeSpy).toHaveBeenCalledTimes(3); + expect(mergeWithRetrySpy).toHaveBeenCalledTimes(3); expect(logDebugSpy).toHaveBeenCalledTimes(3); expect(logInfoSpy.mock.calls[1][0]).toStrictEqual( 'An error ocurred while merging the Pull Request. This is usually caused by the base branch being out of sync with the target branch. In this case, the base branch must be rebased. Some tools, such as Dependabot, do that automatically.', From ef0cedc3674f849c0f0120c0d0d990d2f28537f1 Mon Sep 17 00:00:00 2001 From: Ricardo Souza Date: Fri, 14 Aug 2020 09:01:04 +0200 Subject: [PATCH 10/17] feat: add internal parameter for the minimum wait time in merge --- src/common/merge.ts | 9 ++-- src/eventHandlers/checkSuite/index.spec.ts | 51 ++++++++++++++++----- src/eventHandlers/checkSuite/index.ts | 23 ++++++++-- src/eventHandlers/pullRequest/index.spec.ts | 26 ++++++++--- src/eventHandlers/pullRequest/index.ts | 9 +++- src/eventHandlers/push/index.spec.ts | 45 +++++++++++++----- src/eventHandlers/push/index.ts | 29 +++++++++--- 7 files changed, 149 insertions(+), 43 deletions(-) diff --git a/src/common/merge.ts b/src/common/merge.ts index ae3d288015..6c60701fc9 100644 --- a/src/common/merge.ts +++ b/src/common/merge.ts @@ -14,7 +14,7 @@ export interface PullRequestDetails { } const EXPONENTIAL_BACKOFF = 2; -const DEFAULT_WAIT_TIME = 1000; +const MINIMUM_WAIT_TIME = 1000; const delay = async (duration: number): Promise => { return new Promise((resolve: () => void): void => { @@ -47,10 +47,11 @@ export const mergeWithRetry = async ( octokit: ReturnType, details: PullRequestDetails & { maximumRetries: number; + minimumWaitTime?: number; retryCount: number; }, ): Promise => { - const { retryCount, maximumRetries } = details; + const { retryCount, maximumRetries, minimumWaitTime } = details; try { await merge(octokit, details); @@ -65,7 +66,9 @@ export const mergeWithRetry = async ( logDebug(`Original error: ${(error as Error).toString()}.`); if (retryCount <= maximumRetries) { - const nextRetryIn = retryCount ** EXPONENTIAL_BACKOFF * DEFAULT_WAIT_TIME; + const nextRetryIn = + retryCount ** EXPONENTIAL_BACKOFF * + (minimumWaitTime === undefined ? MINIMUM_WAIT_TIME : minimumWaitTime); logInfo(`Retrying in ${nextRetryIn.toString()}...`); diff --git a/src/eventHandlers/checkSuite/index.spec.ts b/src/eventHandlers/checkSuite/index.spec.ts index 0e4f99d7b6..b2cdab9b63 100644 --- a/src/eventHandlers/checkSuite/index.spec.ts +++ b/src/eventHandlers/checkSuite/index.spec.ts @@ -63,7 +63,10 @@ describe('check Suite event handler', (): void => { }); nock('https://api.github.com').post('/graphql').reply(OK); - await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3); + await checkSuiteHandle(octokit, 'dependabot-preview[bot]', { + maximumRetries: 3, + minimumWaitTime: 100, + }); expect(warningSpy).not.toHaveBeenCalled(); }); @@ -118,7 +121,10 @@ describe('check Suite event handler', (): void => { }) .reply(OK); - await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3); + await checkSuiteHandle(octokit, 'dependabot-preview[bot]', { + maximumRetries: 3, + minimumWaitTime: 100, + }); }); it('does not approve pull requests that are not mergeable', async (): Promise< @@ -162,7 +168,10 @@ describe('check Suite event handler', (): void => { }, }); - await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3); + await checkSuiteHandle(octokit, 'dependabot-preview[bot]', { + maximumRetries: 3, + minimumWaitTime: 100, + }); expect(infoSpy).toHaveBeenCalledWith( 'Pull request is not in a mergeable state: CONFLICTING.', @@ -210,7 +219,10 @@ describe('check Suite event handler', (): void => { }, }); - await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3); + await checkSuiteHandle(octokit, 'dependabot-preview[bot]', { + maximumRetries: 3, + minimumWaitTime: 100, + }); expect(infoSpy).toHaveBeenCalledWith('Pull request is already merged.'); }); @@ -256,7 +268,10 @@ describe('check Suite event handler', (): void => { }, }); - await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3); + await checkSuiteHandle(octokit, 'dependabot-preview[bot]', { + maximumRetries: 3, + minimumWaitTime: 100, + }); expect(infoSpy).toHaveBeenCalledWith( 'Pull request cannot be merged cleanly. Current state: UNKNOWN.', @@ -304,7 +319,10 @@ describe('check Suite event handler', (): void => { }, }); - await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3); + await checkSuiteHandle(octokit, 'dependabot-preview[bot]', { + maximumRetries: 3, + minimumWaitTime: 100, + }); expect(infoSpy).toHaveBeenCalledWith('Pull request is not open: CLOSED.'); }); @@ -314,7 +332,10 @@ describe('check Suite event handler', (): void => { > => { expect.assertions(1); - await checkSuiteHandle(octokit, 'some-other-login', 3); + await checkSuiteHandle(octokit, 'some-other-login', { + maximumRetries: 3, + minimumWaitTime: 100, + }); expect(infoSpy).toHaveBeenCalledWith( 'Pull request created by dependabot-preview[bot], not some-other-login, skipping.', @@ -336,7 +357,10 @@ describe('check Suite event handler', (): void => { }, }); - await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3); + await checkSuiteHandle(octokit, 'dependabot-preview[bot]', { + maximumRetries: 3, + minimumWaitTime: 100, + }); expect(warningSpy).toHaveBeenCalled(); }); @@ -388,7 +412,10 @@ describe('check Suite event handler', (): void => { const logInfoSpy = jest.spyOn(log, 'logInfo'); try { - await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 2); + await checkSuiteHandle(octokit, 'dependabot-preview[bot]', { + maximumRetries: 2, + minimumWaitTime: 100, + }); } catch (error) { expect(error).toBeInstanceOf(Error); expect(error.message).toStrictEqual('Error when merging'); @@ -397,8 +424,8 @@ describe('check Suite event handler', (): void => { expect(logInfoSpy.mock.calls[1][0]).toStrictEqual( 'An error ocurred while merging the Pull Request. This is usually caused by the base branch being out of sync with the target branch. In this case, the base branch must be rebased. Some tools, such as Dependabot, do that automatically.', ); - expect(logInfoSpy.mock.calls[2][0]).toStrictEqual('Retrying in 1000...'); - expect(logInfoSpy.mock.calls[4][0]).toStrictEqual('Retrying in 4000...'); + expect(logInfoSpy.mock.calls[2][0]).toStrictEqual('Retrying in 100...'); + expect(logInfoSpy.mock.calls[4][0]).toStrictEqual('Retrying in 400...'); } - }, 10000); + }); }); diff --git a/src/eventHandlers/checkSuite/index.ts b/src/eventHandlers/checkSuite/index.ts index 3ce938986a..51e351857b 100644 --- a/src/eventHandlers/checkSuite/index.ts +++ b/src/eventHandlers/checkSuite/index.ts @@ -91,7 +91,13 @@ const getPullRequestInformation = async ( const tryMerge = async ( octokit: ReturnType, - maximumRetries: number, + { + maximumRetries, + minimumWaitTime, + }: { + maximumRetries: number; + minimumWaitTime?: number; + }, { commitMessageHeadline, mergeStateStatus, @@ -126,6 +132,7 @@ const tryMerge = async ( await mergeWithRetry(octokit, { commitHeadline: commitMessageHeadline, maximumRetries, + minimumWaitTime, pullRequestId, retryCount: 1, reviewEdge: reviewEdges[0], @@ -136,7 +143,13 @@ const tryMerge = async ( export const checkSuiteHandle = async ( octokit: ReturnType, gitHubLogin: string, - numberOfReties: number, + { + maximumRetries, + minimumWaitTime, + }: { + maximumRetries: number; + minimumWaitTime?: number; + }, ): Promise => { const pullRequests = context.payload.check_suite.pull_requests as Array<{ number: number; @@ -172,7 +185,11 @@ export const checkSuiteHandle = async ( )}.`, ); - await tryMerge(octokit, numberOfReties, pullRequestInformation); + await tryMerge( + octokit, + { maximumRetries, minimumWaitTime }, + pullRequestInformation, + ); } } }; diff --git a/src/eventHandlers/pullRequest/index.spec.ts b/src/eventHandlers/pullRequest/index.spec.ts index a1f0d4640e..e0eb6fbdc9 100644 --- a/src/eventHandlers/pullRequest/index.spec.ts +++ b/src/eventHandlers/pullRequest/index.spec.ts @@ -67,7 +67,10 @@ describe('pull request event handler', (): void => { }); nock('https://api.github.com').post('/graphql').reply(OK); - await pullRequestHandle(octokit, 'dependabot-preview[bot]', 3); + await pullRequestHandle(octokit, 'dependabot-preview[bot]', { + maximumRetries: 2, + minimumWaitTime: 100, + }); expect(warningSpy).not.toHaveBeenCalled(); }); @@ -79,7 +82,10 @@ describe('pull request event handler', (): void => { data: null, }); - await pullRequestHandle(octokit, 'dependabot-preview[bot]', 3); + await pullRequestHandle(octokit, 'dependabot-preview[bot]', { + maximumRetries: 2, + minimumWaitTime: 100, + }); }); it('does not approve an already approved pull request', async (): Promise< @@ -131,7 +137,10 @@ describe('pull request event handler', (): void => { }) .reply(OK); - await pullRequestHandle(octokit, 'dependabot-preview[bot]', 3); + await pullRequestHandle(octokit, 'dependabot-preview[bot]', { + maximumRetries: 2, + minimumWaitTime: 100, + }); }); it('retries up to two times before failing', async (): Promise => { @@ -180,7 +189,10 @@ describe('pull request event handler', (): void => { const logInfoSpy = jest.spyOn(log, 'logInfo'); try { - await pullRequestHandle(octokit, 'dependabot-preview[bot]', 2); + await pullRequestHandle(octokit, 'dependabot-preview[bot]', { + maximumRetries: 2, + minimumWaitTime: 100, + }); } catch (error) { expect(error).toBeInstanceOf(Error); expect(error.message).toStrictEqual('Error when merging'); @@ -189,8 +201,8 @@ describe('pull request event handler', (): void => { expect(logInfoSpy.mock.calls[1][0]).toStrictEqual( 'An error ocurred while merging the Pull Request. This is usually caused by the base branch being out of sync with the target branch. In this case, the base branch must be rebased. Some tools, such as Dependabot, do that automatically.', ); - expect(logInfoSpy.mock.calls[2][0]).toStrictEqual('Retrying in 1000...'); - expect(logInfoSpy.mock.calls[4][0]).toStrictEqual('Retrying in 4000...'); + expect(logInfoSpy.mock.calls[2][0]).toStrictEqual('Retrying in 100...'); + expect(logInfoSpy.mock.calls[4][0]).toStrictEqual('Retrying in 400...'); } - }, 10000); + }); }); diff --git a/src/eventHandlers/pullRequest/index.ts b/src/eventHandlers/pullRequest/index.ts index c62b86f297..d10df03d84 100644 --- a/src/eventHandlers/pullRequest/index.ts +++ b/src/eventHandlers/pullRequest/index.ts @@ -52,7 +52,13 @@ const getPullRequestInformation = async ( export const pullRequestHandle = async ( octokit: ReturnType, gitHubLogin: string, - maximumRetries: number, + { + maximumRetries, + minimumWaitTime, + }: { + maximumRetries: number; + minimumWaitTime?: number; + }, ): Promise => { const { repository, pull_request: pullRequest } = context.payload; @@ -90,6 +96,7 @@ export const pullRequestHandle = async ( await mergeWithRetry(octokit, { commitHeadline: pullRequest.title, maximumRetries, + minimumWaitTime, pullRequestId: pullRequest.node_id, retryCount: 1, reviewEdge: pullRequestInformation.reviewEdges[0], diff --git a/src/eventHandlers/push/index.spec.ts b/src/eventHandlers/push/index.spec.ts index 682c8a7079..67cf600988 100644 --- a/src/eventHandlers/push/index.spec.ts +++ b/src/eventHandlers/push/index.spec.ts @@ -61,7 +61,10 @@ describe('push event handler', (): void => { }); nock('https://api.github.com').post('/graphql').reply(OK); - await pushHandle(octokit, 'dependabot-preview[bot]', 3); + await pushHandle(octokit, 'dependabot-preview[bot]', { + maximumRetries: 2, + minimumWaitTime: 100, + }); expect(warningSpy).not.toHaveBeenCalled(); }); @@ -108,7 +111,10 @@ describe('push event handler', (): void => { }) .reply(OK); - await pushHandle(octokit, 'dependabot-preview[bot]', 3); + await pushHandle(octokit, 'dependabot-preview[bot]', { + maximumRetries: 2, + minimumWaitTime: 100, + }); }); it('does not approve pull requests that are not mergeable', async (): Promise< @@ -144,7 +150,10 @@ describe('push event handler', (): void => { }, }); - await pushHandle(octokit, 'dependabot-preview[bot]', 3); + await pushHandle(octokit, 'dependabot-preview[bot]', { + maximumRetries: 2, + minimumWaitTime: 100, + }); expect(infoSpy).toHaveBeenCalledWith( 'Pull request is not in a mergeable state: CONFLICTING.', @@ -184,7 +193,10 @@ describe('push event handler', (): void => { }, }); - await pushHandle(octokit, 'dependabot-preview[bot]', 3); + await pushHandle(octokit, 'dependabot-preview[bot]', { + maximumRetries: 2, + minimumWaitTime: 100, + }); expect(infoSpy).toHaveBeenCalledWith('Pull request is already merged.'); }); @@ -222,7 +234,10 @@ describe('push event handler', (): void => { }, }); - await pushHandle(octokit, 'dependabot-preview[bot]', 3); + await pushHandle(octokit, 'dependabot-preview[bot]', { + maximumRetries: 2, + minimumWaitTime: 100, + }); expect(infoSpy).toHaveBeenCalledWith('Pull request is not open: CLOSED.'); }); @@ -232,7 +247,9 @@ describe('push event handler', (): void => { > => { expect.assertions(1); - await pushHandle(octokit, 'some-other-login', 3); + await pushHandle(octokit, 'some-other-login', { + maximumRetries: 2, + }); expect(infoSpy).toHaveBeenCalledWith( 'Pull request created by dependabot-preview[bot], not some-other-login, skipping.', @@ -256,7 +273,10 @@ describe('push event handler', (): void => { }, }); - await pushHandle(octokit, 'dependabot-preview[bot]', 3); + await pushHandle(octokit, 'dependabot-preview[bot]', { + maximumRetries: 2, + minimumWaitTime: 100, + }); expect(warningSpy).toHaveBeenCalled(); }); @@ -300,7 +320,10 @@ describe('push event handler', (): void => { const logInfoSpy = jest.spyOn(log, 'logInfo'); try { - await pushHandle(octokit, 'dependabot-preview[bot]', 2); + await pushHandle(octokit, 'dependabot-preview[bot]', { + maximumRetries: 2, + minimumWaitTime: 100, + }); } catch (error) { expect(error).toBeInstanceOf(Error); expect(error.message).toStrictEqual('Error when merging'); @@ -309,8 +332,8 @@ describe('push event handler', (): void => { expect(logInfoSpy.mock.calls[1][0]).toStrictEqual( 'An error ocurred while merging the Pull Request. This is usually caused by the base branch being out of sync with the target branch. In this case, the base branch must be rebased. Some tools, such as Dependabot, do that automatically.', ); - expect(logInfoSpy.mock.calls[2][0]).toStrictEqual('Retrying in 1000...'); - expect(logInfoSpy.mock.calls[4][0]).toStrictEqual('Retrying in 4000...'); + expect(logInfoSpy.mock.calls[2][0]).toStrictEqual('Retrying in 100...'); + expect(logInfoSpy.mock.calls[4][0]).toStrictEqual('Retrying in 400...'); } - }, 10000); + }); }); diff --git a/src/eventHandlers/push/index.ts b/src/eventHandlers/push/index.ts index 1d0eb6bd16..ad42b9b278 100644 --- a/src/eventHandlers/push/index.ts +++ b/src/eventHandlers/push/index.ts @@ -78,7 +78,13 @@ const getPullRequestInformation = async ( const tryMerge = async ( octokit: ReturnType, - maximumRetries: number, + { + maximumRetries, + minimumWaitTime, + }: { + maximumRetries: number; + minimumWaitTime?: number; + }, { commitMessageHeadline, mergeableState, @@ -98,6 +104,7 @@ const tryMerge = async ( await mergeWithRetry(octokit, { commitHeadline: commitMessageHeadline, maximumRetries, + minimumWaitTime, pullRequestId, retryCount: 1, reviewEdge: reviewEdges[0], @@ -108,7 +115,13 @@ const tryMerge = async ( export const pushHandle = async ( octokit: ReturnType, gitHubLogin: string, - maximumRetries: number, + { + maximumRetries, + minimumWaitTime, + }: { + maximumRetries: number; + minimumWaitTime?: number; + }, ): Promise => { if (context.payload.pusher.name !== gitHubLogin) { logInfo( @@ -135,9 +148,13 @@ export const pushHandle = async ( )}.`, ); - await tryMerge(octokit, maximumRetries, { - ...pullRequestInformation, - commitMessageHeadline: getCommitMessageHeadline(), - }); + await tryMerge( + octokit, + { maximumRetries, minimumWaitTime }, + { + ...pullRequestInformation, + commitMessageHeadline: getCommitMessageHeadline(), + }, + ); } }; From 18e4918b4cb206e605bf3f3fc993a95b8e6f17b1 Mon Sep 17 00:00:00 2001 From: Ricardo Souza Date: Fri, 14 Aug 2020 09:18:07 +0200 Subject: [PATCH 11/17] fix: send "maximumRetries" parameters in "index.ts" --- src/index.ts | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/index.ts b/src/index.ts index 5549eed984..1338497733 100644 --- a/src/index.ts +++ b/src/index.ts @@ -24,11 +24,17 @@ const main = async (): Promise => { switch (context.eventName) { case 'check_suite': - return checkSuiteHandle(octokit, GITHUB_LOGIN, MAXIMUM_RETRIES); + return checkSuiteHandle(octokit, GITHUB_LOGIN, { + maximumRetries: MAXIMUM_RETRIES, + }); case 'pull_request': - return pullRequestHandle(octokit, GITHUB_LOGIN, MAXIMUM_RETRIES); + return pullRequestHandle(octokit, GITHUB_LOGIN, { + maximumRetries: MAXIMUM_RETRIES, + }); case 'push': - return pushHandle(octokit, GITHUB_LOGIN, MAXIMUM_RETRIES); + return pushHandle(octokit, GITHUB_LOGIN, { + maximumRetries: MAXIMUM_RETRIES, + }); default: logWarning(`Unknown event ${context.eventName}, skipping.`); } From 7cf677147dd6a52f1ee7d3d11cd4c3e696b25f9a Mon Sep 17 00:00:00 2001 From: Ricardo Souza Date: Fri, 14 Aug 2020 09:19:03 +0200 Subject: [PATCH 12/17] feat: retries only on a specific error (and update tests) --- src/common/merge.ts | 5 ++++- src/eventHandlers/checkSuite/index.spec.ts | 9 +++++++-- src/eventHandlers/pullRequest/index.spec.ts | 9 +++++++-- src/eventHandlers/push/index.spec.ts | 9 +++++++-- 4 files changed, 25 insertions(+), 7 deletions(-) diff --git a/src/common/merge.ts b/src/common/merge.ts index 6c60701fc9..f1e3825d27 100644 --- a/src/common/merge.ts +++ b/src/common/merge.ts @@ -43,6 +43,9 @@ const merge = async ( await octokit.graphql(mutation, { commitHeadline, pullRequestId }); }; +const shouldRetry = (error: Error): boolean => + error.message.includes('Base branch was modified.'); + export const mergeWithRetry = async ( octokit: ReturnType, details: PullRequestDetails & { @@ -65,7 +68,7 @@ export const mergeWithRetry = async ( /* eslint-disable-next-line @typescript-eslint/no-base-to-string */ logDebug(`Original error: ${(error as Error).toString()}.`); - if (retryCount <= maximumRetries) { + if (shouldRetry(error) && retryCount <= maximumRetries) { const nextRetryIn = retryCount ** EXPONENTIAL_BACKOFF * (minimumWaitTime === undefined ? MINIMUM_WAIT_TIME : minimumWaitTime); diff --git a/src/eventHandlers/checkSuite/index.spec.ts b/src/eventHandlers/checkSuite/index.spec.ts index b2cdab9b63..e44e29b9b6 100644 --- a/src/eventHandlers/checkSuite/index.spec.ts +++ b/src/eventHandlers/checkSuite/index.spec.ts @@ -405,7 +405,10 @@ describe('check Suite event handler', (): void => { }) .post('/graphql') .times(3) - .reply(403, 'Error when merging'); + .reply( + 403, + '##[error]GraphqlError: Base branch was modified. Review and try the merge again.', + ); const mergeWithRetrySpy = jest.spyOn(merge, 'mergeWithRetry'); const logDebugSpy = jest.spyOn(log, 'logDebug'); @@ -418,7 +421,9 @@ describe('check Suite event handler', (): void => { }); } catch (error) { expect(error).toBeInstanceOf(Error); - expect(error.message).toStrictEqual('Error when merging'); + expect(error.message).toStrictEqual( + '##[error]GraphqlError: Base branch was modified. Review and try the merge again.', + ); expect(mergeWithRetrySpy).toHaveBeenCalledTimes(3); expect(logDebugSpy).toHaveBeenCalledTimes(3); expect(logInfoSpy.mock.calls[1][0]).toStrictEqual( diff --git a/src/eventHandlers/pullRequest/index.spec.ts b/src/eventHandlers/pullRequest/index.spec.ts index e0eb6fbdc9..0dc56323f1 100644 --- a/src/eventHandlers/pullRequest/index.spec.ts +++ b/src/eventHandlers/pullRequest/index.spec.ts @@ -182,7 +182,10 @@ describe('pull request event handler', (): void => { }) .post('/graphql') .times(3) - .reply(403, 'Error when merging'); + .reply( + 403, + '##[error]GraphqlError: Base branch was modified. Review and try the merge again.', + ); const mergeWithRetrySpy = jest.spyOn(merge, 'mergeWithRetry'); const logDebugSpy = jest.spyOn(log, 'logDebug'); @@ -195,7 +198,9 @@ describe('pull request event handler', (): void => { }); } catch (error) { expect(error).toBeInstanceOf(Error); - expect(error.message).toStrictEqual('Error when merging'); + expect(error.message).toStrictEqual( + '##[error]GraphqlError: Base branch was modified. Review and try the merge again.', + ); expect(mergeWithRetrySpy).toHaveBeenCalledTimes(3); expect(logDebugSpy).toHaveBeenCalledTimes(3); expect(logInfoSpy.mock.calls[1][0]).toStrictEqual( diff --git a/src/eventHandlers/push/index.spec.ts b/src/eventHandlers/push/index.spec.ts index 67cf600988..2c9dad82b2 100644 --- a/src/eventHandlers/push/index.spec.ts +++ b/src/eventHandlers/push/index.spec.ts @@ -313,7 +313,10 @@ describe('push event handler', (): void => { }) .post('/graphql') .times(3) - .reply(403, 'Error when merging'); + .reply( + 403, + '##[error]GraphqlError: Base branch was modified. Review and try the merge again.', + ); const mergeWithRetrySpy = jest.spyOn(merge, 'mergeWithRetry'); const logDebugSpy = jest.spyOn(log, 'logDebug'); @@ -326,7 +329,9 @@ describe('push event handler', (): void => { }); } catch (error) { expect(error).toBeInstanceOf(Error); - expect(error.message).toStrictEqual('Error when merging'); + expect(error.message).toStrictEqual( + '##[error]GraphqlError: Base branch was modified. Review and try the merge again.', + ); expect(mergeWithRetrySpy).toHaveBeenCalledTimes(3); expect(logDebugSpy).toHaveBeenCalledTimes(3); expect(logInfoSpy.mock.calls[1][0]).toStrictEqual( From d0b59dc31a06380a17fe427b2a06f14ce5848540 Mon Sep 17 00:00:00 2001 From: Ricardo Souza Date: Tue, 18 Aug 2020 12:10:06 +0200 Subject: [PATCH 13/17] test: remove mergeWithRetry check --- src/eventHandlers/checkSuite/index.spec.ts | 5 +---- src/eventHandlers/pullRequest/index.spec.ts | 5 +---- src/eventHandlers/push/index.spec.ts | 5 +---- 3 files changed, 3 insertions(+), 12 deletions(-) diff --git a/src/eventHandlers/checkSuite/index.spec.ts b/src/eventHandlers/checkSuite/index.spec.ts index e44e29b9b6..368a523bce 100644 --- a/src/eventHandlers/checkSuite/index.spec.ts +++ b/src/eventHandlers/checkSuite/index.spec.ts @@ -7,7 +7,6 @@ import { getOctokit } from '@actions/github'; import { OK } from 'http-status-codes'; import * as nock from 'nock'; -import * as merge from '../../common/merge'; import { mergePullRequestMutation } from '../../graphql/mutations'; import { AllowedMergeMethods } from '../../utilities/inputParsers'; import * as log from '../../utilities/log'; @@ -366,7 +365,7 @@ describe('check Suite event handler', (): void => { }); it('retries up to two times before failing', async (): Promise => { - expect.assertions(7); + expect.assertions(6); nock('https://api.github.com') .post('/graphql') @@ -410,7 +409,6 @@ describe('check Suite event handler', (): void => { '##[error]GraphqlError: Base branch was modified. Review and try the merge again.', ); - const mergeWithRetrySpy = jest.spyOn(merge, 'mergeWithRetry'); const logDebugSpy = jest.spyOn(log, 'logDebug'); const logInfoSpy = jest.spyOn(log, 'logInfo'); @@ -424,7 +422,6 @@ describe('check Suite event handler', (): void => { expect(error.message).toStrictEqual( '##[error]GraphqlError: Base branch was modified. Review and try the merge again.', ); - expect(mergeWithRetrySpy).toHaveBeenCalledTimes(3); expect(logDebugSpy).toHaveBeenCalledTimes(3); expect(logInfoSpy.mock.calls[1][0]).toStrictEqual( 'An error ocurred while merging the Pull Request. This is usually caused by the base branch being out of sync with the target branch. In this case, the base branch must be rebased. Some tools, such as Dependabot, do that automatically.', diff --git a/src/eventHandlers/pullRequest/index.spec.ts b/src/eventHandlers/pullRequest/index.spec.ts index 0dc56323f1..a459bfa32c 100644 --- a/src/eventHandlers/pullRequest/index.spec.ts +++ b/src/eventHandlers/pullRequest/index.spec.ts @@ -7,7 +7,6 @@ import { getOctokit } from '@actions/github'; import { OK } from 'http-status-codes'; import * as nock from 'nock'; -import * as merge from '../../common/merge'; import { mergePullRequestMutation } from '../../graphql/mutations'; import { AllowedMergeMethods } from '../../utilities/inputParsers'; import * as log from '../../utilities/log'; @@ -144,7 +143,7 @@ describe('pull request event handler', (): void => { }); it('retries up to two times before failing', async (): Promise => { - expect.assertions(7); + expect.assertions(6); nock('https://api.github.com') .post('/graphql') @@ -187,7 +186,6 @@ describe('pull request event handler', (): void => { '##[error]GraphqlError: Base branch was modified. Review and try the merge again.', ); - const mergeWithRetrySpy = jest.spyOn(merge, 'mergeWithRetry'); const logDebugSpy = jest.spyOn(log, 'logDebug'); const logInfoSpy = jest.spyOn(log, 'logInfo'); @@ -201,7 +199,6 @@ describe('pull request event handler', (): void => { expect(error.message).toStrictEqual( '##[error]GraphqlError: Base branch was modified. Review and try the merge again.', ); - expect(mergeWithRetrySpy).toHaveBeenCalledTimes(3); expect(logDebugSpy).toHaveBeenCalledTimes(3); expect(logInfoSpy.mock.calls[1][0]).toStrictEqual( 'An error ocurred while merging the Pull Request. This is usually caused by the base branch being out of sync with the target branch. In this case, the base branch must be rebased. Some tools, such as Dependabot, do that automatically.', diff --git a/src/eventHandlers/push/index.spec.ts b/src/eventHandlers/push/index.spec.ts index 2c9dad82b2..2dcc631095 100644 --- a/src/eventHandlers/push/index.spec.ts +++ b/src/eventHandlers/push/index.spec.ts @@ -7,7 +7,6 @@ import { getOctokit } from '@actions/github'; import { OK } from 'http-status-codes'; import * as nock from 'nock'; -import * as merge from '../../common/merge'; import { mergePullRequestMutation } from '../../graphql/mutations'; import { AllowedMergeMethods } from '../../utilities/inputParsers'; import * as log from '../../utilities/log'; @@ -282,7 +281,7 @@ describe('push event handler', (): void => { }); it('retries up to two times before failing', async (): Promise => { - expect.assertions(7); + expect.assertions(6); nock('https://api.github.com') .post('/graphql') @@ -318,7 +317,6 @@ describe('push event handler', (): void => { '##[error]GraphqlError: Base branch was modified. Review and try the merge again.', ); - const mergeWithRetrySpy = jest.spyOn(merge, 'mergeWithRetry'); const logDebugSpy = jest.spyOn(log, 'logDebug'); const logInfoSpy = jest.spyOn(log, 'logInfo'); @@ -332,7 +330,6 @@ describe('push event handler', (): void => { expect(error.message).toStrictEqual( '##[error]GraphqlError: Base branch was modified. Review and try the merge again.', ); - expect(mergeWithRetrySpy).toHaveBeenCalledTimes(3); expect(logDebugSpy).toHaveBeenCalledTimes(3); expect(logInfoSpy.mock.calls[1][0]).toStrictEqual( 'An error ocurred while merging the Pull Request. This is usually caused by the base branch being out of sync with the target branch. In this case, the base branch must be rebased. Some tools, such as Dependabot, do that automatically.', From d5400abf14aab743e50cfa1f7b37e25f2c609df3 Mon Sep 17 00:00:00 2001 From: Ricardo Souza Date: Tue, 18 Aug 2020 12:12:48 +0200 Subject: [PATCH 14/17] test: checks if merge function throws when a different error is given --- src/eventHandlers/checkSuite/index.spec.ts | 61 +++++++++++++++++++++ src/eventHandlers/pullRequest/index.spec.ts | 60 ++++++++++++++++++++ src/eventHandlers/push/index.spec.ts | 53 ++++++++++++++++++ 3 files changed, 174 insertions(+) diff --git a/src/eventHandlers/checkSuite/index.spec.ts b/src/eventHandlers/checkSuite/index.spec.ts index 368a523bce..41d8cb0d54 100644 --- a/src/eventHandlers/checkSuite/index.spec.ts +++ b/src/eventHandlers/checkSuite/index.spec.ts @@ -430,4 +430,65 @@ describe('check Suite event handler', (): void => { expect(logInfoSpy.mock.calls[4][0]).toStrictEqual('Retrying in 400...'); } }); + + it('fails the backoff strategy when the error is not "Base branch was modified"', async (): Promise< + void + > => { + expect.assertions(3); + + nock('https://api.github.com') + .post('/graphql') + .reply(OK, { + data: { + repository: { + pullRequest: { + commits: { + edges: [ + { + node: { + commit: { + messageHeadline: COMMIT_HEADLINE, + }, + }, + }, + ], + }, + id: PULL_REQUEST_ID, + mergeStateStatus: 'CLEAN', + mergeable: 'MERGEABLE', + merged: false, + reviews: { + edges: [ + { + node: { + state: 'APPROVED', + }, + }, + ], + }, + state: 'OPEN', + }, + }, + }, + }) + .post('/graphql') + .reply(403, '##[error]GraphqlError: This is a different error.'); + + const logInfoSpy = jest.spyOn(log, 'logInfo'); + + try { + await checkSuiteHandle(octokit, 'dependabot-preview[bot]', { + maximumRetries: 2, + minimumWaitTime: 100, + }); + } catch (error) { + expect(error).toBeInstanceOf(Error); + expect(error.message).toStrictEqual( + '##[error]GraphqlError: This is a different error.', + ); + expect(logInfoSpy.mock.calls[1][0]).toStrictEqual( + 'An error ocurred while merging the Pull Request. This is usually caused by the base branch being out of sync with the target branch. In this case, the base branch must be rebased. Some tools, such as Dependabot, do that automatically.', + ); + } + }); }); diff --git a/src/eventHandlers/pullRequest/index.spec.ts b/src/eventHandlers/pullRequest/index.spec.ts index a459bfa32c..3b5e83bb9c 100644 --- a/src/eventHandlers/pullRequest/index.spec.ts +++ b/src/eventHandlers/pullRequest/index.spec.ts @@ -207,4 +207,64 @@ describe('pull request event handler', (): void => { expect(logInfoSpy.mock.calls[4][0]).toStrictEqual('Retrying in 400...'); } }); + + it('fails the backoff strategy when the error is not "Base branch was modified"', async (): Promise< + void + > => { + expect.assertions(3); + + nock('https://api.github.com') + .post('/graphql') + .reply(OK, { + data: { + repository: { + pullRequest: { + commits: { + edges: [ + { + node: { + commit: { + message: COMMIT_HEADLINE, + }, + }, + }, + ], + }, + id: PULL_REQUEST_ID, + mergeable: 'MERGEABLE', + merged: false, + reviews: { + edges: [ + { + node: { + state: 'APPROVED', + }, + }, + ], + }, + state: 'OPEN', + }, + }, + }, + }) + .post('/graphql') + .reply(403, '##[error]GraphqlError: This is a different error.'); + + const logInfoSpy = jest.spyOn(log, 'logInfo'); + + try { + await pullRequestHandle(octokit, 'dependabot-preview[bot]', { + maximumRetries: 2, + minimumWaitTime: 100, + }); + } catch (error) { + expect(error).toBeInstanceOf(Error); + expect(error.message).toStrictEqual( + '##[error]GraphqlError: This is a different error.', + ); + expect(logInfoSpy.mock.calls[1][0]).toStrictEqual( + 'An error ocurred while merging the Pull Request. This is usually caused by the base branch being out of sync with the target branch. In this case, the base branch must be rebased. Some tools, such as Dependabot, do that automatically.', + ); + } + }); }); diff --git a/src/eventHandlers/push/index.spec.ts b/src/eventHandlers/push/index.spec.ts index 2dcc631095..f157d25b71 100644 --- a/src/eventHandlers/push/index.spec.ts +++ b/src/eventHandlers/push/index.spec.ts @@ -338,4 +338,57 @@ describe('push event handler', (): void => { expect(logInfoSpy.mock.calls[4][0]).toStrictEqual('Retrying in 400...'); } }); + + it('fails the backoff strategy when the error is not "Base branch was modified"', async (): Promise< + void + > => { + expect.assertions(3); + + nock('https://api.github.com') + .post('/graphql') + .reply(OK, { + data: { + repository: { + pullRequests: { + nodes: [ + { + id: PULL_REQUEST_ID, + mergeable: 'MERGEABLE', + merged: false, + reviews: { + edges: [ + { + node: { + state: 'APPROVED', + }, + }, + ], + }, + state: 'OPEN', + }, + ], + }, + }, + }, + }) + .post('/graphql') + .reply(403, '##[error]GraphqlError: This is a different error.'); + + const logInfoSpy = jest.spyOn(log, 'logInfo'); + + try { + await pushHandle(octokit, 'dependabot-preview[bot]', { + maximumRetries: 2, + minimumWaitTime: 100, + }); + } catch (error) { + expect(error).toBeInstanceOf(Error); + expect(error.message).toStrictEqual( + '##[error]GraphqlError: This is a different error.', + ); + expect(logInfoSpy.mock.calls[1][0]).toStrictEqual( + 'An error ocurred while merging the Pull Request. This is usually caused by the base branch being out of sync with the target branch. In this case, the base branch must be rebased. Some tools, such as Dependabot, do that automatically.', + ); + } + }); }); From 2032586211d5ab29e8776f241296651d0f7b71d1 Mon Sep 17 00:00:00 2001 From: Ricardo Souza Date: Tue, 18 Aug 2020 12:25:26 +0200 Subject: [PATCH 15/17] test: remove minimum time parameter --- src/common/merge.ts | 7 +-- src/eventHandlers/checkSuite/index.spec.ts | 50 +++++---------------- src/eventHandlers/checkSuite/index.ts | 23 ++-------- src/eventHandlers/pullRequest/index.spec.ts | 10 +---- src/eventHandlers/pullRequest/index.ts | 9 +--- src/eventHandlers/push/index.spec.ts | 44 ++++-------------- src/eventHandlers/push/index.ts | 29 +++--------- 7 files changed, 33 insertions(+), 139 deletions(-) diff --git a/src/common/merge.ts b/src/common/merge.ts index f1e3825d27..6c07685e84 100644 --- a/src/common/merge.ts +++ b/src/common/merge.ts @@ -50,11 +50,10 @@ export const mergeWithRetry = async ( octokit: ReturnType, details: PullRequestDetails & { maximumRetries: number; - minimumWaitTime?: number; retryCount: number; }, ): Promise => { - const { retryCount, maximumRetries, minimumWaitTime } = details; + const { retryCount, maximumRetries } = details; try { await merge(octokit, details); @@ -69,9 +68,7 @@ export const mergeWithRetry = async ( logDebug(`Original error: ${(error as Error).toString()}.`); if (shouldRetry(error) && retryCount <= maximumRetries) { - const nextRetryIn = - retryCount ** EXPONENTIAL_BACKOFF * - (minimumWaitTime === undefined ? MINIMUM_WAIT_TIME : minimumWaitTime); + const nextRetryIn = retryCount ** EXPONENTIAL_BACKOFF * MINIMUM_WAIT_TIME; logInfo(`Retrying in ${nextRetryIn.toString()}...`); diff --git a/src/eventHandlers/checkSuite/index.spec.ts b/src/eventHandlers/checkSuite/index.spec.ts index 41d8cb0d54..d51639af8f 100644 --- a/src/eventHandlers/checkSuite/index.spec.ts +++ b/src/eventHandlers/checkSuite/index.spec.ts @@ -62,10 +62,7 @@ describe('check Suite event handler', (): void => { }); nock('https://api.github.com').post('/graphql').reply(OK); - await checkSuiteHandle(octokit, 'dependabot-preview[bot]', { - maximumRetries: 3, - minimumWaitTime: 100, - }); + await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3); expect(warningSpy).not.toHaveBeenCalled(); }); @@ -120,10 +117,7 @@ describe('check Suite event handler', (): void => { }) .reply(OK); - await checkSuiteHandle(octokit, 'dependabot-preview[bot]', { - maximumRetries: 3, - minimumWaitTime: 100, - }); + await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3); }); it('does not approve pull requests that are not mergeable', async (): Promise< @@ -167,10 +161,7 @@ describe('check Suite event handler', (): void => { }, }); - await checkSuiteHandle(octokit, 'dependabot-preview[bot]', { - maximumRetries: 3, - minimumWaitTime: 100, - }); + await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3); expect(infoSpy).toHaveBeenCalledWith( 'Pull request is not in a mergeable state: CONFLICTING.', @@ -218,10 +209,7 @@ describe('check Suite event handler', (): void => { }, }); - await checkSuiteHandle(octokit, 'dependabot-preview[bot]', { - maximumRetries: 3, - minimumWaitTime: 100, - }); + await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3); expect(infoSpy).toHaveBeenCalledWith('Pull request is already merged.'); }); @@ -267,10 +255,7 @@ describe('check Suite event handler', (): void => { }, }); - await checkSuiteHandle(octokit, 'dependabot-preview[bot]', { - maximumRetries: 3, - minimumWaitTime: 100, - }); + await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3); expect(infoSpy).toHaveBeenCalledWith( 'Pull request cannot be merged cleanly. Current state: UNKNOWN.', @@ -318,10 +303,7 @@ describe('check Suite event handler', (): void => { }, }); - await checkSuiteHandle(octokit, 'dependabot-preview[bot]', { - maximumRetries: 3, - minimumWaitTime: 100, - }); + await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3); expect(infoSpy).toHaveBeenCalledWith('Pull request is not open: CLOSED.'); }); @@ -331,10 +313,7 @@ describe('check Suite event handler', (): void => { > => { expect.assertions(1); - await checkSuiteHandle(octokit, 'some-other-login', { - maximumRetries: 3, - minimumWaitTime: 100, - }); + await checkSuiteHandle(octokit, 'some-other-login', 3); expect(infoSpy).toHaveBeenCalledWith( 'Pull request created by dependabot-preview[bot], not some-other-login, skipping.', @@ -356,10 +335,7 @@ describe('check Suite event handler', (): void => { }, }); - await checkSuiteHandle(octokit, 'dependabot-preview[bot]', { - maximumRetries: 3, - minimumWaitTime: 100, - }); + await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3); expect(warningSpy).toHaveBeenCalled(); }); @@ -413,10 +389,7 @@ describe('check Suite event handler', (): void => { const logInfoSpy = jest.spyOn(log, 'logInfo'); try { - await checkSuiteHandle(octokit, 'dependabot-preview[bot]', { - maximumRetries: 2, - minimumWaitTime: 100, - }); + await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 2); } catch (error) { expect(error).toBeInstanceOf(Error); expect(error.message).toStrictEqual( @@ -477,10 +450,7 @@ describe('check Suite event handler', (): void => { const logInfoSpy = jest.spyOn(log, 'logInfo'); try { - await checkSuiteHandle(octokit, 'dependabot-preview[bot]', { - maximumRetries: 2, - minimumWaitTime: 100, - }); + await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 2); } catch (error) { expect(error).toBeInstanceOf(Error); expect(error.message).toStrictEqual( diff --git a/src/eventHandlers/checkSuite/index.ts b/src/eventHandlers/checkSuite/index.ts index 51e351857b..8272511ae9 100644 --- a/src/eventHandlers/checkSuite/index.ts +++ b/src/eventHandlers/checkSuite/index.ts @@ -91,13 +91,7 @@ const getPullRequestInformation = async ( const tryMerge = async ( octokit: ReturnType, - { - maximumRetries, - minimumWaitTime, - }: { - maximumRetries: number; - minimumWaitTime?: number; - }, + maximumRetries: number, { commitMessageHeadline, mergeStateStatus, @@ -132,7 +126,6 @@ const tryMerge = async ( await mergeWithRetry(octokit, { commitHeadline: commitMessageHeadline, maximumRetries, - minimumWaitTime, pullRequestId, retryCount: 1, reviewEdge: reviewEdges[0], @@ -143,13 +136,7 @@ const tryMerge = async ( export const checkSuiteHandle = async ( octokit: ReturnType, gitHubLogin: string, - { - maximumRetries, - minimumWaitTime, - }: { - maximumRetries: number; - minimumWaitTime?: number; - }, + maximumRetries: number, ): Promise => { const pullRequests = context.payload.check_suite.pull_requests as Array<{ number: number; @@ -185,11 +172,7 @@ export const checkSuiteHandle = async ( )}.`, ); - await tryMerge( - octokit, - { maximumRetries, minimumWaitTime }, - pullRequestInformation, - ); + await tryMerge(octokit, maximumRetries, pullRequestInformation); } } }; diff --git a/src/eventHandlers/pullRequest/index.spec.ts b/src/eventHandlers/pullRequest/index.spec.ts index 3b5e83bb9c..1da2581589 100644 --- a/src/eventHandlers/pullRequest/index.spec.ts +++ b/src/eventHandlers/pullRequest/index.spec.ts @@ -190,10 +190,7 @@ describe('pull request event handler', (): void => { const logInfoSpy = jest.spyOn(log, 'logInfo'); try { - await pullRequestHandle(octokit, 'dependabot-preview[bot]', { - maximumRetries: 2, - minimumWaitTime: 100, - }); + await pullRequestHandle(octokit, 'dependabot-preview[bot]', 2); } catch (error) { expect(error).toBeInstanceOf(Error); expect(error.message).toStrictEqual( @@ -253,10 +250,7 @@ describe('pull request event handler', (): void => { const logInfoSpy = jest.spyOn(log, 'logInfo'); try { - await pullRequestHandle(octokit, 'dependabot-preview[bot]', { - maximumRetries: 2, - minimumWaitTime: 100, - }); + await pullRequestHandle(octokit, 'dependabot-preview[bot]', 2); } catch (error) { expect(error).toBeInstanceOf(Error); expect(error.message).toStrictEqual( diff --git a/src/eventHandlers/pullRequest/index.ts b/src/eventHandlers/pullRequest/index.ts index d10df03d84..c62b86f297 100644 --- a/src/eventHandlers/pullRequest/index.ts +++ b/src/eventHandlers/pullRequest/index.ts @@ -52,13 +52,7 @@ const getPullRequestInformation = async ( export const pullRequestHandle = async ( octokit: ReturnType, gitHubLogin: string, - { - maximumRetries, - minimumWaitTime, - }: { - maximumRetries: number; - minimumWaitTime?: number; - }, + maximumRetries: number, ): Promise => { const { repository, pull_request: pullRequest } = context.payload; @@ -96,7 +90,6 @@ export const pullRequestHandle = async ( await mergeWithRetry(octokit, { commitHeadline: pullRequest.title, maximumRetries, - minimumWaitTime, pullRequestId: pullRequest.node_id, retryCount: 1, reviewEdge: pullRequestInformation.reviewEdges[0], diff --git a/src/eventHandlers/push/index.spec.ts b/src/eventHandlers/push/index.spec.ts index f157d25b71..a7cbad6dd9 100644 --- a/src/eventHandlers/push/index.spec.ts +++ b/src/eventHandlers/push/index.spec.ts @@ -60,10 +60,7 @@ describe('push event handler', (): void => { }); nock('https://api.github.com').post('/graphql').reply(OK); - await pushHandle(octokit, 'dependabot-preview[bot]', { - maximumRetries: 2, - minimumWaitTime: 100, - }); + await pushHandle(octokit, 'dependabot-preview[bot]', 2); expect(warningSpy).not.toHaveBeenCalled(); }); @@ -110,10 +107,7 @@ describe('push event handler', (): void => { }) .reply(OK); - await pushHandle(octokit, 'dependabot-preview[bot]', { - maximumRetries: 2, - minimumWaitTime: 100, - }); + await pushHandle(octokit, 'dependabot-preview[bot]', 2); }); it('does not approve pull requests that are not mergeable', async (): Promise< @@ -149,10 +143,7 @@ describe('push event handler', (): void => { }, }); - await pushHandle(octokit, 'dependabot-preview[bot]', { - maximumRetries: 2, - minimumWaitTime: 100, - }); + await pushHandle(octokit, 'dependabot-preview[bot]', 2); expect(infoSpy).toHaveBeenCalledWith( 'Pull request is not in a mergeable state: CONFLICTING.', @@ -192,10 +183,7 @@ describe('push event handler', (): void => { }, }); - await pushHandle(octokit, 'dependabot-preview[bot]', { - maximumRetries: 2, - minimumWaitTime: 100, - }); + await pushHandle(octokit, 'dependabot-preview[bot]', 2); expect(infoSpy).toHaveBeenCalledWith('Pull request is already merged.'); }); @@ -233,10 +221,7 @@ describe('push event handler', (): void => { }, }); - await pushHandle(octokit, 'dependabot-preview[bot]', { - maximumRetries: 2, - minimumWaitTime: 100, - }); + await pushHandle(octokit, 'dependabot-preview[bot]', 2); expect(infoSpy).toHaveBeenCalledWith('Pull request is not open: CLOSED.'); }); @@ -246,9 +231,7 @@ describe('push event handler', (): void => { > => { expect.assertions(1); - await pushHandle(octokit, 'some-other-login', { - maximumRetries: 2, - }); + await pushHandle(octokit, 'some-other-login', 2); expect(infoSpy).toHaveBeenCalledWith( 'Pull request created by dependabot-preview[bot], not some-other-login, skipping.', @@ -272,10 +255,7 @@ describe('push event handler', (): void => { }, }); - await pushHandle(octokit, 'dependabot-preview[bot]', { - maximumRetries: 2, - minimumWaitTime: 100, - }); + await pushHandle(octokit, 'dependabot-preview[bot]', 2); expect(warningSpy).toHaveBeenCalled(); }); @@ -321,10 +301,7 @@ describe('push event handler', (): void => { const logInfoSpy = jest.spyOn(log, 'logInfo'); try { - await pushHandle(octokit, 'dependabot-preview[bot]', { - maximumRetries: 2, - minimumWaitTime: 100, - }); + await pushHandle(octokit, 'dependabot-preview[bot]', 2); } catch (error) { expect(error).toBeInstanceOf(Error); expect(error.message).toStrictEqual( @@ -377,10 +354,7 @@ describe('push event handler', (): void => { const logInfoSpy = jest.spyOn(log, 'logInfo'); try { - await pushHandle(octokit, 'dependabot-preview[bot]', { - maximumRetries: 2, - minimumWaitTime: 100, - }); + await pushHandle(octokit, 'dependabot-preview[bot]', 2); } catch (error) { expect(error).toBeInstanceOf(Error); expect(error.message).toStrictEqual( diff --git a/src/eventHandlers/push/index.ts b/src/eventHandlers/push/index.ts index ad42b9b278..1d0eb6bd16 100644 --- a/src/eventHandlers/push/index.ts +++ b/src/eventHandlers/push/index.ts @@ -78,13 +78,7 @@ const getPullRequestInformation = async ( const tryMerge = async ( octokit: ReturnType, - { - maximumRetries, - minimumWaitTime, - }: { - maximumRetries: number; - minimumWaitTime?: number; - }, + maximumRetries: number, { commitMessageHeadline, mergeableState, @@ -104,7 +98,6 @@ const tryMerge = async ( await mergeWithRetry(octokit, { commitHeadline: commitMessageHeadline, maximumRetries, - minimumWaitTime, pullRequestId, retryCount: 1, reviewEdge: reviewEdges[0], @@ -115,13 +108,7 @@ const tryMerge = async ( export const pushHandle = async ( octokit: ReturnType, gitHubLogin: string, - { - maximumRetries, - minimumWaitTime, - }: { - maximumRetries: number; - minimumWaitTime?: number; - }, + maximumRetries: number, ): Promise => { if (context.payload.pusher.name !== gitHubLogin) { logInfo( @@ -148,13 +135,9 @@ export const pushHandle = async ( )}.`, ); - await tryMerge( - octokit, - { maximumRetries, minimumWaitTime }, - { - ...pullRequestInformation, - commitMessageHeadline: getCommitMessageHeadline(), - }, - ); + await tryMerge(octokit, maximumRetries, { + ...pullRequestInformation, + commitMessageHeadline: getCommitMessageHeadline(), + }); } }; From aabf783197dd2c382687cbdcc6f1d7d3a1267da7 Mon Sep 17 00:00:00 2001 From: Ricardo Souza Date: Tue, 18 Aug 2020 13:14:23 +0200 Subject: [PATCH 16/17] test: test backoff by mocking setTimeout --- src/eventHandlers/checkSuite/index.spec.ts | 7 +++++-- src/eventHandlers/pullRequest/index.spec.ts | 22 ++++++++------------- src/eventHandlers/push/index.spec.ts | 7 +++++-- test/utilities.ts | 10 ++++++++++ 4 files changed, 28 insertions(+), 18 deletions(-) create mode 100644 test/utilities.ts diff --git a/src/eventHandlers/checkSuite/index.spec.ts b/src/eventHandlers/checkSuite/index.spec.ts index d51639af8f..f93b5491d0 100644 --- a/src/eventHandlers/checkSuite/index.spec.ts +++ b/src/eventHandlers/checkSuite/index.spec.ts @@ -7,6 +7,7 @@ import { getOctokit } from '@actions/github'; import { OK } from 'http-status-codes'; import * as nock from 'nock'; +import { useSetTimeoutImmediateInvocation } from '../../../test/utilities'; import { mergePullRequestMutation } from '../../graphql/mutations'; import { AllowedMergeMethods } from '../../utilities/inputParsers'; import * as log from '../../utilities/log'; @@ -388,6 +389,8 @@ describe('check Suite event handler', (): void => { const logDebugSpy = jest.spyOn(log, 'logDebug'); const logInfoSpy = jest.spyOn(log, 'logInfo'); + useSetTimeoutImmediateInvocation(); + try { await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 2); } catch (error) { @@ -399,8 +402,8 @@ describe('check Suite event handler', (): void => { expect(logInfoSpy.mock.calls[1][0]).toStrictEqual( 'An error ocurred while merging the Pull Request. This is usually caused by the base branch being out of sync with the target branch. In this case, the base branch must be rebased. Some tools, such as Dependabot, do that automatically.', ); - expect(logInfoSpy.mock.calls[2][0]).toStrictEqual('Retrying in 100...'); - expect(logInfoSpy.mock.calls[4][0]).toStrictEqual('Retrying in 400...'); + expect(logInfoSpy.mock.calls[2][0]).toStrictEqual('Retrying in 1000...'); + expect(logInfoSpy.mock.calls[4][0]).toStrictEqual('Retrying in 4000...'); } }); diff --git a/src/eventHandlers/pullRequest/index.spec.ts b/src/eventHandlers/pullRequest/index.spec.ts index 1da2581589..1694deb657 100644 --- a/src/eventHandlers/pullRequest/index.spec.ts +++ b/src/eventHandlers/pullRequest/index.spec.ts @@ -7,6 +7,7 @@ import { getOctokit } from '@actions/github'; import { OK } from 'http-status-codes'; import * as nock from 'nock'; +import { useSetTimeoutImmediateInvocation } from '../../../test/utilities'; import { mergePullRequestMutation } from '../../graphql/mutations'; import { AllowedMergeMethods } from '../../utilities/inputParsers'; import * as log from '../../utilities/log'; @@ -66,10 +67,7 @@ describe('pull request event handler', (): void => { }); nock('https://api.github.com').post('/graphql').reply(OK); - await pullRequestHandle(octokit, 'dependabot-preview[bot]', { - maximumRetries: 2, - minimumWaitTime: 100, - }); + await pullRequestHandle(octokit, 'dependabot-preview[bot]', 2); expect(warningSpy).not.toHaveBeenCalled(); }); @@ -81,10 +79,7 @@ describe('pull request event handler', (): void => { data: null, }); - await pullRequestHandle(octokit, 'dependabot-preview[bot]', { - maximumRetries: 2, - minimumWaitTime: 100, - }); + await pullRequestHandle(octokit, 'dependabot-preview[bot]', 2); }); it('does not approve an already approved pull request', async (): Promise< @@ -136,10 +131,7 @@ describe('pull request event handler', (): void => { }) .reply(OK); - await pullRequestHandle(octokit, 'dependabot-preview[bot]', { - maximumRetries: 2, - minimumWaitTime: 100, - }); + await pullRequestHandle(octokit, 'dependabot-preview[bot]', 2); }); it('retries up to two times before failing', async (): Promise => { @@ -189,6 +181,8 @@ describe('pull request event handler', (): void => { const logDebugSpy = jest.spyOn(log, 'logDebug'); const logInfoSpy = jest.spyOn(log, 'logInfo'); + useSetTimeoutImmediateInvocation(); + try { await pullRequestHandle(octokit, 'dependabot-preview[bot]', 2); } catch (error) { @@ -200,8 +194,8 @@ describe('pull request event handler', (): void => { expect(logInfoSpy.mock.calls[1][0]).toStrictEqual( 'An error ocurred while merging the Pull Request. This is usually caused by the base branch being out of sync with the target branch. In this case, the base branch must be rebased. Some tools, such as Dependabot, do that automatically.', ); - expect(logInfoSpy.mock.calls[2][0]).toStrictEqual('Retrying in 100...'); - expect(logInfoSpy.mock.calls[4][0]).toStrictEqual('Retrying in 400...'); + expect(logInfoSpy.mock.calls[2][0]).toStrictEqual('Retrying in 1000...'); + expect(logInfoSpy.mock.calls[4][0]).toStrictEqual('Retrying in 4000...'); } }); diff --git a/src/eventHandlers/push/index.spec.ts b/src/eventHandlers/push/index.spec.ts index a7cbad6dd9..fac9eb8249 100644 --- a/src/eventHandlers/push/index.spec.ts +++ b/src/eventHandlers/push/index.spec.ts @@ -7,6 +7,7 @@ import { getOctokit } from '@actions/github'; import { OK } from 'http-status-codes'; import * as nock from 'nock'; +import { useSetTimeoutImmediateInvocation } from '../../../test/utilities'; import { mergePullRequestMutation } from '../../graphql/mutations'; import { AllowedMergeMethods } from '../../utilities/inputParsers'; import * as log from '../../utilities/log'; @@ -300,6 +301,8 @@ describe('push event handler', (): void => { const logDebugSpy = jest.spyOn(log, 'logDebug'); const logInfoSpy = jest.spyOn(log, 'logInfo'); + useSetTimeoutImmediateInvocation(); + try { await pushHandle(octokit, 'dependabot-preview[bot]', 2); } catch (error) { @@ -311,8 +314,8 @@ describe('push event handler', (): void => { expect(logInfoSpy.mock.calls[1][0]).toStrictEqual( 'An error ocurred while merging the Pull Request. This is usually caused by the base branch being out of sync with the target branch. In this case, the base branch must be rebased. Some tools, such as Dependabot, do that automatically.', ); - expect(logInfoSpy.mock.calls[2][0]).toStrictEqual('Retrying in 100...'); - expect(logInfoSpy.mock.calls[4][0]).toStrictEqual('Retrying in 400...'); + expect(logInfoSpy.mock.calls[2][0]).toStrictEqual('Retrying in 1000...'); + expect(logInfoSpy.mock.calls[4][0]).toStrictEqual('Retrying in 4000...'); } }); diff --git a/test/utilities.ts b/test/utilities.ts new file mode 100644 index 0000000000..9f4c6d698b --- /dev/null +++ b/test/utilities.ts @@ -0,0 +1,10 @@ +export const useSetTimeoutImmediateInvocation = (): jest.SpyInstance< + NodeJS.Timeout, + [(...args: unknown[]) => void, number, ...unknown[]] +> => + jest + .spyOn(global, 'setTimeout') + .mockImplementation( + (callback: () => void): NodeJS.Timeout => + (callback() as unknown) as NodeJS.Timeout, + ); From 020ed6d07a8b993ff7b1dbf7c388484d19895f9c Mon Sep 17 00:00:00 2001 From: Ricardo Souza Date: Tue, 18 Aug 2020 13:25:55 +0200 Subject: [PATCH 17/17] fix: lint --- src/index.ts | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/index.ts b/src/index.ts index 1338497733..a3cb89bc25 100644 --- a/src/index.ts +++ b/src/index.ts @@ -13,7 +13,7 @@ const DEFAULT_MAXIMUM_RETRIES = 3; const GITHUB_TOKEN = getInput('GITHUB_TOKEN'); const GITHUB_LOGIN = getInput('GITHUB_LOGIN'); const MAXIMUM_RETRIES = - getInput('MAXIMUM_RETRIES') === undefined + getInput('MAXIMUM_RETRIES').trim() === '' ? DEFAULT_MAXIMUM_RETRIES : parseInt(getInput('MAXIMUM_RETRIES'), 10); @@ -24,17 +24,11 @@ const main = async (): Promise => { switch (context.eventName) { case 'check_suite': - return checkSuiteHandle(octokit, GITHUB_LOGIN, { - maximumRetries: MAXIMUM_RETRIES, - }); + return checkSuiteHandle(octokit, GITHUB_LOGIN, MAXIMUM_RETRIES); case 'pull_request': - return pullRequestHandle(octokit, GITHUB_LOGIN, { - maximumRetries: MAXIMUM_RETRIES, - }); + return pullRequestHandle(octokit, GITHUB_LOGIN, MAXIMUM_RETRIES); case 'push': - return pushHandle(octokit, GITHUB_LOGIN, { - maximumRetries: MAXIMUM_RETRIES, - }); + return pushHandle(octokit, GITHUB_LOGIN, MAXIMUM_RETRIES); default: logWarning(`Unknown event ${context.eventName}, skipping.`); }