diff --git a/lib/workers/branch/index.spec.ts b/lib/workers/branch/index.spec.ts index 939e47a667e682..cbc93bce7f2c16 100644 --- a/lib/workers/branch/index.spec.ts +++ b/lib/workers/branch/index.spec.ts @@ -22,6 +22,7 @@ import { PR_STATE_OPEN, } from '../../constants/pull-requests'; import { StatusResult } from '../../platform/git/storage'; +import { PrResult } from '../pr'; jest.mock('./get-updated'); jest.mock('./schedule'); @@ -291,7 +292,9 @@ describe('workers/branch', () => { platform.branchExists.mockResolvedValueOnce(true); commit.commitFilesToBranch.mockResolvedValueOnce(null); automerge.tryBranchAutomerge.mockResolvedValueOnce('failed'); - prWorker.ensurePr.mockResolvedValueOnce('needs-pr-approval'); + prWorker.ensurePr.mockResolvedValueOnce({ + result: PrResult.AwaitingApproval, + }); expect(await branchWorker.processBranch(config)).toEqual( 'needs-pr-approval' ); @@ -308,7 +311,9 @@ describe('workers/branch', () => { platform.branchExists.mockResolvedValueOnce(true); commit.commitFilesToBranch.mockResolvedValueOnce(null); automerge.tryBranchAutomerge.mockResolvedValueOnce('failed'); - prWorker.ensurePr.mockResolvedValueOnce('pending'); + prWorker.ensurePr.mockResolvedValueOnce({ + result: PrResult.AwaitingNotPending, + }); expect(await branchWorker.processBranch(config)).toEqual('pending'); }); it('returns if branch exists but updated', async () => { @@ -341,7 +346,10 @@ describe('workers/branch', () => { } as never); platform.branchExists.mockResolvedValueOnce(true); automerge.tryBranchAutomerge.mockResolvedValueOnce('failed'); - prWorker.ensurePr.mockResolvedValueOnce({} as never); + prWorker.ensurePr.mockResolvedValueOnce({ + result: PrResult.Created, + pr: {}, + } as never); prWorker.checkAutoMerge.mockResolvedValueOnce(true); commit.commitFilesToBranch.mockResolvedValueOnce(null); await branchWorker.processBranch(config); @@ -359,7 +367,10 @@ describe('workers/branch', () => { } as never); platform.branchExists.mockResolvedValueOnce(true); automerge.tryBranchAutomerge.mockResolvedValueOnce('failed'); - prWorker.ensurePr.mockResolvedValueOnce({} as never); + prWorker.ensurePr.mockResolvedValueOnce({ + result: PrResult.Created, + pr: {}, + } as never); prWorker.checkAutoMerge.mockResolvedValueOnce(true); commit.commitFilesToBranch.mockResolvedValueOnce(null); await branchWorker.processBranch(config); @@ -378,7 +389,10 @@ describe('workers/branch', () => { } as never); platform.branchExists.mockResolvedValueOnce(true); automerge.tryBranchAutomerge.mockResolvedValueOnce('failed'); - prWorker.ensurePr.mockResolvedValueOnce({} as never); + prWorker.ensurePr.mockResolvedValueOnce({ + result: PrResult.Created, + pr: {}, + } as never); prWorker.checkAutoMerge.mockResolvedValueOnce(true); config.releaseTimestamp = '2018-04-26T05:15:51.877Z'; commit.commitFilesToBranch.mockResolvedValueOnce(null); @@ -398,7 +412,10 @@ describe('workers/branch', () => { } as never); platform.branchExists.mockResolvedValueOnce(true); automerge.tryBranchAutomerge.mockResolvedValueOnce('failed'); - prWorker.ensurePr.mockResolvedValueOnce({} as never); + prWorker.ensurePr.mockResolvedValueOnce({ + result: PrResult.Created, + pr: {}, + } as never); prWorker.checkAutoMerge.mockResolvedValueOnce(true); config.releaseTimestamp = new Date().toISOString(); commit.commitFilesToBranch.mockResolvedValueOnce(null); @@ -418,7 +435,10 @@ describe('workers/branch', () => { } as never); platform.branchExists.mockResolvedValueOnce(false); automerge.tryBranchAutomerge.mockResolvedValueOnce('failed'); - prWorker.ensurePr.mockResolvedValueOnce({} as never); + prWorker.ensurePr.mockResolvedValueOnce({ + result: PrResult.Created, + pr: {}, + } as never); prWorker.checkAutoMerge.mockResolvedValueOnce(true); config.releaseTimestamp = new Date().toISOString(); await expect(branchWorker.processBranch(config)).rejects.toThrow( @@ -436,7 +456,10 @@ describe('workers/branch', () => { config.recreateClosed = true; platform.branchExists.mockResolvedValueOnce(true); automerge.tryBranchAutomerge.mockResolvedValueOnce('failed'); - prWorker.ensurePr.mockResolvedValueOnce({} as never); + prWorker.ensurePr.mockResolvedValueOnce({ + result: PrResult.Created, + pr: {}, + } as never); prWorker.checkAutoMerge.mockResolvedValueOnce(true); commit.commitFilesToBranch.mockResolvedValueOnce(null); await branchWorker.processBranch(config); @@ -547,7 +570,10 @@ describe('workers/branch', () => { } as never); schedule.isScheduledNow.mockReturnValueOnce(false); - prWorker.ensurePr.mockResolvedValueOnce({} as never); + prWorker.ensurePr.mockResolvedValueOnce({ + result: PrResult.Created, + pr: {}, + } as never); commit.commitFilesToBranch.mockResolvedValueOnce(null); expect( await branchWorker.processBranch({ diff --git a/lib/workers/branch/index.ts b/lib/workers/branch/index.ts index 856a3ee75983a8..77d9f85bfbefc3 100644 --- a/lib/workers/branch/index.ts +++ b/lib/workers/branch/index.ts @@ -15,7 +15,7 @@ import { getParentBranch } from './parent'; import { tryBranchAutomerge } from './automerge'; import { setStability, setUnpublishable } from './status-checks'; import { prAlreadyExisted } from './check-existing'; -import { ensurePr, checkAutoMerge } from '../pr'; +import { ensurePr, checkAutoMerge, PrResult } from '../pr'; import { RenovateConfig } from '../../config'; import { platform } from '../../platform'; import { emojify } from '../../util/emoji'; @@ -546,12 +546,15 @@ export async function processBranch( logger.debug( `There are ${config.errors.length} errors and ${config.warnings.length} warnings` ); - const pr = await ensurePr(config); + const { result, pr } = await ensurePr(config); // TODO: ensurePr should check for automerge itself - if (pr === 'needs-pr-approval') { + if (result === PrResult.AwaitingApproval) { return 'needs-pr-approval'; } - if (pr === 'pending') { + if ( + result === PrResult.AwaitingGreenBranch || + result === PrResult.AwaitingNotPending + ) { return 'pending'; } if (pr) { diff --git a/lib/workers/pr/index.spec.ts b/lib/workers/pr/index.spec.ts index a1748f71e16498..780c26af3d0c0e 100644 --- a/lib/workers/pr/index.spec.ts +++ b/lib/workers/pr/index.spec.ts @@ -5,6 +5,7 @@ import { platform as _platform, Pr } from '../../platform'; import { mocked } from '../../../test/util'; import { BranchStatus } from '../../types'; import { PLATFORM_TYPE_GITLAB } from '../../constants/platforms'; +import { PrResult } from '.'; const changelogHelper = mocked(_changelogHelper); const platform = mocked(_platform); @@ -159,27 +160,31 @@ describe('workers/pr', () => { }); config.newValue = '1.2.0'; platform.getBranchPr.mockResolvedValueOnce(existingPr); - const pr = await prWorker.ensurePr(config); - expect(pr).toBeNull(); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.Error); + expect(pr).toBeUndefined(); }); it('should return null if waiting for success', async () => { platform.getBranchStatus.mockResolvedValueOnce(BranchStatus.red); config.prCreation = 'status-success'; - const pr = await prWorker.ensurePr(config); - expect(pr).toEqual('pending'); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.AwaitingGreenBranch); + expect(pr).toBeUndefined(); }); it('should return needs-approval if prCreation set to approval', async () => { platform.getBranchStatus.mockResolvedValueOnce(BranchStatus.green); config.prCreation = 'approval'; - const pr = await prWorker.ensurePr(config); - expect(pr).toBe('needs-pr-approval'); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.AwaitingApproval); + expect(pr).toBeUndefined(); }); it('should create PR if success', async () => { platform.getBranchStatus.mockResolvedValueOnce(BranchStatus.green); config.prCreation = 'status-success'; config.automerge = true; config.schedule = 'before 5am'; - const pr = await prWorker.ensurePr(config); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.Created); expect(pr).toMatchObject({ displayNumber: 'New Pull Request' }); expect(platform.createPr.mock.calls[0]).toMatchSnapshot(); existingPr.body = platform.createPr.mock.calls[0][0].prBody; @@ -212,7 +217,8 @@ describe('workers/pr', () => { config.updateType = 'lockFileMaintenance'; config.recreateClosed = true; config.rebaseWhen = 'never'; - const pr = await prWorker.ensurePr(config); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.Created); expect(pr).toMatchObject({ displayNumber: 'New Pull Request' }); expect(platform.createPr.mock.calls[0]).toMatchSnapshot(); }); @@ -224,7 +230,8 @@ describe('workers/pr', () => { config.schedule = 'before 5am'; config.timezone = 'some timezone'; config.rebaseWhen = 'behind-base-branch'; - const pr = await prWorker.ensurePr(config); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.Created); expect(pr).toMatchObject({ displayNumber: 'New Pull Request' }); expect(platform.createPr.mock.calls[0]).toMatchSnapshot(); expect( @@ -238,8 +245,9 @@ describe('workers/pr', () => { throw new Error('Validation Failed (422)'); }); config.prCreation = 'status-success'; - const pr = await prWorker.ensurePr(config); - expect(pr).toBeNull(); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.Error); + expect(pr).toBeUndefined(); }); it('should return null if waiting for not pending', async () => { platform.getBranchStatus.mockResolvedValueOnce(BranchStatus.yellow); @@ -247,8 +255,9 @@ describe('workers/pr', () => { Promise.resolve(new Date()) ); config.prCreation = 'not-pending'; - const pr = await prWorker.ensurePr(config); - expect(pr).toEqual('pending'); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.AwaitingNotPending); + expect(pr).toBeUndefined(); }); it('should create PR if pending timeout hit', async () => { platform.getBranchStatus.mockResolvedValueOnce(BranchStatus.yellow); @@ -256,23 +265,27 @@ describe('workers/pr', () => { Promise.resolve(new Date('2017-01-01')) ); config.prCreation = 'not-pending'; - const pr = await prWorker.ensurePr(config); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.Created); expect(pr).toMatchObject({ displayNumber: 'New Pull Request' }); }); it('should create PR if no longer pending', async () => { platform.getBranchStatus.mockResolvedValueOnce(BranchStatus.red); config.prCreation = 'not-pending'; - const pr = await prWorker.ensurePr(config); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.Created); expect(pr).toMatchObject({ displayNumber: 'New Pull Request' }); }); it('should create new branch if none exists', async () => { - const pr = await prWorker.ensurePr(config); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.Created); expect(pr).toMatchObject({ displayNumber: 'New Pull Request' }); }); it('should add assignees and reviewers to new PR', async () => { config.assignees = ['@foo', 'bar']; config.reviewers = ['baz', '@boo']; - const pr = await prWorker.ensurePr(config); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.Created); expect(pr).toMatchObject({ displayNumber: 'New Pull Request' }); expect(platform.addAssignees).toHaveBeenCalledTimes(1); expect(platform.addAssignees.mock.calls).toMatchSnapshot(); @@ -285,7 +298,8 @@ describe('workers/pr', () => { }); config.assignees = ['@foo', 'bar']; config.reviewers = ['baz', '@boo']; - const pr = await prWorker.ensurePr(config); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.Created); expect(pr).toMatchObject({ displayNumber: 'New Pull Request' }); expect(platform.addAssignees).toHaveBeenCalledTimes(1); expect(platform.addReviewers).toHaveBeenCalledTimes(1); @@ -296,7 +310,8 @@ describe('workers/pr', () => { }); config.assignees = ['@foo', 'bar']; config.reviewers = ['baz', '@boo']; - const pr = await prWorker.ensurePr(config); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.Created); expect(pr).toMatchObject({ displayNumber: 'New Pull Request' }); expect(platform.addAssignees).toHaveBeenCalledTimes(1); expect(platform.addReviewers).toHaveBeenCalledTimes(1); @@ -305,7 +320,8 @@ describe('workers/pr', () => { config.assignees = ['bar']; config.reviewers = ['baz']; config.automerge = true; - const pr = await prWorker.ensurePr(config); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.Created); expect(pr).toMatchObject({ displayNumber: 'New Pull Request' }); expect(platform.addAssignees).toHaveBeenCalledTimes(0); expect(platform.addReviewers).toHaveBeenCalledTimes(0); @@ -315,7 +331,8 @@ describe('workers/pr', () => { config.reviewers = ['baz']; config.automerge = true; config.assignAutomerge = true; - const pr = await prWorker.ensurePr(config); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.Created); expect(pr).toMatchObject({ displayNumber: 'New Pull Request' }); expect(platform.addAssignees).toHaveBeenCalledTimes(1); expect(platform.addReviewers).toHaveBeenCalledTimes(1); @@ -325,8 +342,9 @@ describe('workers/pr', () => { config.assigneesSampleSize = 2; config.reviewers = ['baz', 'boo', 'bor']; config.reviewersSampleSize = 2; - await prWorker.ensurePr(config); - + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.Created); + expect(pr).toMatchObject({ displayNumber: 'New Pull Request' }); expect(platform.addAssignees).toHaveBeenCalledTimes(1); const assignees = platform.addAssignees.mock.calls[0][1]; expect(assignees.length).toEqual(2); @@ -340,7 +358,8 @@ describe('workers/pr', () => { it('should add and deduplicate additionalReviewers on new PR', async () => { config.reviewers = ['@foo', 'bar']; config.additionalReviewers = ['bar', 'baz', '@boo']; - const pr = await prWorker.ensurePr(config); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.Created); expect(pr).toMatchObject({ displayNumber: 'New Pull Request' }); expect(platform.addReviewers).toHaveBeenCalledTimes(1); expect(platform.addReviewers.mock.calls).toMatchSnapshot(); @@ -350,7 +369,8 @@ describe('workers/pr', () => { config.semanticCommitScope = null; config.automerge = true; config.schedule = 'before 5am'; - const pr = await prWorker.ensurePr(config); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.NotUpdated); expect(platform.updatePr.mock.calls).toMatchSnapshot(); expect(platform.updatePr).toHaveBeenCalledTimes(0); expect(pr).toMatchObject(existingPr); @@ -365,7 +385,8 @@ describe('workers/pr', () => { config.semanticCommitScope = null; config.automerge = true; config.schedule = 'before 5am'; - const pr = await prWorker.ensurePr(config); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.NotUpdated); expect(platform.updatePr).toHaveBeenCalledTimes(0); expect(pr).toMatchObject(modifiedPr); }); @@ -374,7 +395,8 @@ describe('workers/pr', () => { config.automerge = true; config.schedule = 'before 5am'; platform.getBranchPr.mockResolvedValueOnce(existingPr); - const pr = await prWorker.ensurePr(config); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.NotUpdated); expect(pr).toMatchSnapshot(); }); it('should return modified existing PR title', async () => { @@ -383,7 +405,8 @@ describe('workers/pr', () => { ...existingPr, title: 'wrong', }); - const pr = await prWorker.ensurePr(config); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.Updated); expect(pr).toMatchSnapshot(); }); it('should create PR if branch tests failed', async () => { @@ -391,7 +414,8 @@ describe('workers/pr', () => { config.automergeType = 'branch'; config.branchAutomergeFailureMessage = 'branch status error'; platform.getBranchStatus.mockResolvedValueOnce(BranchStatus.red); - const pr = await prWorker.ensurePr(config); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.Created); expect(pr).toMatchObject({ displayNumber: 'New Pull Request' }); }); it('should create PR if branch automerging failed', async () => { @@ -399,37 +423,42 @@ describe('workers/pr', () => { config.automergeType = 'branch'; platform.getBranchStatus.mockResolvedValueOnce(BranchStatus.green); config.forcePr = true; - const pr = await prWorker.ensurePr(config); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.Created); expect(pr).toMatchObject({ displayNumber: 'New Pull Request' }); }); - it('should return null if branch automerging not failed', async () => { + it('should return no PR if branch automerging not failed', async () => { config.automerge = true; config.automergeType = 'branch'; platform.getBranchStatus.mockResolvedValueOnce(BranchStatus.yellow); platform.getBranchLastCommitTime.mockResolvedValueOnce(new Date()); - const pr = await prWorker.ensurePr(config); - expect(pr).toBeNull(); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.AwaitingBranchAutomerge); + expect(pr).toBeUndefined(); }); - it('should not return null if branch automerging taking too long', async () => { + it('should not return no PR if branch automerging taking too long', async () => { config.automerge = true; config.automergeType = 'branch'; platform.getBranchStatus.mockResolvedValueOnce(BranchStatus.yellow); platform.getBranchLastCommitTime.mockResolvedValueOnce( new Date('2018-01-01') ); - const pr = await prWorker.ensurePr(config); - expect(pr).not.toBeNull(); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.Created); + expect(pr).toBeDefined(); }); it('handles duplicate upgrades', async () => { config.upgrades.push(config.upgrades[0]); - const pr = await prWorker.ensurePr(config); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.Created); expect(pr).toMatchObject({ displayNumber: 'New Pull Request' }); }); it('should create privateRepo PR if success', async () => { platform.getBranchStatus.mockResolvedValueOnce(BranchStatus.green); config.prCreation = 'status-success'; config.privateRepo = false; - const pr = await prWorker.ensurePr(config); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.Created); expect(pr).toMatchObject({ displayNumber: 'New Pull Request' }); expect(platform.createPr.mock.calls[0]).toMatchSnapshot(); existingPr.body = platform.createPr.mock.calls[0][0].prBody; @@ -440,7 +469,8 @@ describe('workers/pr', () => { config.prCreation = 'not-pending'; config.artifactErrors = [{}]; config.platform = PLATFORM_TYPE_GITLAB; - const pr = await prWorker.ensurePr(config); + const { result, pr } = await prWorker.ensurePr(config); + expect(result).toEqual(PrResult.Created); expect(pr).toMatchObject({ displayNumber: 'New Pull Request' }); }); diff --git a/lib/workers/pr/index.ts b/lib/workers/pr/index.ts index 755b28a0405d17..82454269f90ea7 100644 --- a/lib/workers/pr/index.ts +++ b/lib/workers/pr/index.ts @@ -70,10 +70,25 @@ export async function addAssigneesReviewers(config, pr: Pr): Promise { } } +export enum PrResult { + AwaitingBranchAutomerge = 'awaiting-branch-automerge', + AwaitingApproval = 'awaiting-approval', + AwaitingGreenBranch = 'awaiting-green-branch', + AwaitingNotPending = 'awaiting-not-pending', + Created = 'created', + Error = 'error', + ErrorAlreadyExists = 'error-already-exists', + NotUpdated = 'not-updated', + Updated = 'updated', +} + // Ensures that PR exists with matching title/body export async function ensurePr( prConfig: BranchConfig -): Promise { +): Promise<{ + result: PrResult; + pr?: Pr; +}> { const config: BranchConfig = { ...prConfig }; logger.trace({ config }, 'ensurePr'); @@ -129,7 +144,8 @@ export async function ensurePr( if (config.forcePr || (await getBranchStatus()) === BranchStatus.red) { logger.debug(`Branch tests failed, so will create PR`); } else { - return null; + // Branch should be automerged, so we don't want to create a PR + return { result: PrResult.AwaitingBranchAutomerge }; } } if (config.prCreation === 'status-success') { @@ -138,7 +154,7 @@ export async function ensurePr( logger.debug( `Branch status is "${await getBranchStatus()}" - not creating PR` ); - return 'pending'; + return { result: PrResult.AwaitingGreenBranch }; } logger.debug('Branch status success'); } else if ( @@ -146,7 +162,7 @@ export async function ensurePr( !existingPr && masterIssueCheck !== 'approvePr' ) { - return 'needs-pr-approval'; + return { result: PrResult.AwaitingApproval }; } else if ( config.prCreation === 'not-pending' && !existingPr && @@ -167,7 +183,7 @@ export async function ensurePr( logger.debug( `Branch is ${elapsedHours} hours old - skipping PR creation` ); - return 'pending'; + return { result: PrResult.AwaitingNotPending }; } logger.debug( `prNotPendingHours=${config.prNotPendingHours} threshold hit - creating PR` @@ -269,7 +285,7 @@ export async function ensurePr( noWhitespace(existingPrBody) === noWhitespace(prBody) ) { logger.debug(`${existingPr.displayNumber} does not need updating`); - return existingPr; + return { result: PrResult.NotUpdated, pr: existingPr }; } // PR must need updating if (existingPr.title !== prTitle) { @@ -298,7 +314,7 @@ export async function ensurePr( await platform.updatePr(existingPr.number, prTitle, prBody); logger.info({ pr: existingPr.number, prTitle }, `PR updated`); } - return existingPr; + return { result: PrResult.Updated, pr: existingPr }; } logger.debug({ branch: branchName, prTitle }, `Creating PR`); // istanbul ignore if @@ -342,7 +358,7 @@ export async function ensurePr( ) ) { logger.warn('A pull requests already exists'); - return null; + return { result: PrResult.ErrorAlreadyExists }; } } } @@ -357,7 +373,7 @@ export async function ensurePr( await platform.deleteBranch(branchName); } } - return null; + return { result: PrResult.Error }; } if ( config.branchAutomergeFailureMessage && @@ -394,7 +410,7 @@ export async function ensurePr( await addAssigneesReviewers(config, pr); } logger.debug(`Created ${pr.displayNumber}`); - return pr; + return { result: PrResult.Created, pr }; } catch (err) { // istanbul ignore if if ( @@ -408,7 +424,7 @@ export async function ensurePr( } logger.error({ err }, 'Failed to ensure PR: ' + prTitle); } - return null; + return { result: PrResult.Error }; } export async function checkAutoMerge(pr: Pr, config): Promise {