From 966e141de70a2f5527a8e8fddd6a2ea30bbfcfff Mon Sep 17 00:00:00 2001 From: Rhys Arkins Date: Wed, 14 Apr 2021 22:18:57 +0200 Subject: [PATCH] refactor: separate automerge function in pr --- lib/workers/branch/index.spec.ts | 27 +++++----- lib/workers/branch/index.ts | 3 +- lib/workers/pr/automerge.spec.ts | 83 +++++++++++++++++++++++++++++ lib/workers/pr/automerge.ts | 91 ++++++++++++++++++++++++++++++++ lib/workers/pr/index.spec.ts | 17 +++--- lib/workers/pr/index.ts | 91 +------------------------------- 6 files changed, 201 insertions(+), 111 deletions(-) create mode 100644 lib/workers/pr/automerge.spec.ts create mode 100644 lib/workers/pr/automerge.ts diff --git a/lib/workers/branch/index.spec.ts b/lib/workers/branch/index.spec.ts index 359b54c8239102..17b546a9bbbe14 100644 --- a/lib/workers/branch/index.spec.ts +++ b/lib/workers/branch/index.spec.ts @@ -20,6 +20,7 @@ import * as _sanitize from '../../util/sanitize'; import * as _limits from '../global/limits'; import * as _prWorker from '../pr'; import type { EnsurePrResult } from '../pr'; +import * as _prAutomerge from '../pr/automerge'; import type { Pr } from '../repository/onboarding/branch/check'; import type { BranchConfig, BranchUpgradeConfig } from '../types'; import { PrResult, ProcessBranchResult } from '../types'; @@ -40,6 +41,7 @@ jest.mock('../../manager/npm/post-update'); jest.mock('./automerge'); jest.mock('./commit'); jest.mock('../pr'); +jest.mock('../pr/automerge'); jest.mock('../../util/exec'); jest.mock('../../util/sanitize'); jest.mock('../../util/git'); @@ -53,6 +55,7 @@ const reuse = mocked(_reuse); const npmPostExtract = mocked(_npmPostExtract); const automerge = mocked(_automerge); const commit = mocked(_commit); +const prAutomerge = mocked(_prAutomerge); const prWorker = mocked(_prWorker); const exec = mocked(_exec); const sanitize = mocked(_sanitize); @@ -69,7 +72,7 @@ describe(getName(__filename), () => { let config: BranchConfig; beforeEach(() => { prWorker.ensurePr = jest.fn(); - prWorker.checkAutoMerge = jest.fn(); + prAutomerge.checkAutoMerge = jest.fn(); config = { ...defaultConfig, branchName: 'renovate/some-branch', @@ -448,12 +451,12 @@ describe(getName(__filename), () => { prResult: PrResult.Created, pr: {}, } as EnsurePrResult); - prWorker.checkAutoMerge.mockResolvedValueOnce(true); + prAutomerge.checkAutoMerge.mockResolvedValueOnce(true); commit.commitFilesToBranch.mockResolvedValueOnce(null); await branchWorker.processBranch(config); expect(prWorker.ensurePr).toHaveBeenCalledTimes(1); expect(platform.ensureCommentRemoval).toHaveBeenCalledTimes(0); - expect(prWorker.checkAutoMerge).toHaveBeenCalledTimes(1); + expect(prAutomerge.checkAutoMerge).toHaveBeenCalledTimes(1); }); it('ensures PR and adds lock file error comment if no releaseTimestamp', async () => { getUpdated.getUpdatedPackageFiles.mockResolvedValueOnce({ @@ -469,12 +472,12 @@ describe(getName(__filename), () => { prResult: PrResult.Created, pr: {}, } as EnsurePrResult); - prWorker.checkAutoMerge.mockResolvedValueOnce(true); + prAutomerge.checkAutoMerge.mockResolvedValueOnce(true); commit.commitFilesToBranch.mockResolvedValueOnce(null); await branchWorker.processBranch(config); expect(platform.ensureComment).toHaveBeenCalledTimes(1); expect(prWorker.ensurePr).toHaveBeenCalledTimes(1); - expect(prWorker.checkAutoMerge).toHaveBeenCalledTimes(0); + expect(prAutomerge.checkAutoMerge).toHaveBeenCalledTimes(0); }); it('ensures PR and adds lock file error comment if old releaseTimestamp', async () => { getUpdated.getUpdatedPackageFiles.mockResolvedValueOnce({ @@ -490,13 +493,13 @@ describe(getName(__filename), () => { prResult: PrResult.Created, pr: {}, } as EnsurePrResult); - prWorker.checkAutoMerge.mockResolvedValueOnce(true); + prAutomerge.checkAutoMerge.mockResolvedValueOnce(true); config.releaseTimestamp = '2018-04-26T05:15:51.877Z'; commit.commitFilesToBranch.mockResolvedValueOnce(null); await branchWorker.processBranch(config); expect(platform.ensureComment).toHaveBeenCalledTimes(1); expect(prWorker.ensurePr).toHaveBeenCalledTimes(1); - expect(prWorker.checkAutoMerge).toHaveBeenCalledTimes(0); + expect(prAutomerge.checkAutoMerge).toHaveBeenCalledTimes(0); }); it('ensures PR and adds lock file error comment if new releaseTimestamp and branch exists', async () => { getUpdated.getUpdatedPackageFiles.mockResolvedValueOnce({ @@ -512,13 +515,13 @@ describe(getName(__filename), () => { prResult: PrResult.Created, pr: {}, } as EnsurePrResult); - prWorker.checkAutoMerge.mockResolvedValueOnce(true); + prAutomerge.checkAutoMerge.mockResolvedValueOnce(true); config.releaseTimestamp = new Date().toISOString(); commit.commitFilesToBranch.mockResolvedValueOnce(null); await branchWorker.processBranch(config); expect(platform.ensureComment).toHaveBeenCalledTimes(1); expect(prWorker.ensurePr).toHaveBeenCalledTimes(1); - expect(prWorker.checkAutoMerge).toHaveBeenCalledTimes(0); + expect(prAutomerge.checkAutoMerge).toHaveBeenCalledTimes(0); }); it('throws error if lock file errors and new releaseTimestamp', async () => { getUpdated.getUpdatedPackageFiles.mockResolvedValueOnce({ @@ -534,7 +537,7 @@ describe(getName(__filename), () => { prResult: PrResult.Created, pr: {}, } as EnsurePrResult); - prWorker.checkAutoMerge.mockResolvedValueOnce(true); + prAutomerge.checkAutoMerge.mockResolvedValueOnce(true); config.releaseTimestamp = new Date().toISOString(); await expect(branchWorker.processBranch(config)).rejects.toThrow( Error(MANAGER_LOCKFILE_ERROR) @@ -555,12 +558,12 @@ describe(getName(__filename), () => { prResult: PrResult.Created, pr: {}, } as EnsurePrResult); - prWorker.checkAutoMerge.mockResolvedValueOnce(true); + prAutomerge.checkAutoMerge.mockResolvedValueOnce(true); commit.commitFilesToBranch.mockResolvedValueOnce(null); await branchWorker.processBranch(config); expect(platform.ensureComment).toHaveBeenCalledTimes(1); expect(prWorker.ensurePr).toHaveBeenCalledTimes(1); - expect(prWorker.checkAutoMerge).toHaveBeenCalledTimes(0); + expect(prAutomerge.checkAutoMerge).toHaveBeenCalledTimes(0); }); it('swallows branch errors', async () => { getUpdated.getUpdatedPackageFiles.mockImplementationOnce(() => { diff --git a/lib/workers/branch/index.ts b/lib/workers/branch/index.ts index e0bc3eef37119b..10bf5edfea956d 100644 --- a/lib/workers/branch/index.ts +++ b/lib/workers/branch/index.ts @@ -27,7 +27,8 @@ import { isBranchModified, } from '../../util/git'; import { Limit, isLimitReached } from '../global/limits'; -import { checkAutoMerge, ensurePr, getPlatformPrOptions } from '../pr'; +import { ensurePr, getPlatformPrOptions } from '../pr'; +import { checkAutoMerge } from '../pr/automerge'; import { BranchConfig, PrResult, ProcessBranchResult } from '../types'; import { tryBranchAutomerge } from './automerge'; import { prAlreadyExisted } from './check-existing'; diff --git a/lib/workers/pr/automerge.spec.ts b/lib/workers/pr/automerge.spec.ts new file mode 100644 index 00000000000000..f6deec360d525d --- /dev/null +++ b/lib/workers/pr/automerge.spec.ts @@ -0,0 +1,83 @@ +import { getConfig, getName, git, mocked, partial } from '../../../test/util'; +import { Pr, platform as _platform } from '../../platform'; +import { BranchStatus } from '../../types'; +import { BranchConfig } from '../types'; +import * as prAutomerge from './automerge'; + +jest.mock('../../util/git'); + +const platform = mocked(_platform); +const defaultConfig = getConfig(); + +describe(getName(__filename), () => { + describe('checkAutoMerge(pr, config)', () => { + let config: BranchConfig; + let pr: Pr; + beforeEach(() => { + config = partial({ + ...defaultConfig, + }); + pr = partial({ + canMerge: true, + }); + }); + afterEach(() => { + jest.clearAllMocks(); + }); + it('should not automerge if not configured', async () => { + await prAutomerge.checkAutoMerge(pr, config); + expect(platform.mergePr).toHaveBeenCalledTimes(0); + }); + it('should automerge if enabled and pr is mergeable', async () => { + config.automerge = true; + platform.getBranchStatus.mockResolvedValueOnce(BranchStatus.green); + platform.mergePr.mockResolvedValueOnce(true); + await prAutomerge.checkAutoMerge(pr, config); + expect(platform.mergePr).toHaveBeenCalledTimes(1); + }); + it('should automerge comment', async () => { + config.automerge = true; + config.automergeType = 'pr-comment'; + config.automergeComment = '!merge'; + platform.getBranchStatus.mockResolvedValueOnce(BranchStatus.green); + await prAutomerge.checkAutoMerge(pr, config); + expect(platform.ensureCommentRemoval).toHaveBeenCalledTimes(0); + expect(platform.ensureComment).toHaveBeenCalledTimes(1); + }); + it('should remove previous automerge comment when rebasing', async () => { + config.automerge = true; + config.automergeType = 'pr-comment'; + config.automergeComment = '!merge'; + config.rebaseRequested = true; + platform.getBranchStatus.mockResolvedValueOnce(BranchStatus.green); + await prAutomerge.checkAutoMerge(pr, config); + expect(platform.ensureCommentRemoval).toHaveBeenCalledTimes(1); + expect(platform.ensureComment).toHaveBeenCalledTimes(1); + }); + it('should not automerge if enabled and pr is mergeable but cannot rebase', async () => { + config.automerge = true; + platform.getBranchStatus.mockResolvedValueOnce(BranchStatus.green); + git.isBranchModified.mockResolvedValueOnce(true); + await prAutomerge.checkAutoMerge(pr, config); + expect(platform.mergePr).toHaveBeenCalledTimes(0); + }); + it('should not automerge if enabled and pr is mergeable but branch status is not success', async () => { + config.automerge = true; + platform.getBranchStatus.mockResolvedValueOnce(BranchStatus.yellow); + await prAutomerge.checkAutoMerge(pr, config); + expect(platform.mergePr).toHaveBeenCalledTimes(0); + }); + it('should not automerge if enabled and pr is mergeable but unstable', async () => { + config.automerge = true; + pr.canMerge = undefined; + await prAutomerge.checkAutoMerge(pr, config); + expect(platform.mergePr).toHaveBeenCalledTimes(0); + }); + it('should not automerge if enabled and pr is unmergeable', async () => { + config.automerge = true; + pr.isConflicted = true; + await prAutomerge.checkAutoMerge(pr, config); + expect(platform.mergePr).toHaveBeenCalledTimes(0); + }); + }); +}); diff --git a/lib/workers/pr/automerge.ts b/lib/workers/pr/automerge.ts new file mode 100644 index 00000000000000..fd6b5b5567fc4f --- /dev/null +++ b/lib/workers/pr/automerge.ts @@ -0,0 +1,91 @@ +import { getAdminConfig } from '../../config/admin'; +import { logger } from '../../logger'; +import { Pr, platform } from '../../platform'; +import { BranchStatus } from '../../types'; +import { isBranchModified } from '../../util/git'; +import { BranchConfig } from '../types'; + +export async function checkAutoMerge( + pr: Pr, + config: BranchConfig +): Promise { + logger.trace({ config }, 'checkAutoMerge'); + const { + branchName, + automerge, + automergeType, + automergeComment, + requiredStatusChecks, + rebaseRequested, + } = config; + logger.debug( + { automerge, automergeType, automergeComment }, + `Checking #${pr.number} for automerge` + ); + if (automerge) { + logger.debug('PR is configured for automerge'); + // Return if PR not ready for automerge + if (pr.isConflicted) { + logger.debug('PR is conflicted'); + logger.debug({ pr }); + return false; + } + if (requiredStatusChecks && pr.canMerge !== true) { + logger.debug( + { canMergeReason: pr.canMergeReason }, + 'PR is not ready for merge' + ); + return false; + } + const branchStatus = await platform.getBranchStatus( + branchName, + requiredStatusChecks + ); + if (branchStatus !== BranchStatus.green) { + logger.debug( + `PR is not ready for merge (branch status is ${branchStatus})` + ); + return false; + } + // Check if it's been touched + if (await isBranchModified(branchName)) { + logger.debug('PR is ready for automerge but has been modified'); + return false; + } + if (automergeType === 'pr-comment') { + logger.debug(`Applying automerge comment: ${automergeComment}`); + // istanbul ignore if + if (getAdminConfig().dryRun) { + logger.info( + `DRY-RUN: Would add PR automerge comment to PR #${pr.number}` + ); + return false; + } + if (rebaseRequested) { + await platform.ensureCommentRemoval({ + number: pr.number, + content: automergeComment, + }); + } + return platform.ensureComment({ + number: pr.number, + topic: null, + content: automergeComment, + }); + } + // Let's merge this + logger.debug(`Automerging #${pr.number}`); + // istanbul ignore if + if (getAdminConfig().dryRun) { + logger.info(`DRY-RUN: Would merge PR #${pr.number}`); + return false; + } + const res = await platform.mergePr(pr.number, branchName); + if (res) { + logger.info({ pr: pr.number, prTitle: pr.title }, 'PR automerged'); + } + return res; + } + logger.debug('No automerge'); + return false; +} diff --git a/lib/workers/pr/index.spec.ts b/lib/workers/pr/index.spec.ts index 46be91afb6a552..47d8de271219cd 100644 --- a/lib/workers/pr/index.spec.ts +++ b/lib/workers/pr/index.spec.ts @@ -5,6 +5,7 @@ import { Pr, platform as _platform } from '../../platform'; import { BranchStatus } from '../../types'; import * as _limits from '../global/limits'; import { BranchConfig, PrResult } from '../types'; +import * as prAutomerge from './automerge'; import * as _changelogHelper from './changelog'; import { getChangeLogJSON } from './changelog'; import * as codeOwners from './code-owners'; @@ -114,14 +115,14 @@ describe(getName(__filename), () => { jest.clearAllMocks(); }); it('should not automerge if not configured', async () => { - await prWorker.checkAutoMerge(pr, config); + await prAutomerge.checkAutoMerge(pr, config); expect(platform.mergePr).toHaveBeenCalledTimes(0); }); it('should automerge if enabled and pr is mergeable', async () => { config.automerge = true; platform.getBranchStatus.mockResolvedValueOnce(BranchStatus.green); platform.mergePr.mockResolvedValueOnce(true); - await prWorker.checkAutoMerge(pr, config); + await prAutomerge.checkAutoMerge(pr, config); expect(platform.mergePr).toHaveBeenCalledTimes(1); }); it('should automerge comment', async () => { @@ -129,7 +130,7 @@ describe(getName(__filename), () => { config.automergeType = 'pr-comment'; config.automergeComment = '!merge'; platform.getBranchStatus.mockResolvedValueOnce(BranchStatus.green); - await prWorker.checkAutoMerge(pr, config); + await prAutomerge.checkAutoMerge(pr, config); expect(platform.ensureCommentRemoval).toHaveBeenCalledTimes(0); expect(platform.ensureComment).toHaveBeenCalledTimes(1); }); @@ -139,7 +140,7 @@ describe(getName(__filename), () => { config.automergeComment = '!merge'; config.rebaseRequested = true; platform.getBranchStatus.mockResolvedValueOnce(BranchStatus.green); - await prWorker.checkAutoMerge(pr, config); + await prAutomerge.checkAutoMerge(pr, config); expect(platform.ensureCommentRemoval).toHaveBeenCalledTimes(1); expect(platform.ensureComment).toHaveBeenCalledTimes(1); }); @@ -147,25 +148,25 @@ describe(getName(__filename), () => { config.automerge = true; platform.getBranchStatus.mockResolvedValueOnce(BranchStatus.green); git.isBranchModified.mockResolvedValueOnce(true); - await prWorker.checkAutoMerge(pr, config); + await prAutomerge.checkAutoMerge(pr, config); expect(platform.mergePr).toHaveBeenCalledTimes(0); }); it('should not automerge if enabled and pr is mergeable but branch status is not success', async () => { config.automerge = true; platform.getBranchStatus.mockResolvedValueOnce(BranchStatus.yellow); - await prWorker.checkAutoMerge(pr, config); + await prAutomerge.checkAutoMerge(pr, config); expect(platform.mergePr).toHaveBeenCalledTimes(0); }); it('should not automerge if enabled and pr is mergeable but unstable', async () => { config.automerge = true; pr.canMerge = undefined; - await prWorker.checkAutoMerge(pr, config); + await prAutomerge.checkAutoMerge(pr, config); expect(platform.mergePr).toHaveBeenCalledTimes(0); }); it('should not automerge if enabled and pr is unmergeable', async () => { config.automerge = true; pr.isConflicted = true; - await prWorker.checkAutoMerge(pr, config); + await prAutomerge.checkAutoMerge(pr, config); expect(platform.mergePr).toHaveBeenCalledTimes(0); }); }); diff --git a/lib/workers/pr/index.ts b/lib/workers/pr/index.ts index 23bbb44b1b13f2..d210137832c001 100644 --- a/lib/workers/pr/index.ts +++ b/lib/workers/pr/index.ts @@ -10,11 +10,7 @@ import { PlatformPrOptions, Pr, platform } from '../../platform'; import { BranchStatus } from '../../types'; import { ExternalHostError } from '../../types/errors/external-host-error'; import { sampleSize } from '../../util'; -import { - deleteBranch, - getBranchLastCommitTime, - isBranchModified, -} from '../../util/git'; +import { deleteBranch, getBranchLastCommitTime } from '../../util/git'; import * as template from '../../util/template'; import { Limit, incLimitedValue, isLimitReached } from '../global/limits'; import { BranchConfig, PrResult } from '../types'; @@ -479,88 +475,3 @@ export async function ensurePr( } return { prResult: PrResult.Error }; } - -export async function checkAutoMerge( - pr: Pr, - config: BranchConfig -): Promise { - logger.trace({ config }, 'checkAutoMerge'); - const { - branchName, - automerge, - automergeType, - automergeComment, - requiredStatusChecks, - rebaseRequested, - } = config; - logger.debug( - { automerge, automergeType, automergeComment }, - `Checking #${pr.number} for automerge` - ); - if (automerge) { - logger.debug('PR is configured for automerge'); - // Return if PR not ready for automerge - if (pr.isConflicted) { - logger.debug('PR is conflicted'); - logger.debug({ pr }); - return false; - } - if (requiredStatusChecks && pr.canMerge !== true) { - logger.debug( - { canMergeReason: pr.canMergeReason }, - 'PR is not ready for merge' - ); - return false; - } - const branchStatus = await platform.getBranchStatus( - branchName, - requiredStatusChecks - ); - if (branchStatus !== BranchStatus.green) { - logger.debug( - `PR is not ready for merge (branch status is ${branchStatus})` - ); - return false; - } - // Check if it's been touched - if (await isBranchModified(branchName)) { - logger.debug('PR is ready for automerge but has been modified'); - return false; - } - if (automergeType === 'pr-comment') { - logger.debug(`Applying automerge comment: ${automergeComment}`); - // istanbul ignore if - if (getAdminConfig().dryRun) { - logger.info( - `DRY-RUN: Would add PR automerge comment to PR #${pr.number}` - ); - return false; - } - if (rebaseRequested) { - await platform.ensureCommentRemoval({ - number: pr.number, - content: automergeComment, - }); - } - return platform.ensureComment({ - number: pr.number, - topic: null, - content: automergeComment, - }); - } - // Let's merge this - logger.debug(`Automerging #${pr.number}`); - // istanbul ignore if - if (getAdminConfig().dryRun) { - logger.info(`DRY-RUN: Would merge PR #${pr.number}`); - return false; - } - const res = await platform.mergePr(pr.number, branchName); - if (res) { - logger.info({ pr: pr.number, prTitle: pr.title }, 'PR automerged'); - } - return res; - } - logger.debug('No automerge'); - return false; -}