From 1c3919985bd9694d9150b59aded4f3c2816638bb Mon Sep 17 00:00:00 2001 From: Yoriyasu Yano <430092+yorinasub17@users.noreply.github.com> Date: Thu, 19 Oct 2023 10:36:31 -0500 Subject: [PATCH 1/3] chore: Add regression tests for from_github.ts Signed-off-by: Yoriyasu Yano <430092+yorinasub17@users.noreply.github.com> --- .github/workflows/lint-test-release.yml | 2 + src/engine/from_github.test.ts | 200 ++++++++++++++++++++++++ 2 files changed, 202 insertions(+) create mode 100644 src/engine/from_github.test.ts diff --git a/.github/workflows/lint-test-release.yml b/.github/workflows/lint-test-release.yml index b2ddf48..dd79bb3 100644 --- a/.github/workflows/lint-test-release.yml +++ b/.github/workflows/lint-test-release.yml @@ -35,6 +35,8 @@ jobs: - name: test run: pnpm test + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - name: report uses: mikepenz/action-junit-report@75b84e78b3f0aaea7ed7cf8d1d100d7f97f963ec # v4.0.0 diff --git a/src/engine/from_github.test.ts b/src/engine/from_github.test.ts new file mode 100644 index 0000000..47352ba --- /dev/null +++ b/src/engine/from_github.test.ts @@ -0,0 +1,200 @@ +import { expect, test } from "@jest/globals"; +import { Octokit } from "@octokit/rest"; + +import { + IGitHubRepository, + patchFromGitHubPullRequest, +} from "./from_github.ts"; +import { IPatch, PatchOp, LineOp } from "./patch_types.ts"; + +const maybeToken = process.env.GITHUB_TOKEN; +let octokit: Octokit; +if (maybeToken) { + octokit = new Octokit({ auth: maybeToken }); +} else { + octokit = new Octokit(); +} +const testRepo: IGitHubRepository = { + owner: "fensak-test", + name: "test-fensak-rules-engine", +}; + +test("a single file change from GitHub is parsed correctly", async () => { + // View PR at + // https://github.com/fensak-test/test-fensak-rules-engine/pull/1 + const patches = await patchFromGitHubPullRequest(octokit, testRepo, 1); + expect(patches.metadata).toEqual({ + sourceBranch: "test/bump-one-json", + targetBranch: "main", + linkedPRs: [], + }); + expect(patches.patchList.length).toEqual(1); + + const patch = patches.patchList[0]; + expect(patch.path).toEqual("appversions.json"); + expect(patch.op).toEqual(PatchOp.Modified); + expect(patch.diff.length).toEqual(1); + + const hunk = patch.diff[0]; + expect(hunk.originalStart).toEqual(1); + expect(hunk.originalLength).toEqual(5); + expect(hunk.updatedStart).toEqual(1); + expect(hunk.updatedLength).toEqual(5); + expect(hunk.diffOperations).toEqual([ + { + op: LineOp.Untouched, + text: "{", + newText: "", + }, + { + op: LineOp.Untouched, + text: ` "coreapp": "v0.1.0",`, + newText: "", + }, + { + op: LineOp.Modified, + text: ` "subapp": "v1.1.0",`, + newText: ` "subapp": "v1.2.0",`, + }, + { + op: LineOp.Untouched, + text: ` "logapp": "v100.1.0"`, + newText: "", + }, + { + op: LineOp.Untouched, + text: "}", + newText: "", + }, + ]); +}); + +test("multiple file changes from GitHub is parsed correctly", async () => { + // View PR at + // https://github.com/fensak-test/test-fensak-rules-engine/pull/25 + const patches = await patchFromGitHubPullRequest(octokit, testRepo, 25); + expect(patches.metadata).toEqual({ + sourceBranch: "test/mult-files-with-bad", + targetBranch: "main", + linkedPRs: [], + }); + expect(patches.patchList.length).toEqual(3); + + let jsonPatch: IPatch | null = null; + let tfvarsPatch: IPatch | null = null; + let tomlPatch: IPatch | null = null; + for (const p of patches.patchList) { + switch (p.path) { + default: + throw new Error(`Unexpected patch ${p.path}`); + return; + + case "appversions.json": + jsonPatch = p; + break; + case "appversions.tfvars": + tfvarsPatch = p; + break; + case "appversions.toml": + tomlPatch = p; + break; + } + } + + expect(jsonPatch).not.toBeNull(); + expect(tfvarsPatch).not.toBeNull(); + expect(tomlPatch).not.toBeNull(); + + // The following is impossible, but makes the type checker happy. + if (jsonPatch === null || tfvarsPatch === null || tomlPatch === null) { + throw new Error("impossible condition"); + } + + // Check JSON patch + expect(jsonPatch.op).toEqual(PatchOp.Modified); + expect(jsonPatch.diff.length).toEqual(1); + const jsonHunk = jsonPatch.diff[0]; + expect(jsonHunk.originalStart).toEqual(1); + expect(jsonHunk.originalLength).toEqual(5); + expect(jsonHunk.updatedStart).toEqual(1); + expect(jsonHunk.updatedLength).toEqual(5); + expect(jsonHunk.diffOperations).toEqual([ + { + op: LineOp.Untouched, + text: "{", + newText: "", + }, + { + op: LineOp.Untouched, + text: ` "coreapp": "v0.1.0",`, + newText: "", + }, + { + op: LineOp.Modified, + text: ` "subapp": "v1.1.0",`, + newText: ` "subapp": "v1.2.0",`, + }, + { + op: LineOp.Untouched, + text: ` "logapp": "v100.1.0"`, + newText: "", + }, + { + op: LineOp.Untouched, + text: "}", + newText: "", + }, + ]); + + // Check tfvars patch + expect(tfvarsPatch.op).toEqual(PatchOp.Modified); + expect(tfvarsPatch.diff.length).toEqual(1); + const tfvarsHunk = tfvarsPatch.diff[0]; + expect(tfvarsHunk.originalStart).toEqual(1); + expect(tfvarsHunk.originalLength).toEqual(3); + expect(tfvarsHunk.updatedStart).toEqual(1); + expect(tfvarsHunk.updatedLength).toEqual(3); + expect(tfvarsHunk.diffOperations).toEqual([ + { + op: LineOp.Untouched, + text: `coreapp_version = "v0.1.0"`, + newText: "", + }, + { + op: LineOp.Modified, + text: `subapp_version = "v1.1.0"`, + newText: `subapp_version = "v1.2.0"`, + }, + { + op: LineOp.Untouched, + text: `logapp_version = "v100.1.0"`, + newText: "", + }, + ]); + + // Check toml patch + expect(tomlPatch.op).toEqual(PatchOp.Modified); + expect(tomlPatch.diff.length).toEqual(1); + const tomlHunk = tomlPatch.diff[0]; + expect(tomlHunk.originalStart).toEqual(1); + expect(tomlHunk.originalLength).toEqual(3); + expect(tomlHunk.updatedStart).toEqual(1); + expect(tomlHunk.updatedLength).toEqual(3); + expect(tomlHunk.diffOperations).toEqual([ + { + op: LineOp.Modified, + text: `coreapp = "v0.1.0"`, + newText: `coreapp = "v0.2.0"`, + }, + { + op: LineOp.Untouched, + text: `subapp = "v1.1.0"`, + newText: "", + }, + { + op: LineOp.Untouched, + text: `logapp = "v100.1.0"`, + newText: "", + }, + ]); +}); From 38901187c90fef76d0f0a38eeaf2fca17d5ed109 Mon Sep 17 00:00:00 2001 From: Yoriyasu Yano <430092+yorinasub17@users.noreply.github.com> Date: Thu, 19 Oct 2023 13:17:36 -0500 Subject: [PATCH 2/3] chore: Add regression tests for parsing linked PR info in description frontmatter Signed-off-by: Yoriyasu Yano <430092+yorinasub17@users.noreply.github.com> --- src/engine/from_github.test.ts | 110 +++++++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+) diff --git a/src/engine/from_github.test.ts b/src/engine/from_github.test.ts index 47352ba..8227088 100644 --- a/src/engine/from_github.test.ts +++ b/src/engine/from_github.test.ts @@ -198,3 +198,113 @@ test("multiple file changes from GitHub is parsed correctly", async () => { }, ]); }); + +test("extracts linked PRs in front matter", async () => { + // View PR at + // https://github.com/fensak-test/test-fensak-rules-engine/pull/39 + const patches = await patchFromGitHubPullRequest(octokit, testRepo, 39); + expect(patches.metadata).toEqual({ + sourceBranch: "test/one-linked", + targetBranch: "main", + linkedPRs: [ + { + repo: "", + prNum: 1, + isMerged: false, + isClosed: false, + }, + ], + }); +}); + +test("extracts closed linked PRs in front matter", async () => { + // View PR at + // https://github.com/fensak-test/test-fensak-rules-engine/pull/40 + const patches = await patchFromGitHubPullRequest(octokit, testRepo, 40); + expect(patches.metadata).toEqual({ + sourceBranch: "test/one-linked-closed", + targetBranch: "main", + linkedPRs: [ + { + repo: "", + prNum: 38, + isMerged: false, + isClosed: true, + }, + ], + }); +}); + +test("extracts merged linked PRs in front matter", async () => { + // View PR at + // https://github.com/fensak-test/test-fensak-rules-engine/pull/42 + const patches = await patchFromGitHubPullRequest(octokit, testRepo, 42); + expect(patches.metadata).toEqual({ + sourceBranch: "test/one-linked-merged", + targetBranch: "main", + linkedPRs: [ + { + repo: "", + prNum: 41, + isMerged: true, + isClosed: true, + }, + ], + }); +}); + +test("extracts external linked PRs in front matter", async () => { + // View PR at + // https://github.com/fensak-test/test-fensak-rules-engine/pull/44 + const patches = await patchFromGitHubPullRequest(octokit, testRepo, 44); + expect(patches.metadata).toEqual({ + sourceBranch: "test/external-linked", + targetBranch: "main", + linkedPRs: [ + { + repo: "terraform-null-testfensak", + prNum: 1, + isMerged: false, + isClosed: true, + }, + ], + }); +}); + +test("extracts multiple linked PRs in front matter", async () => { + // View PR at + // https://github.com/fensak-test/test-fensak-rules-engine/pull/43 + const patches = await patchFromGitHubPullRequest(octokit, testRepo, 43); + expect(patches.metadata).toEqual({ + sourceBranch: "test/multiple-linked", + targetBranch: "main", + linkedPRs: [ + { + repo: "", + prNum: 1, + isMerged: false, + isClosed: false, + }, + { + repo: "", + prNum: 38, + isMerged: false, + isClosed: true, + }, + { + repo: "", + prNum: 41, + isMerged: true, + isClosed: true, + }, + ], + }); +}); + +test("errors with bad front matter", async () => { + // View PR at + // https://github.com/fensak-test/test-fensak-rules-engine/pull/45 + expect( + async () => await patchFromGitHubPullRequest(octokit, testRepo, 45), + ).rejects.toThrow(TypeError); +}); From 63f50da5235af6cab5a639758ea97fda043f72c0 Mon Sep 17 00:00:00 2001 From: Yoriyasu Yano <430092+yorinasub17@users.noreply.github.com> Date: Thu, 19 Oct 2023 13:17:54 -0500 Subject: [PATCH 3/3] fix: Fix bug where the linked PR was not pulling in merge info correctly Signed-off-by: Yoriyasu Yano <430092+yorinasub17@users.noreply.github.com> --- src/engine/from_github.ts | 61 +++++++++++++++++++++++++++++++++++---- 1 file changed, 55 insertions(+), 6 deletions(-) diff --git a/src/engine/from_github.ts b/src/engine/from_github.ts index f44e121..64d53e1 100644 --- a/src/engine/from_github.ts +++ b/src/engine/from_github.ts @@ -80,7 +80,13 @@ export async function patchFromGitHubPullRequest( metadata: { sourceBranch: pullReq.head.ref, targetBranch: pullReq.base.ref, - linkedPRs: extractLinkedPRs(pullReq.body), + linkedPRs: await extractLinkedPRs( + clt, + repo.owner, + repo.name, + prNum, + pullReq.body, + ), }, patchList: [], patchFetchMap: {}, @@ -162,18 +168,61 @@ async function getGitHubPRFileID(salt: string, url: URL): Promise { return hexEncode(new Uint8Array(digest)); } -function extractLinkedPRs(prDescription: string | null): ILinkedPR[] { - if (!prDescription || !hasParsableFrontMatter(prDescription)) { - return []; +async function extractLinkedPRs( + clt: Octokit, + owner: string, + repo: string, + prNum: number, + prDescription: string | null, +): Promise { + interface IFrontMatterLinkedPR { + repo?: string; + prNum: number; } interface IExpectedFrontMatter { fensak: { - linked: ILinkedPR[]; + linked: IFrontMatterLinkedPR[]; }; } + + if (!prDescription || !hasParsableFrontMatter(prDescription)) { + return []; + } + const fm = extractFrontMatter(prDescription); - return fm.attrs.fensak.linked; + if (!fm.attrs.fensak) { + return []; + } + if (!fm.attrs.fensak.linked) { + throw new TypeError( + `PR ${owner}/${repo}#${prNum} has front matter, but it is not in the expected format`, + ); + } + + const out: ILinkedPR[] = await Promise.all( + fm.attrs.fensak.linked.map(async (l): Promise => { + let outR = ""; + let r = repo; + if (l.repo) { + outR = l.repo; + r = l.repo; + } + const { data: pullReq } = await clt.pulls.get({ + owner: owner, + repo: r, + pull_number: l.prNum, + }); + + return { + repo: outR, + prNum: l.prNum, + isMerged: pullReq.merged, + isClosed: pullReq.state === "closed", + }; + }), + ); + return out; } function hexEncode(hb: Uint8Array): string {