From f3285115a9c0e4a199f86038319bafd6d604d96a Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Fri, 14 Oct 2022 12:15:15 -0700 Subject: [PATCH] fix: updating a pull request uses overflow handler if body is too large (#1702) * test: failing test for updating pull request with overflow handler * fix: updating a pull request uses overflow handler if body is too large * test: add tests for updatePullRequest handling overflow --- src/github.ts | 17 ++++- src/manifest.ts | 2 + test/github.ts | 35 ++++++++++ test/helpers.ts | 34 +++++++++- test/manifest.ts | 169 +++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 254 insertions(+), 3 deletions(-) diff --git a/src/github.ts b/src/github.ts index bb78e3a48..759451329 100644 --- a/src/github.ts +++ b/src/github.ts @@ -49,6 +49,7 @@ import { import {Logger} from 'code-suggester/build/src/types'; import {HttpsProxyAgent} from 'https-proxy-agent'; import {HttpProxyAgent} from 'http-proxy-agent'; +import {PullRequestOverflowHandler} from './util/pull-request-overflow-handler'; // Extract some types from the `request` package. type RequestBuilderType = typeof request; @@ -1162,7 +1163,12 @@ export class GitHub { * Update a pull request's title and body. * @param {number} number The pull request number * @param {ReleasePullRequest} releasePullRequest Pull request data to update - * @param {} + * @param {string} targetBranch The target branch of the pull request + * @param {string} options.signoffUser Optional. Commit signoff message + * @param {boolean} options.fork Optional. Whether to open the pull request from + * a fork or not. Defaults to `false` + * @param {PullRequestOverflowHandler} options.pullRequestOverflowHandler Optional. + * Handles extra large pull request body messages. */ updatePullRequest = wrapAsync( async ( @@ -1172,6 +1178,7 @@ export class GitHub { options?: { signoffUser?: string; fork?: boolean; + pullRequestOverflowHandler?: PullRequestOverflowHandler; } ): Promise => { // Update the files for the release if not already supplied @@ -1184,7 +1191,13 @@ export class GitHub { message = signoffCommitMessage(message, options.signoffUser); } const title = releasePullRequest.title.toString(); - const body = releasePullRequest.body + const body = ( + options?.pullRequestOverflowHandler + ? await options.pullRequestOverflowHandler.handleOverflow( + releasePullRequest + ) + : releasePullRequest.body + ) .toString() .slice(0, MAX_ISSUE_BODY_SIZE); const prNumber = await createPullRequest(this.octokit, changes, { diff --git a/src/manifest.ts b/src/manifest.ts index 64b1dae6c..1702e633f 100644 --- a/src/manifest.ts +++ b/src/manifest.ts @@ -975,6 +975,7 @@ export class Manifest { { fork: this.fork, signoffUser: this.signoffUser, + pullRequestOverflowHandler: this.pullRequestOverflowHandler, } ); return updatedPullRequest; @@ -999,6 +1000,7 @@ export class Manifest { { fork: this.fork, signoffUser: this.signoffUser, + pullRequestOverflowHandler: this.pullRequestOverflowHandler, } ); // TODO: consider leaving the snooze label diff --git a/test/github.ts b/test/github.ts index 72d792616..2a359f05c 100644 --- a/test/github.ts +++ b/test/github.ts @@ -40,6 +40,7 @@ import {RawContent} from '../src/updaters/raw-content'; import {HttpsProxyAgent} from 'https-proxy-agent'; import {HttpProxyAgent} from 'http-proxy-agent'; import {Commit} from '../src/commit'; +import {mockReleaseData, MockPullRequestOverflowHandler} from './helpers'; const fixturesPath = './test/fixtures'; const sandbox = sinon.createSandbox(); @@ -1141,4 +1142,38 @@ describe('GitHub', () => { expect(url).to.eql('https://github.com/fake/fake/blob/new-file.txt'); }); }); + + describe('updatePullRequest', () => { + it('handles a PR body that is too big', async () => { + req = req.patch('/repos/fake/fake/pulls/123').reply(200, { + number: 123, + title: 'updated-title', + body: 'updated body', + labels: [], + head: { + ref: 'abc123', + }, + base: { + ref: 'def234', + }, + }); + const pullRequest = { + title: PullRequestTitle.ofTargetBranch('main'), + body: new PullRequestBody(mockReleaseData(1000), {useComponents: true}), + labels: [], + headRefName: 'release-please--branches--main', + draft: false, + updates: [], + }; + const pullRequestOverflowHandler = new MockPullRequestOverflowHandler(); + const handleOverflowStub = sandbox + .stub(pullRequestOverflowHandler, 'handleOverflow') + .resolves('overflow message'); + await github.updatePullRequest(123, pullRequest, 'main', { + pullRequestOverflowHandler, + }); + sinon.assert.calledOnce(handleOverflowStub); + req.done(); + }); + }); }); diff --git a/test/helpers.ts b/test/helpers.ts index a4356cbe7..5544ca12c 100644 --- a/test/helpers.ts +++ b/test/helpers.ts @@ -27,7 +27,7 @@ import {expect} from 'chai'; import {CandidateReleasePullRequest} from '../src/manifest'; import {Version} from '../src/version'; import {PullRequestTitle} from '../src/util/pull-request-title'; -import {PullRequestBody} from '../src/util/pull-request-body'; +import {PullRequestBody, ReleaseData} from '../src/util/pull-request-body'; import {BranchName} from '../src/util/branch-name'; import {ReleaseType} from '../src/factory'; import { @@ -35,6 +35,9 @@ import { DEFAULT_FILE_MODE, } from '@google-automations/git-file-utils'; import {CompositeUpdater} from '../src/updaters/composite'; +import {PullRequestOverflowHandler} from '../src/util/pull-request-overflow-handler'; +import {ReleasePullRequest} from '../src/release-pull-request'; +import {PullRequest} from '../src/pull-request'; export function stubSuggesterWithSnapshot( sandbox: sinon.SinonSandbox, @@ -372,3 +375,32 @@ export function mockTags( } return sandbox.stub(github, 'tagIterator').returns(fakeGenerator()); } + +export function mockReleaseData(count: number): ReleaseData[] { + const releaseData: ReleaseData[] = []; + const version = Version.parse('1.2.3'); + for (let i = 0; i < count; i++) { + releaseData.push({ + component: `component${i}`, + version, + notes: `release notes for component${i}`, + }); + } + return releaseData; +} + +export class MockPullRequestOverflowHandler + implements PullRequestOverflowHandler +{ + async handleOverflow( + pullRequest: ReleasePullRequest, + _maxSize?: number | undefined + ): Promise { + return pullRequest.body.toString(); + } + async parseOverflow( + pullRequest: PullRequest + ): Promise { + return PullRequestBody.parse(pullRequest.body); + } +} diff --git a/test/manifest.ts b/test/manifest.ts index 9ff9cb543..be24dec48 100644 --- a/test/manifest.ts +++ b/test/manifest.ts @@ -28,6 +28,7 @@ import { mockReleases, mockTags, assertNoHasUpdate, + mockReleaseData, } from './helpers'; import {expect} from 'chai'; import * as assert from 'assert'; @@ -3735,6 +3736,174 @@ describe('Manifest', () => { expect(pullRequestNumbers).lengthOf(1); }); + describe('with an overflowing body', () => { + const body = new PullRequestBody(mockReleaseData(1000), { + useComponents: true, + }); + + it('updates an existing pull request', async function () { + sandbox + .stub(github, 'getFileContentsOnBranch') + .withArgs('README.md', 'main') + .resolves(buildGitHubFileRaw('some-content')); + stubSuggesterWithSnapshot(sandbox, this.test!.fullTitle()); + mockPullRequests( + github, + [ + { + number: 22, + title: 'pr title1', + body: pullRequestBody('release-notes/single.txt'), + headBranchName: 'release-please/branches/main', + baseBranchName: 'main', + labels: ['autorelease: pending'], + files: [], + }, + ], + [] + ); + sandbox + .stub(github, 'updatePullRequest') + .withArgs( + 22, + sinon.match.any, + sinon.match.any, + sinon.match.has('pullRequestOverflowHandler', sinon.match.truthy) + ) + .resolves({ + number: 22, + title: 'pr title1', + body: 'pr body1', + headBranchName: 'release-please/branches/main', + baseBranchName: 'main', + labels: [], + files: [], + }); + const manifest = new Manifest( + github, + 'main', + { + 'path/a': { + releaseType: 'node', + component: 'pkg1', + }, + 'path/b': { + releaseType: 'node', + component: 'pkg2', + }, + }, + { + 'path/a': Version.parse('1.0.0'), + 'path/b': Version.parse('0.2.3'), + }, + { + separatePullRequests: true, + plugins: ['node-workspace'], + } + ); + sandbox.stub(manifest, 'buildPullRequests').resolves([ + { + title: PullRequestTitle.ofTargetBranch('main'), + body, + updates: [ + { + path: 'README.md', + createIfMissing: false, + updater: new RawContent('some raw content'), + }, + ], + labels: [], + headRefName: 'release-please/branches/main', + draft: false, + }, + ]); + const pullRequestNumbers = await manifest.createPullRequests(); + expect(pullRequestNumbers).lengthOf(1); + }); + + it('ignores an existing pull request if there are no changes', async function () { + sandbox + .stub(github, 'getFileContentsOnBranch') + .withArgs('README.md', 'main') + .resolves(buildGitHubFileRaw('some-content')) + .withArgs('release-notes.md', 'my-head-branch--release-notes') + .resolves(buildGitHubFileRaw(body.toString())); + stubSuggesterWithSnapshot(sandbox, this.test!.fullTitle()); + mockPullRequests( + github, + [ + { + number: 22, + title: 'pr title1', + body: pullRequestBody('release-notes/overflow.txt'), + headBranchName: 'release-please/branches/main', + baseBranchName: 'main', + labels: ['autorelease: pending'], + files: [], + }, + ], + [] + ); + sandbox + .stub(github, 'updatePullRequest') + .withArgs( + 22, + sinon.match.any, + sinon.match.any, + sinon.match.has('pullRequestOverflowHandler', sinon.match.truthy) + ) + .resolves({ + number: 22, + title: 'pr title1', + body: 'pr body1', + headBranchName: 'release-please/branches/main', + baseBranchName: 'main', + labels: [], + files: [], + }); + const manifest = new Manifest( + github, + 'main', + { + 'path/a': { + releaseType: 'node', + component: 'pkg1', + }, + 'path/b': { + releaseType: 'node', + component: 'pkg2', + }, + }, + { + 'path/a': Version.parse('1.0.0'), + 'path/b': Version.parse('0.2.3'), + }, + { + separatePullRequests: true, + plugins: ['node-workspace'], + } + ); + sandbox.stub(manifest, 'buildPullRequests').resolves([ + { + title: PullRequestTitle.ofTargetBranch('main'), + body, + updates: [ + { + path: 'README.md', + createIfMissing: false, + updater: new RawContent('some raw content'), + }, + ], + labels: [], + headRefName: 'release-please/branches/main', + draft: false, + }, + ]); + const pullRequestNumbers = await manifest.createPullRequests(); + expect(pullRequestNumbers).lengthOf(0); + }); + }); + it('updates an existing snapshot pull request', async function () { sandbox .stub(github, 'getFileContentsOnBranch')