From 2f8f8e3270012fc4710f8a7148080f1b5fb4ed6d Mon Sep 17 00:00:00 2001 From: Nico Korthout Date: Tue, 8 Aug 2023 18:54:18 +0200 Subject: [PATCH 1/5] refactor: turn git into a class This will make it easier to inject execa into git and to inject git into backport. --- src/backport.ts | 20 +++++--- src/git.ts | 126 +++++++++++++++++++++++++----------------------- 2 files changed, 80 insertions(+), 66 deletions(-) diff --git a/src/backport.ts b/src/backport.ts index 5870dde..b228f68 100644 --- a/src/backport.ts +++ b/src/backport.ts @@ -3,7 +3,7 @@ import dedent from "dedent"; import { CreatePullRequestResponse, PullRequest } from "./github"; import { GithubApi } from "./github"; -import * as git from "./git"; +import { Git, GitRefNotFoundError } from "./git"; import * as utils from "./utils"; type PRContent = { @@ -32,10 +32,12 @@ enum Output { export class Backport { private github; private config; + private git; constructor(github: GithubApi, config: Config) { this.github = github; this.config = config; + this.git = new Git(); } async run(): Promise { @@ -70,7 +72,7 @@ export class Backport { console.log( `Fetching all the commits from the pull request: ${mainpr.commits + 1}`, ); - await git.fetch( + await this.git.fetch( `refs/pull/${pull_number}/head`, this.config.pwd, mainpr.commits + 1, // +1 in case this concerns a shallowly cloned repo @@ -104,9 +106,9 @@ export class Backport { console.log(`Backporting to target branch '${target}...'`); try { - await git.fetch(target, this.config.pwd, 1); + await this.git.fetch(target, this.config.pwd, 1); } catch (error) { - if (error instanceof git.GitRefNotFoundError) { + if (error instanceof GitRefNotFoundError) { const message = this.composeMessageForFetchTargetFailure(error.ref); console.error(message); successByTarget.set(target, false); @@ -127,7 +129,11 @@ export class Backport { console.log(`Start backport to ${branchname}`); try { - await git.checkout(branchname, `origin/${target}`, this.config.pwd); + await this.git.checkout( + branchname, + `origin/${target}`, + this.config.pwd, + ); } catch (error) { const message = this.composeMessageForBackportScriptFailure( target, @@ -148,7 +154,7 @@ export class Backport { } try { - await git.cherryPick(commitShas, this.config.pwd); + await this.git.cherryPick(commitShas, this.config.pwd); } catch (error) { const message = this.composeMessageForBackportScriptFailure( target, @@ -169,7 +175,7 @@ export class Backport { } console.info(`Push branch to origin`); - const pushExitCode = await git.push(branchname, this.config.pwd); + const pushExitCode = await this.git.push(branchname, this.config.pwd); if (pushExitCode != 0) { const message = this.composeMessageForGitPushFailure( target, diff --git a/src/git.ts b/src/git.ts index 2cdd83b..2c07218 100644 --- a/src/git.ts +++ b/src/git.ts @@ -8,71 +8,79 @@ export class GitRefNotFoundError extends Error { } } -/** - * Fetches a ref from origin - * - * @param ref the sha, branchname, etc to fetch - * @param pwd the root of the git repository - * @param depth the number of commits to fetch - * @throws GitRefNotFoundError when ref not found - * @throws Error for any other non-zero exit code - */ -export async function fetch(ref: string, pwd: string, depth: number) { - const { exitCode } = await git( - "fetch", - [`--depth=${depth}`, "origin", ref], - pwd, - ); - if (exitCode === 128) { - throw new GitRefNotFoundError( - `Expected to fetch '${ref}', but couldn't find it`, - ref, - ); - } else if (exitCode !== 0) { - throw new Error( - `'git fetch origin ${ref}' failed with exit code ${exitCode}`, - ); - } -} +export class Git { + constructor() {} -export async function push(branchname: string, pwd: string) { - const { exitCode } = await git( - "push", - ["--set-upstream", "origin", branchname], - pwd, - ); - return exitCode; -} + private async git(command: string, args: string[], pwd: string) { + console.log(`git ${command} ${args.join(" ")}`); + const child = execa("git", [command, ...args], { + cwd: pwd, + env: { + GIT_COMMITTER_NAME: "github-actions[bot]", + GIT_COMMITTER_EMAIL: "github-actions[bot]@users.noreply.github.com", + }, + reject: false, + }); + child.stderr?.pipe(process.stderr); + return child; + } -async function git(command: string, args: string[], pwd: string) { - console.log(`git ${command} ${args.join(" ")}`); - const child = execa("git", [command, ...args], { - cwd: pwd, - env: { - GIT_COMMITTER_NAME: "github-actions[bot]", - GIT_COMMITTER_EMAIL: "github-actions[bot]@users.noreply.github.com", - }, - reject: false, - }); - child.stderr?.pipe(process.stderr); - return child; -} + /** + * Fetches a ref from origin + * + * @param ref the sha, branchname, etc to fetch + * @param pwd the root of the git repository + * @param depth the number of commits to fetch + * @throws GitRefNotFoundError when ref not found + * @throws Error for any other non-zero exit code + */ + public async fetch(ref: string, pwd: string, depth: number) { + const { exitCode } = await this.git( + "fetch", + [`--depth=${depth}`, "origin", ref], + pwd, + ); + if (exitCode === 128) { + throw new GitRefNotFoundError( + `Expected to fetch '${ref}', but couldn't find it`, + ref, + ); + } else if (exitCode !== 0) { + throw new Error( + `'git fetch origin ${ref}' failed with exit code ${exitCode}`, + ); + } + } -export async function checkout(branch: string, start: string, pwd: string) { - const { exitCode } = await git("switch", ["-c", branch, start], pwd); - if (exitCode !== 0) { - throw new Error( - `'git switch -c ${branch} ${start}' failed with exit code ${exitCode}`, + public async push(branchname: string, pwd: string) { + const { exitCode } = await this.git( + "push", + ["--set-upstream", "origin", branchname], + pwd, ); + return exitCode; + } + + public async checkout(branch: string, start: string, pwd: string) { + const { exitCode } = await this.git("switch", ["-c", branch, start], pwd); + if (exitCode !== 0) { + throw new Error( + `'git switch -c ${branch} ${start}' failed with exit code ${exitCode}`, + ); + } } -} -export async function cherryPick(commitShas: string[], pwd: string) { - const { exitCode } = await git("cherry-pick", ["-x", ...commitShas], pwd); - if (exitCode !== 0) { - await git("cherry-pick", ["--abort"], pwd); - throw new Error( - `'git cherry-pick -x ${commitShas}' failed with exit code ${exitCode}`, + public async cherryPick(commitShas: string[], pwd: string) { + const { exitCode } = await this.git( + "cherry-pick", + ["-x", ...commitShas], + pwd, ); + if (exitCode !== 0) { + await this.git("cherry-pick", ["--abort"], pwd); + throw new Error( + `'git cherry-pick -x ${commitShas}' failed with exit code ${exitCode}`, + ); + } } } From 1f2038bc8fcebc15ac683bd8abda953f64d78ad5 Mon Sep 17 00:00:00 2001 From: Nico Korthout Date: Tue, 8 Aug 2023 19:06:58 +0200 Subject: [PATCH 2/5] refactor: inject execa and git Inversion of control makes it easier to test backport.ts and git.ts. I had to add a bit of a strange type for execa, using the import() function. This is because execa does not provide the types in an easily importable way. If you know of a better way to do this, please let me know. The idea of this type is to take the type of the import of execa, which is an object that contains all the functions of the execa package. ``` type Execa = (typeof import("execa"))... ``` Then, we take the `execa` function from this object using an indexed access type: ``` type Execa = ...["execa"] ``` --- src/backport.ts | 4 ++-- src/git.ts | 6 +++--- src/main.ts | 8 ++++++-- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/backport.ts b/src/backport.ts index b228f68..3b5375f 100644 --- a/src/backport.ts +++ b/src/backport.ts @@ -34,10 +34,10 @@ export class Backport { private config; private git; - constructor(github: GithubApi, config: Config) { + constructor(github: GithubApi, config: Config, git: Git) { this.github = github; this.config = config; - this.git = new Git(); + this.git = git; } async run(): Promise { diff --git a/src/git.ts b/src/git.ts index 2c07218..e59a3f4 100644 --- a/src/git.ts +++ b/src/git.ts @@ -1,4 +1,4 @@ -import { execa } from "execa"; +export type Execa = (typeof import("execa"))["execa"]; export class GitRefNotFoundError extends Error { ref: string; @@ -9,11 +9,11 @@ export class GitRefNotFoundError extends Error { } export class Git { - constructor() {} + constructor(private execa: Execa) {} private async git(command: string, args: string[], pwd: string) { console.log(`git ${command} ${args.join(" ")}`); - const child = execa("git", [command, ...args], { + const child = this.execa("git", [command, ...args], { cwd: pwd, env: { GIT_COMMITTER_NAME: "github-actions[bot]", diff --git a/src/main.ts b/src/main.ts index f995c70..d69ee5e 100644 --- a/src/main.ts +++ b/src/main.ts @@ -1,6 +1,8 @@ import * as core from "@actions/core"; import { Backport } from "./backport"; import { Github } from "./github"; +import { Git } from "./git"; +import { execa } from "execa"; /** * Called from the action.yml. @@ -17,7 +19,8 @@ async function run(): Promise { const target_branches = core.getInput("target_branches"); const github = new Github(token); - const backport = new Backport(github, { + const git = new Git(execa); + const config = { pwd, labels: { pattern: pattern === "" ? undefined : new RegExp(pattern) }, pull: { description, title }, @@ -25,7 +28,8 @@ async function run(): Promise { copy_labels_pattern === "" ? undefined : new RegExp(copy_labels_pattern), target_branches: target_branches === "" ? undefined : (target_branches as string), - }); + }; + const backport = new Backport(github, config, git); return backport.run(); } From b0868a08ad6431bb0acdd43cc4912c883299a388 Mon Sep 17 00:00:00 2001 From: Nico Korthout Date: Tue, 8 Aug 2023 19:08:11 +0200 Subject: [PATCH 3/5] test: add git.test.ts Adds some tests to make sure git works as I expect --- src/test/git.test.ts | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 src/test/git.test.ts diff --git a/src/test/git.test.ts b/src/test/git.test.ts new file mode 100644 index 0000000..23359b3 --- /dev/null +++ b/src/test/git.test.ts @@ -0,0 +1,35 @@ +import { Git, GitRefNotFoundError } from "../git"; +import { execa } from "execa"; + +const git = new Git(execa); +let response = { exitCode: 0, stdout: "" }; + +jest.mock("execa", () => ({ + execa: jest.fn(() => response), +})); + +describe("git.fetch", () => { + describe("throws GitRefNotFoundError", () => { + it("when fetching an unknown ref, i.e. exit code 128", async () => { + response.exitCode = 128; + expect.assertions(3); + await git.fetch("unknown", "", 1).catch((error) => { + expect(error).toBeInstanceOf(GitRefNotFoundError); + expect(error).toHaveProperty( + "message", + "Expected to fetch 'unknown', but couldn't find it", + ); + expect(error).toHaveProperty("ref", "unknown"); + }); + }); + }); + + describe("throws Error", () => { + it("when failing with an unexpected non-zero exit code", async () => { + response.exitCode = 1; + await expect(git.fetch("unknown", "", 1)).rejects.toThrowError( + `'git fetch origin unknown' failed with exit code 1`, + ); + }); + }); +}); From 992d7ca0e3796e4c6977947d7a6a5203409b7659 Mon Sep 17 00:00:00 2001 From: Nico Korthout Date: Sat, 12 Aug 2023 09:47:28 +0200 Subject: [PATCH 4/5] test: cherryPick can throw error Adds another simple test case that shows that cherryPick can throw an error when it exits with a non-zero exit code. --- src/test/git.test.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/test/git.test.ts b/src/test/git.test.ts index 23359b3..e3e9ff5 100644 --- a/src/test/git.test.ts +++ b/src/test/git.test.ts @@ -33,3 +33,14 @@ describe("git.fetch", () => { }); }); }); + +describe("git.cherryPick", () => { + describe("throws Error", () => { + it("when failing with an unexpected non-zero exit code", async () => { + response.exitCode = 1; + await expect(git.cherryPick(["unknown"], "")).rejects.toThrowError( + `'git cherry-pick -x unknown' failed with exit code 1`, + ); + }); + }); +}); From 7a4690500a5369dfaaf01ad582693a8453aba149 Mon Sep 17 00:00:00 2001 From: Nico Korthout Date: Sat, 12 Aug 2023 13:46:24 +0200 Subject: [PATCH 5/5] test: remove unnecessary mock It's not longer necessary to mock away execa --- src/test/backport.test.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/test/backport.test.ts b/src/test/backport.test.ts index 06b9634..bf9317d 100644 --- a/src/test/backport.test.ts +++ b/src/test/backport.test.ts @@ -1,9 +1,5 @@ import { findTargetBranches } from "../backport"; -jest.mock("execa", () => ({ - execa: jest.fn(), -})); - const default_pattern = /^backport ([^ ]+)$/; describe("find target branches", () => {