From ab9a234834ad5ec5498a878a38529c1a00228a7b Mon Sep 17 00:00:00 2001 From: Sam Lanning Date: Fri, 23 Aug 2024 17:33:31 +0100 Subject: [PATCH 1/2] Test the resulting git tree hashes after mutation --- src/core.ts | 1 + src/github/graphql/queries.ts | 24 +++++ src/test/integration/node.test.ts | 155 ++++++++++++++++++++++++------ 3 files changed, 150 insertions(+), 30 deletions(-) diff --git a/src/core.ts b/src/core.ts index 4ff9686..6c1f642 100644 --- a/src/core.ts +++ b/src/core.ts @@ -142,6 +142,7 @@ export const commitFilesFromBase64 = async ({ input: { refId: info.targetBranch.id, oid: baseOid, + force: true, }, }); diff --git a/src/github/graphql/queries.ts b/src/github/graphql/queries.ts index 1fa372a..be89ac0 100644 --- a/src/github/graphql/queries.ts +++ b/src/github/graphql/queries.ts @@ -9,6 +9,8 @@ import type { CreateRefMutationVariables, DeleteRefMutation, DeleteRefMutationVariables, + GetRefTreeQuery, + GetRefTreeQueryVariables, GetRepositoryMetadataQuery, GetRepositoryMetadataQueryVariables, UpdateRefMutation, @@ -83,6 +85,23 @@ const CREATE_COMMIT_ON_BRANCH = /* GraphQL */ ` } `; +/** For tests only */ +const GET_REF_TREE = /* GraphQL */ ` + query getRefTree($owner: String!, $name: String!, $ref: String!) { + repository(owner: $owner, name: $name) { + ref(qualifiedName: $ref) { + target { + ... on Commit { + tree { + oid + } + } + } + } + } + } +`; + export const getRepositoryMetadata = async ( o: GitHubClient, v: GetRepositoryMetadataQueryVariables, @@ -117,3 +136,8 @@ export const createCommitOnBranchQuery = async ( v: CreateCommitOnBranchMutationVariables, ): Promise => o.graphql(CREATE_COMMIT_ON_BRANCH, v); + +export const getRefTreeQuery = async ( + o: GitHubClient, + v: GetRefTreeQueryVariables, +): Promise => o.graphql(GET_REF_TREE, v); diff --git a/src/test/integration/node.test.ts b/src/test/integration/node.test.ts index 4115f1c..e1bb337 100644 --- a/src/test/integration/node.test.ts +++ b/src/test/integration/node.test.ts @@ -5,6 +5,7 @@ import { commitFilesFromBuffers } from "../../node.js"; import { deleteBranches } from "./util.js"; import { createRefMutation, + getRefTreeQuery, getRepositoryMetadata, } from "../../github/graphql/queries.js"; @@ -12,31 +13,63 @@ const octokit = getOctokit(ENV.GITHUB_TOKEN); const TEST_BRANCH_PREFIX = `${ROOT_TEST_BRANCH_PREFIX}-node`; +const TEST_TARGET_COMMIT = "fce2760017eab6d85388ed5cfdfac171559d80b3"; +/** + * For tests, important that this commit is not an ancestor of TEST_TARGET_COMMIT, + * to ensure that non-fast-forward pushes are tested + */ +const TEST_TARGET_COMMIT_2 = "7ba8473f02849de3b5449b25fc83c5245d338d94"; +const TEST_TARGET_TREE_2 = "95c9ea756f3686614dcdc1c42f7f654b684cdac2"; + +const BASIC_FILE_CONTENTS = { + message: { + headline: "Test commit", + body: "This is a test commit", + }, + fileChanges: { + additions: [ + { + path: `foo.txt`, + contents: Buffer.alloc(1024, "Hello, world!"), + }, + ], + }, + log, +}; + +const TEST_TARGET_TREE_WITH_BASIC_CHANGES = + "a3431c9b42b71115c52bc6fbf9da3682cf0ed5e8"; + describe("node", () => { const branches: string[] = []; // Set timeout to 1 minute jest.setTimeout(60 * 1000); - const contents = Buffer.alloc(1024, "Hello, world!"); - const BASIC_FILE_CONTENTS = { - message: { - headline: "Test commit", - body: "This is a test commit", - }, - fileChanges: { - additions: [ - { - path: `foo.txt`, - contents, - }, - ], - }, - log, - }; - let repositoryId: string; + const expectBranchHasTree = async ({ + branch, + oid, + }: { + branch: string; + oid: string; + }) => { + const ref = ( + await getRefTreeQuery(octokit, { + owner: REPO.owner, + name: REPO.repository, + ref: `refs/heads/${branch}`, + }) + ).repository?.ref?.target; + + if (ref && "tree" in ref) { + expect(ref.tree.oid).toEqual(oid); + } else { + throw new Error("Expected ref to have a tree"); + } + }; + beforeAll(async () => { const response = await getRepositoryMetadata(octokit, { owner: REPO.owner, @@ -53,12 +86,23 @@ describe("node", () => { describe("commitFilesFromBuffers", () => { describe("can commit single file of various sizes", () => { const SIZES_BYTES = { - "1KiB": 1024, - "1MiB": 1024 * 1024, - "10MiB": 1024 * 1024 * 10, + "1KiB": { + sizeBytes: 1024, + tree: "547dfe4079b53c3b45a6717ac1ed6d98512f0a1c", + }, + "1MiB": { + sizeBytes: 1024 * 1024, + tree: "a6dca57388cf08de146bcc01a2113b218d6c2858", + }, + "10MiB": { + sizeBytes: 1024 * 1024 * 10, + tree: "c4788256a2c1e3ea4267cff0502a656d992248ec", + }, }; - for (const [sizeName, sizeBytes] of Object.entries(SIZES_BYTES)) { + for (const [sizeName, { sizeBytes, tree }] of Object.entries( + SIZES_BYTES, + )) { it(`Can commit a ${sizeName}`, async () => { const branch = `${TEST_BRANCH_PREFIX}-${sizeName}`; branches.push(branch); @@ -69,7 +113,7 @@ describe("node", () => { ...REPO, branch, base: { - branch: "main", + commit: TEST_TARGET_COMMIT, }, message: { headline: "Test commit", @@ -85,10 +129,33 @@ describe("node", () => { }, log, }); + + await expectBranchHasTree({ + branch, + oid: tree, + }); }); } }); + it("can commit using branch as a base", async () => { + const branch = `${TEST_BRANCH_PREFIX}-branch-base`; + branches.push(branch); + + await commitFilesFromBuffers({ + octokit, + ...REPO, + branch, + base: { + branch: "main", + }, + ...BASIC_FILE_CONTENTS, + }); + + // Don't test tree for this one as it will change over time / be unstable + // TODO: Get the oid of the specific files, and test that + }); + it("can commit using tag as a base", async () => { const branch = `${TEST_BRANCH_PREFIX}-tag-base`; branches.push(branch); @@ -102,6 +169,9 @@ describe("node", () => { }, ...BASIC_FILE_CONTENTS, }); + + // Don't test tree for this one as it will change over time / be unstable + // TODO: Get the oid of the specific files, and test that }); it("can commit using commit as a base", async () => { @@ -113,10 +183,15 @@ describe("node", () => { ...REPO, branch, base: { - commit: "fce2760017eab6d85388ed5cfdfac171559d80b3", + commit: TEST_TARGET_COMMIT, }, ...BASIC_FILE_CONTENTS, }); + + await expectBranchHasTree({ + branch, + oid: TEST_TARGET_TREE_WITH_BASIC_CHANGES, + }); }); describe("existing branches", () => { @@ -129,7 +204,7 @@ describe("node", () => { input: { repositoryId, name: `refs/heads/${branch}`, - oid: "31ded45f25a07726e02fd111d4c230718b49fa2a", + oid: TEST_TARGET_COMMIT_2, }, }); @@ -138,11 +213,16 @@ describe("node", () => { ...REPO, branch, base: { - commit: "fce2760017eab6d85388ed5cfdfac171559d80b3", + commit: TEST_TARGET_COMMIT, }, ...BASIC_FILE_CONTENTS, force: true, }); + + await expectBranchHasTree({ + branch, + oid: TEST_TARGET_TREE_WITH_BASIC_CHANGES, + }); }); it("cannot commit to existing branch when force is false", async () => { @@ -154,7 +234,7 @@ describe("node", () => { input: { repositoryId, name: `refs/heads/${branch}`, - oid: "31ded45f25a07726e02fd111d4c230718b49fa2a", + oid: TEST_TARGET_COMMIT_2, }, }); @@ -164,13 +244,18 @@ describe("node", () => { ...REPO, branch, base: { - commit: "fce2760017eab6d85388ed5cfdfac171559d80b3", + commit: TEST_TARGET_COMMIT, }, ...BASIC_FILE_CONTENTS, }), ).rejects.toThrow( `Branch ${branch} exists already and does not match base`, ); + + await expectBranchHasTree({ + branch, + oid: TEST_TARGET_TREE_2, + }); }); it("can commit to existing branch when force is false and target matches base", async () => { @@ -182,7 +267,7 @@ describe("node", () => { input: { repositoryId, name: `refs/heads/${branch}`, - oid: "31ded45f25a07726e02fd111d4c230718b49fa2a", + oid: TEST_TARGET_COMMIT, }, }); @@ -191,10 +276,15 @@ describe("node", () => { ...REPO, branch, base: { - commit: "31ded45f25a07726e02fd111d4c230718b49fa2a", + commit: TEST_TARGET_COMMIT, }, ...BASIC_FILE_CONTENTS, }); + + await expectBranchHasTree({ + branch, + oid: TEST_TARGET_TREE_WITH_BASIC_CHANGES, + }); }); it("can commit to same branch as base", async () => { @@ -206,7 +296,7 @@ describe("node", () => { input: { repositoryId, name: `refs/heads/${branch}`, - oid: "31ded45f25a07726e02fd111d4c230718b49fa2a", + oid: TEST_TARGET_COMMIT, }, }); @@ -219,6 +309,11 @@ describe("node", () => { }, ...BASIC_FILE_CONTENTS, }); + + await expectBranchHasTree({ + branch, + oid: TEST_TARGET_TREE_WITH_BASIC_CHANGES, + }); }); }); }); From 4c8d91266741786ac79df43ca528eb8f3afc0f5a Mon Sep 17 00:00:00 2001 From: Sam Lanning Date: Fri, 23 Aug 2024 18:10:21 +0100 Subject: [PATCH 2/2] Test the file hashes in tests To support deeper tests for the cases where the tree hash will be unstable, test the hash of the file contents instead, and also do this for places where we also test the tree hash. --- src/github/graphql/queries.ts | 10 +++- src/test/integration/node.test.ts | 86 +++++++++++++++++++++++++------ 2 files changed, 78 insertions(+), 18 deletions(-) diff --git a/src/github/graphql/queries.ts b/src/github/graphql/queries.ts index be89ac0..311122f 100644 --- a/src/github/graphql/queries.ts +++ b/src/github/graphql/queries.ts @@ -87,7 +87,12 @@ const CREATE_COMMIT_ON_BRANCH = /* GraphQL */ ` /** For tests only */ const GET_REF_TREE = /* GraphQL */ ` - query getRefTree($owner: String!, $name: String!, $ref: String!) { + query getRefTree( + $owner: String! + $name: String! + $ref: String! + $path: String! + ) { repository(owner: $owner, name: $name) { ref(qualifiedName: $ref) { target { @@ -95,6 +100,9 @@ const GET_REF_TREE = /* GraphQL */ ` tree { oid } + file(path: $path) { + oid + } } } } diff --git a/src/test/integration/node.test.ts b/src/test/integration/node.test.ts index e1bb337..e8433be 100644 --- a/src/test/integration/node.test.ts +++ b/src/test/integration/node.test.ts @@ -21,6 +21,8 @@ const TEST_TARGET_COMMIT = "fce2760017eab6d85388ed5cfdfac171559d80b3"; const TEST_TARGET_COMMIT_2 = "7ba8473f02849de3b5449b25fc83c5245d338d94"; const TEST_TARGET_TREE_2 = "95c9ea756f3686614dcdc1c42f7f654b684cdac2"; +const BASIC_FILE_CHANGES_PATH = "foo.txt"; +const BASIC_FILE_CHANGES_OID = "0e23339619d605319ec4b49a0ac9dd94598eff8e"; const BASIC_FILE_CONTENTS = { message: { headline: "Test commit", @@ -29,7 +31,7 @@ const BASIC_FILE_CONTENTS = { fileChanges: { additions: [ { - path: `foo.txt`, + path: BASIC_FILE_CHANGES_PATH, contents: Buffer.alloc(1024, "Hello, world!"), }, ], @@ -50,21 +52,36 @@ describe("node", () => { const expectBranchHasTree = async ({ branch, - oid, + treeOid, + file, }: { branch: string; - oid: string; + treeOid?: string; + file?: { + path: string; + oid: string; + }; }) => { const ref = ( await getRefTreeQuery(octokit, { owner: REPO.owner, name: REPO.repository, ref: `refs/heads/${branch}`, + path: file?.path ?? "package.json", }) ).repository?.ref?.target; - if (ref && "tree" in ref) { - expect(ref.tree.oid).toEqual(oid); + if (!ref) { + throw new Error("Unexpected missing ref"); + } + + if ("tree" in ref) { + if (treeOid) { + expect(ref.tree.oid).toEqual(treeOid); + } + if (file) { + expect(ref.file?.oid).toEqual(file.oid); + } } else { throw new Error("Expected ref to have a tree"); } @@ -88,19 +105,22 @@ describe("node", () => { const SIZES_BYTES = { "1KiB": { sizeBytes: 1024, - tree: "547dfe4079b53c3b45a6717ac1ed6d98512f0a1c", + treeOid: "547dfe4079b53c3b45a6717ac1ed6d98512f0a1c", + fileOid: "0e23339619d605319ec4b49a0ac9dd94598eff8e", }, "1MiB": { sizeBytes: 1024 * 1024, - tree: "a6dca57388cf08de146bcc01a2113b218d6c2858", + treeOid: "a6dca57388cf08de146bcc01a2113b218d6c2858", + fileOid: "a1d7fed1b4a8de1b665dc4f604015b2d87ef978f", }, "10MiB": { sizeBytes: 1024 * 1024 * 10, - tree: "c4788256a2c1e3ea4267cff0502a656d992248ec", + treeOid: "c4788256a2c1e3ea4267cff0502a656d992248ec", + fileOid: "e36e74edbb6d3fc181ef584a50f8ee55585d27cc", }, }; - for (const [sizeName, { sizeBytes, tree }] of Object.entries( + for (const [sizeName, { sizeBytes, treeOid, fileOid }] of Object.entries( SIZES_BYTES, )) { it(`Can commit a ${sizeName}`, async () => { @@ -132,7 +152,11 @@ describe("node", () => { await expectBranchHasTree({ branch, - oid: tree, + treeOid, + file: { + path: `${sizeName}.txt`, + oid: fileOid, + }, }); }); } @@ -153,7 +177,13 @@ describe("node", () => { }); // Don't test tree for this one as it will change over time / be unstable - // TODO: Get the oid of the specific files, and test that + await expectBranchHasTree({ + branch, + file: { + path: BASIC_FILE_CHANGES_PATH, + oid: BASIC_FILE_CHANGES_OID, + }, + }); }); it("can commit using tag as a base", async () => { @@ -171,7 +201,13 @@ describe("node", () => { }); // Don't test tree for this one as it will change over time / be unstable - // TODO: Get the oid of the specific files, and test that + await expectBranchHasTree({ + branch, + file: { + path: BASIC_FILE_CHANGES_PATH, + oid: BASIC_FILE_CHANGES_OID, + }, + }); }); it("can commit using commit as a base", async () => { @@ -190,7 +226,11 @@ describe("node", () => { await expectBranchHasTree({ branch, - oid: TEST_TARGET_TREE_WITH_BASIC_CHANGES, + treeOid: TEST_TARGET_TREE_WITH_BASIC_CHANGES, + file: { + path: BASIC_FILE_CHANGES_PATH, + oid: BASIC_FILE_CHANGES_OID, + }, }); }); @@ -221,7 +261,11 @@ describe("node", () => { await expectBranchHasTree({ branch, - oid: TEST_TARGET_TREE_WITH_BASIC_CHANGES, + treeOid: TEST_TARGET_TREE_WITH_BASIC_CHANGES, + file: { + path: BASIC_FILE_CHANGES_PATH, + oid: BASIC_FILE_CHANGES_OID, + }, }); }); @@ -254,7 +298,7 @@ describe("node", () => { await expectBranchHasTree({ branch, - oid: TEST_TARGET_TREE_2, + treeOid: TEST_TARGET_TREE_2, }); }); @@ -283,7 +327,11 @@ describe("node", () => { await expectBranchHasTree({ branch, - oid: TEST_TARGET_TREE_WITH_BASIC_CHANGES, + treeOid: TEST_TARGET_TREE_WITH_BASIC_CHANGES, + file: { + path: BASIC_FILE_CHANGES_PATH, + oid: BASIC_FILE_CHANGES_OID, + }, }); }); @@ -312,7 +360,11 @@ describe("node", () => { await expectBranchHasTree({ branch, - oid: TEST_TARGET_TREE_WITH_BASIC_CHANGES, + treeOid: TEST_TARGET_TREE_WITH_BASIC_CHANGES, + file: { + path: BASIC_FILE_CHANGES_PATH, + oid: BASIC_FILE_CHANGES_OID, + }, }); }); });