From 31c73566fd75eeabfe0f477c69ed83b48439da6e Mon Sep 17 00:00:00 2001 From: Hsu Zhong Jun <27919917+dcshzj@users.noreply.github.com> Date: Wed, 16 Aug 2023 17:31:16 +0800 Subject: [PATCH] fix(GGs): hotfixes from bugs identified during testing (#903) * Have getLatestCommitOfBranch retry with origin prefix * Adjust logFields to use const asserted earlier * Use git mv only when absolutely necessary * Retry pushing exactly once if it failed the first time * Revert to check logFields directly * Fix missing SHA when deleting directories * fix: handle errors in routehandler --------- Co-authored-by: Alexander Lee --- src/middleware/routeHandler.js | 25 +- src/services/db/GitFileSystemService.ts | 191 ++++++---- src/services/db/RepoService.ts | 2 +- .../db/__tests__/GitFileSystemService.spec.ts | 340 ++++++++++++------ 4 files changed, 375 insertions(+), 183 deletions(-) diff --git a/src/middleware/routeHandler.js b/src/middleware/routeHandler.js index 04c49e1c9..ea7bd3912 100644 --- a/src/middleware/routeHandler.js +++ b/src/middleware/routeHandler.js @@ -36,12 +36,21 @@ const attachWriteRouteHandlerWrapper = (routeHandler) => async ( next ) => { const { siteName } = req.params - await lock(siteName) + try { + await lock(siteName) + } catch (err) { + next(err) + } + routeHandler(req, res, next).catch(async (err) => { await unlock(siteName) next(err) }) - await unlock(siteName) + try { + await unlock(siteName) + } catch (err) { + next(err) + } } const gitFileSystemService = new GitFileSystemService(new SimpleGit()) @@ -59,7 +68,11 @@ const attachRollbackRouteHandlerWrapper = (routeHandler) => async ( siteName ) - await lock(siteName) + try { + await lock(siteName) + } catch (err) { + next(err) + } let originalCommitSha if (isRepoWhitelisted) { @@ -129,7 +142,11 @@ const attachRollbackRouteHandlerWrapper = (routeHandler) => async ( next(err) }) - await unlock(siteName) + try { + await unlock(siteName) + } catch (err) { + next(err) + } } module.exports = { diff --git a/src/services/db/GitFileSystemService.ts b/src/services/db/GitFileSystemService.ts index 6b196f653..35bb18950 100644 --- a/src/services/db/GitFileSystemService.ts +++ b/src/services/db/GitFileSystemService.ts @@ -9,7 +9,13 @@ import { Result, ResultAsync, } from "neverthrow" -import { CleanOptions, GitError, SimpleGit, DefaultLogFields } from "simple-git" +import { + CleanOptions, + GitError, + SimpleGit, + DefaultLogFields, + LogResult, +} from "simple-git" import { config } from "@config/config" @@ -24,7 +30,7 @@ import { ISOMER_GITHUB_ORG_NAME } from "@constants/constants" import { SessionDataProps } from "@root/classes" import { MediaTypeError } from "@root/errors/MediaTypeError" -import { MediaFileInput, MediaFileOutput } from "@root/types" +import { MediaFileOutput } from "@root/types" import { GitHubCommitData } from "@root/types/commitData" import type { GitCommitResult, @@ -213,6 +219,29 @@ export default class GitFileSystemService { ) } + // Get the Git log of a particular branch + getGitLog( + repoName: string, + branchName: string + ): ResultAsync, GitFileSystemError> { + return ResultAsync.fromPromise( + this.git.cwd(`${EFS_VOL_PATH}/${repoName}`).log([branchName]), + (error) => { + logger.error( + `Error when getting latest commit of "${branchName}" branch: ${error}` + ) + + if (error instanceof GitError) { + return new GitFileSystemError( + "Unable to retrieve branch log info from disk" + ) + } + + return new GitFileSystemError("An unknown error occurred") + } + ) + } + // Reset the state of the local Git repository to a specific commit rollback( repoName: string, @@ -363,24 +392,45 @@ export default class GitFileSystemService { ) } - return this.ensureCorrectBranch(repoName).andThen(() => - ResultAsync.fromPromise( - isForce - ? this.git.cwd(`${EFS_VOL_PATH}/${repoName}`).push(["--force"]) - : this.git.cwd(`${EFS_VOL_PATH}/${repoName}`).push(), - (error) => { - logger.error(`Error when pushing ${repoName}: ${error}`) + return this.ensureCorrectBranch(repoName) + .andThen(() => + ResultAsync.fromPromise( + isForce + ? this.git.cwd(`${EFS_VOL_PATH}/${repoName}`).push(["--force"]) + : this.git.cwd(`${EFS_VOL_PATH}/${repoName}`).push(), + (error) => { + logger.error(`Error when pushing ${repoName}: ${error}`) - if (error instanceof GitError) { - return new GitFileSystemError( - "Unable to push latest changes of repo" - ) + if (error instanceof GitError) { + return new GitFileSystemError( + "Unable to push latest changes of repo" + ) + } + + return new GitFileSystemError("An unknown error occurred") } + ) + ) + .orElse(() => + // Retry push once + ResultAsync.fromPromise( + isForce + ? this.git.cwd(`${EFS_VOL_PATH}/${repoName}`).push(["--force"]) + : this.git.cwd(`${EFS_VOL_PATH}/${repoName}`).push(), + (error) => { + logger.error(`Error when pushing ${repoName}: ${error}`) - return new GitFileSystemError("An unknown error occurred") - } - ).map(() => `${EFS_VOL_PATH}/${repoName}`) - ) + if (error instanceof GitError) { + return new GitFileSystemError( + "Unable to push latest changes of repo" + ) + } + + return new GitFileSystemError("An unknown error occurred") + } + ) + ) + .map(() => `${EFS_VOL_PATH}/${repoName}`) }) } @@ -389,7 +439,8 @@ export default class GitFileSystemService { repoName: string, pathSpec: string[], userId: SessionDataProps["isomerUserId"], - message: string + message: string, + skipGitAdd?: boolean ): ResultAsync { return this.isValidGitRepo(repoName).andThen((isValid) => { if (!isValid) { @@ -421,12 +472,34 @@ export default class GitFileSystemService { const commitMessage = JSON.stringify(commitMessageObj) return this.ensureCorrectBranch(repoName) + .andThen(() => { + if (skipGitAdd) { + // This is necessary when we have performed a git mv + return okAsync(true) + } + + return ResultAsync.fromPromise( + this.git.cwd(`${EFS_VOL_PATH}/${repoName}`).add(pathSpec), + (error) => { + logger.error( + `Error when Git adding files to ${repoName}: ${error}` + ) + + if (error instanceof GitError) { + return new GitFileSystemNeedsRollbackError( + "Unable to commit changes" + ) + } + + return new GitFileSystemNeedsRollbackError( + "An unknown error occurred" + ) + } + ) + }) .andThen(() => ResultAsync.fromPromise( - this.git - .cwd(`${EFS_VOL_PATH}/${repoName}`) - .add(pathSpec) - .commit(commitMessage), + this.git.cwd(`${EFS_VOL_PATH}/${repoName}`).commit(commitMessage), (error) => { logger.error(`Error when committing ${repoName}: ${error}`) @@ -489,7 +562,7 @@ export default class GitFileSystemService { ) return new GitFileSystemError("An unknown error occurred") } - ).map((_) => true) + ).map(() => true) } return err(error) }) @@ -863,7 +936,7 @@ export default class GitFileSystemService { repoName, [path], userId, - `Delete ${ + `Delete ${ isDir ? `directory: ${path}` : `file: ${path.split("/").pop()}` }` ) @@ -934,7 +1007,8 @@ export default class GitFileSystemService { repoName, [oldPath, newPath], userId, - message || `Renamed ${oldPath} to ${newPath}` + message || `Renamed ${oldPath} to ${newPath}`, + true ) ) .orElse((error) => { @@ -1013,16 +1087,12 @@ export default class GitFileSystemService { return errAsync(error) }) - .andThen(() => { - const oldFilePath = - oldPath === "" ? targetFile : `${oldPath}/${targetFile}` - const newFilePath = - newPath === "" ? targetFile : `${newPath}/${targetFile}` - - return ResultAsync.fromPromise( - this.git - .cwd(`${EFS_VOL_PATH}/${repoName}`) - .mv(`${oldFilePath}`, `${newFilePath}`), + .andThen(() => + ResultAsync.fromPromise( + fs.promises.rename( + `${EFS_VOL_PATH}/${repoName}/${oldPath}/${targetFile}`, + `${EFS_VOL_PATH}/${repoName}/${newPath}/${targetFile}` + ), (error) => { logger.error( `Error when moving ${targetFile} in ${oldPath} to ${newPath}: ${error}` @@ -1039,7 +1109,7 @@ export default class GitFileSystemService { ) } ) - }) + ) ) ) ) @@ -1066,37 +1136,26 @@ export default class GitFileSystemService { repoName: string, branchName: string ): ResultAsync { - return ResultAsync.fromPromise( - this.git.cwd(`${EFS_VOL_PATH}/${repoName}`).log([branchName]), - (error) => { - logger.error(`Error when getting latest commit of branch: ${error}`) - - if (error instanceof GitError) { - return new GitFileSystemError( - "Unable to retrieve branch log info from disk" - ) + return this.getGitLog(repoName, branchName) + .orElse(() => this.getGitLog(repoName, `origin/${branchName}`)) + .andThen((logSummary) => { + const possibleCommit = logSummary.latest + if (this.isDefaultLogFields(possibleCommit)) { + return okAsync({ + author: { + name: possibleCommit.author_name, + email: possibleCommit.author_email, + date: possibleCommit.date, + }, + message: possibleCommit.message, + sha: possibleCommit.hash, + }) } - - return new GitFileSystemError("An unknown error occurred") - } - ).andThen((logSummary) => { - const possibleCommit = logSummary.latest - if (this.isDefaultLogFields(possibleCommit)) { - return okAsync({ - author: { - name: possibleCommit.author_name, - email: possibleCommit.author_email, - date: possibleCommit.date, - }, - message: possibleCommit.message, - sha: possibleCommit.hash, - }) - } - return errAsync( - new GitFileSystemError( - "Unable to retrieve latest commit info from disk" + return errAsync( + new GitFileSystemError( + "Unable to retrieve latest commit info from disk" + ) ) - ) - }) + }) } } diff --git a/src/services/db/RepoService.ts b/src/services/db/RepoService.ts index 2f7d85bc2..a6a42b73d 100644 --- a/src/services/db/RepoService.ts +++ b/src/services/db/RepoService.ts @@ -396,7 +396,7 @@ export default class RepoService extends GitHubService { sha: null, })) - const newCommitSha = this.updateTree(sessionData, githubSessionData, { + const newCommitSha = await this.updateTree(sessionData, githubSessionData, { gitTree: newGitTree, message, }) diff --git a/src/services/db/__tests__/GitFileSystemService.spec.ts b/src/services/db/__tests__/GitFileSystemService.spec.ts index 048b7dea0..3e640a4e1 100644 --- a/src/services/db/__tests__/GitFileSystemService.spec.ts +++ b/src/services/db/__tests__/GitFileSystemService.spec.ts @@ -478,6 +478,34 @@ describe("GitFileSystemService", () => { }) }) + describe("getGitLog", () => { + it("should return the Git log for a valid branch", async () => { + MockSimpleGit.cwd.mockReturnValueOnce({ + log: jest.fn().mockResolvedValueOnce(undefined), + }) + + const result = await GitFileSystemService.getGitLog( + "fake-repo", + "fake-commit-sha" + ) + + expect(result.isOk()).toBeTrue() + }) + + it("should return GitFileSystemError if an error occurred when getting the Git log", async () => { + MockSimpleGit.cwd.mockReturnValueOnce({ + log: jest.fn().mockRejectedValueOnce(new GitError()), + }) + + const result = await GitFileSystemService.getGitLog( + "fake-repo", + "fake-commit-sha" + ) + + expect(result._unsafeUnwrapErr()).toBeInstanceOf(GitFileSystemError) + }) + }) + describe("rollback", () => { it("should rollback successfully for a valid Git repo", async () => { MockSimpleGit.cwd.mockReturnValueOnce({ @@ -662,6 +690,32 @@ describe("GitFileSystemService", () => { expect(result.isOk()).toBeTrue() }) + it("should retry pushing once if a Git error occurs when pushing the first time", async () => { + MockSimpleGit.cwd.mockReturnValueOnce({ + checkIsRepo: jest.fn().mockResolvedValueOnce(true), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + remote: jest + .fn() + .mockResolvedValueOnce( + `git@github.com:${ISOMER_GITHUB_ORG_NAME}/fake-repo.git` + ), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + revparse: jest.fn().mockResolvedValueOnce(BRANCH_REF), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + push: jest.fn().mockRejectedValueOnce(new GitError()), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + push: jest.fn().mockResolvedValueOnce(undefined), + }) + + const result = await GitFileSystemService.push("fake-repo") + + expect(result.isOk()).toBeTrue() + }) + it("should return a GitFileSystemError if a Git error occurs when pushing", async () => { MockSimpleGit.cwd.mockReturnValueOnce({ checkIsRepo: jest.fn().mockResolvedValueOnce(true), @@ -679,6 +733,9 @@ describe("GitFileSystemService", () => { MockSimpleGit.cwd.mockReturnValueOnce({ push: jest.fn().mockRejectedValueOnce(new GitError()), }) + MockSimpleGit.cwd.mockReturnValueOnce({ + push: jest.fn().mockRejectedValueOnce(new GitError()), + }) const result = await GitFileSystemService.push("fake-repo") @@ -711,14 +768,15 @@ describe("GitFileSystemService", () => { MockSimpleGit.cwd.mockReturnValueOnce({ revparse: jest.fn().mockResolvedValueOnce(BRANCH_REF), }) + MockSimpleGit.cwd.mockReturnValueOnce({ + add: jest.fn().mockResolvedValueOnce(undefined), + }) const mockCommitSha = "fake-commit-sha" const mockCommitFn = jest .fn() .mockResolvedValueOnce({ commit: mockCommitSha }) MockSimpleGit.cwd.mockReturnValueOnce({ - add: jest.fn().mockReturnValueOnce({ - commit: mockCommitFn, - }), + commit: mockCommitFn, }) const fakePath = `fake-dir/${MOCK_GITHUB_FILENAME_ALPHA_ONE}` @@ -739,6 +797,48 @@ describe("GitFileSystemService", () => { expect(mockCommitFn).toHaveBeenCalledWith(expectedCommitMessage) }) + it("should commit successfully without adding files if skipGitAdd was set to true", async () => { + MockSimpleGit.cwd.mockReturnValueOnce({ + checkIsRepo: jest.fn().mockResolvedValueOnce(true), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + remote: jest + .fn() + .mockResolvedValueOnce( + `git@github.com:${ISOMER_GITHUB_ORG_NAME}/fake-repo.git` + ), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + revparse: jest.fn().mockResolvedValueOnce(BRANCH_REF), + }) + const mockCommitSha = "fake-commit-sha" + const mockCommitFn = jest + .fn() + .mockResolvedValueOnce({ commit: mockCommitSha }) + MockSimpleGit.cwd.mockReturnValueOnce({ + commit: mockCommitFn, + }) + + const fakePath = `fake-dir/${MOCK_GITHUB_FILENAME_ALPHA_ONE}` + const expectedCommitMessage = JSON.stringify({ + message: MOCK_GITHUB_COMMIT_MESSAGE_ALPHA_ONE, + userId: MOCK_USER_ID_ONE.toString(), + fileName: MOCK_GITHUB_FILENAME_ALPHA_ONE, + }) + + const result = await GitFileSystemService.commit( + "fake-repo", + [fakePath], + MOCK_USER_ID_ONE.toString(), + MOCK_GITHUB_COMMIT_MESSAGE_ALPHA_ONE, + true + ) + + expect(result._unsafeUnwrap()).toBe(mockCommitSha) + expect(mockCommitFn).toHaveBeenCalledWith(expectedCommitMessage) + expect(MockSimpleGit.cwd).toHaveBeenCalledTimes(4) + }) + it("should return a GitFileSystemNeedsRollbackError if a Git error occurs when committing", async () => { MockSimpleGit.cwd.mockReturnValueOnce({ checkIsRepo: jest.fn().mockResolvedValueOnce(true), @@ -754,9 +854,40 @@ describe("GitFileSystemService", () => { revparse: jest.fn().mockResolvedValueOnce(BRANCH_REF), }) MockSimpleGit.cwd.mockReturnValueOnce({ - add: jest.fn().mockReturnValueOnce({ - commit: jest.fn().mockRejectedValueOnce(new GitError()), - }), + add: jest.fn().mockResolvedValueOnce(undefined), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + commit: jest.fn().mockRejectedValueOnce(new GitError()), + }) + + const result = await GitFileSystemService.commit( + "fake-repo", + ["fake-dir/fake-file"], + "fake-hash", + "fake message" + ) + + expect(result._unsafeUnwrapErr()).toBeInstanceOf( + GitFileSystemNeedsRollbackError + ) + }) + + it("should return a GitFileSystemNeedsRollbackError if a Git error occurs when adding files", async () => { + MockSimpleGit.cwd.mockReturnValueOnce({ + checkIsRepo: jest.fn().mockResolvedValueOnce(true), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + remote: jest + .fn() + .mockResolvedValueOnce( + `git@github.com:${ISOMER_GITHUB_ORG_NAME}/fake-repo.git` + ), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + revparse: jest.fn().mockResolvedValueOnce(BRANCH_REF), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + add: jest.fn().mockRejectedValueOnce(new GitError()), }) const result = await GitFileSystemService.commit( @@ -863,9 +994,10 @@ describe("GitFileSystemService", () => { checkout: jest.fn().mockResolvedValueOnce(undefined), }) MockSimpleGit.cwd.mockReturnValueOnce({ - add: jest.fn().mockReturnValueOnce({ - commit: jest.fn().mockResolvedValueOnce({ commit: expectedSha }), - }), + add: jest.fn().mockResolvedValueOnce(undefined), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + commit: jest.fn().mockResolvedValueOnce({ commit: expectedSha }), }) const expected = { @@ -914,9 +1046,10 @@ describe("GitFileSystemService", () => { checkout: jest.fn().mockResolvedValueOnce(undefined), }) MockSimpleGit.cwd.mockReturnValueOnce({ - add: jest.fn().mockReturnValueOnce({ - commit: jest.fn().mockResolvedValueOnce({ commit: expectedSha }), - }), + add: jest.fn().mockResolvedValueOnce(undefined), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + commit: jest.fn().mockResolvedValueOnce({ commit: expectedSha }), }) const expected = { @@ -964,9 +1097,10 @@ describe("GitFileSystemService", () => { checkout: jest.fn().mockResolvedValueOnce(undefined), }) MockSimpleGit.cwd.mockReturnValueOnce({ - add: jest.fn().mockReturnValueOnce({ - commit: jest.fn().mockResolvedValueOnce({ commit: expectedSha }), - }), + add: jest.fn().mockResolvedValueOnce(undefined), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + commit: jest.fn().mockResolvedValueOnce({ commit: expectedSha }), }) const expected = { @@ -1037,9 +1171,10 @@ describe("GitFileSystemService", () => { checkout: jest.fn().mockResolvedValueOnce(undefined), }) MockSimpleGit.cwd.mockReturnValueOnce({ - add: jest.fn().mockReturnValueOnce({ - commit: jest.fn().mockRejectedValueOnce(new GitError()), - }), + add: jest.fn().mockResolvedValueOnce(undefined), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + commit: jest.fn().mockRejectedValueOnce(new GitError()), }) MockSimpleGit.cwd.mockReturnValueOnce({ reset: jest.fn().mockReturnValueOnce({ @@ -1137,9 +1272,10 @@ describe("GitFileSystemService", () => { revparse: jest.fn().mockResolvedValueOnce(BRANCH_REF), }) MockSimpleGit.cwd.mockReturnValueOnce({ - add: jest.fn().mockReturnValueOnce({ - commit: jest.fn().mockResolvedValueOnce({ commit: "fake-new-hash" }), - }), + add: jest.fn().mockResolvedValueOnce(undefined), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + commit: jest.fn().mockResolvedValueOnce({ commit: "fake-new-hash" }), }) const actual = await GitFileSystemService.update( @@ -1182,9 +1318,10 @@ describe("GitFileSystemService", () => { revparse: jest.fn().mockResolvedValueOnce(BRANCH_REF), }) MockSimpleGit.cwd.mockReturnValueOnce({ - add: jest.fn().mockReturnValueOnce({ - commit: jest.fn().mockRejectedValueOnce(new GitError()), - }), + add: jest.fn().mockResolvedValueOnce(undefined), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + commit: jest.fn().mockRejectedValueOnce(new GitError()), }) MockSimpleGit.cwd.mockReturnValueOnce({ reset: jest.fn().mockReturnValueOnce({ @@ -1313,9 +1450,7 @@ describe("GitFileSystemService", () => { revparse: jest.fn().mockResolvedValueOnce(BRANCH_REF), }) MockSimpleGit.cwd.mockReturnValueOnce({ - add: jest.fn().mockReturnValueOnce({ - commit: jest.fn().mockResolvedValueOnce({ commit: "fake-new-hash" }), - }), + commit: jest.fn().mockResolvedValueOnce({ commit: "fake-new-hash" }), }) const actual = await GitFileSystemService.renameSinglePath( @@ -1364,9 +1499,7 @@ describe("GitFileSystemService", () => { revparse: jest.fn().mockResolvedValueOnce(BRANCH_REF), }) MockSimpleGit.cwd.mockReturnValueOnce({ - add: jest.fn().mockReturnValueOnce({ - commit: jest.fn().mockResolvedValueOnce({ commit: "fake-new-hash" }), - }), + commit: jest.fn().mockResolvedValueOnce({ commit: "fake-new-hash" }), }) const actual = await GitFileSystemService.renameSinglePath( @@ -1410,9 +1543,7 @@ describe("GitFileSystemService", () => { revparse: jest.fn().mockResolvedValueOnce(BRANCH_REF), }) MockSimpleGit.cwd.mockReturnValueOnce({ - add: jest.fn().mockReturnValueOnce({ - commit: jest.fn().mockRejectedValueOnce(new GitError()), - }), + commit: jest.fn().mockRejectedValueOnce(new GitError()), }) MockSimpleGit.cwd.mockReturnValueOnce({ reset: jest.fn().mockReturnValueOnce({ @@ -1531,11 +1662,6 @@ describe("GitFileSystemService", () => { }, }), }) - // Note: This will not cause the actual file to be renamed - const mockGitMv = jest.fn().mockResolvedValueOnce(undefined) - MockSimpleGit.cwd.mockReturnValueOnce({ - mv: mockGitMv, - }) MockSimpleGit.cwd.mockReturnValueOnce({ checkIsRepo: jest.fn().mockResolvedValueOnce(true), }) @@ -1550,9 +1676,10 @@ describe("GitFileSystemService", () => { revparse: jest.fn().mockResolvedValueOnce(BRANCH_REF), }) MockSimpleGit.cwd.mockReturnValueOnce({ - add: jest.fn().mockReturnValueOnce({ - commit: jest.fn().mockResolvedValueOnce({ commit: "fake-new-hash" }), - }), + add: jest.fn().mockResolvedValueOnce(undefined), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + commit: jest.fn().mockResolvedValueOnce({ commit: "fake-new-hash" }), }) const actual = await GitFileSystemService.moveFiles( @@ -1565,10 +1692,6 @@ describe("GitFileSystemService", () => { ) expect(actual._unsafeUnwrap()).toEqual("fake-new-hash") - expect(mockGitMv).toHaveBeenCalledWith( - "another-fake-file", - "fake-dir/another-fake-file" - ) }) it("should move files to a new directory successfully", async () => { @@ -1583,11 +1706,6 @@ describe("GitFileSystemService", () => { }, }), }) - // Note: This will not cause the actual file to be renamed - const mockGitMv = jest.fn().mockResolvedValueOnce(undefined) - MockSimpleGit.cwd.mockReturnValueOnce({ - mv: mockGitMv, - }) MockSimpleGit.cwd.mockReturnValueOnce({ checkIsRepo: jest.fn().mockResolvedValueOnce(true), }) @@ -1602,9 +1720,10 @@ describe("GitFileSystemService", () => { revparse: jest.fn().mockResolvedValueOnce(BRANCH_REF), }) MockSimpleGit.cwd.mockReturnValueOnce({ - add: jest.fn().mockReturnValueOnce({ - commit: jest.fn().mockResolvedValueOnce({ commit: "fake-new-hash" }), - }), + add: jest.fn().mockResolvedValueOnce(undefined), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + commit: jest.fn().mockResolvedValueOnce({ commit: "fake-new-hash" }), }) const actual = await GitFileSystemService.moveFiles( @@ -1617,10 +1736,6 @@ describe("GitFileSystemService", () => { ) expect(actual._unsafeUnwrap()).toEqual("fake-new-hash") - expect(mockGitMv).toHaveBeenCalledWith( - "another-fake-file", - "fake-new-dir/another-fake-file" - ) }) it("should rollback changes if an error occurred when committing", async () => { @@ -1635,11 +1750,6 @@ describe("GitFileSystemService", () => { }, }), }) - // Note: This will not cause the actual file to be renamed - const mockGitMv = jest.fn().mockResolvedValueOnce(undefined) - MockSimpleGit.cwd.mockReturnValueOnce({ - mv: mockGitMv, - }) MockSimpleGit.cwd.mockReturnValueOnce({ checkIsRepo: jest.fn().mockResolvedValueOnce(true), }) @@ -1654,44 +1764,10 @@ describe("GitFileSystemService", () => { revparse: jest.fn().mockResolvedValueOnce(BRANCH_REF), }) MockSimpleGit.cwd.mockReturnValueOnce({ - add: jest.fn().mockReturnValueOnce({ - commit: jest.fn().mockRejectedValueOnce(new GitError()), - }), - }) - MockSimpleGit.cwd.mockReturnValueOnce({ - reset: jest.fn().mockReturnValueOnce({ - clean: jest.fn().mockResolvedValueOnce(undefined), - }), - }) - const spyRollback = jest.spyOn(GitFileSystemService, "rollback") - - const actual = await GitFileSystemService.moveFiles( - "fake-repo", - "", - "fake-dir", - "fake-user-id", - ["another-fake-file"], - "fake-message" - ) - - expect(actual._unsafeUnwrapErr()).toBeInstanceOf(GitFileSystemError) - expect(spyRollback).toHaveBeenCalledWith("fake-repo", "fake-hash") - }) - - it("should rollback changes if an error occurred when moving files", async () => { - MockSimpleGit.cwd.mockReturnValueOnce({ - log: jest.fn().mockResolvedValueOnce({ - latest: { - hash: "fake-hash", - date: "fake-date", - message: "fake-message", - author_name: "fake-author", - author_email: "fake-email", - }, - }), + add: jest.fn().mockResolvedValueOnce(undefined), }) MockSimpleGit.cwd.mockReturnValueOnce({ - mv: jest.fn().mockRejectedValueOnce(new GitError()), + commit: jest.fn().mockRejectedValueOnce(new GitError()), }) MockSimpleGit.cwd.mockReturnValueOnce({ reset: jest.fn().mockReturnValueOnce({ @@ -1818,12 +1894,50 @@ describe("GitFileSystemService", () => { ) expect(actual._unsafeUnwrap()).toStrictEqual(expected) + expect(MockSimpleGit.cwd).toHaveBeenCalledTimes(1) + }) + + it("should retry with origin prefix if simple-git throws error the first time", async () => { + MockSimpleGit.cwd.mockReturnValueOnce({ + log: jest.fn().mockRejectedValueOnce(new GitError()), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + log: jest.fn().mockResolvedValueOnce({ + latest: { + hash: "fake-hash", + date: "fake-date", + message: "fake-message", + author_name: "fake-author", + author_email: "fake-email", + }, + }), + }) + const expected: GitHubCommitData = { + sha: "fake-hash", + message: "fake-message", + author: { + date: "fake-date", + name: "fake-author", + email: "fake-email", + }, + } + + const actual = await GitFileSystemService.getLatestCommitOfBranch( + "fake-repo-2", + "master" + ) + + expect(actual._unsafeUnwrap()).toStrictEqual(expected) + expect(MockSimpleGit.cwd).toHaveBeenCalledTimes(2) }) it("should throw error when simple-git throws error", async () => { MockSimpleGit.cwd.mockReturnValueOnce({ log: jest.fn().mockRejectedValueOnce(new GitError()), }) + MockSimpleGit.cwd.mockReturnValueOnce({ + log: jest.fn().mockRejectedValueOnce(new GitError()), + }) const result = await GitFileSystemService.getLatestCommitOfBranch( "fake-repo-2", @@ -1912,11 +2026,12 @@ describe("GitFileSystemService", () => { // commit MockSimpleGit.cwd.mockReturnValueOnce({ - add: jest.fn().mockReturnValueOnce({ - commit: jest - .fn() - .mockResolvedValueOnce({ commit: "fake-new-hash" }), - }), + add: jest.fn().mockResolvedValueOnce(undefined), + }) + + // commit + MockSimpleGit.cwd.mockReturnValueOnce({ + commit: jest.fn().mockResolvedValueOnce({ commit: "fake-new-hash" }), }) const actual = await GitFileSystemService.delete( @@ -2041,11 +2156,12 @@ describe("GitFileSystemService", () => { // commit MockSimpleGit.cwd.mockReturnValueOnce({ - add: jest.fn().mockReturnValueOnce({ - commit: jest - .fn() - .mockResolvedValueOnce({ commit: "fake-new-hash" }), - }), + add: jest.fn().mockResolvedValueOnce(undefined), + }) + + // commit + MockSimpleGit.cwd.mockReturnValueOnce({ + commit: jest.fn().mockResolvedValueOnce({ commit: "fake-new-hash" }), }) const actual = await GitFileSystemService.delete( @@ -2055,7 +2171,6 @@ describe("GitFileSystemService", () => { "fake-user-id", true ) - console.log(`ACTUAL`, actual) expect(actual._unsafeUnwrap()).toEqual("fake-new-hash") }) @@ -2131,11 +2246,12 @@ describe("GitFileSystemService", () => { MockSimpleGit.cwd.mockReturnValueOnce({ revparse: jest.fn().mockResolvedValueOnce(BRANCH_REF), }) + MockSimpleGit.cwd.mockReturnValueOnce({ + add: jest.fn().mockResolvedValueOnce(undefined), + }) MockSimpleGit.cwd.mockReturnValueOnce({ - add: jest.fn().mockReturnValueOnce({ - commit: jest.fn().mockRejectedValueOnce(new GitError()), - }), + commit: jest.fn().mockRejectedValueOnce(new GitError()), }) MockSimpleGit.cwd.mockReturnValueOnce({ reset: jest.fn().mockReturnValueOnce({