From 1142d1b98190fac0031609960f07a8124dc4b6ed Mon Sep 17 00:00:00 2001 From: Rhys Arkins Date: Sun, 8 Mar 2020 16:24:45 +0100 Subject: [PATCH 1/7] refactor: PrResult enum --- lib/types/index.ts | 2 +- lib/workers/branch/index.spec.ts | 44 ++++++-- lib/workers/branch/index.ts | 11 +- .../pr/__snapshots__/index.spec.ts.snap | 12 +- lib/workers/pr/index.spec.ts | 106 +++++++++++------- lib/workers/pr/index.ts | 40 +++++-- 6 files changed, 146 insertions(+), 69 deletions(-) diff --git a/lib/types/index.ts b/lib/types/index.ts index 95cfeb6540df39..7c79edbff553c4 100644 --- a/lib/types/index.ts +++ b/lib/types/index.ts @@ -1,3 +1,3 @@ +export * from './branch-status'; export * from './host-rules'; export * from './versioning'; -export * from './branch-status'; 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/__snapshots__/index.spec.ts.snap b/lib/workers/pr/__snapshots__/index.spec.ts.snap index 571e4b3c243437..acc733ac143bbf 100644 --- a/lib/workers/pr/__snapshots__/index.spec.ts.snap +++ b/lib/workers/pr/__snapshots__/index.spec.ts.snap @@ -106,22 +106,22 @@ Array [ ] `; -exports[`workers/pr ensurePr should return modified existing PR 1`] = ` +exports[`workers/pr ensurePr should return modified existing PR title 1`] = ` Object { "body": "This PR contains the following updates:\\n\\n| Package | Type | Update | Change |\\n|---|---|---|---|\\n| [dummy](https://dummy.com) ([source](https://github.com/renovateapp/dummy), [changelog](https://github.com/renovateapp/dummy/changelog.md)) | devDependencies | minor | \`1.0.0\` -> \`1.1.0\` |\\n\\n---\\n\\n### Release Notes\\n\\n
\\nrenovateapp/dummy\\n\\n### [\`v1.1.0\`](https://github.com/renovateapp/dummy/compare/v1.0.0...v1.1.0)\\n\\n[Compare Source](https://github.com/renovateapp/dummy/compare/v1.0.0...v1.1.0)\\n\\n
\\n\\n---\\n\\n### Renovate configuration\\n\\n:date: **Schedule**: \\"before 5am\\" (UTC).\\n\\n:vertical_traffic_light: **Automerge**: Enabled.\\n\\n:recycle: **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.\\n\\n:no_bell: **Ignore**: Close this PR and you won't be reminded about this update again.\\n\\n---\\n\\n - [ ] If you want to rebase/retry this PR, check this box", "displayNumber": "Existing PR", "isModified": false, - "title": "Update dependency dummy to v1.1.0", + "title": "wrong", } `; -exports[`workers/pr ensurePr should return modified existing PR title 1`] = ` +exports[`workers/pr ensurePr should return unmodified existing PR 1`] = `Array []`; + +exports[`workers/pr ensurePr should return unmodified existing PR 2`] = ` Object { "body": "This PR contains the following updates:\\n\\n| Package | Type | Update | Change |\\n|---|---|---|---|\\n| [dummy](https://dummy.com) ([source](https://github.com/renovateapp/dummy), [changelog](https://github.com/renovateapp/dummy/changelog.md)) | devDependencies | minor | \`1.0.0\` -> \`1.1.0\` |\\n\\n---\\n\\n### Release Notes\\n\\n
\\nrenovateapp/dummy\\n\\n### [\`v1.1.0\`](https://github.com/renovateapp/dummy/compare/v1.0.0...v1.1.0)\\n\\n[Compare Source](https://github.com/renovateapp/dummy/compare/v1.0.0...v1.1.0)\\n\\n
\\n\\n---\\n\\n### Renovate configuration\\n\\n:date: **Schedule**: \\"before 5am\\" (UTC).\\n\\n:vertical_traffic_light: **Automerge**: Enabled.\\n\\n:recycle: **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.\\n\\n:no_bell: **Ignore**: Close this PR and you won't be reminded about this update again.\\n\\n---\\n\\n - [ ] If you want to rebase/retry this PR, check this box", "displayNumber": "Existing PR", "isModified": false, - "title": "wrong", + "title": "Update dependency dummy to v1.1.0", } `; - -exports[`workers/pr ensurePr should return unmodified existing PR 1`] = `Array []`; diff --git a/lib/workers/pr/index.spec.ts b/lib/workers/pr/index.spec.ts index a1748f71e16498..d49d1abfe2e708 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,16 +385,18 @@ 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); }); - it('should return modified existing PR', async () => { + it('should return unmodified existing PR', async () => { config.newValue = '1.2.0'; 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,7 +423,8 @@ 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 () => { @@ -407,8 +432,9 @@ describe('workers/pr', () => { 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.AutomergeBranchInstead); + expect(pr).toBeUndefined(); }); it('should not return null if branch automerging taking too long', async () => { config.automerge = true; @@ -417,19 +443,22 @@ describe('workers/pr', () => { 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..0bfd6be4a8a646 100644 --- a/lib/workers/pr/index.ts +++ b/lib/workers/pr/index.ts @@ -70,10 +70,27 @@ export async function addAssigneesReviewers(config, pr: Pr): Promise { } } +export enum PrResult { + AutomergeBranchInstead = 'automerge-branch-instead', + AwaitingApproval = 'awaiting-approval', + AwaitingGreenBranch = 'awaiting-green-branch', + AwaitingNotPending = 'awaiting-not-pending', + Created = 'created', + Error = 'error', + ErrorAlreadyExists = 'error-already-exists', + NotUpdated = 'not-updated', + Updated = 'updated', +} + +export interface EnsurePrResult { + result: PrResult; + pr?: Pr; +} + // Ensures that PR exists with matching title/body export async function ensurePr( prConfig: BranchConfig -): Promise { +): Promise { const config: BranchConfig = { ...prConfig }; logger.trace({ config }, 'ensurePr'); @@ -129,7 +146,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.AutomergeBranchInstead }; } } if (config.prCreation === 'status-success') { @@ -138,7 +156,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 +164,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 +185,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 +287,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 +316,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 +360,7 @@ export async function ensurePr( ) ) { logger.warn('A pull requests already exists'); - return null; + return { result: PrResult.ErrorAlreadyExists }; } } } @@ -357,7 +375,7 @@ export async function ensurePr( await platform.deleteBranch(branchName); } } - return null; + return { result: PrResult.Error }; } if ( config.branchAutomergeFailureMessage && @@ -394,7 +412,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 +426,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 { From 369d3642d07bebe9a9123675a68ce8c5b3e39ad2 Mon Sep 17 00:00:00 2001 From: Rhys Arkins Date: Sun, 8 Mar 2020 16:55:21 +0100 Subject: [PATCH 2/7] revert types ordering --- lib/types/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/types/index.ts b/lib/types/index.ts index 7c79edbff553c4..95cfeb6540df39 100644 --- a/lib/types/index.ts +++ b/lib/types/index.ts @@ -1,3 +1,3 @@ -export * from './branch-status'; export * from './host-rules'; export * from './versioning'; +export * from './branch-status'; From 7de9cf75a99601228aee7a4e697d4f6a0494d2d8 Mon Sep 17 00:00:00 2001 From: Rhys Arkins Date: Sun, 8 Mar 2020 16:58:01 +0100 Subject: [PATCH 3/7] revert test name change --- lib/workers/pr/index.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/workers/pr/index.spec.ts b/lib/workers/pr/index.spec.ts index d49d1abfe2e708..fea306aaa88c78 100644 --- a/lib/workers/pr/index.spec.ts +++ b/lib/workers/pr/index.spec.ts @@ -390,7 +390,7 @@ describe('workers/pr', () => { expect(platform.updatePr).toHaveBeenCalledTimes(0); expect(pr).toMatchObject(modifiedPr); }); - it('should return unmodified existing PR', async () => { + it('should return modified existing PR', async () => { config.newValue = '1.2.0'; config.automerge = true; config.schedule = 'before 5am'; From 1e09389f2eeb065db40670022f2801d38bd7e316 Mon Sep 17 00:00:00 2001 From: Rhys Arkins Date: Sun, 8 Mar 2020 17:01:35 +0100 Subject: [PATCH 4/7] update snapshot --- lib/workers/pr/__snapshots__/index.spec.ts.snap | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/workers/pr/__snapshots__/index.spec.ts.snap b/lib/workers/pr/__snapshots__/index.spec.ts.snap index acc733ac143bbf..571e4b3c243437 100644 --- a/lib/workers/pr/__snapshots__/index.spec.ts.snap +++ b/lib/workers/pr/__snapshots__/index.spec.ts.snap @@ -106,22 +106,22 @@ Array [ ] `; -exports[`workers/pr ensurePr should return modified existing PR title 1`] = ` +exports[`workers/pr ensurePr should return modified existing PR 1`] = ` Object { "body": "This PR contains the following updates:\\n\\n| Package | Type | Update | Change |\\n|---|---|---|---|\\n| [dummy](https://dummy.com) ([source](https://github.com/renovateapp/dummy), [changelog](https://github.com/renovateapp/dummy/changelog.md)) | devDependencies | minor | \`1.0.0\` -> \`1.1.0\` |\\n\\n---\\n\\n### Release Notes\\n\\n
\\nrenovateapp/dummy\\n\\n### [\`v1.1.0\`](https://github.com/renovateapp/dummy/compare/v1.0.0...v1.1.0)\\n\\n[Compare Source](https://github.com/renovateapp/dummy/compare/v1.0.0...v1.1.0)\\n\\n
\\n\\n---\\n\\n### Renovate configuration\\n\\n:date: **Schedule**: \\"before 5am\\" (UTC).\\n\\n:vertical_traffic_light: **Automerge**: Enabled.\\n\\n:recycle: **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.\\n\\n:no_bell: **Ignore**: Close this PR and you won't be reminded about this update again.\\n\\n---\\n\\n - [ ] If you want to rebase/retry this PR, check this box", "displayNumber": "Existing PR", "isModified": false, - "title": "wrong", + "title": "Update dependency dummy to v1.1.0", } `; -exports[`workers/pr ensurePr should return unmodified existing PR 1`] = `Array []`; - -exports[`workers/pr ensurePr should return unmodified existing PR 2`] = ` +exports[`workers/pr ensurePr should return modified existing PR title 1`] = ` Object { "body": "This PR contains the following updates:\\n\\n| Package | Type | Update | Change |\\n|---|---|---|---|\\n| [dummy](https://dummy.com) ([source](https://github.com/renovateapp/dummy), [changelog](https://github.com/renovateapp/dummy/changelog.md)) | devDependencies | minor | \`1.0.0\` -> \`1.1.0\` |\\n\\n---\\n\\n### Release Notes\\n\\n
\\nrenovateapp/dummy\\n\\n### [\`v1.1.0\`](https://github.com/renovateapp/dummy/compare/v1.0.0...v1.1.0)\\n\\n[Compare Source](https://github.com/renovateapp/dummy/compare/v1.0.0...v1.1.0)\\n\\n
\\n\\n---\\n\\n### Renovate configuration\\n\\n:date: **Schedule**: \\"before 5am\\" (UTC).\\n\\n:vertical_traffic_light: **Automerge**: Enabled.\\n\\n:recycle: **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.\\n\\n:no_bell: **Ignore**: Close this PR and you won't be reminded about this update again.\\n\\n---\\n\\n - [ ] If you want to rebase/retry this PR, check this box", "displayNumber": "Existing PR", "isModified": false, - "title": "Update dependency dummy to v1.1.0", + "title": "wrong", } `; + +exports[`workers/pr ensurePr should return unmodified existing PR 1`] = `Array []`; From 5cfb21eb49f53c563d6fc92c0ff495d8b313a9f4 Mon Sep 17 00:00:00 2001 From: Rhys Arkins Date: Sun, 8 Mar 2020 21:03:55 +0100 Subject: [PATCH 5/7] simplify --- lib/workers/pr/index.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/lib/workers/pr/index.ts b/lib/workers/pr/index.ts index 0bfd6be4a8a646..d3f245806f81d6 100644 --- a/lib/workers/pr/index.ts +++ b/lib/workers/pr/index.ts @@ -82,15 +82,13 @@ export enum PrResult { Updated = 'updated', } -export interface EnsurePrResult { - result: PrResult; - pr?: Pr; -} - // 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'); From 2c34c155463ab07af5dc23dc71db888ec15aec74 Mon Sep 17 00:00:00 2001 From: Rhys Arkins Date: Tue, 10 Mar 2020 07:03:42 +0100 Subject: [PATCH 6/7] rename branch automerge option --- lib/workers/pr/index.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/workers/pr/index.ts b/lib/workers/pr/index.ts index d3f245806f81d6..82454269f90ea7 100644 --- a/lib/workers/pr/index.ts +++ b/lib/workers/pr/index.ts @@ -71,7 +71,7 @@ export async function addAssigneesReviewers(config, pr: Pr): Promise { } export enum PrResult { - AutomergeBranchInstead = 'automerge-branch-instead', + AwaitingBranchAutomerge = 'awaiting-branch-automerge', AwaitingApproval = 'awaiting-approval', AwaitingGreenBranch = 'awaiting-green-branch', AwaitingNotPending = 'awaiting-not-pending', @@ -145,7 +145,7 @@ export async function ensurePr( logger.debug(`Branch tests failed, so will create PR`); } else { // Branch should be automerged, so we don't want to create a PR - return { result: PrResult.AutomergeBranchInstead }; + return { result: PrResult.AwaitingBranchAutomerge }; } } if (config.prCreation === 'status-success') { From 65c1980257435b9c9b4731e24b621e5f0515d95e Mon Sep 17 00:00:00 2001 From: Rhys Arkins Date: Tue, 10 Mar 2020 07:09:01 +0100 Subject: [PATCH 7/7] fix test --- lib/workers/pr/index.spec.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/workers/pr/index.spec.ts b/lib/workers/pr/index.spec.ts index fea306aaa88c78..780c26af3d0c0e 100644 --- a/lib/workers/pr/index.spec.ts +++ b/lib/workers/pr/index.spec.ts @@ -427,16 +427,16 @@ describe('workers/pr', () => { 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 { result, pr } = await prWorker.ensurePr(config); - expect(result).toEqual(PrResult.AutomergeBranchInstead); + 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);