From c36d2c921bbee3ecc961b60e9de8d43b47ffa318 Mon Sep 17 00:00:00 2001 From: Jake Awe Date: Tue, 16 Jan 2024 10:10:36 -0600 Subject: [PATCH 01/19] add forward-merger code --- package-lock.json | 33 ++ package.json | 1 + src/config.ts | 2 + src/index.ts | 2 + src/plugins/ForwardMerger/forward_merger.ts | 135 +++++++ src/plugins/ForwardMerger/index.ts | 28 ++ test/forward_merger.test.ts | 417 ++++++++++++++++++++ 7 files changed, 618 insertions(+) create mode 100644 src/plugins/ForwardMerger/forward_merger.ts create mode 100644 src/plugins/ForwardMerger/index.ts create mode 100644 test/forward_merger.test.ts diff --git a/package-lock.json b/package-lock.json index f6a6807..323180d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -25,6 +25,7 @@ "@typescript-eslint/parser": "^5.62.0", "eslint": "^8.48.0", "jest": "^29.6.4", + "smee-client": "^2.0.0", "ts-jest": "^29.1.1", "typescript": "^4.9.5" }, @@ -4533,6 +4534,15 @@ "node": ">= 0.8" } }, + "node_modules/commander": { + "version": "11.1.0", + "resolved": "https://registry.npmjs.org/commander/-/commander-11.1.0.tgz", + "integrity": "sha512-yPVavfyCcRhmorC7rWlkHn15b4wDVgVmBA7kV4QVBsF7kv/9TKJAbAXVTxvTnwP8HHKjRCJDClKbciiYS7p0DQ==", + "dev": true, + "engines": { + "node": ">=16" + } + }, "node_modules/concat-map": { "version": "0.0.1", "resolved": "https://registry.npmjs.org/concat-map/-/concat-map-0.0.1.tgz", @@ -8117,6 +8127,20 @@ "node": ">=8" } }, + "node_modules/smee-client": { + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/smee-client/-/smee-client-2.0.0.tgz", + "integrity": "sha512-LqJAAU4uayG909u8q3lBQZma9TDsQl2pOoXCqrsAda58oxy2o81yQAeBvFc2ilexgQDJc3YPDoPgViBM26M8vw==", + "dev": true, + "dependencies": { + "commander": "^11.1.0", + "eventsource": "^2.0.2", + "validator": "^13.11.0" + }, + "bin": { + "smee": "bin/smee.js" + } + }, "node_modules/sonic-boom": { "version": "2.8.0", "resolved": "https://registry.npmjs.org/sonic-boom/-/sonic-boom-2.8.0.tgz", @@ -8620,6 +8644,15 @@ "integrity": "sha512-ASFBup0Mz1uyiIjANan1jzLQami9z1PoYSZCiiYW2FczPbenXc45FZdBZLzOT+r6+iciuEModtmCti+hjaAk0A==", "dev": true }, + "node_modules/validator": { + "version": "13.11.0", + "resolved": "https://registry.npmjs.org/validator/-/validator-13.11.0.tgz", + "integrity": "sha512-Ii+sehpSfZy+At5nPdnyMhx78fEoPDkR2XW/zimHEL3MyGJQOCQ7WeP20jPYRz7ZCpcKLB21NxuXHF3bxjStBQ==", + "dev": true, + "engines": { + "node": ">= 0.10" + } + }, "node_modules/vary": { "version": "1.1.2", "resolved": "https://registry.npmjs.org/vary/-/vary-1.1.2.tgz", diff --git a/package.json b/package.json index 9b9ca21..d65e20d 100644 --- a/package.json +++ b/package.json @@ -37,6 +37,7 @@ "@typescript-eslint/parser": "^5.62.0", "eslint": "^8.48.0", "jest": "^29.6.4", + "smee-client": "^2.0.0", "ts-jest": "^29.1.1", "typescript": "^4.9.5" }, diff --git a/src/config.ts b/src/config.ts index f1d7ecd..b671bf5 100644 --- a/src/config.ts +++ b/src/config.ts @@ -24,6 +24,7 @@ export type OpsBotConfigFeatureNames = { label_checker: boolean; release_drafter: boolean; recently_updated: boolean; + forward_merger: boolean; }; /** @@ -49,6 +50,7 @@ export const DefaultOpsBotConfig: OpsBotConfig = { release_drafter: false, recently_updated: false, recently_updated_threshold: 5, + forward_merger: false, }; /** diff --git a/src/index.ts b/src/index.ts index 264ea4c..5318da1 100644 --- a/src/index.ts +++ b/src/index.ts @@ -20,6 +20,7 @@ import { initBranchChecker } from "./plugins/BranchChecker"; import { initLabelChecker } from "./plugins/LabelChecker"; import { initRecentlyUpdated } from "./plugins/RecentlyUpdated"; import { initReleaseDrafter } from "./plugins/ReleaseDrafter"; +import { initForwardMerger } from "./plugins/ForwardMerger"; export = (app: Probot) => { initBranchChecker(app); @@ -27,4 +28,5 @@ export = (app: Probot) => { initReleaseDrafter(app); initAutoMerger(app); initRecentlyUpdated(app); + initForwardMerger(app); }; diff --git a/src/plugins/ForwardMerger/forward_merger.ts b/src/plugins/ForwardMerger/forward_merger.ts new file mode 100644 index 0000000..92ddb97 --- /dev/null +++ b/src/plugins/ForwardMerger/forward_merger.ts @@ -0,0 +1,135 @@ +/* + * Copyright (c) 2024, NVIDIA CORPORATION. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { OpsBotPlugin } from "../../plugin"; +import { PayloadRepository } from "../../types"; +import { + isVersionedBranch, + getVersionFromBranch, +} from "../../shared"; +import { basename } from "path"; +import { Context } from "probot"; +import { PushEventsType } from "."; + + +export class ForwardMerger extends OpsBotPlugin { + context: Context; + branchName: string; + repo: PayloadRepository; + branchVersionNumber: string; + defaultBranch: string; + + constructor(context: Context, private payload: Context["payload"]) { + super("forward_merger", context); + this.context = context; + this.branchName = basename(this.payload.ref); + this.repo = payload.repository; + this.branchVersionNumber = getVersionFromBranch(this.branchName); + this.defaultBranch = this.repo.default_branch; + } + + async mergeForward() { + if (await this.pluginIsDisabled()) return; + + if (!isVersionedBranch(this.branchName)) { + this.logger.info("Will not merge forward on non-versioned branch"); + return; + } + + const branches = await this.getBranches(); + const sortedBranches = this.sortBranches(branches); + const nextBranch = this.getNextBranch(sortedBranches); + + if (nextBranch) { + const pr = await this.openPR(nextBranch); + + if (pr) { + const merge = await this.mergePR(pr); + if (merge.merged) { + await this.issueComment(pr.number, "**SUCCESS** - forward-merge complete."); + } else { + await this.issueComment(pr.number, "**FAILURE** - Unable to forward-merge due to an error, **manual** merge is necessary. Do not use the `Resolve conflicts` option in this PR, follow these instructions https://docs.rapids.ai/maintainers/forward-merger/ \n **IMPORTANT**: When merging this PR, do not use the [auto-merger](https://docs.rapids.ai/resources/auto-merger/) (i.e. the `/merge` comment). Instead, an admin must manually merge by changing the merging strategy to `Create a Merge Commit`. Otherwise, history will be lost and the branches become incompatible."); + } + } + } + } + + async getBranches() { + const { data: branches } = await this.context.octokit.repos.listBranches({ + owner: this.repo.owner.login, + repo: this.repo.name, + per_page: 100, + }); + return branches.filter((branch: any) => isVersionedBranch(branch.name)); + } + + sortBranches(branches: any) { + return branches.sort((a: any, b: any) => { + const [yearA, monthA] = getVersionFromBranch(a.name).split('.').map(Number) + const [yearB, monthB] = getVersionFromBranch(b.name).split('.').map(Number) + if (yearA !== yearB) { + return yearA - yearB; + } else { + return monthA - monthB; + } + }); + } + + getNextBranch(sortedBranches: any) { + const currentBranchIndex = sortedBranches.findIndex( + (branch: any) => branch.name === this.branchName + ); + const nextBranch = sortedBranches[currentBranchIndex + 1]; + return nextBranch; + } + + async openPR(nextBranch: any) { + if (!nextBranch) { + this.logger.info("No next branch found, will not open PR"); + return; + }; + const { data: pr } = await this.context.octokit.pulls.create({ + owner: this.repo.owner.login, + repo: this.repo.name, + title: "Forward-merge " + this.branchName + " into " + nextBranch.name, + head: this.branchName, + base: nextBranch.name, + maintainer_can_modify: true, + body: `Forward-merge triggered by push to ${this.branchName} that creates a PR to keep ${nextBranch.name} up-to-date. If this PR is unable to be immediately merged due to conflicts, it will remain open for the team to manually merge.`, + }); + return pr; + } + + async mergePR(pr: any) { + this.logger.info("Merging PR"); + const { data: merge } = await this.context.octokit.pulls.merge({ + owner: this.repo.owner.login, + repo: this.repo.name, + pull_number: pr.number, + sha: pr.head.sha, + }); + return merge; + } + + async issueComment(id, comment) { + await this.context.octokit.issues.createComment({ + owner: this.repo.owner.login, + repo: this.repo.name, + issue_number: id, + body: comment, + }); + } +} diff --git a/src/plugins/ForwardMerger/index.ts b/src/plugins/ForwardMerger/index.ts new file mode 100644 index 0000000..8059f4e --- /dev/null +++ b/src/plugins/ForwardMerger/index.ts @@ -0,0 +1,28 @@ +/* +* Copyright (c) 2024, NVIDIA CORPORATION. +* +* Licensed under the Apache License, Version 2.0 (the "License"); +* you may not use this file except in compliance with the License. +* You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* See the License for the specific language governing permissions and +* limitations under the License. +*/ + +import { Probot } from "probot"; +import { ForwardMerger } from "./forward_merger"; +import { EmitterWebhookEventName as WebhookEvents } from "@octokit/webhooks/dist-types/types"; + +export const pushEvents = ["push"] satisfies WebhookEvents[] +export type PushEventsType = (typeof pushEvents)[number] + +export const initForwardMerger = (app: Probot) => { + app.on(["push"], async (context) => { + await new ForwardMerger(context, context.payload).mergeForward(); + }); +}; diff --git a/test/forward_merger.test.ts b/test/forward_merger.test.ts new file mode 100644 index 0000000..db62ee0 --- /dev/null +++ b/test/forward_merger.test.ts @@ -0,0 +1,417 @@ +/* + * Copyright (c) 2024, NVIDIA CORPORATION. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { ForwardMerger } from "../src/plugins/ForwardMerger/forward_merger"; +import { makePushContext } from "./fixtures/contexts/push"; +import { mockConfigGet, mockContextRepo } from "./mocks"; +import { default as repoResp } from "./fixtures/responses/context_repo.json"; +import { makeConfigReponse } from "./fixtures/responses/get_config"; + +describe("Forward Merger", () => { + beforeAll(() => { + mockContextRepo.mockReturnValue(repoResp); + mockConfigGet.mockResolvedValue( + makeConfigReponse({ + forward_merger: true, + }) + ); + }); + + test("mergeForward should not run on push to non-versioned branch", async () => { + const context = makePushContext({ + ref: "refs/heads/unversioned", + }); + const forwardMerger = new ForwardMerger(context, context.payload); + const mockGetBranches = jest.fn().mockName("getBranches"); + forwardMerger.getBranches = mockGetBranches; + await forwardMerger.mergeForward(); + + expect(mockGetBranches).not.toBeCalled(); + }) + + test("mergeForward should not run when plugin is disabled", async () => { + const context = makePushContext({ + ref: "refs/heads/branch-21.12", + }); + const forwardMerger = new ForwardMerger(context, context.payload); + const mockGetBranches = jest.fn().mockName("getBranches"); + forwardMerger.getBranches = mockGetBranches; + const mockPluginIsDisabled = jest.fn().mockName("pluginIsDisabled").mockResolvedValue(true); + forwardMerger.pluginIsDisabled = mockPluginIsDisabled; + await forwardMerger.mergeForward(); + + expect(mockPluginIsDisabled).toBeCalled(); + expect(mockGetBranches).not.toBeCalled(); + }) + + test("mergeForward should sort branches", async () => { + const context = makePushContext({ + ref: "refs/heads/branch-21.12", + }); + const forwardMerger = new ForwardMerger(context, context.payload); + const branches = [ + { + name: "branch-21.12", + }, + { + name: "branch-21.10", + }] + // mock getBranches, sortBranches, and getNextBranch + const mockGetBranches = jest.fn().mockName("getBranches").mockResolvedValue(branches); + forwardMerger.getBranches = mockGetBranches; + const sortedBranches = [] + const mockSortBranches = jest.fn().mockName("sortBranches").mockReturnValue(sortedBranches); + forwardMerger.sortBranches = mockSortBranches; + const mockGetNextBranch = jest.fn().mockName("getNextBranch").mockReturnValue(null); + forwardMerger.getNextBranch = mockGetNextBranch; + + await forwardMerger.mergeForward(); + + expect(mockGetBranches).toBeCalled(); + expect(mockSortBranches).toBeCalledWith(branches); + expect(mockGetNextBranch).toBeCalledWith(sortedBranches); + }) + + test("mergeForward should open PR on valid next branch", async () => { + const context = makePushContext({ + ref: "refs/heads/branch-21.12", + }); + const forwardMerger = new ForwardMerger(context, context.payload); + forwardMerger.getBranches = jest.fn().mockName("getBranches").mockResolvedValue(null); + forwardMerger.sortBranches = jest.fn().mockName("sortBranches").mockReturnValue(null); + const nextBranch = { + name: "branch-21.10", + } + const mockGetNextBranch = jest.fn().mockName("getNextBranch").mockReturnValue(nextBranch); + forwardMerger.getNextBranch = mockGetNextBranch; + const mockOpenPR = jest.fn().mockName("openPR").mockResolvedValue(null); + forwardMerger.openPR = mockOpenPR; + + await forwardMerger.mergeForward(); + + expect(mockOpenPR).toBeCalledWith(nextBranch); + }) + + test("mergeForward should not open PR on invalid next branch", async () => { + const context = makePushContext({ + ref: "refs/heads/branch-22.02", + }); + const forwardMerger = new ForwardMerger(context, context.payload); + forwardMerger.getBranches = jest.fn().mockName("getBranches").mockResolvedValue(null); + forwardMerger.sortBranches = jest.fn().mockName("sortBranches").mockReturnValue(null); + const nextBranch = null + const mockGetNextBranch = jest.fn().mockName("getNextBranch").mockReturnValue(nextBranch); + forwardMerger.getNextBranch = mockGetNextBranch; + const mockOpenPR = jest.fn().mockName("openPR").mockResolvedValue(null); + forwardMerger.openPR = mockOpenPR; + + await forwardMerger.mergeForward(); + + expect(mockOpenPR).not.toBeCalled(); + }) + + test("mergeForward should merge PR after opening PR", async () => { + const context = makePushContext({ + ref: "refs/heads/branch-21.12", + }); + const forwardMerger = new ForwardMerger(context, context.payload); + forwardMerger.getBranches = jest.fn().mockName("getBranches").mockResolvedValue(null); + forwardMerger.sortBranches = jest.fn().mockName("sortBranches").mockReturnValue(null); + const nextBranch = { + name: "branch-21.10", + } + forwardMerger.getNextBranch = jest.fn().mockName("getNextBranch").mockReturnValue(nextBranch); + const pr = {} + forwardMerger.openPR = jest.fn().mockName("openPR").mockResolvedValue(pr); + const mockMergePR = jest.fn().mockName("mergePR").mockResolvedValue({merged:true}); + forwardMerger.mergePR = mockMergePR; + forwardMerger.issueComment = jest.fn().mockName("issueComment").mockResolvedValue(null); + + await forwardMerger.mergeForward(); + + expect(mockMergePR).toBeCalledWith(pr); + }) + + test("should not merge PR if there is no PR", async () => { + const context = makePushContext({ + ref: "refs/heads/branch-21.12", + }); + const forwardMerger = new ForwardMerger(context, context.payload); + forwardMerger.getBranches = jest.fn().mockName("getBranches").mockResolvedValue(null); + forwardMerger.sortBranches = jest.fn().mockName("sortBranches").mockReturnValue(null); + const nextBranch = { + name: "branch-21.10", + } + forwardMerger.getNextBranch = jest.fn().mockName("getNextBranch").mockReturnValue(nextBranch); + forwardMerger.openPR = jest.fn().mockName("openPR").mockResolvedValue(null); + const mockMergePR = jest.fn().mockName("mergePR").mockResolvedValue(null); + forwardMerger.mergePR = mockMergePR; + + await forwardMerger.mergeForward(); + + expect(mockMergePR).not.toBeCalled(); + }) + + test("should comment failure on PR if merge is successful", async () => { + const context = makePushContext({ + ref: "refs/heads/branch-21.12", + }); + const forwardMerger = new ForwardMerger(context, context.payload); + forwardMerger.getBranches = jest.fn().mockName("getBranches").mockResolvedValue(null); + forwardMerger.sortBranches = jest.fn().mockName("sortBranches").mockReturnValue(null); + const nextBranch = { + name: "branch-21.10", + } + forwardMerger.getNextBranch = jest.fn().mockName("getNextBranch").mockReturnValue(nextBranch); + const pr = { + number: 1, + } + forwardMerger.openPR = jest.fn().mockName("openPR").mockResolvedValue(pr); + forwardMerger.mergePR = jest.fn().mockName("mergePR").mockResolvedValue({merged: true}); + const mockIssueComment = jest.fn().mockName("issueComment").mockResolvedValue(null); + forwardMerger.issueComment = mockIssueComment; + + await forwardMerger.mergeForward(); + + expect(mockIssueComment).toBeCalledWith(pr.number, "**SUCCESS** - forward-merge complete."); + }) + + test("should comment failure on PR if merge is unsuccessful", async () => { + const context = makePushContext({ + ref: "refs/heads/branch-21.12", + }); + const forwardMerger = new ForwardMerger(context, context.payload); + forwardMerger.getBranches = jest.fn().mockName("getBranches").mockResolvedValue(null); + forwardMerger.sortBranches = jest.fn().mockName("sortBranches").mockReturnValue(null); + const nextBranch = { + name: "branch-21.10", + } + forwardMerger.getNextBranch = jest.fn().mockName("getNextBranch").mockReturnValue(nextBranch); + const pr = { + number: 1, + } + forwardMerger.openPR = jest.fn().mockName("openPR").mockResolvedValue(pr); + forwardMerger.mergePR = jest.fn().mockName("mergePR").mockResolvedValue({merged: false}); + const mockIssueComment = jest.fn().mockName("issueComment").mockResolvedValue(null); + forwardMerger.issueComment = mockIssueComment; + + await forwardMerger.mergeForward(); + + expect(mockIssueComment).toBeCalledWith(pr.number, "**FAILURE** - Unable to forward-merge due to an error, **manual** merge is necessary. Do not use the `Resolve conflicts` option in this PR, follow these instructions https://docs.rapids.ai/maintainers/forward-merger/ \n **IMPORTANT**: When merging this PR, do not use the [auto-merger](https://docs.rapids.ai/resources/auto-merger/) (i.e. the `/merge` comment). Instead, an admin must manually merge by changing the merging strategy to `Create a Merge Commit`. Otherwise, history will be lost and the branches become incompatible."); + }) + + test("mergeForward should obtain the correct next branch from a given list of unsorted branches", async () => { + const context = makePushContext({ + ref: "refs/heads/branch-22.02", + }); + const forwardMerger = new ForwardMerger(context, context.payload); + const branches = [ + { + name: "branch-22.04" + }, + { + name: "branch-21.12", + }, + { + name: "branch-21.10", + }, + { + name: "branch-22.02", + }] + const mockGetBranches = jest.fn().mockName("getBranches").mockResolvedValue(branches); + const mockOpenPR = jest.fn().mockName("openPR").mockResolvedValue(null) + forwardMerger.getBranches = mockGetBranches + forwardMerger.openPR = mockOpenPR + await forwardMerger.mergeForward(); + + expect(mockOpenPR.mock.calls[0][0]).toMatchObject({name: "branch-22.04"}); + }) + + test("getBranches should return versioned branches", async () => { + const context = makePushContext({ + ref: "refs/heads/branch-21.12", + }); + const forwardMerger = new ForwardMerger(context, context.payload); + const branches = [ + { + name: "branch-21.12", + }, + { + name: "non-versioned", + }, + { + name: "branch-21.10", + }] + const mockListBranches = jest.fn().mockName("listBranches").mockResolvedValue({data: branches}); + forwardMerger.context.octokit.repos.listBranches = mockListBranches as any; + + const result = await forwardMerger.getBranches(); + + expect(result.length).toEqual(2); + expect(result[0].name).toEqual("branch-21.12"); + expect(result[1].name).toEqual("branch-21.10"); + }) + + test("sortBranches should sort branches by version", async () => { + const context = makePushContext({ + ref: "refs/heads/branch-21.12", + }); + const forwardMerger = new ForwardMerger(context, context.payload); + const branches = [ + { + name: "branch-21.12", + }, + { + name: "branch-21.10", + }, + { + name: "branch-22.02", + }] + const mockListBranches = jest.fn().mockName("listBranches").mockResolvedValue({data: branches}); + forwardMerger.context.octokit.repos.listBranches = mockListBranches as any; + + const result = await forwardMerger.getBranches(); + + expect(result.length).toEqual(3); + expect(result[0].name).toEqual("branch-21.12"); + expect(result[1].name).toEqual("branch-21.10"); + expect(result[2].name).toEqual("branch-22.02"); + }) + + test("getNextBranch should return next branch", async () => { + const context = makePushContext({ + ref: "refs/heads/branch-21.12", + }); + const forwardMerger = new ForwardMerger(context, context.payload); + const branches = [ + { + name: "branch-21.12", + }, + { + name: "branch-21.10", + }, + { + name: "branch-21.10", + }, + { + name: "branch-22.02", + }] + const sortedBranches = forwardMerger.sortBranches(branches) + const result = await forwardMerger.getNextBranch(sortedBranches); + + expect(result.name).toEqual("branch-22.02"); + }) + + test("getNextBranch should return null if there is no next branch", async () => { + const context = makePushContext({ + ref: "refs/heads/branch-22.02", + }); + const forwardMerger = new ForwardMerger(context, context.payload); + const branches = [ + { + name: "branch-21.12", + }, + { + name: "branch-21.10", + }, + { + name: "branch-22.02", + }] + const sortedBranches = forwardMerger.sortBranches(branches) + const result = await forwardMerger.getNextBranch(sortedBranches); + + expect(result).toBeFalsy(); + }) + + test("openPR should create PR when there is a valid next branch", async () => { + const context = makePushContext({ + ref: "refs/heads/branch-22.02", + }); + const forwardMerger = new ForwardMerger(context, context.payload); + const nextBranch = { + name: "branch-22.04", + } + const mockCreatePR = jest.fn().mockName("createPR").mockResolvedValue({data: {number: 1}}); + forwardMerger.context.octokit.pulls.create = mockCreatePR as any; + const result = await forwardMerger.openPR(nextBranch); + + expect(result!.number).toEqual(1); + expect(mockCreatePR.mock.calls[0][0]).toMatchObject({ + owner: context.payload.repository.owner.login, + repo: context.payload.repository.name, + title: "Forward-merge " + forwardMerger.branchName + " into " + nextBranch.name, + head: forwardMerger.branchName, + base: nextBranch.name, + maintainer_can_modify: true, + body: `Forward-merge triggered by push to ${forwardMerger.branchName} that creates a PR to keep ${nextBranch.name} up-to-date. If this PR is unable to be immediately merged due to conflicts, it will remain open for the team to manually merge.`, + }); + }) + + test("openPR should return null when there is no next branch", async () => { + const context = makePushContext({ + ref: "refs/heads/branch-22.02", + }); + const forwardMerger = new ForwardMerger(context, context.payload); + const nextBranch = null + const mockCreatePR = jest.fn().mockName("createPR").mockResolvedValue({data: {number: 1}}); + forwardMerger.context.octokit.pulls.create = mockCreatePR as any; + const result = await forwardMerger.openPR(nextBranch); + + expect(result).toBeFalsy(); + expect(mockCreatePR).not.toBeCalled(); + }) + + test("mergePR should merge PR", async () => { + const context = makePushContext({ + ref: "refs/heads/branch-22.02", + }); + const forwardMerger = new ForwardMerger(context, context.payload); + const pr = { + number: 1, + head: { + sha: "sha", + } + } + const mockMergePR = jest.fn().mockName("mergePR").mockResolvedValue({data: {merged: true}}); + forwardMerger.context.octokit.pulls.merge = mockMergePR as any; + const result = await forwardMerger.mergePR(pr); + + expect(result!.merged).toEqual(true); + expect(mockMergePR.mock.calls[0][0]).toMatchObject({ + owner: context.payload.repository.owner.login, + repo: context.payload.repository.name, + pull_number: pr.number, + sha: pr.head.sha, + }); + }) + + test("issueComment should create comment", async () => { + const context = makePushContext({ + ref: "refs/heads/branch-22.02", + }); + const forwardMerger = new ForwardMerger(context, context.payload); + const mockCreateComment = jest.fn().mockName("createComment").mockResolvedValue(null); + forwardMerger.context.octokit.issues.createComment = mockCreateComment as any; + await forwardMerger.issueComment(1, "comment"); + + expect(mockCreateComment.mock.calls[0][0]).toMatchObject({ + owner: context.payload.repository.owner.login, + repo: context.payload.repository.name, + issue_number: 1, + body: "comment", + }); + }) +}) From cf9f3e9ff1486cd42627cdd5e5e3259ab9dee683 Mon Sep 17 00:00:00 2001 From: Jake Awe Date: Tue, 16 Jan 2024 12:07:47 -0600 Subject: [PATCH 02/19] rm smee-client --- package-lock.json | 33 --------------------------------- package.json | 1 - 2 files changed, 34 deletions(-) diff --git a/package-lock.json b/package-lock.json index 323180d..f6a6807 100644 --- a/package-lock.json +++ b/package-lock.json @@ -25,7 +25,6 @@ "@typescript-eslint/parser": "^5.62.0", "eslint": "^8.48.0", "jest": "^29.6.4", - "smee-client": "^2.0.0", "ts-jest": "^29.1.1", "typescript": "^4.9.5" }, @@ -4534,15 +4533,6 @@ "node": ">= 0.8" } }, - "node_modules/commander": { - "version": "11.1.0", - "resolved": "https://registry.npmjs.org/commander/-/commander-11.1.0.tgz", - "integrity": "sha512-yPVavfyCcRhmorC7rWlkHn15b4wDVgVmBA7kV4QVBsF7kv/9TKJAbAXVTxvTnwP8HHKjRCJDClKbciiYS7p0DQ==", - "dev": true, - "engines": { - "node": ">=16" - } - }, "node_modules/concat-map": { "version": "0.0.1", "resolved": "https://registry.npmjs.org/concat-map/-/concat-map-0.0.1.tgz", @@ -8127,20 +8117,6 @@ "node": ">=8" } }, - "node_modules/smee-client": { - "version": "2.0.0", - "resolved": "https://registry.npmjs.org/smee-client/-/smee-client-2.0.0.tgz", - "integrity": "sha512-LqJAAU4uayG909u8q3lBQZma9TDsQl2pOoXCqrsAda58oxy2o81yQAeBvFc2ilexgQDJc3YPDoPgViBM26M8vw==", - "dev": true, - "dependencies": { - "commander": "^11.1.0", - "eventsource": "^2.0.2", - "validator": "^13.11.0" - }, - "bin": { - "smee": "bin/smee.js" - } - }, "node_modules/sonic-boom": { "version": "2.8.0", "resolved": "https://registry.npmjs.org/sonic-boom/-/sonic-boom-2.8.0.tgz", @@ -8644,15 +8620,6 @@ "integrity": "sha512-ASFBup0Mz1uyiIjANan1jzLQami9z1PoYSZCiiYW2FczPbenXc45FZdBZLzOT+r6+iciuEModtmCti+hjaAk0A==", "dev": true }, - "node_modules/validator": { - "version": "13.11.0", - "resolved": "https://registry.npmjs.org/validator/-/validator-13.11.0.tgz", - "integrity": "sha512-Ii+sehpSfZy+At5nPdnyMhx78fEoPDkR2XW/zimHEL3MyGJQOCQ7WeP20jPYRz7ZCpcKLB21NxuXHF3bxjStBQ==", - "dev": true, - "engines": { - "node": ">= 0.10" - } - }, "node_modules/vary": { "version": "1.1.2", "resolved": "https://registry.npmjs.org/vary/-/vary-1.1.2.tgz", diff --git a/package.json b/package.json index d65e20d..9b9ca21 100644 --- a/package.json +++ b/package.json @@ -37,7 +37,6 @@ "@typescript-eslint/parser": "^5.62.0", "eslint": "^8.48.0", "jest": "^29.6.4", - "smee-client": "^2.0.0", "ts-jest": "^29.1.1", "typescript": "^4.9.5" }, From 0266fdd016e33279098ede3763f45da501aa9b52 Mon Sep 17 00:00:00 2001 From: Jake Awe Date: Wed, 17 Jan 2024 09:37:45 -0600 Subject: [PATCH 03/19] use paginate method --- src/plugins/ForwardMerger/forward_merger.ts | 3 +-- test/forward_merger.test.ts | 24 +++++++++++++++------ 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/src/plugins/ForwardMerger/forward_merger.ts b/src/plugins/ForwardMerger/forward_merger.ts index 92ddb97..3e384ee 100644 --- a/src/plugins/ForwardMerger/forward_merger.ts +++ b/src/plugins/ForwardMerger/forward_merger.ts @@ -68,10 +68,9 @@ export class ForwardMerger extends OpsBotPlugin { } async getBranches() { - const { data: branches } = await this.context.octokit.repos.listBranches({ + const branches = await this.context.octokit.paginate(this.context.octokit.repos.listBranches, { owner: this.repo.owner.login, repo: this.repo.name, - per_page: 100, }); return branches.filter((branch: any) => isVersionedBranch(branch.name)); } diff --git a/test/forward_merger.test.ts b/test/forward_merger.test.ts index db62ee0..df53db9 100644 --- a/test/forward_merger.test.ts +++ b/test/forward_merger.test.ts @@ -255,7 +255,9 @@ describe("Forward Merger", () => { { name: "branch-21.10", }] - const mockListBranches = jest.fn().mockName("listBranches").mockResolvedValue({data: branches}); + const mockListBranchesPaginate = jest.fn().mockName("listBranches").mockResolvedValue(branches); + forwardMerger.context.octokit.paginate = mockListBranchesPaginate as any; + const mockListBranches = jest.fn().mockName("listBranches").mockResolvedValue("something"); forwardMerger.context.octokit.repos.listBranches = mockListBranches as any; const result = await forwardMerger.getBranches(); @@ -263,6 +265,11 @@ describe("Forward Merger", () => { expect(result.length).toEqual(2); expect(result[0].name).toEqual("branch-21.12"); expect(result[1].name).toEqual("branch-21.10"); + expect(mockListBranchesPaginate.mock.calls[0][0]).toBe(mockListBranches) + expect(mockListBranchesPaginate.mock.calls[0][1]).toMatchObject({ + owner: context.payload.repository.owner.login, + repo: context.payload.repository.name, + }); }) test("sortBranches should sort branches by version", async () => { @@ -277,18 +284,21 @@ describe("Forward Merger", () => { { name: "branch-21.10", }, + { + name: "branch-19.10", + }, { name: "branch-22.02", }] - const mockListBranches = jest.fn().mockName("listBranches").mockResolvedValue({data: branches}); - forwardMerger.context.octokit.repos.listBranches = mockListBranches as any; - const result = await forwardMerger.getBranches(); + const result = await forwardMerger.sortBranches(branches); + console.log(result) - expect(result.length).toEqual(3); - expect(result[0].name).toEqual("branch-21.12"); + expect(result.length).toEqual(4); + expect(result[0].name).toEqual("branch-19.10"); expect(result[1].name).toEqual("branch-21.10"); - expect(result[2].name).toEqual("branch-22.02"); + expect(result[2].name).toEqual("branch-21.12"); + expect(result[3].name).toEqual("branch-22.02"); }) test("getNextBranch should return next branch", async () => { From 1b1576deda16f8e25d20fea60c9eb4e6721c7806 Mon Sep 17 00:00:00 2001 From: Jake Awe Date: Wed, 17 Jan 2024 10:11:20 -0600 Subject: [PATCH 04/19] remove references of type any in main class --- src/plugins/ForwardMerger/forward_merger.ts | 15 ++++++++------- test/forward_merger.test.ts | 13 ++++++------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/plugins/ForwardMerger/forward_merger.ts b/src/plugins/ForwardMerger/forward_merger.ts index 3e384ee..0b47376 100644 --- a/src/plugins/ForwardMerger/forward_merger.ts +++ b/src/plugins/ForwardMerger/forward_merger.ts @@ -23,6 +23,7 @@ import { import { basename } from "path"; import { Context } from "probot"; import { PushEventsType } from "."; +type Branches = Awaited>; export class ForwardMerger extends OpsBotPlugin { @@ -72,11 +73,11 @@ export class ForwardMerger extends OpsBotPlugin { owner: this.repo.owner.login, repo: this.repo.name, }); - return branches.filter((branch: any) => isVersionedBranch(branch.name)); + return branches.filter((branch) => isVersionedBranch(branch.name)); } - sortBranches(branches: any) { - return branches.sort((a: any, b: any) => { + sortBranches(branches: Branches) { + return branches.sort((a, b) => { const [yearA, monthA] = getVersionFromBranch(a.name).split('.').map(Number) const [yearB, monthB] = getVersionFromBranch(b.name).split('.').map(Number) if (yearA !== yearB) { @@ -87,15 +88,15 @@ export class ForwardMerger extends OpsBotPlugin { }); } - getNextBranch(sortedBranches: any) { + getNextBranch(sortedBranches: Branches) { const currentBranchIndex = sortedBranches.findIndex( - (branch: any) => branch.name === this.branchName + (branch) => branch.name === this.branchName ); const nextBranch = sortedBranches[currentBranchIndex + 1]; return nextBranch; } - async openPR(nextBranch: any) { + async openPR(nextBranch: Branches[number]) { if (!nextBranch) { this.logger.info("No next branch found, will not open PR"); return; @@ -112,7 +113,7 @@ export class ForwardMerger extends OpsBotPlugin { return pr; } - async mergePR(pr: any) { + async mergePR(pr) { this.logger.info("Merging PR"); const { data: merge } = await this.context.octokit.pulls.merge({ owner: this.repo.owner.login, diff --git a/test/forward_merger.test.ts b/test/forward_merger.test.ts index df53db9..c1a0dde 100644 --- a/test/forward_merger.test.ts +++ b/test/forward_merger.test.ts @@ -277,7 +277,7 @@ describe("Forward Merger", () => { ref: "refs/heads/branch-21.12", }); const forwardMerger = new ForwardMerger(context, context.payload); - const branches = [ + const branches: any[] = [ { name: "branch-21.12", }, @@ -291,8 +291,7 @@ describe("Forward Merger", () => { name: "branch-22.02", }] - const result = await forwardMerger.sortBranches(branches); - console.log(result) + const result = forwardMerger.sortBranches(branches); expect(result.length).toEqual(4); expect(result[0].name).toEqual("branch-19.10"); @@ -306,7 +305,7 @@ describe("Forward Merger", () => { ref: "refs/heads/branch-21.12", }); const forwardMerger = new ForwardMerger(context, context.payload); - const branches = [ + const branches: any[] = [ { name: "branch-21.12", }, @@ -330,7 +329,7 @@ describe("Forward Merger", () => { ref: "refs/heads/branch-22.02", }); const forwardMerger = new ForwardMerger(context, context.payload); - const branches = [ + const branches: any[] = [ { name: "branch-21.12", }, @@ -356,7 +355,7 @@ describe("Forward Merger", () => { } const mockCreatePR = jest.fn().mockName("createPR").mockResolvedValue({data: {number: 1}}); forwardMerger.context.octokit.pulls.create = mockCreatePR as any; - const result = await forwardMerger.openPR(nextBranch); + const result = await forwardMerger.openPR(nextBranch as any); expect(result!.number).toEqual(1); expect(mockCreatePR.mock.calls[0][0]).toMatchObject({ @@ -378,7 +377,7 @@ describe("Forward Merger", () => { const nextBranch = null const mockCreatePR = jest.fn().mockName("createPR").mockResolvedValue({data: {number: 1}}); forwardMerger.context.octokit.pulls.create = mockCreatePR as any; - const result = await forwardMerger.openPR(nextBranch); + const result = await forwardMerger.openPR(nextBranch as any); expect(result).toBeFalsy(); expect(mockCreatePR).not.toBeCalled(); From aa6db9f9724946c2dba04fe837180cbe5c0b4e3e Mon Sep 17 00:00:00 2001 From: Jake Awe Date: Wed, 24 Jan 2024 07:43:27 -0600 Subject: [PATCH 05/19] add forward-merger docs link --- src/plugins/ForwardMerger/forward_merger.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/plugins/ForwardMerger/forward_merger.ts b/src/plugins/ForwardMerger/forward_merger.ts index 0b47376..f2274b9 100644 --- a/src/plugins/ForwardMerger/forward_merger.ts +++ b/src/plugins/ForwardMerger/forward_merger.ts @@ -107,8 +107,8 @@ export class ForwardMerger extends OpsBotPlugin { title: "Forward-merge " + this.branchName + " into " + nextBranch.name, head: this.branchName, base: nextBranch.name, - maintainer_can_modify: true, - body: `Forward-merge triggered by push to ${this.branchName} that creates a PR to keep ${nextBranch.name} up-to-date. If this PR is unable to be immediately merged due to conflicts, it will remain open for the team to manually merge.`, + maintainer_can_modify: false, + body: `Forward-merge triggered by push to ${this.branchName} that creates a PR to keep ${nextBranch.name} up-to-date. If this PR is unable to be immediately merged due to conflicts, it will remain open for the team to manually merge. See [forward-merger docs](https://docs.rapids.ai/maintainers/forward-merger/) for more info.`, }); return pr; } From dad53dbd5d772ec3f9eede12e304179992e9286a Mon Sep 17 00:00:00 2001 From: Jake Awe Date: Wed, 24 Jan 2024 08:14:39 -0600 Subject: [PATCH 06/19] add delay after createPR --- src/plugins/ForwardMerger/forward_merger.ts | 3 ++- test/forward_merger.test.ts | 14 +++++++------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/plugins/ForwardMerger/forward_merger.ts b/src/plugins/ForwardMerger/forward_merger.ts index f2274b9..4154253 100644 --- a/src/plugins/ForwardMerger/forward_merger.ts +++ b/src/plugins/ForwardMerger/forward_merger.ts @@ -56,6 +56,7 @@ export class ForwardMerger extends OpsBotPlugin { if (nextBranch) { const pr = await this.openPR(nextBranch); + await new Promise(resolve => setTimeout(resolve, 10000)); if (pr) { const merge = await this.mergePR(pr); @@ -107,7 +108,7 @@ export class ForwardMerger extends OpsBotPlugin { title: "Forward-merge " + this.branchName + " into " + nextBranch.name, head: this.branchName, base: nextBranch.name, - maintainer_can_modify: false, + maintainer_can_modify: true, body: `Forward-merge triggered by push to ${this.branchName} that creates a PR to keep ${nextBranch.name} up-to-date. If this PR is unable to be immediately merged due to conflicts, it will remain open for the team to manually merge. See [forward-merger docs](https://docs.rapids.ai/maintainers/forward-merger/) for more info.`, }); return pr; diff --git a/test/forward_merger.test.ts b/test/forward_merger.test.ts index c1a0dde..1312afe 100644 --- a/test/forward_merger.test.ts +++ b/test/forward_merger.test.ts @@ -103,7 +103,7 @@ describe("Forward Merger", () => { await forwardMerger.mergeForward(); expect(mockOpenPR).toBeCalledWith(nextBranch); - }) + }, 11000) test("mergeForward should not open PR on invalid next branch", async () => { const context = makePushContext({ @@ -143,7 +143,7 @@ describe("Forward Merger", () => { await forwardMerger.mergeForward(); expect(mockMergePR).toBeCalledWith(pr); - }) + }, 11000) test("should not merge PR if there is no PR", async () => { const context = makePushContext({ @@ -163,7 +163,7 @@ describe("Forward Merger", () => { await forwardMerger.mergeForward(); expect(mockMergePR).not.toBeCalled(); - }) + }, 11000) test("should comment failure on PR if merge is successful", async () => { const context = makePushContext({ @@ -187,7 +187,7 @@ describe("Forward Merger", () => { await forwardMerger.mergeForward(); expect(mockIssueComment).toBeCalledWith(pr.number, "**SUCCESS** - forward-merge complete."); - }) + }, 11000) test("should comment failure on PR if merge is unsuccessful", async () => { const context = makePushContext({ @@ -211,7 +211,7 @@ describe("Forward Merger", () => { await forwardMerger.mergeForward(); expect(mockIssueComment).toBeCalledWith(pr.number, "**FAILURE** - Unable to forward-merge due to an error, **manual** merge is necessary. Do not use the `Resolve conflicts` option in this PR, follow these instructions https://docs.rapids.ai/maintainers/forward-merger/ \n **IMPORTANT**: When merging this PR, do not use the [auto-merger](https://docs.rapids.ai/resources/auto-merger/) (i.e. the `/merge` comment). Instead, an admin must manually merge by changing the merging strategy to `Create a Merge Commit`. Otherwise, history will be lost and the branches become incompatible."); - }) + }, 11000) test("mergeForward should obtain the correct next branch from a given list of unsorted branches", async () => { const context = makePushContext({ @@ -238,7 +238,7 @@ describe("Forward Merger", () => { await forwardMerger.mergeForward(); expect(mockOpenPR.mock.calls[0][0]).toMatchObject({name: "branch-22.04"}); - }) + }, 11000) test("getBranches should return versioned branches", async () => { const context = makePushContext({ @@ -365,7 +365,7 @@ describe("Forward Merger", () => { head: forwardMerger.branchName, base: nextBranch.name, maintainer_can_modify: true, - body: `Forward-merge triggered by push to ${forwardMerger.branchName} that creates a PR to keep ${nextBranch.name} up-to-date. If this PR is unable to be immediately merged due to conflicts, it will remain open for the team to manually merge.`, + body: `Forward-merge triggered by push to ${forwardMerger.branchName} that creates a PR to keep ${nextBranch.name} up-to-date. If this PR is unable to be immediately merged due to conflicts, it will remain open for the team to manually merge. See [forward-merger docs](https://docs.rapids.ai/maintainers/forward-merger/) for more info.`, }); }) From e10553e4ce9d2e9d4c27f7a7fba4e6898f67b8d4 Mon Sep 17 00:00:00 2001 From: Jake Awe Date: Wed, 24 Jan 2024 10:05:57 -0600 Subject: [PATCH 07/19] add new test case --- test/forward_merger.test.ts | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/test/forward_merger.test.ts b/test/forward_merger.test.ts index 1312afe..d0cefe3 100644 --- a/test/forward_merger.test.ts +++ b/test/forward_merger.test.ts @@ -68,7 +68,7 @@ describe("Forward Merger", () => { }, { name: "branch-21.10", - }] + }] // mock getBranches, sortBranches, and getNextBranch const mockGetBranches = jest.fn().mockName("getBranches").mockResolvedValue(branches); forwardMerger.getBranches = mockGetBranches; @@ -300,6 +300,34 @@ describe("Forward Merger", () => { expect(result[3].name).toEqual("branch-22.02"); }) + test("sortBranches should sort 0.9/0.10-type branches", async () => { + const context = makePushContext({ + ref: "refs/heads/branch-21.12", + }); + const forwardMerger = new ForwardMerger(context, context.payload); + const branches: any[] = [ + { + name: "branch-0.12", + }, + { + name: "branch-0.10", + }, + { + name: "branch-2.10", + }, + { + name: "branch-1.02", + }] + + const result = forwardMerger.sortBranches(branches); + + expect(result.length).toEqual(4); + expect(result[0].name).toEqual("branch-0.10"); + expect(result[1].name).toEqual("branch-0.12"); + expect(result[2].name).toEqual("branch-1.02"); + expect(result[3].name).toEqual("branch-2.10"); +}) + test("getNextBranch should return next branch", async () => { const context = makePushContext({ ref: "refs/heads/branch-21.12", From b26b6b3e31093c37ad79bdc29e2d675ebeea3efc Mon Sep 17 00:00:00 2001 From: Jake Awe Date: Mon, 29 Jan 2024 05:27:24 -0600 Subject: [PATCH 08/19] rearrange methods --- src/plugins/ForwardMerger/forward_merger.ts | 75 +++++------ test/forward_merger.test.ts | 131 ++++++-------------- 2 files changed, 68 insertions(+), 138 deletions(-) diff --git a/src/plugins/ForwardMerger/forward_merger.ts b/src/plugins/ForwardMerger/forward_merger.ts index 4154253..2fb796f 100644 --- a/src/plugins/ForwardMerger/forward_merger.ts +++ b/src/plugins/ForwardMerger/forward_merger.ts @@ -28,24 +28,20 @@ type Branches = Awaited>; export class ForwardMerger extends OpsBotPlugin { context: Context; - branchName: string; + currentBranch: string; repo: PayloadRepository; - branchVersionNumber: string; - defaultBranch: string; constructor(context: Context, private payload: Context["payload"]) { super("forward_merger", context); this.context = context; - this.branchName = basename(this.payload.ref); + this.currentBranch = basename(this.payload.ref); this.repo = payload.repository; - this.branchVersionNumber = getVersionFromBranch(this.branchName); - this.defaultBranch = this.repo.default_branch; } async mergeForward() { if (await this.pluginIsDisabled()) return; - if (!isVersionedBranch(this.branchName)) { + if (!isVersionedBranch(this.currentBranch)) { this.logger.info("Will not merge forward on non-versioned branch"); return; } @@ -54,18 +50,31 @@ export class ForwardMerger extends OpsBotPlugin { const sortedBranches = this.sortBranches(branches); const nextBranch = this.getNextBranch(sortedBranches); - if (nextBranch) { - const pr = await this.openPR(nextBranch); - await new Promise(resolve => setTimeout(resolve, 10000)); + if(!nextBranch) return - if (pr) { - const merge = await this.mergePR(pr); - if (merge.merged) { - await this.issueComment(pr.number, "**SUCCESS** - forward-merge complete."); - } else { - await this.issueComment(pr.number, "**FAILURE** - Unable to forward-merge due to an error, **manual** merge is necessary. Do not use the `Resolve conflicts` option in this PR, follow these instructions https://docs.rapids.ai/maintainers/forward-merger/ \n **IMPORTANT**: When merging this PR, do not use the [auto-merger](https://docs.rapids.ai/resources/auto-merger/) (i.e. the `/merge` comment). Instead, an admin must manually merge by changing the merging strategy to `Create a Merge Commit`. Otherwise, history will be lost and the branches become incompatible."); - } - } + const { data: pr } = await this.context.octokit.pulls.create({ + owner: this.repo.owner.login, + repo: this.repo.name, + title: "Forward-merge " + this.currentBranch + " into " + nextBranch.name, + head: this.currentBranch, + base: nextBranch.name, + maintainer_can_modify: true, + body: `Forward-merge triggered by push to ${this.currentBranch} that creates a PR to keep ${nextBranch.name} up-to-date. If this PR is unable to be immediately merged due to conflicts, it will remain open for the team to manually merge. See [forward-merger docs](https://docs.rapids.ai/maintainers/forward-merger/) for more info.`, + }); + + await new Promise(resolve => setTimeout(resolve, 10000)); + + try { + this.logger.info("Merging PR"); + await this.context.octokit.pulls.merge({ + owner: this.repo.owner.login, + repo: this.repo.name, + pull_number: pr.number, + sha: pr.head.sha, + }); + await this.issueComment(pr.number, "**SUCCESS** - forward-merge complete."); + } catch (error) { + await this.issueComment(pr.number, "**FAILURE** - Unable to forward-merge due to an error, **manual** merge is necessary. Do not use the `Resolve conflicts` option in this PR, follow these instructions https://docs.rapids.ai/maintainers/forward-merger/ \n **IMPORTANT**: When merging this PR, do not use the [auto-merger](https://docs.rapids.ai/resources/auto-merger/) (i.e. the `/merge` comment). Instead, an admin must manually merge by changing the merging strategy to `Create a Merge Commit`. Otherwise, history will be lost and the branches become incompatible."); } } @@ -91,40 +100,12 @@ export class ForwardMerger extends OpsBotPlugin { getNextBranch(sortedBranches: Branches) { const currentBranchIndex = sortedBranches.findIndex( - (branch) => branch.name === this.branchName + (branch) => branch.name === this.currentBranch ); const nextBranch = sortedBranches[currentBranchIndex + 1]; return nextBranch; } - async openPR(nextBranch: Branches[number]) { - if (!nextBranch) { - this.logger.info("No next branch found, will not open PR"); - return; - }; - const { data: pr } = await this.context.octokit.pulls.create({ - owner: this.repo.owner.login, - repo: this.repo.name, - title: "Forward-merge " + this.branchName + " into " + nextBranch.name, - head: this.branchName, - base: nextBranch.name, - maintainer_can_modify: true, - body: `Forward-merge triggered by push to ${this.branchName} that creates a PR to keep ${nextBranch.name} up-to-date. If this PR is unable to be immediately merged due to conflicts, it will remain open for the team to manually merge. See [forward-merger docs](https://docs.rapids.ai/maintainers/forward-merger/) for more info.`, - }); - return pr; - } - - async mergePR(pr) { - this.logger.info("Merging PR"); - const { data: merge } = await this.context.octokit.pulls.merge({ - owner: this.repo.owner.login, - repo: this.repo.name, - pull_number: pr.number, - sha: pr.head.sha, - }); - return merge; - } - async issueComment(id, comment) { await this.context.octokit.issues.createComment({ owner: this.repo.owner.login, diff --git a/test/forward_merger.test.ts b/test/forward_merger.test.ts index d0cefe3..61454bb 100644 --- a/test/forward_merger.test.ts +++ b/test/forward_merger.test.ts @@ -97,12 +97,20 @@ describe("Forward Merger", () => { } const mockGetNextBranch = jest.fn().mockName("getNextBranch").mockReturnValue(nextBranch); forwardMerger.getNextBranch = mockGetNextBranch; - const mockOpenPR = jest.fn().mockName("openPR").mockResolvedValue(null); - forwardMerger.openPR = mockOpenPR; + const mockCreatePR = jest.fn().mockName("openPR").mockResolvedValue({data: {}}); + forwardMerger.context.octokit.pulls.create = mockCreatePR as any; await forwardMerger.mergeForward(); - expect(mockOpenPR).toBeCalledWith(nextBranch); + expect(mockCreatePR.mock.calls[0][0]).toMatchObject({ + owner: context.payload.repository.owner.login, + repo: context.payload.repository.name, + title: "Forward-merge " + forwardMerger.currentBranch + " into " + nextBranch.name, + head: forwardMerger.currentBranch, + base: nextBranch.name, + maintainer_can_modify: true, + body: `Forward-merge triggered by push to ${forwardMerger.currentBranch} that creates a PR to keep ${nextBranch.name} up-to-date. If this PR is unable to be immediately merged due to conflicts, it will remain open for the team to manually merge. See [forward-merger docs](https://docs.rapids.ai/maintainers/forward-merger/) for more info.`, + }); }, 11000) test("mergeForward should not open PR on invalid next branch", async () => { @@ -115,12 +123,12 @@ describe("Forward Merger", () => { const nextBranch = null const mockGetNextBranch = jest.fn().mockName("getNextBranch").mockReturnValue(nextBranch); forwardMerger.getNextBranch = mockGetNextBranch; - const mockOpenPR = jest.fn().mockName("openPR").mockResolvedValue(null); - forwardMerger.openPR = mockOpenPR; + const mockCreatePR = jest.fn().mockName("openPR").mockResolvedValue(null); + forwardMerger.context.octokit.pulls.create = mockCreatePR as any; await forwardMerger.mergeForward(); - expect(mockOpenPR).not.toBeCalled(); + expect(mockCreatePR).not.toBeCalled(); }) test("mergeForward should merge PR after opening PR", async () => { @@ -134,15 +142,20 @@ describe("Forward Merger", () => { name: "branch-21.10", } forwardMerger.getNextBranch = jest.fn().mockName("getNextBranch").mockReturnValue(nextBranch); - const pr = {} - forwardMerger.openPR = jest.fn().mockName("openPR").mockResolvedValue(pr); + const pr = {data: {number: 1, head: {sha: 123456}}} + forwardMerger.context.octokit.pulls.create = jest.fn().mockName("openPR").mockResolvedValue(pr); const mockMergePR = jest.fn().mockName("mergePR").mockResolvedValue({merged:true}); - forwardMerger.mergePR = mockMergePR; + forwardMerger.context.octokit.pulls.merge = mockMergePR; forwardMerger.issueComment = jest.fn().mockName("issueComment").mockResolvedValue(null); await forwardMerger.mergeForward(); - expect(mockMergePR).toBeCalledWith(pr); + expect(mockMergePR.mock.calls[0][0]).toMatchObject({ + owner: context.payload.repository.owner.login, + repo: context.payload.repository.name, + pull_number: pr.data.number, + sha: pr.data.head.sha, + }); }, 11000) test("should not merge PR if there is no PR", async () => { @@ -156,9 +169,9 @@ describe("Forward Merger", () => { name: "branch-21.10", } forwardMerger.getNextBranch = jest.fn().mockName("getNextBranch").mockReturnValue(nextBranch); - forwardMerger.openPR = jest.fn().mockName("openPR").mockResolvedValue(null); + forwardMerger.context.octokit.pulls.create = jest.fn().mockName("openPR").mockResolvedValue({data: {}}); const mockMergePR = jest.fn().mockName("mergePR").mockResolvedValue(null); - forwardMerger.mergePR = mockMergePR; + forwardMerger.context.octokit.pulls.merge = mockMergePR; await forwardMerger.mergeForward(); @@ -175,18 +188,16 @@ describe("Forward Merger", () => { const nextBranch = { name: "branch-21.10", } - forwardMerger.getNextBranch = jest.fn().mockName("getNextBranch").mockReturnValue(nextBranch); - const pr = { - number: 1, - } - forwardMerger.openPR = jest.fn().mockName("openPR").mockResolvedValue(pr); - forwardMerger.mergePR = jest.fn().mockName("mergePR").mockResolvedValue({merged: true}); + forwardMerger.getNextBranch = jest.fn().mockName("getNextBranch").mockResolvedValue(nextBranch); + const pr = {data: {number: 1, head: {sha: 123456}}} + forwardMerger.context.octokit.pulls.create = jest.fn().mockName("openPR").mockResolvedValue(pr); + forwardMerger.context.octokit.pulls.merge = jest.fn().mockName("mergePR").mockResolvedValue({merged: true}); const mockIssueComment = jest.fn().mockName("issueComment").mockResolvedValue(null); forwardMerger.issueComment = mockIssueComment; await forwardMerger.mergeForward(); - expect(mockIssueComment).toBeCalledWith(pr.number, "**SUCCESS** - forward-merge complete."); + expect(mockIssueComment).toBeCalledWith(pr.data.number, "**SUCCESS** - forward-merge complete."); }, 11000) test("should comment failure on PR if merge is unsuccessful", async () => { @@ -195,22 +206,22 @@ describe("Forward Merger", () => { }); const forwardMerger = new ForwardMerger(context, context.payload); forwardMerger.getBranches = jest.fn().mockName("getBranches").mockResolvedValue(null); - forwardMerger.sortBranches = jest.fn().mockName("sortBranches").mockReturnValue(null); + forwardMerger.sortBranches = jest.fn().mockName("sortBranches").mockResolvedValue(null); const nextBranch = { name: "branch-21.10", } forwardMerger.getNextBranch = jest.fn().mockName("getNextBranch").mockReturnValue(nextBranch); - const pr = { - number: 1, - } - forwardMerger.openPR = jest.fn().mockName("openPR").mockResolvedValue(pr); - forwardMerger.mergePR = jest.fn().mockName("mergePR").mockResolvedValue({merged: false}); + const pr = {data: {number: 1, head: {sha: 123456}}} + forwardMerger.context.octokit.pulls.create = jest.fn().mockName("openPR").mockResolvedValue(pr); + const mockMergePR = jest.fn().mockName("mergePR").mockResolvedValue({merged: false}); + forwardMerger.context.octokit.pulls.merge = mockMergePR; + mockMergePR.mockRejectedValueOnce(new Error("error")); const mockIssueComment = jest.fn().mockName("issueComment").mockResolvedValue(null); forwardMerger.issueComment = mockIssueComment; await forwardMerger.mergeForward(); - expect(mockIssueComment).toBeCalledWith(pr.number, "**FAILURE** - Unable to forward-merge due to an error, **manual** merge is necessary. Do not use the `Resolve conflicts` option in this PR, follow these instructions https://docs.rapids.ai/maintainers/forward-merger/ \n **IMPORTANT**: When merging this PR, do not use the [auto-merger](https://docs.rapids.ai/resources/auto-merger/) (i.e. the `/merge` comment). Instead, an admin must manually merge by changing the merging strategy to `Create a Merge Commit`. Otherwise, history will be lost and the branches become incompatible."); + expect(mockIssueComment).toBeCalledWith(pr.data.number, "**FAILURE** - Unable to forward-merge due to an error, **manual** merge is necessary. Do not use the `Resolve conflicts` option in this PR, follow these instructions https://docs.rapids.ai/maintainers/forward-merger/ \n **IMPORTANT**: When merging this PR, do not use the [auto-merger](https://docs.rapids.ai/resources/auto-merger/) (i.e. the `/merge` comment). Instead, an admin must manually merge by changing the merging strategy to `Create a Merge Commit`. Otherwise, history will be lost and the branches become incompatible."); }, 11000) test("mergeForward should obtain the correct next branch from a given list of unsorted branches", async () => { @@ -232,12 +243,12 @@ describe("Forward Merger", () => { name: "branch-22.02", }] const mockGetBranches = jest.fn().mockName("getBranches").mockResolvedValue(branches); - const mockOpenPR = jest.fn().mockName("openPR").mockResolvedValue(null) + const mockCreatePR = jest.fn().mockName("openPR").mockResolvedValue({data: {}}) forwardMerger.getBranches = mockGetBranches - forwardMerger.openPR = mockOpenPR + forwardMerger.context.octokit.pulls.create = mockCreatePR as any; await forwardMerger.mergeForward(); - expect(mockOpenPR.mock.calls[0][0]).toMatchObject({name: "branch-22.04"}); + expect(mockCreatePR.mock.calls[0][0].base).toBe("branch-22.04"); }, 11000) test("getBranches should return versioned branches", async () => { @@ -373,68 +384,6 @@ describe("Forward Merger", () => { expect(result).toBeFalsy(); }) - test("openPR should create PR when there is a valid next branch", async () => { - const context = makePushContext({ - ref: "refs/heads/branch-22.02", - }); - const forwardMerger = new ForwardMerger(context, context.payload); - const nextBranch = { - name: "branch-22.04", - } - const mockCreatePR = jest.fn().mockName("createPR").mockResolvedValue({data: {number: 1}}); - forwardMerger.context.octokit.pulls.create = mockCreatePR as any; - const result = await forwardMerger.openPR(nextBranch as any); - - expect(result!.number).toEqual(1); - expect(mockCreatePR.mock.calls[0][0]).toMatchObject({ - owner: context.payload.repository.owner.login, - repo: context.payload.repository.name, - title: "Forward-merge " + forwardMerger.branchName + " into " + nextBranch.name, - head: forwardMerger.branchName, - base: nextBranch.name, - maintainer_can_modify: true, - body: `Forward-merge triggered by push to ${forwardMerger.branchName} that creates a PR to keep ${nextBranch.name} up-to-date. If this PR is unable to be immediately merged due to conflicts, it will remain open for the team to manually merge. See [forward-merger docs](https://docs.rapids.ai/maintainers/forward-merger/) for more info.`, - }); - }) - - test("openPR should return null when there is no next branch", async () => { - const context = makePushContext({ - ref: "refs/heads/branch-22.02", - }); - const forwardMerger = new ForwardMerger(context, context.payload); - const nextBranch = null - const mockCreatePR = jest.fn().mockName("createPR").mockResolvedValue({data: {number: 1}}); - forwardMerger.context.octokit.pulls.create = mockCreatePR as any; - const result = await forwardMerger.openPR(nextBranch as any); - - expect(result).toBeFalsy(); - expect(mockCreatePR).not.toBeCalled(); - }) - - test("mergePR should merge PR", async () => { - const context = makePushContext({ - ref: "refs/heads/branch-22.02", - }); - const forwardMerger = new ForwardMerger(context, context.payload); - const pr = { - number: 1, - head: { - sha: "sha", - } - } - const mockMergePR = jest.fn().mockName("mergePR").mockResolvedValue({data: {merged: true}}); - forwardMerger.context.octokit.pulls.merge = mockMergePR as any; - const result = await forwardMerger.mergePR(pr); - - expect(result!.merged).toEqual(true); - expect(mockMergePR.mock.calls[0][0]).toMatchObject({ - owner: context.payload.repository.owner.login, - repo: context.payload.repository.name, - pull_number: pr.number, - sha: pr.head.sha, - }); - }) - test("issueComment should create comment", async () => { const context = makePushContext({ ref: "refs/heads/branch-22.02", From 3e193671875c48d4898a373730f05de1ccb75fa9 Mon Sep 17 00:00:00 2001 From: Jake Awe Date: Mon, 29 Jan 2024 05:35:52 -0600 Subject: [PATCH 09/19] simplify to string[] --- src/plugins/ForwardMerger/forward_merger.ts | 19 ++-- test/forward_merger.test.ts | 107 +++++--------------- 2 files changed, 33 insertions(+), 93 deletions(-) diff --git a/src/plugins/ForwardMerger/forward_merger.ts b/src/plugins/ForwardMerger/forward_merger.ts index 2fb796f..047acc1 100644 --- a/src/plugins/ForwardMerger/forward_merger.ts +++ b/src/plugins/ForwardMerger/forward_merger.ts @@ -23,7 +23,6 @@ import { import { basename } from "path"; import { Context } from "probot"; import { PushEventsType } from "."; -type Branches = Awaited>; export class ForwardMerger extends OpsBotPlugin { @@ -55,11 +54,11 @@ export class ForwardMerger extends OpsBotPlugin { const { data: pr } = await this.context.octokit.pulls.create({ owner: this.repo.owner.login, repo: this.repo.name, - title: "Forward-merge " + this.currentBranch + " into " + nextBranch.name, + title: "Forward-merge " + this.currentBranch + " into " + nextBranch, head: this.currentBranch, - base: nextBranch.name, + base: nextBranch, maintainer_can_modify: true, - body: `Forward-merge triggered by push to ${this.currentBranch} that creates a PR to keep ${nextBranch.name} up-to-date. If this PR is unable to be immediately merged due to conflicts, it will remain open for the team to manually merge. See [forward-merger docs](https://docs.rapids.ai/maintainers/forward-merger/) for more info.`, + body: `Forward-merge triggered by push to ${this.currentBranch} that creates a PR to keep ${nextBranch} up-to-date. If this PR is unable to be immediately merged due to conflicts, it will remain open for the team to manually merge. See [forward-merger docs](https://docs.rapids.ai/maintainers/forward-merger/) for more info.`, }); await new Promise(resolve => setTimeout(resolve, 10000)); @@ -83,13 +82,13 @@ export class ForwardMerger extends OpsBotPlugin { owner: this.repo.owner.login, repo: this.repo.name, }); - return branches.filter((branch) => isVersionedBranch(branch.name)); + return branches.filter((branch) => isVersionedBranch(branch.name)).map((branch) => branch.name); } - sortBranches(branches: Branches) { + sortBranches(branches: string[]) { return branches.sort((a, b) => { - const [yearA, monthA] = getVersionFromBranch(a.name).split('.').map(Number) - const [yearB, monthB] = getVersionFromBranch(b.name).split('.').map(Number) + const [yearA, monthA] = getVersionFromBranch(a).split('.').map(Number) + const [yearB, monthB] = getVersionFromBranch(b).split('.').map(Number) if (yearA !== yearB) { return yearA - yearB; } else { @@ -98,9 +97,9 @@ export class ForwardMerger extends OpsBotPlugin { }); } - getNextBranch(sortedBranches: Branches) { + getNextBranch(sortedBranches: string[]) { const currentBranchIndex = sortedBranches.findIndex( - (branch) => branch.name === this.currentBranch + (branch) => branch === this.currentBranch ); const nextBranch = sortedBranches[currentBranchIndex + 1]; return nextBranch; diff --git a/test/forward_merger.test.ts b/test/forward_merger.test.ts index 61454bb..cf3fcf7 100644 --- a/test/forward_merger.test.ts +++ b/test/forward_merger.test.ts @@ -92,9 +92,7 @@ describe("Forward Merger", () => { const forwardMerger = new ForwardMerger(context, context.payload); forwardMerger.getBranches = jest.fn().mockName("getBranches").mockResolvedValue(null); forwardMerger.sortBranches = jest.fn().mockName("sortBranches").mockReturnValue(null); - const nextBranch = { - name: "branch-21.10", - } + const nextBranch = "branch-21.10" const mockGetNextBranch = jest.fn().mockName("getNextBranch").mockReturnValue(nextBranch); forwardMerger.getNextBranch = mockGetNextBranch; const mockCreatePR = jest.fn().mockName("openPR").mockResolvedValue({data: {}}); @@ -105,11 +103,11 @@ describe("Forward Merger", () => { expect(mockCreatePR.mock.calls[0][0]).toMatchObject({ owner: context.payload.repository.owner.login, repo: context.payload.repository.name, - title: "Forward-merge " + forwardMerger.currentBranch + " into " + nextBranch.name, + title: "Forward-merge " + forwardMerger.currentBranch + " into " + nextBranch, head: forwardMerger.currentBranch, - base: nextBranch.name, + base: nextBranch, maintainer_can_modify: true, - body: `Forward-merge triggered by push to ${forwardMerger.currentBranch} that creates a PR to keep ${nextBranch.name} up-to-date. If this PR is unable to be immediately merged due to conflicts, it will remain open for the team to manually merge. See [forward-merger docs](https://docs.rapids.ai/maintainers/forward-merger/) for more info.`, + body: `Forward-merge triggered by push to ${forwardMerger.currentBranch} that creates a PR to keep ${nextBranch} up-to-date. If this PR is unable to be immediately merged due to conflicts, it will remain open for the team to manually merge. See [forward-merger docs](https://docs.rapids.ai/maintainers/forward-merger/) for more info.`, }); }, 11000) @@ -229,19 +227,7 @@ describe("Forward Merger", () => { ref: "refs/heads/branch-22.02", }); const forwardMerger = new ForwardMerger(context, context.payload); - const branches = [ - { - name: "branch-22.04" - }, - { - name: "branch-21.12", - }, - { - name: "branch-21.10", - }, - { - name: "branch-22.02", - }] + const branches = ["branch-22.04", "branch-21.12", "branch-21.10", "branch-22.02"] const mockGetBranches = jest.fn().mockName("getBranches").mockResolvedValue(branches); const mockCreatePR = jest.fn().mockName("openPR").mockResolvedValue({data: {}}) forwardMerger.getBranches = mockGetBranches @@ -274,8 +260,8 @@ describe("Forward Merger", () => { const result = await forwardMerger.getBranches(); expect(result.length).toEqual(2); - expect(result[0].name).toEqual("branch-21.12"); - expect(result[1].name).toEqual("branch-21.10"); + expect(result[0]).toEqual("branch-21.12"); + expect(result[1]).toEqual("branch-21.10"); expect(mockListBranchesPaginate.mock.calls[0][0]).toBe(mockListBranches) expect(mockListBranchesPaginate.mock.calls[0][1]).toMatchObject({ owner: context.payload.repository.owner.login, @@ -288,27 +274,15 @@ describe("Forward Merger", () => { ref: "refs/heads/branch-21.12", }); const forwardMerger = new ForwardMerger(context, context.payload); - const branches: any[] = [ - { - name: "branch-21.12", - }, - { - name: "branch-21.10", - }, - { - name: "branch-19.10", - }, - { - name: "branch-22.02", - }] - - const result = forwardMerger.sortBranches(branches); - - expect(result.length).toEqual(4); - expect(result[0].name).toEqual("branch-19.10"); - expect(result[1].name).toEqual("branch-21.10"); - expect(result[2].name).toEqual("branch-21.12"); - expect(result[3].name).toEqual("branch-22.02"); + const branches = ["branch-21.12", "branch-21.10", "branch-19.10", "branch-22.02"] + + const result = forwardMerger.sortBranches(branches); + + expect(result.length).toEqual(4); + expect(result[0]).toEqual("branch-19.10"); + expect(result[1]).toEqual("branch-21.10"); + expect(result[2]).toEqual("branch-21.12"); + expect(result[3]).toEqual("branch-22.02"); }) test("sortBranches should sort 0.9/0.10-type branches", async () => { @@ -316,27 +290,15 @@ describe("Forward Merger", () => { ref: "refs/heads/branch-21.12", }); const forwardMerger = new ForwardMerger(context, context.payload); - const branches: any[] = [ - { - name: "branch-0.12", - }, - { - name: "branch-0.10", - }, - { - name: "branch-2.10", - }, - { - name: "branch-1.02", - }] + const branches = ["branch-0.12", "branch-0.10", "branch-2.10", "branch-1.02"] const result = forwardMerger.sortBranches(branches); expect(result.length).toEqual(4); - expect(result[0].name).toEqual("branch-0.10"); - expect(result[1].name).toEqual("branch-0.12"); - expect(result[2].name).toEqual("branch-1.02"); - expect(result[3].name).toEqual("branch-2.10"); + expect(result[0]).toEqual("branch-0.10"); + expect(result[1]).toEqual("branch-0.12"); + expect(result[2]).toEqual("branch-1.02"); + expect(result[3]).toEqual("branch-2.10"); }) test("getNextBranch should return next branch", async () => { @@ -344,23 +306,11 @@ describe("Forward Merger", () => { ref: "refs/heads/branch-21.12", }); const forwardMerger = new ForwardMerger(context, context.payload); - const branches: any[] = [ - { - name: "branch-21.12", - }, - { - name: "branch-21.10", - }, - { - name: "branch-21.10", - }, - { - name: "branch-22.02", - }] + const branches = ["branch-21.12", "branch-21.10", "branch-22.02"] const sortedBranches = forwardMerger.sortBranches(branches) const result = await forwardMerger.getNextBranch(sortedBranches); - expect(result.name).toEqual("branch-22.02"); + expect(result).toEqual("branch-22.02"); }) test("getNextBranch should return null if there is no next branch", async () => { @@ -368,16 +318,7 @@ describe("Forward Merger", () => { ref: "refs/heads/branch-22.02", }); const forwardMerger = new ForwardMerger(context, context.payload); - const branches: any[] = [ - { - name: "branch-21.12", - }, - { - name: "branch-21.10", - }, - { - name: "branch-22.02", - }] + const branches = ["branch-21.12", "branch-21.10", "branch-22.02"] const sortedBranches = forwardMerger.sortBranches(branches) const result = await forwardMerger.getNextBranch(sortedBranches); From ec41b569622dad279059f941640cc1de2e268479 Mon Sep 17 00:00:00 2001 From: Jake Awe Date: Mon, 29 Jan 2024 05:39:42 -0600 Subject: [PATCH 10/19] define fn return types --- src/plugins/ForwardMerger/forward_merger.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/plugins/ForwardMerger/forward_merger.ts b/src/plugins/ForwardMerger/forward_merger.ts index 047acc1..3e9f25a 100644 --- a/src/plugins/ForwardMerger/forward_merger.ts +++ b/src/plugins/ForwardMerger/forward_merger.ts @@ -37,7 +37,7 @@ export class ForwardMerger extends OpsBotPlugin { this.repo = payload.repository; } - async mergeForward() { + async mergeForward(): Promise { if (await this.pluginIsDisabled()) return; if (!isVersionedBranch(this.currentBranch)) { @@ -77,7 +77,7 @@ export class ForwardMerger extends OpsBotPlugin { } } - async getBranches() { + async getBranches(): Promise { const branches = await this.context.octokit.paginate(this.context.octokit.repos.listBranches, { owner: this.repo.owner.login, repo: this.repo.name, @@ -85,7 +85,7 @@ export class ForwardMerger extends OpsBotPlugin { return branches.filter((branch) => isVersionedBranch(branch.name)).map((branch) => branch.name); } - sortBranches(branches: string[]) { + sortBranches(branches: string[]): string[] { return branches.sort((a, b) => { const [yearA, monthA] = getVersionFromBranch(a).split('.').map(Number) const [yearB, monthB] = getVersionFromBranch(b).split('.').map(Number) @@ -97,7 +97,7 @@ export class ForwardMerger extends OpsBotPlugin { }); } - getNextBranch(sortedBranches: string[]) { + getNextBranch(sortedBranches: string[]): string | undefined { const currentBranchIndex = sortedBranches.findIndex( (branch) => branch === this.currentBranch ); @@ -105,7 +105,7 @@ export class ForwardMerger extends OpsBotPlugin { return nextBranch; } - async issueComment(id, comment) { + async issueComment(id, comment): Promise { await this.context.octokit.issues.createComment({ owner: this.repo.owner.login, repo: this.repo.name, From 2d38201ed4435307a3016c31276fdd6cad37b067 Mon Sep 17 00:00:00 2001 From: Jake Awe Date: Mon, 29 Jan 2024 06:12:19 -0600 Subject: [PATCH 11/19] format files with Prettier --- src/plugins/ForwardMerger/forward_merger.ts | 44 +- test/forward_merger.test.ts | 478 +++++++++++++------- 2 files changed, 333 insertions(+), 189 deletions(-) diff --git a/src/plugins/ForwardMerger/forward_merger.ts b/src/plugins/ForwardMerger/forward_merger.ts index 3e9f25a..1dde0d8 100644 --- a/src/plugins/ForwardMerger/forward_merger.ts +++ b/src/plugins/ForwardMerger/forward_merger.ts @@ -16,21 +16,20 @@ import { OpsBotPlugin } from "../../plugin"; import { PayloadRepository } from "../../types"; -import { - isVersionedBranch, - getVersionFromBranch, -} from "../../shared"; +import { isVersionedBranch, getVersionFromBranch } from "../../shared"; import { basename } from "path"; import { Context } from "probot"; import { PushEventsType } from "."; - export class ForwardMerger extends OpsBotPlugin { context: Context; currentBranch: string; repo: PayloadRepository; - constructor(context: Context, private payload: Context["payload"]) { + constructor( + context: Context, + private payload: Context["payload"] + ) { super("forward_merger", context); this.context = context; this.currentBranch = basename(this.payload.ref); @@ -49,7 +48,7 @@ export class ForwardMerger extends OpsBotPlugin { const sortedBranches = this.sortBranches(branches); const nextBranch = this.getNextBranch(sortedBranches); - if(!nextBranch) return + if (!nextBranch) return; const { data: pr } = await this.context.octokit.pulls.create({ owner: this.repo.owner.login, @@ -61,7 +60,7 @@ export class ForwardMerger extends OpsBotPlugin { body: `Forward-merge triggered by push to ${this.currentBranch} that creates a PR to keep ${nextBranch} up-to-date. If this PR is unable to be immediately merged due to conflicts, it will remain open for the team to manually merge. See [forward-merger docs](https://docs.rapids.ai/maintainers/forward-merger/) for more info.`, }); - await new Promise(resolve => setTimeout(resolve, 10000)); + await new Promise((resolve) => setTimeout(resolve, 10000)); try { this.logger.info("Merging PR"); @@ -71,24 +70,35 @@ export class ForwardMerger extends OpsBotPlugin { pull_number: pr.number, sha: pr.head.sha, }); - await this.issueComment(pr.number, "**SUCCESS** - forward-merge complete."); + await this.issueComment( + pr.number, + "**SUCCESS** - forward-merge complete." + ); } catch (error) { - await this.issueComment(pr.number, "**FAILURE** - Unable to forward-merge due to an error, **manual** merge is necessary. Do not use the `Resolve conflicts` option in this PR, follow these instructions https://docs.rapids.ai/maintainers/forward-merger/ \n **IMPORTANT**: When merging this PR, do not use the [auto-merger](https://docs.rapids.ai/resources/auto-merger/) (i.e. the `/merge` comment). Instead, an admin must manually merge by changing the merging strategy to `Create a Merge Commit`. Otherwise, history will be lost and the branches become incompatible."); + await this.issueComment( + pr.number, + "**FAILURE** - Unable to forward-merge due to an error, **manual** merge is necessary. Do not use the `Resolve conflicts` option in this PR, follow these instructions https://docs.rapids.ai/maintainers/forward-merger/ \n **IMPORTANT**: When merging this PR, do not use the [auto-merger](https://docs.rapids.ai/resources/auto-merger/) (i.e. the `/merge` comment). Instead, an admin must manually merge by changing the merging strategy to `Create a Merge Commit`. Otherwise, history will be lost and the branches become incompatible." + ); } } async getBranches(): Promise { - const branches = await this.context.octokit.paginate(this.context.octokit.repos.listBranches, { - owner: this.repo.owner.login, - repo: this.repo.name, - }); - return branches.filter((branch) => isVersionedBranch(branch.name)).map((branch) => branch.name); + const branches = await this.context.octokit.paginate( + this.context.octokit.repos.listBranches, + { + owner: this.repo.owner.login, + repo: this.repo.name, + } + ); + return branches + .filter((branch) => isVersionedBranch(branch.name)) + .map((branch) => branch.name); } sortBranches(branches: string[]): string[] { return branches.sort((a, b) => { - const [yearA, monthA] = getVersionFromBranch(a).split('.').map(Number) - const [yearB, monthB] = getVersionFromBranch(b).split('.').map(Number) + const [yearA, monthA] = getVersionFromBranch(a).split(".").map(Number); + const [yearB, monthB] = getVersionFromBranch(b).split(".").map(Number); if (yearA !== yearB) { return yearA - yearB; } else { diff --git a/test/forward_merger.test.ts b/test/forward_merger.test.ts index cf3fcf7..b8cb43a 100644 --- a/test/forward_merger.test.ts +++ b/test/forward_merger.test.ts @@ -32,7 +32,7 @@ describe("Forward Merger", () => { test("mergeForward should not run on push to non-versioned branch", async () => { const context = makePushContext({ - ref: "refs/heads/unversioned", + ref: "refs/heads/unversioned", }); const forwardMerger = new ForwardMerger(context, context.payload); const mockGetBranches = jest.fn().mockName("getBranches"); @@ -40,7 +40,7 @@ describe("Forward Merger", () => { await forwardMerger.mergeForward(); expect(mockGetBranches).not.toBeCalled(); - }) + }); test("mergeForward should not run when plugin is disabled", async () => { const context = makePushContext({ @@ -49,16 +49,19 @@ describe("Forward Merger", () => { const forwardMerger = new ForwardMerger(context, context.payload); const mockGetBranches = jest.fn().mockName("getBranches"); forwardMerger.getBranches = mockGetBranches; - const mockPluginIsDisabled = jest.fn().mockName("pluginIsDisabled").mockResolvedValue(true); + const mockPluginIsDisabled = jest + .fn() + .mockName("pluginIsDisabled") + .mockResolvedValue(true); forwardMerger.pluginIsDisabled = mockPluginIsDisabled; await forwardMerger.mergeForward(); expect(mockPluginIsDisabled).toBeCalled(); expect(mockGetBranches).not.toBeCalled(); - }) + }); test("mergeForward should sort branches", async () => { - const context = makePushContext({ + const context = makePushContext({ ref: "refs/heads/branch-21.12", }); const forwardMerger = new ForwardMerger(context, context.payload); @@ -68,34 +71,56 @@ describe("Forward Merger", () => { }, { name: "branch-21.10", - }] - // mock getBranches, sortBranches, and getNextBranch - const mockGetBranches = jest.fn().mockName("getBranches").mockResolvedValue(branches); - forwardMerger.getBranches = mockGetBranches; - const sortedBranches = [] - const mockSortBranches = jest.fn().mockName("sortBranches").mockReturnValue(sortedBranches); - forwardMerger.sortBranches = mockSortBranches; - const mockGetNextBranch = jest.fn().mockName("getNextBranch").mockReturnValue(null); - forwardMerger.getNextBranch = mockGetNextBranch; - - await forwardMerger.mergeForward(); - - expect(mockGetBranches).toBeCalled(); - expect(mockSortBranches).toBeCalledWith(branches); - expect(mockGetNextBranch).toBeCalledWith(sortedBranches); - }) + }, + ]; + // mock getBranches, sortBranches, and getNextBranch + const mockGetBranches = jest + .fn() + .mockName("getBranches") + .mockResolvedValue(branches); + forwardMerger.getBranches = mockGetBranches; + const sortedBranches = []; + const mockSortBranches = jest + .fn() + .mockName("sortBranches") + .mockReturnValue(sortedBranches); + forwardMerger.sortBranches = mockSortBranches; + const mockGetNextBranch = jest + .fn() + .mockName("getNextBranch") + .mockReturnValue(null); + forwardMerger.getNextBranch = mockGetNextBranch; + + await forwardMerger.mergeForward(); + + expect(mockGetBranches).toBeCalled(); + expect(mockSortBranches).toBeCalledWith(branches); + expect(mockGetNextBranch).toBeCalledWith(sortedBranches); + }); test("mergeForward should open PR on valid next branch", async () => { const context = makePushContext({ ref: "refs/heads/branch-21.12", }); const forwardMerger = new ForwardMerger(context, context.payload); - forwardMerger.getBranches = jest.fn().mockName("getBranches").mockResolvedValue(null); - forwardMerger.sortBranches = jest.fn().mockName("sortBranches").mockReturnValue(null); - const nextBranch = "branch-21.10" - const mockGetNextBranch = jest.fn().mockName("getNextBranch").mockReturnValue(nextBranch); + forwardMerger.getBranches = jest + .fn() + .mockName("getBranches") + .mockResolvedValue(null); + forwardMerger.sortBranches = jest + .fn() + .mockName("sortBranches") + .mockReturnValue(null); + const nextBranch = "branch-21.10"; + const mockGetNextBranch = jest + .fn() + .mockName("getNextBranch") + .mockReturnValue(nextBranch); forwardMerger.getNextBranch = mockGetNextBranch; - const mockCreatePR = jest.fn().mockName("openPR").mockResolvedValue({data: {}}); + const mockCreatePR = jest + .fn() + .mockName("openPR") + .mockResolvedValue({ data: {} }); forwardMerger.context.octokit.pulls.create = mockCreatePR as any; await forwardMerger.mergeForward(); @@ -103,23 +128,33 @@ describe("Forward Merger", () => { expect(mockCreatePR.mock.calls[0][0]).toMatchObject({ owner: context.payload.repository.owner.login, repo: context.payload.repository.name, - title: "Forward-merge " + forwardMerger.currentBranch + " into " + nextBranch, + title: + "Forward-merge " + forwardMerger.currentBranch + " into " + nextBranch, head: forwardMerger.currentBranch, base: nextBranch, maintainer_can_modify: true, body: `Forward-merge triggered by push to ${forwardMerger.currentBranch} that creates a PR to keep ${nextBranch} up-to-date. If this PR is unable to be immediately merged due to conflicts, it will remain open for the team to manually merge. See [forward-merger docs](https://docs.rapids.ai/maintainers/forward-merger/) for more info.`, }); - }, 11000) + }, 11000); test("mergeForward should not open PR on invalid next branch", async () => { const context = makePushContext({ ref: "refs/heads/branch-22.02", }); const forwardMerger = new ForwardMerger(context, context.payload); - forwardMerger.getBranches = jest.fn().mockName("getBranches").mockResolvedValue(null); - forwardMerger.sortBranches = jest.fn().mockName("sortBranches").mockReturnValue(null); - const nextBranch = null - const mockGetNextBranch = jest.fn().mockName("getNextBranch").mockReturnValue(nextBranch); + forwardMerger.getBranches = jest + .fn() + .mockName("getBranches") + .mockResolvedValue(null); + forwardMerger.sortBranches = jest + .fn() + .mockName("sortBranches") + .mockReturnValue(null); + const nextBranch = null; + const mockGetNextBranch = jest + .fn() + .mockName("getNextBranch") + .mockReturnValue(nextBranch); forwardMerger.getNextBranch = mockGetNextBranch; const mockCreatePR = jest.fn().mockName("openPR").mockResolvedValue(null); forwardMerger.context.octokit.pulls.create = mockCreatePR as any; @@ -127,24 +162,41 @@ describe("Forward Merger", () => { await forwardMerger.mergeForward(); expect(mockCreatePR).not.toBeCalled(); - }) + }); test("mergeForward should merge PR after opening PR", async () => { const context = makePushContext({ ref: "refs/heads/branch-21.12", }); const forwardMerger = new ForwardMerger(context, context.payload); - forwardMerger.getBranches = jest.fn().mockName("getBranches").mockResolvedValue(null); - forwardMerger.sortBranches = jest.fn().mockName("sortBranches").mockReturnValue(null); + forwardMerger.getBranches = jest + .fn() + .mockName("getBranches") + .mockResolvedValue(null); + forwardMerger.sortBranches = jest + .fn() + .mockName("sortBranches") + .mockReturnValue(null); const nextBranch = { name: "branch-21.10", - } - forwardMerger.getNextBranch = jest.fn().mockName("getNextBranch").mockReturnValue(nextBranch); - const pr = {data: {number: 1, head: {sha: 123456}}} - forwardMerger.context.octokit.pulls.create = jest.fn().mockName("openPR").mockResolvedValue(pr); - const mockMergePR = jest.fn().mockName("mergePR").mockResolvedValue({merged:true}); + }; + forwardMerger.getNextBranch = jest + .fn() + .mockName("getNextBranch") + .mockReturnValue(nextBranch); + const pr = { data: { number: 1, head: { sha: 123456 } } }; + forwardMerger.context.octokit.pulls.create = ( + jest.fn().mockName("openPR").mockResolvedValue(pr) + ); + const mockMergePR = jest + .fn() + .mockName("mergePR") + .mockResolvedValue({ merged: true }); forwardMerger.context.octokit.pulls.merge = mockMergePR; - forwardMerger.issueComment = jest.fn().mockName("issueComment").mockResolvedValue(null); + forwardMerger.issueComment = jest + .fn() + .mockName("issueComment") + .mockResolvedValue(null); await forwardMerger.mergeForward(); @@ -154,191 +206,273 @@ describe("Forward Merger", () => { pull_number: pr.data.number, sha: pr.data.head.sha, }); - }, 11000) + }, 11000); test("should not merge PR if there is no PR", async () => { const context = makePushContext({ ref: "refs/heads/branch-21.12", }); const forwardMerger = new ForwardMerger(context, context.payload); - forwardMerger.getBranches = jest.fn().mockName("getBranches").mockResolvedValue(null); - forwardMerger.sortBranches = jest.fn().mockName("sortBranches").mockReturnValue(null); - const nextBranch = { - name: "branch-21.10", - } - forwardMerger.getNextBranch = jest.fn().mockName("getNextBranch").mockReturnValue(nextBranch); - forwardMerger.context.octokit.pulls.create = jest.fn().mockName("openPR").mockResolvedValue({data: {}}); - const mockMergePR = jest.fn().mockName("mergePR").mockResolvedValue(null); - forwardMerger.context.octokit.pulls.merge = mockMergePR; + forwardMerger.getBranches = jest + .fn() + .mockName("getBranches") + .mockResolvedValue(null); + forwardMerger.sortBranches = jest + .fn() + .mockName("sortBranches") + .mockReturnValue(null); + const nextBranch = { + name: "branch-21.10", + }; + forwardMerger.getNextBranch = jest + .fn() + .mockName("getNextBranch") + .mockReturnValue(nextBranch); + forwardMerger.context.octokit.pulls.create = ( + jest.fn().mockName("openPR").mockResolvedValue({ data: {} }) + ); + const mockMergePR = jest.fn().mockName("mergePR").mockResolvedValue(null); + forwardMerger.context.octokit.pulls.merge = mockMergePR; - await forwardMerger.mergeForward(); + await forwardMerger.mergeForward(); - expect(mockMergePR).not.toBeCalled(); - }, 11000) + expect(mockMergePR).not.toBeCalled(); + }, 11000); test("should comment failure on PR if merge is successful", async () => { const context = makePushContext({ ref: "refs/heads/branch-21.12", }); const forwardMerger = new ForwardMerger(context, context.payload); - forwardMerger.getBranches = jest.fn().mockName("getBranches").mockResolvedValue(null); - forwardMerger.sortBranches = jest.fn().mockName("sortBranches").mockReturnValue(null); - const nextBranch = { - name: "branch-21.10", - } - forwardMerger.getNextBranch = jest.fn().mockName("getNextBranch").mockResolvedValue(nextBranch); - const pr = {data: {number: 1, head: {sha: 123456}}} - forwardMerger.context.octokit.pulls.create = jest.fn().mockName("openPR").mockResolvedValue(pr); - forwardMerger.context.octokit.pulls.merge = jest.fn().mockName("mergePR").mockResolvedValue({merged: true}); - const mockIssueComment = jest.fn().mockName("issueComment").mockResolvedValue(null); - forwardMerger.issueComment = mockIssueComment; + forwardMerger.getBranches = jest + .fn() + .mockName("getBranches") + .mockResolvedValue(null); + forwardMerger.sortBranches = jest + .fn() + .mockName("sortBranches") + .mockReturnValue(null); + const nextBranch = { + name: "branch-21.10", + }; + forwardMerger.getNextBranch = jest + .fn() + .mockName("getNextBranch") + .mockResolvedValue(nextBranch); + const pr = { data: { number: 1, head: { sha: 123456 } } }; + forwardMerger.context.octokit.pulls.create = ( + jest.fn().mockName("openPR").mockResolvedValue(pr) + ); + forwardMerger.context.octokit.pulls.merge = ( + jest.fn().mockName("mergePR").mockResolvedValue({ merged: true }) + ); + const mockIssueComment = jest + .fn() + .mockName("issueComment") + .mockResolvedValue(null); + forwardMerger.issueComment = mockIssueComment; - await forwardMerger.mergeForward(); + await forwardMerger.mergeForward(); - expect(mockIssueComment).toBeCalledWith(pr.data.number, "**SUCCESS** - forward-merge complete."); - }, 11000) + expect(mockIssueComment).toBeCalledWith( + pr.data.number, + "**SUCCESS** - forward-merge complete." + ); + }, 11000); test("should comment failure on PR if merge is unsuccessful", async () => { const context = makePushContext({ ref: "refs/heads/branch-21.12", }); const forwardMerger = new ForwardMerger(context, context.payload); - forwardMerger.getBranches = jest.fn().mockName("getBranches").mockResolvedValue(null); - forwardMerger.sortBranches = jest.fn().mockName("sortBranches").mockResolvedValue(null); + forwardMerger.getBranches = jest + .fn() + .mockName("getBranches") + .mockResolvedValue(null); + forwardMerger.sortBranches = jest + .fn() + .mockName("sortBranches") + .mockResolvedValue(null); const nextBranch = { name: "branch-21.10", - } - forwardMerger.getNextBranch = jest.fn().mockName("getNextBranch").mockReturnValue(nextBranch); - const pr = {data: {number: 1, head: {sha: 123456}}} - forwardMerger.context.octokit.pulls.create = jest.fn().mockName("openPR").mockResolvedValue(pr); - const mockMergePR = jest.fn().mockName("mergePR").mockResolvedValue({merged: false}); + }; + forwardMerger.getNextBranch = jest + .fn() + .mockName("getNextBranch") + .mockReturnValue(nextBranch); + const pr = { data: { number: 1, head: { sha: 123456 } } }; + forwardMerger.context.octokit.pulls.create = ( + jest.fn().mockName("openPR").mockResolvedValue(pr) + ); + const mockMergePR = jest + .fn() + .mockName("mergePR") + .mockResolvedValue({ merged: false }); forwardMerger.context.octokit.pulls.merge = mockMergePR; mockMergePR.mockRejectedValueOnce(new Error("error")); - const mockIssueComment = jest.fn().mockName("issueComment").mockResolvedValue(null); + const mockIssueComment = jest + .fn() + .mockName("issueComment") + .mockResolvedValue(null); forwardMerger.issueComment = mockIssueComment; await forwardMerger.mergeForward(); - expect(mockIssueComment).toBeCalledWith(pr.data.number, "**FAILURE** - Unable to forward-merge due to an error, **manual** merge is necessary. Do not use the `Resolve conflicts` option in this PR, follow these instructions https://docs.rapids.ai/maintainers/forward-merger/ \n **IMPORTANT**: When merging this PR, do not use the [auto-merger](https://docs.rapids.ai/resources/auto-merger/) (i.e. the `/merge` comment). Instead, an admin must manually merge by changing the merging strategy to `Create a Merge Commit`. Otherwise, history will be lost and the branches become incompatible."); - }, 11000) + expect(mockIssueComment).toBeCalledWith( + pr.data.number, + "**FAILURE** - Unable to forward-merge due to an error, **manual** merge is necessary. Do not use the `Resolve conflicts` option in this PR, follow these instructions https://docs.rapids.ai/maintainers/forward-merger/ \n **IMPORTANT**: When merging this PR, do not use the [auto-merger](https://docs.rapids.ai/resources/auto-merger/) (i.e. the `/merge` comment). Instead, an admin must manually merge by changing the merging strategy to `Create a Merge Commit`. Otherwise, history will be lost and the branches become incompatible." + ); + }, 11000); test("mergeForward should obtain the correct next branch from a given list of unsorted branches", async () => { const context = makePushContext({ ref: "refs/heads/branch-22.02", }); const forwardMerger = new ForwardMerger(context, context.payload); - const branches = ["branch-22.04", "branch-21.12", "branch-21.10", "branch-22.02"] - const mockGetBranches = jest.fn().mockName("getBranches").mockResolvedValue(branches); - const mockCreatePR = jest.fn().mockName("openPR").mockResolvedValue({data: {}}) - forwardMerger.getBranches = mockGetBranches + const branches = [ + "branch-22.04", + "branch-21.12", + "branch-21.10", + "branch-22.02", + ]; + const mockGetBranches = jest + .fn() + .mockName("getBranches") + .mockResolvedValue(branches); + const mockCreatePR = jest + .fn() + .mockName("openPR") + .mockResolvedValue({ data: {} }); + forwardMerger.getBranches = mockGetBranches; forwardMerger.context.octokit.pulls.create = mockCreatePR as any; await forwardMerger.mergeForward(); expect(mockCreatePR.mock.calls[0][0].base).toBe("branch-22.04"); - }, 11000) + }, 11000); test("getBranches should return versioned branches", async () => { - const context = makePushContext({ - ref: "refs/heads/branch-21.12", - }); - const forwardMerger = new ForwardMerger(context, context.payload); - const branches = [ - { - name: "branch-21.12", - }, - { - name: "non-versioned", - }, - { - name: "branch-21.10", - }] - const mockListBranchesPaginate = jest.fn().mockName("listBranches").mockResolvedValue(branches); - forwardMerger.context.octokit.paginate = mockListBranchesPaginate as any; - const mockListBranches = jest.fn().mockName("listBranches").mockResolvedValue("something"); - forwardMerger.context.octokit.repos.listBranches = mockListBranches as any; - - const result = await forwardMerger.getBranches(); - - expect(result.length).toEqual(2); - expect(result[0]).toEqual("branch-21.12"); - expect(result[1]).toEqual("branch-21.10"); - expect(mockListBranchesPaginate.mock.calls[0][0]).toBe(mockListBranches) - expect(mockListBranchesPaginate.mock.calls[0][1]).toMatchObject({ - owner: context.payload.repository.owner.login, - repo: context.payload.repository.name, - }); - }) + const context = makePushContext({ + ref: "refs/heads/branch-21.12", + }); + const forwardMerger = new ForwardMerger(context, context.payload); + const branches = [ + { + name: "branch-21.12", + }, + { + name: "non-versioned", + }, + { + name: "branch-21.10", + }, + ]; + const mockListBranchesPaginate = jest + .fn() + .mockName("listBranches") + .mockResolvedValue(branches); + forwardMerger.context.octokit.paginate = mockListBranchesPaginate as any; + const mockListBranches = jest + .fn() + .mockName("listBranches") + .mockResolvedValue("something"); + forwardMerger.context.octokit.repos.listBranches = mockListBranches as any; + + const result = await forwardMerger.getBranches(); + + expect(result.length).toEqual(2); + expect(result[0]).toEqual("branch-21.12"); + expect(result[1]).toEqual("branch-21.10"); + expect(mockListBranchesPaginate.mock.calls[0][0]).toBe(mockListBranches); + expect(mockListBranchesPaginate.mock.calls[0][1]).toMatchObject({ + owner: context.payload.repository.owner.login, + repo: context.payload.repository.name, + }); + }); test("sortBranches should sort branches by version", async () => { - const context = makePushContext({ - ref: "refs/heads/branch-21.12", - }); - const forwardMerger = new ForwardMerger(context, context.payload); - const branches = ["branch-21.12", "branch-21.10", "branch-19.10", "branch-22.02"] - - const result = forwardMerger.sortBranches(branches); - - expect(result.length).toEqual(4); - expect(result[0]).toEqual("branch-19.10"); - expect(result[1]).toEqual("branch-21.10"); - expect(result[2]).toEqual("branch-21.12"); - expect(result[3]).toEqual("branch-22.02"); - }) + const context = makePushContext({ + ref: "refs/heads/branch-21.12", + }); + const forwardMerger = new ForwardMerger(context, context.payload); + const branches = [ + "branch-21.12", + "branch-21.10", + "branch-19.10", + "branch-22.02", + ]; + + const result = forwardMerger.sortBranches(branches); + + expect(result.length).toEqual(4); + expect(result[0]).toEqual("branch-19.10"); + expect(result[1]).toEqual("branch-21.10"); + expect(result[2]).toEqual("branch-21.12"); + expect(result[3]).toEqual("branch-22.02"); + }); test("sortBranches should sort 0.9/0.10-type branches", async () => { const context = makePushContext({ - ref: "refs/heads/branch-21.12", + ref: "refs/heads/branch-21.12", }); const forwardMerger = new ForwardMerger(context, context.payload); - const branches = ["branch-0.12", "branch-0.10", "branch-2.10", "branch-1.02"] - - const result = forwardMerger.sortBranches(branches); - - expect(result.length).toEqual(4); - expect(result[0]).toEqual("branch-0.10"); - expect(result[1]).toEqual("branch-0.12"); - expect(result[2]).toEqual("branch-1.02"); - expect(result[3]).toEqual("branch-2.10"); -}) + const branches = [ + "branch-0.12", + "branch-0.10", + "branch-2.10", + "branch-1.02", + ]; + + const result = forwardMerger.sortBranches(branches); + + expect(result.length).toEqual(4); + expect(result[0]).toEqual("branch-0.10"); + expect(result[1]).toEqual("branch-0.12"); + expect(result[2]).toEqual("branch-1.02"); + expect(result[3]).toEqual("branch-2.10"); + }); test("getNextBranch should return next branch", async () => { - const context = makePushContext({ - ref: "refs/heads/branch-21.12", - }); - const forwardMerger = new ForwardMerger(context, context.payload); - const branches = ["branch-21.12", "branch-21.10", "branch-22.02"] - const sortedBranches = forwardMerger.sortBranches(branches) - const result = await forwardMerger.getNextBranch(sortedBranches); + const context = makePushContext({ + ref: "refs/heads/branch-21.12", + }); + const forwardMerger = new ForwardMerger(context, context.payload); + const branches = ["branch-21.12", "branch-21.10", "branch-22.02"]; + const sortedBranches = forwardMerger.sortBranches(branches); + const result = await forwardMerger.getNextBranch(sortedBranches); - expect(result).toEqual("branch-22.02"); - }) + expect(result).toEqual("branch-22.02"); + }); test("getNextBranch should return null if there is no next branch", async () => { - const context = makePushContext({ - ref: "refs/heads/branch-22.02", - }); - const forwardMerger = new ForwardMerger(context, context.payload); - const branches = ["branch-21.12", "branch-21.10", "branch-22.02"] - const sortedBranches = forwardMerger.sortBranches(branches) - const result = await forwardMerger.getNextBranch(sortedBranches); + const context = makePushContext({ + ref: "refs/heads/branch-22.02", + }); + const forwardMerger = new ForwardMerger(context, context.payload); + const branches = ["branch-21.12", "branch-21.10", "branch-22.02"]; + const sortedBranches = forwardMerger.sortBranches(branches); + const result = await forwardMerger.getNextBranch(sortedBranches); - expect(result).toBeFalsy(); - }) + expect(result).toBeFalsy(); + }); test("issueComment should create comment", async () => { - const context = makePushContext({ - ref: "refs/heads/branch-22.02", - }); - const forwardMerger = new ForwardMerger(context, context.payload); - const mockCreateComment = jest.fn().mockName("createComment").mockResolvedValue(null); - forwardMerger.context.octokit.issues.createComment = mockCreateComment as any; - await forwardMerger.issueComment(1, "comment"); - - expect(mockCreateComment.mock.calls[0][0]).toMatchObject({ - owner: context.payload.repository.owner.login, - repo: context.payload.repository.name, - issue_number: 1, - body: "comment", - }); - }) -}) + const context = makePushContext({ + ref: "refs/heads/branch-22.02", + }); + const forwardMerger = new ForwardMerger(context, context.payload); + const mockCreateComment = jest + .fn() + .mockName("createComment") + .mockResolvedValue(null); + forwardMerger.context.octokit.issues.createComment = + mockCreateComment as any; + await forwardMerger.issueComment(1, "comment"); + + expect(mockCreateComment.mock.calls[0][0]).toMatchObject({ + owner: context.payload.repository.owner.login, + repo: context.payload.repository.name, + issue_number: 1, + body: "comment", + }); + }); +}); From b6f1b8ae74427f0087ef4ee40e99917b1c7107f3 Mon Sep 17 00:00:00 2001 From: Jake Awe Date: Mon, 29 Jan 2024 08:28:08 -0600 Subject: [PATCH 12/19] disable maintainer modification --- src/plugins/ForwardMerger/forward_merger.ts | 2 +- test/forward_merger.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/plugins/ForwardMerger/forward_merger.ts b/src/plugins/ForwardMerger/forward_merger.ts index 1dde0d8..8a4f32d 100644 --- a/src/plugins/ForwardMerger/forward_merger.ts +++ b/src/plugins/ForwardMerger/forward_merger.ts @@ -56,7 +56,7 @@ export class ForwardMerger extends OpsBotPlugin { title: "Forward-merge " + this.currentBranch + " into " + nextBranch, head: this.currentBranch, base: nextBranch, - maintainer_can_modify: true, + maintainer_can_modify: false, body: `Forward-merge triggered by push to ${this.currentBranch} that creates a PR to keep ${nextBranch} up-to-date. If this PR is unable to be immediately merged due to conflicts, it will remain open for the team to manually merge. See [forward-merger docs](https://docs.rapids.ai/maintainers/forward-merger/) for more info.`, }); diff --git a/test/forward_merger.test.ts b/test/forward_merger.test.ts index b8cb43a..708c949 100644 --- a/test/forward_merger.test.ts +++ b/test/forward_merger.test.ts @@ -132,7 +132,7 @@ describe("Forward Merger", () => { "Forward-merge " + forwardMerger.currentBranch + " into " + nextBranch, head: forwardMerger.currentBranch, base: nextBranch, - maintainer_can_modify: true, + maintainer_can_modify: false, body: `Forward-merge triggered by push to ${forwardMerger.currentBranch} that creates a PR to keep ${nextBranch} up-to-date. If this PR is unable to be immediately merged due to conflicts, it will remain open for the team to manually merge. See [forward-merger docs](https://docs.rapids.ai/maintainers/forward-merger/) for more info.`, }); }, 11000); From ef080fcb139de5e1188e710283277c5235bbd8a1 Mon Sep 17 00:00:00 2001 From: Jake Awe Date: Mon, 29 Jan 2024 10:45:53 -0600 Subject: [PATCH 13/19] fix main class --- src/plugins/ForwardMerger/forward_merger.ts | 15 +++++++-------- src/plugins/ForwardMerger/index.ts | 4 ---- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/src/plugins/ForwardMerger/forward_merger.ts b/src/plugins/ForwardMerger/forward_merger.ts index 8a4f32d..fe39545 100644 --- a/src/plugins/ForwardMerger/forward_merger.ts +++ b/src/plugins/ForwardMerger/forward_merger.ts @@ -19,7 +19,6 @@ import { PayloadRepository } from "../../types"; import { isVersionedBranch, getVersionFromBranch } from "../../shared"; import { basename } from "path"; import { Context } from "probot"; -import { PushEventsType } from "."; export class ForwardMerger extends OpsBotPlugin { context: Context; @@ -28,7 +27,7 @@ export class ForwardMerger extends OpsBotPlugin { constructor( context: Context, - private payload: Context["payload"] + private payload: Context<"push">["payload"] ) { super("forward_merger", context); this.context = context; @@ -60,8 +59,6 @@ export class ForwardMerger extends OpsBotPlugin { body: `Forward-merge triggered by push to ${this.currentBranch} that creates a PR to keep ${nextBranch} up-to-date. If this PR is unable to be immediately merged due to conflicts, it will remain open for the team to manually merge. See [forward-merger docs](https://docs.rapids.ai/maintainers/forward-merger/) for more info.`, }); - await new Promise((resolve) => setTimeout(resolve, 10000)); - try { this.logger.info("Merging PR"); await this.context.octokit.pulls.merge({ @@ -70,16 +67,18 @@ export class ForwardMerger extends OpsBotPlugin { pull_number: pr.number, sha: pr.head.sha, }); - await this.issueComment( - pr.number, - "**SUCCESS** - forward-merge complete." - ); } catch (error) { await this.issueComment( pr.number, "**FAILURE** - Unable to forward-merge due to an error, **manual** merge is necessary. Do not use the `Resolve conflicts` option in this PR, follow these instructions https://docs.rapids.ai/maintainers/forward-merger/ \n **IMPORTANT**: When merging this PR, do not use the [auto-merger](https://docs.rapids.ai/resources/auto-merger/) (i.e. the `/merge` comment). Instead, an admin must manually merge by changing the merging strategy to `Create a Merge Commit`. Otherwise, history will be lost and the branches become incompatible." ); + return; } + + await this.issueComment( + pr.number, + "**SUCCESS** - forward-merge complete." + ); } async getBranches(): Promise { diff --git a/src/plugins/ForwardMerger/index.ts b/src/plugins/ForwardMerger/index.ts index 8059f4e..947cc30 100644 --- a/src/plugins/ForwardMerger/index.ts +++ b/src/plugins/ForwardMerger/index.ts @@ -16,10 +16,6 @@ import { Probot } from "probot"; import { ForwardMerger } from "./forward_merger"; -import { EmitterWebhookEventName as WebhookEvents } from "@octokit/webhooks/dist-types/types"; - -export const pushEvents = ["push"] satisfies WebhookEvents[] -export type PushEventsType = (typeof pushEvents)[number] export const initForwardMerger = (app: Probot) => { app.on(["push"], async (context) => { From b3a2a5f35f44db88ce9e260612d76eeedbc11f68 Mon Sep 17 00:00:00 2001 From: Jake Awe Date: Mon, 29 Jan 2024 11:09:26 -0600 Subject: [PATCH 14/19] remove unneeded test cases --- test/forward_merger.test.ts | 150 ------------------------------------ 1 file changed, 150 deletions(-) diff --git a/test/forward_merger.test.ts b/test/forward_merger.test.ts index 708c949..5a2391c 100644 --- a/test/forward_merger.test.ts +++ b/test/forward_merger.test.ts @@ -60,82 +60,6 @@ describe("Forward Merger", () => { expect(mockGetBranches).not.toBeCalled(); }); - test("mergeForward should sort branches", async () => { - const context = makePushContext({ - ref: "refs/heads/branch-21.12", - }); - const forwardMerger = new ForwardMerger(context, context.payload); - const branches = [ - { - name: "branch-21.12", - }, - { - name: "branch-21.10", - }, - ]; - // mock getBranches, sortBranches, and getNextBranch - const mockGetBranches = jest - .fn() - .mockName("getBranches") - .mockResolvedValue(branches); - forwardMerger.getBranches = mockGetBranches; - const sortedBranches = []; - const mockSortBranches = jest - .fn() - .mockName("sortBranches") - .mockReturnValue(sortedBranches); - forwardMerger.sortBranches = mockSortBranches; - const mockGetNextBranch = jest - .fn() - .mockName("getNextBranch") - .mockReturnValue(null); - forwardMerger.getNextBranch = mockGetNextBranch; - - await forwardMerger.mergeForward(); - - expect(mockGetBranches).toBeCalled(); - expect(mockSortBranches).toBeCalledWith(branches); - expect(mockGetNextBranch).toBeCalledWith(sortedBranches); - }); - - test("mergeForward should open PR on valid next branch", async () => { - const context = makePushContext({ - ref: "refs/heads/branch-21.12", - }); - const forwardMerger = new ForwardMerger(context, context.payload); - forwardMerger.getBranches = jest - .fn() - .mockName("getBranches") - .mockResolvedValue(null); - forwardMerger.sortBranches = jest - .fn() - .mockName("sortBranches") - .mockReturnValue(null); - const nextBranch = "branch-21.10"; - const mockGetNextBranch = jest - .fn() - .mockName("getNextBranch") - .mockReturnValue(nextBranch); - forwardMerger.getNextBranch = mockGetNextBranch; - const mockCreatePR = jest - .fn() - .mockName("openPR") - .mockResolvedValue({ data: {} }); - forwardMerger.context.octokit.pulls.create = mockCreatePR as any; - - await forwardMerger.mergeForward(); - - expect(mockCreatePR.mock.calls[0][0]).toMatchObject({ - owner: context.payload.repository.owner.login, - repo: context.payload.repository.name, - title: - "Forward-merge " + forwardMerger.currentBranch + " into " + nextBranch, - head: forwardMerger.currentBranch, - base: nextBranch, - maintainer_can_modify: false, - body: `Forward-merge triggered by push to ${forwardMerger.currentBranch} that creates a PR to keep ${nextBranch} up-to-date. If this PR is unable to be immediately merged due to conflicts, it will remain open for the team to manually merge. See [forward-merger docs](https://docs.rapids.ai/maintainers/forward-merger/) for more info.`, - }); - }, 11000); test("mergeForward should not open PR on invalid next branch", async () => { const context = makePushContext({ @@ -164,80 +88,6 @@ describe("Forward Merger", () => { expect(mockCreatePR).not.toBeCalled(); }); - test("mergeForward should merge PR after opening PR", async () => { - const context = makePushContext({ - ref: "refs/heads/branch-21.12", - }); - const forwardMerger = new ForwardMerger(context, context.payload); - forwardMerger.getBranches = jest - .fn() - .mockName("getBranches") - .mockResolvedValue(null); - forwardMerger.sortBranches = jest - .fn() - .mockName("sortBranches") - .mockReturnValue(null); - const nextBranch = { - name: "branch-21.10", - }; - forwardMerger.getNextBranch = jest - .fn() - .mockName("getNextBranch") - .mockReturnValue(nextBranch); - const pr = { data: { number: 1, head: { sha: 123456 } } }; - forwardMerger.context.octokit.pulls.create = ( - jest.fn().mockName("openPR").mockResolvedValue(pr) - ); - const mockMergePR = jest - .fn() - .mockName("mergePR") - .mockResolvedValue({ merged: true }); - forwardMerger.context.octokit.pulls.merge = mockMergePR; - forwardMerger.issueComment = jest - .fn() - .mockName("issueComment") - .mockResolvedValue(null); - - await forwardMerger.mergeForward(); - - expect(mockMergePR.mock.calls[0][0]).toMatchObject({ - owner: context.payload.repository.owner.login, - repo: context.payload.repository.name, - pull_number: pr.data.number, - sha: pr.data.head.sha, - }); - }, 11000); - - test("should not merge PR if there is no PR", async () => { - const context = makePushContext({ - ref: "refs/heads/branch-21.12", - }); - const forwardMerger = new ForwardMerger(context, context.payload); - forwardMerger.getBranches = jest - .fn() - .mockName("getBranches") - .mockResolvedValue(null); - forwardMerger.sortBranches = jest - .fn() - .mockName("sortBranches") - .mockReturnValue(null); - const nextBranch = { - name: "branch-21.10", - }; - forwardMerger.getNextBranch = jest - .fn() - .mockName("getNextBranch") - .mockReturnValue(nextBranch); - forwardMerger.context.octokit.pulls.create = ( - jest.fn().mockName("openPR").mockResolvedValue({ data: {} }) - ); - const mockMergePR = jest.fn().mockName("mergePR").mockResolvedValue(null); - forwardMerger.context.octokit.pulls.merge = mockMergePR; - - await forwardMerger.mergeForward(); - - expect(mockMergePR).not.toBeCalled(); - }, 11000); test("should comment failure on PR if merge is successful", async () => { const context = makePushContext({ From 8c86e719015c7fcf505f25b8881c35e3b9ade10f Mon Sep 17 00:00:00 2001 From: Jake Awe Date: Mon, 29 Jan 2024 11:23:47 -0600 Subject: [PATCH 15/19] clean up test cases --- test/forward_merger.test.ts | 43 +++++++++++-------------------------- 1 file changed, 13 insertions(+), 30 deletions(-) diff --git a/test/forward_merger.test.ts b/test/forward_merger.test.ts index 5a2391c..9cf2df9 100644 --- a/test/forward_merger.test.ts +++ b/test/forward_merger.test.ts @@ -128,7 +128,7 @@ describe("Forward Merger", () => { pr.data.number, "**SUCCESS** - forward-merge complete." ); - }, 11000); + }); test("should comment failure on PR if merge is unsuccessful", async () => { const context = makePushContext({ @@ -172,7 +172,7 @@ describe("Forward Merger", () => { pr.data.number, "**FAILURE** - Unable to forward-merge due to an error, **manual** merge is necessary. Do not use the `Resolve conflicts` option in this PR, follow these instructions https://docs.rapids.ai/maintainers/forward-merger/ \n **IMPORTANT**: When merging this PR, do not use the [auto-merger](https://docs.rapids.ai/resources/auto-merger/) (i.e. the `/merge` comment). Instead, an admin must manually merge by changing the merging strategy to `Create a Merge Commit`. Otherwise, history will be lost and the branches become incompatible." ); - }, 11000); + }); test("mergeForward should obtain the correct next branch from a given list of unsorted branches", async () => { const context = makePushContext({ @@ -198,7 +198,7 @@ describe("Forward Merger", () => { await forwardMerger.mergeForward(); expect(mockCreatePR.mock.calls[0][0].base).toBe("branch-22.04"); - }, 11000); + }); test("getBranches should return versioned branches", async () => { const context = makePushContext({ @@ -239,46 +239,29 @@ describe("Forward Merger", () => { }); }); - test("sortBranches should sort branches by version", async () => { + test("sortBranches should sort branches", async () => { const context = makePushContext({ ref: "refs/heads/branch-21.12", }); const forwardMerger = new ForwardMerger(context, context.payload); const branches = [ + "branch-0.10", "branch-21.12", "branch-21.10", "branch-19.10", "branch-22.02", + "branch-0.9", ]; const result = forwardMerger.sortBranches(branches); - expect(result.length).toEqual(4); - expect(result[0]).toEqual("branch-19.10"); - expect(result[1]).toEqual("branch-21.10"); - expect(result[2]).toEqual("branch-21.12"); - expect(result[3]).toEqual("branch-22.02"); - }); - - test("sortBranches should sort 0.9/0.10-type branches", async () => { - const context = makePushContext({ - ref: "refs/heads/branch-21.12", - }); - const forwardMerger = new ForwardMerger(context, context.payload); - const branches = [ - "branch-0.12", - "branch-0.10", - "branch-2.10", - "branch-1.02", - ]; - - const result = forwardMerger.sortBranches(branches); - - expect(result.length).toEqual(4); - expect(result[0]).toEqual("branch-0.10"); - expect(result[1]).toEqual("branch-0.12"); - expect(result[2]).toEqual("branch-1.02"); - expect(result[3]).toEqual("branch-2.10"); + expect(result.length).toEqual(6); + expect(result[0]).toEqual("branch-0.9"); + expect(result[1]).toEqual("branch-0.10"); + expect(result[2]).toEqual("branch-19.10"); + expect(result[3]).toEqual("branch-21.10"); + expect(result[4]).toEqual("branch-21.12"); + expect(result[5]).toEqual("branch-22.02"); }); test("getNextBranch should return next branch", async () => { From 4dd42c18b82e833ccf5a682f5556482e0cf119b7 Mon Sep 17 00:00:00 2001 From: Jake Awe Date: Mon, 29 Jan 2024 12:45:36 -0600 Subject: [PATCH 16/19] update mocking technique for consistency --- test/fixtures/contexts/base.ts | 4 +++ test/forward_merger.test.ts | 51 +++++++++++++--------------------- test/mocks.ts | 2 ++ 3 files changed, 26 insertions(+), 31 deletions(-) diff --git a/test/fixtures/contexts/base.ts b/test/fixtures/contexts/base.ts index 9be95f3..172aedf 100644 --- a/test/fixtures/contexts/base.ts +++ b/test/fixtures/contexts/base.ts @@ -35,6 +35,8 @@ import { mockPaginate, mockPullsGet, mockUpdateRelease, + mockCreatePR, + mockListBranches, } from "../../mocks"; import type { EmitterWebhookEventName } from "@octokit/webhooks/dist-types/types"; @@ -57,6 +59,7 @@ export const makeContext = (payload, name: EmitterWebhookEventName) => { list: mockListPulls, listReviews: mockListReviews, merge: mockMerge, + create: mockCreatePR, }, repos: { createCommitStatus: mockCreateCommitStatus, @@ -66,6 +69,7 @@ export const makeContext = (payload, name: EmitterWebhookEventName) => { getReleaseByTag: mockGetReleaseByTag, updateRelease: mockUpdateRelease, compareCommitsWithBasehead: mockCompareCommitsWithBasehead, + listBranches: mockListBranches, }, users: { getByUsername: mockGetByUsername, diff --git a/test/forward_merger.test.ts b/test/forward_merger.test.ts index 9cf2df9..33e02e6 100644 --- a/test/forward_merger.test.ts +++ b/test/forward_merger.test.ts @@ -16,12 +16,20 @@ import { ForwardMerger } from "../src/plugins/ForwardMerger/forward_merger"; import { makePushContext } from "./fixtures/contexts/push"; -import { mockConfigGet, mockContextRepo } from "./mocks"; +import { mockConfigGet, mockContextRepo, mockCreatePR, mockListBranches, mockMerge, mockPaginate } from "./mocks"; import { default as repoResp } from "./fixtures/responses/context_repo.json"; import { makeConfigReponse } from "./fixtures/responses/get_config"; describe("Forward Merger", () => { + beforeEach(() => { + mockCreatePR.mockReset(); + mockMerge.mockReset(); + mockListBranches.mockReset(); + mockPaginate.mockReset(); + }); + beforeAll(() => { + mockContextRepo.mockReset(); mockContextRepo.mockReturnValue(repoResp); mockConfigGet.mockResolvedValue( makeConfigReponse({ @@ -80,8 +88,7 @@ describe("Forward Merger", () => { .mockName("getNextBranch") .mockReturnValue(nextBranch); forwardMerger.getNextBranch = mockGetNextBranch; - const mockCreatePR = jest.fn().mockName("openPR").mockResolvedValue(null); - forwardMerger.context.octokit.pulls.create = mockCreatePR as any; + mockCreatePR.mockResolvedValue(null); await forwardMerger.mergeForward(); @@ -110,9 +117,8 @@ describe("Forward Merger", () => { .mockName("getNextBranch") .mockResolvedValue(nextBranch); const pr = { data: { number: 1, head: { sha: 123456 } } }; - forwardMerger.context.octokit.pulls.create = ( - jest.fn().mockName("openPR").mockResolvedValue(pr) - ); + mockCreatePR.mockResolvedValue(pr) + forwardMerger.context.octokit.pulls.merge = ( jest.fn().mockName("mergePR").mockResolvedValue({ merged: true }) ); @@ -151,15 +157,9 @@ describe("Forward Merger", () => { .mockName("getNextBranch") .mockReturnValue(nextBranch); const pr = { data: { number: 1, head: { sha: 123456 } } }; - forwardMerger.context.octokit.pulls.create = ( - jest.fn().mockName("openPR").mockResolvedValue(pr) - ); - const mockMergePR = jest - .fn() - .mockName("mergePR") - .mockResolvedValue({ merged: false }); - forwardMerger.context.octokit.pulls.merge = mockMergePR; - mockMergePR.mockRejectedValueOnce(new Error("error")); + mockCreatePR.mockResolvedValue(pr) + mockMerge.mockResolvedValue({ merged: false }); + mockMerge.mockRejectedValueOnce(new Error("error")); const mockIssueComment = jest .fn() .mockName("issueComment") @@ -189,12 +189,8 @@ describe("Forward Merger", () => { .fn() .mockName("getBranches") .mockResolvedValue(branches); - const mockCreatePR = jest - .fn() - .mockName("openPR") - .mockResolvedValue({ data: {} }); + mockCreatePR.mockResolvedValue({ data: {} }); forwardMerger.getBranches = mockGetBranches; - forwardMerger.context.octokit.pulls.create = mockCreatePR as any; await forwardMerger.mergeForward(); expect(mockCreatePR.mock.calls[0][0].base).toBe("branch-22.04"); @@ -216,15 +212,8 @@ describe("Forward Merger", () => { name: "branch-21.10", }, ]; - const mockListBranchesPaginate = jest - .fn() - .mockName("listBranches") - .mockResolvedValue(branches); - forwardMerger.context.octokit.paginate = mockListBranchesPaginate as any; - const mockListBranches = jest - .fn() - .mockName("listBranches") - .mockResolvedValue("something"); + mockPaginate.mockResolvedValue(branches); + mockListBranches.mockResolvedValue("something"); forwardMerger.context.octokit.repos.listBranches = mockListBranches as any; const result = await forwardMerger.getBranches(); @@ -232,8 +221,8 @@ describe("Forward Merger", () => { expect(result.length).toEqual(2); expect(result[0]).toEqual("branch-21.12"); expect(result[1]).toEqual("branch-21.10"); - expect(mockListBranchesPaginate.mock.calls[0][0]).toBe(mockListBranches); - expect(mockListBranchesPaginate.mock.calls[0][1]).toMatchObject({ + expect(mockPaginate.mock.calls[0][0]).toBe(mockListBranches); + expect(mockPaginate.mock.calls[0][1]).toMatchObject({ owner: context.payload.repository.owner.login, repo: context.payload.repository.name, }); diff --git a/test/mocks.ts b/test/mocks.ts index 7929541..2ff525b 100644 --- a/test/mocks.ts +++ b/test/mocks.ts @@ -49,3 +49,5 @@ export const mockSearchIssuesAndPullRequests = jest .fn() .mockName("mockSearchIssuesAndPullRequests"); export const mockUpdateRelease = jest.fn().mockName("mockUpdateRelease"); +export const mockCreatePR = jest.fn().mockName("mockCreatePR"); +export const mockListBranches = jest.fn().mockName("mockListBranches"); From b12d05e0b8b30b36cd609ec2e7f8e912b7223c65 Mon Sep 17 00:00:00 2001 From: Jake Awe Date: Mon, 29 Jan 2024 15:52:46 -0600 Subject: [PATCH 17/19] PR reviews --- test/forward_merger.test.ts | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/test/forward_merger.test.ts b/test/forward_merger.test.ts index 33e02e6..64bd6b6 100644 --- a/test/forward_merger.test.ts +++ b/test/forward_merger.test.ts @@ -16,7 +16,7 @@ import { ForwardMerger } from "../src/plugins/ForwardMerger/forward_merger"; import { makePushContext } from "./fixtures/contexts/push"; -import { mockConfigGet, mockContextRepo, mockCreatePR, mockListBranches, mockMerge, mockPaginate } from "./mocks"; +import { mockConfigGet, mockContextRepo, mockCreateComment, mockCreatePR, mockListBranches, mockMerge, mockPaginate } from "./mocks"; import { default as repoResp } from "./fixtures/responses/context_repo.json"; import { makeConfigReponse } from "./fixtures/responses/get_config"; @@ -26,6 +26,7 @@ describe("Forward Merger", () => { mockMerge.mockReset(); mockListBranches.mockReset(); mockPaginate.mockReset(); + mockCreateComment.mockReset(); }); beforeAll(() => { @@ -82,7 +83,7 @@ describe("Forward Merger", () => { .fn() .mockName("sortBranches") .mockReturnValue(null); - const nextBranch = null; + const nextBranch = undefined; const mockGetNextBranch = jest .fn() .mockName("getNextBranch") @@ -96,7 +97,7 @@ describe("Forward Merger", () => { }); - test("should comment failure on PR if merge is successful", async () => { + test("should comment success on PR if merge is successful", async () => { const context = makePushContext({ ref: "refs/heads/branch-21.12", }); @@ -119,9 +120,7 @@ describe("Forward Merger", () => { const pr = { data: { number: 1, head: { sha: 123456 } } }; mockCreatePR.mockResolvedValue(pr) - forwardMerger.context.octokit.pulls.merge = ( - jest.fn().mockName("mergePR").mockResolvedValue({ merged: true }) - ); + mockMerge.mockResolvedValue({ merged: true }) const mockIssueComment = jest .fn() .mockName("issueComment") @@ -158,7 +157,6 @@ describe("Forward Merger", () => { .mockReturnValue(nextBranch); const pr = { data: { number: 1, head: { sha: 123456 } } }; mockCreatePR.mockResolvedValue(pr) - mockMerge.mockResolvedValue({ merged: false }); mockMerge.mockRejectedValueOnce(new Error("error")); const mockIssueComment = jest .fn() @@ -214,7 +212,6 @@ describe("Forward Merger", () => { ]; mockPaginate.mockResolvedValue(branches); mockListBranches.mockResolvedValue("something"); - forwardMerger.context.octokit.repos.listBranches = mockListBranches as any; const result = await forwardMerger.getBranches(); @@ -265,7 +262,7 @@ describe("Forward Merger", () => { expect(result).toEqual("branch-22.02"); }); - test("getNextBranch should return null if there is no next branch", async () => { + test("getNextBranch should return undefined if there is no next branch", async () => { const context = makePushContext({ ref: "refs/heads/branch-22.02", }); @@ -274,7 +271,7 @@ describe("Forward Merger", () => { const sortedBranches = forwardMerger.sortBranches(branches); const result = await forwardMerger.getNextBranch(sortedBranches); - expect(result).toBeFalsy(); + expect(result).toBeUndefined(); }); test("issueComment should create comment", async () => { @@ -282,12 +279,7 @@ describe("Forward Merger", () => { ref: "refs/heads/branch-22.02", }); const forwardMerger = new ForwardMerger(context, context.payload); - const mockCreateComment = jest - .fn() - .mockName("createComment") - .mockResolvedValue(null); - forwardMerger.context.octokit.issues.createComment = - mockCreateComment as any; + mockCreateComment.mockResolvedValue(null); await forwardMerger.issueComment(1, "comment"); expect(mockCreateComment.mock.calls[0][0]).toMatchObject({ From 7787c9b1ceaaf3a997df30c59a8f49c709c30262 Mon Sep 17 00:00:00 2001 From: Jake Awe Date: Mon, 29 Jan 2024 22:33:22 -0600 Subject: [PATCH 18/19] small fix --- test/forward_merger.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/forward_merger.test.ts b/test/forward_merger.test.ts index 64bd6b6..23ac5fe 100644 --- a/test/forward_merger.test.ts +++ b/test/forward_merger.test.ts @@ -89,7 +89,6 @@ describe("Forward Merger", () => { .mockName("getNextBranch") .mockReturnValue(nextBranch); forwardMerger.getNextBranch = mockGetNextBranch; - mockCreatePR.mockResolvedValue(null); await forwardMerger.mergeForward(); @@ -120,7 +119,7 @@ describe("Forward Merger", () => { const pr = { data: { number: 1, head: { sha: 123456 } } }; mockCreatePR.mockResolvedValue(pr) - mockMerge.mockResolvedValue({ merged: true }) + mockMerge.mockResolvedValue(true) const mockIssueComment = jest .fn() .mockName("issueComment") From 45357421b241a164754e1f2c7dd53af4a1ccefe0 Mon Sep 17 00:00:00 2001 From: Jake Awe Date: Mon, 29 Jan 2024 22:34:47 -0600 Subject: [PATCH 19/19] format file --- test/forward_merger.test.ts | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/test/forward_merger.test.ts b/test/forward_merger.test.ts index 23ac5fe..3d37cb3 100644 --- a/test/forward_merger.test.ts +++ b/test/forward_merger.test.ts @@ -16,7 +16,15 @@ import { ForwardMerger } from "../src/plugins/ForwardMerger/forward_merger"; import { makePushContext } from "./fixtures/contexts/push"; -import { mockConfigGet, mockContextRepo, mockCreateComment, mockCreatePR, mockListBranches, mockMerge, mockPaginate } from "./mocks"; +import { + mockConfigGet, + mockContextRepo, + mockCreateComment, + mockCreatePR, + mockListBranches, + mockMerge, + mockPaginate, +} from "./mocks"; import { default as repoResp } from "./fixtures/responses/context_repo.json"; import { makeConfigReponse } from "./fixtures/responses/get_config"; @@ -69,7 +77,6 @@ describe("Forward Merger", () => { expect(mockGetBranches).not.toBeCalled(); }); - test("mergeForward should not open PR on invalid next branch", async () => { const context = makePushContext({ ref: "refs/heads/branch-22.02", @@ -95,7 +102,6 @@ describe("Forward Merger", () => { expect(mockCreatePR).not.toBeCalled(); }); - test("should comment success on PR if merge is successful", async () => { const context = makePushContext({ ref: "refs/heads/branch-21.12", @@ -117,9 +123,9 @@ describe("Forward Merger", () => { .mockName("getNextBranch") .mockResolvedValue(nextBranch); const pr = { data: { number: 1, head: { sha: 123456 } } }; - mockCreatePR.mockResolvedValue(pr) - - mockMerge.mockResolvedValue(true) + mockCreatePR.mockResolvedValue(pr); + + mockMerge.mockResolvedValue(true); const mockIssueComment = jest .fn() .mockName("issueComment") @@ -155,7 +161,7 @@ describe("Forward Merger", () => { .mockName("getNextBranch") .mockReturnValue(nextBranch); const pr = { data: { number: 1, head: { sha: 123456 } } }; - mockCreatePR.mockResolvedValue(pr) + mockCreatePR.mockResolvedValue(pr); mockMerge.mockRejectedValueOnce(new Error("error")); const mockIssueComment = jest .fn()