Skip to content

Commit

Permalink
fix(clone): correct fork and clone logic
Browse files Browse the repository at this point in the history 
This commit fixes the logic for forking and cloning repositories.

Also changed the testing logic to include the type of call for GH.
This is because the testing was just on the argument and was missing
context when running the unit test.
  • Loading branch information
@sledigabel
sledigabel committed Oct 23, 2024
1 parent e83236d commit 3822206
Show file tree
Hide file tree
Showing 5 changed files with 125 additions and 80 deletions.
12 changes: 6 additions & 6 deletions cmd/clone/clone.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,10 @@ func run(c *cobra.Command, _ []string) {
}
}

if !fork {
cloneActivity = logger.StartActivity("Cloning %s into %s/%s", repo.FullRepoName, orgDirPath, repo.RepoName)
} else {
if fork {
cloneActivity = logger.StartActivity("Forking and cloning %s into %s/%s", repo.FullRepoName, orgDirPath, repo.RepoName)
} else {
cloneActivity = logger.StartActivity("Cloning %s into %s/%s", repo.FullRepoName, orgDirPath, repo.RepoName)
}

err := os.MkdirAll(orgDirPath, os.ModeDir|0o755)
Expand All @@ -106,10 +106,10 @@ func run(c *cobra.Command, _ []string) {
continue
}

if !fork {
err = gh.Clone(cloneActivity.Writer(), orgDirPath, repo.FullRepoName)
} else {
if fork {
err = gh.ForkAndClone(cloneActivity.Writer(), orgDirPath, repo.FullRepoName)
} else {
err = gh.Clone(cloneActivity.Writer(), orgDirPath, repo.FullRepoName)
}

if err != nil {
Expand Down
122 changes: 84 additions & 38 deletions cmd/clone/clone_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,10 @@ func TestItLogsCloneErrorsButContinuesToTryAll(t *testing.T) {
assert.Contains(t, out, "2 repos errored")

fakeGitHub.AssertCalledWith(t, [][]string{
{"work/org/repo1"},
{"work/org", "org/repo1"},
{"work/org/repo2"},
{"work/org", "org/repo2"},
{"user_can_push", "work/org/repo1"},
{"clone", "work/org", "org/repo1"},
{"user_can_push", "work/org/repo2"},
{"clone", "work/org", "org/repo2"},
})
fakeGit.AssertCalledWith(t, [][]string{})
}
Expand All @@ -103,8 +103,8 @@ func TestItLogsForkAndCloneErrorsButContinuesToTryAll(t *testing.T) {
assert.Contains(t, out, "2 repos errored")

fakeGitHub.AssertCalledWith(t, [][]string{
{"work/org", "org/repo1"},
{"work/org", "org/repo2"},
{"fork_and_clone", "work/org", "org/repo1"},
{"fork_and_clone", "work/org", "org/repo2"},
})
fakeGit.AssertCalledWith(t, [][]string{})
}
Expand All @@ -124,8 +124,8 @@ func TestItLogsCheckoutErrorsButContinuesToTryAll(t *testing.T) {
assert.Contains(t, out, "2 repos errored")

fakeGitHub.AssertCalledWith(t, [][]string{
{"work/org", "org/repo1"},
{"work/org", "org/repo2"},
{"fork_and_clone", "work/org", "org/repo1"},
{"fork_and_clone", "work/org", "org/repo2"},
})
fakeGit.AssertCalledWith(t, [][]string{
{"checkout", "work/org/repo1", testsupport.Pwd()},
Expand All @@ -148,10 +148,10 @@ func TestItPullsFromUpstreamWhenCloningWithFork(t *testing.T) {
assert.Contains(t, out, "turbolift clone completed (2 repos cloned, 0 repos skipped)")

fakeGitHub.AssertCalledWith(t, [][]string{
{"work/org1", "org1/repo1"},
{"work/org1/repo1", "org1/repo1"},
{"work/org2", "org2/repo2"},
{"work/org2/repo2", "org2/repo2"},
{"fork_and_clone", "work/org1", "org1/repo1"},
{"get_default_branch", "work/org1/repo1", "org1/repo1"},
{"fork_and_clone", "work/org2", "org2/repo2"},
{"get_default_branch", "work/org2/repo2", "org2/repo2"},
})
fakeGit.AssertCalledWith(t, [][]string{
{"checkout", "work/org1/repo1", testsupport.Pwd()},
Expand Down Expand Up @@ -187,11 +187,12 @@ func TestItDoesNotPullFromUpstreamWhenCloningWithoutFork(t *testing.T) {
assert.Contains(t, out, "turbolift clone completed (2 repos cloned, 0 repos skipped)")

fakeGitHub.AssertCalledWith(t, [][]string{
{"work/org1/repo1"},
{"work/org1", "org1/repo1"},
{"work/org2/repo2"},
{"work/org2", "org2/repo2"},
{"user_can_push", "work/org1/repo1"},
{"clone", "work/org1", "org1/repo1"},
{"user_can_push", "work/org2/repo2"},
{"clone", "work/org2", "org2/repo2"},
})

fakeGit.AssertCalledWith(t, [][]string{
{"checkout", "work/org1/repo1", testsupport.Pwd()},
{"checkout", "work/org2/repo2", testsupport.Pwd()},
Expand All @@ -213,10 +214,10 @@ func TestItLogsDefaultBranchErrorsButContinuesToTryAll(t *testing.T) {
assert.Contains(t, out, "2 repos errored")

fakeGitHub.AssertCalledWith(t, [][]string{
{"work/org1", "org1/repo1"},
{"work/org1/repo1", "org1/repo1"},
{"work/org2", "org2/repo2"},
{"work/org2/repo2", "org2/repo2"},
{"fork_and_clone", "work/org1", "org1/repo1"},
{"get_default_branch", "work/org1/repo1", "org1/repo1"},
{"fork_and_clone", "work/org2", "org2/repo2"},
{"get_default_branch", "work/org2/repo2", "org2/repo2"},
})
fakeGit.AssertCalledWith(t, [][]string{
{"checkout", "work/org1/repo1", testsupport.Pwd()},
Expand All @@ -241,10 +242,10 @@ func TestItLogsPullErrorsButContinuesToTryAll(t *testing.T) {
assert.Contains(t, out, "2 repos errored")

fakeGitHub.AssertCalledWith(t, [][]string{
{"work/org1", "org1/repo1"},
{"work/org1/repo1", "org1/repo1"},
{"work/org2", "org2/repo2"},
{"work/org2/repo2", "org2/repo2"},
{"fork_and_clone", "work/org1", "org1/repo1"},
{"get_default_branch", "work/org1/repo1", "org1/repo1"},
{"fork_and_clone", "work/org2", "org2/repo2"},
{"get_default_branch", "work/org2/repo2", "org2/repo2"},
})
fakeGit.AssertCalledWith(t, [][]string{
{"checkout", "work/org1/repo1", testsupport.Pwd()},
Expand All @@ -268,10 +269,10 @@ func TestItClonesReposFoundInReposFile(t *testing.T) {
assert.Contains(t, out, "turbolift clone completed (2 repos cloned, 0 repos skipped)")

fakeGitHub.AssertCalledWith(t, [][]string{
{"work/org/repo1"},
{"work/org", "org/repo1"},
{"work/org/repo2"},
{"work/org", "org/repo2"},
{"user_can_push", "work/org/repo1"},
{"clone", "work/org", "org/repo1"},
{"user_can_push", "work/org/repo2"},
{"clone", "work/org", "org/repo2"},
})
fakeGit.AssertCalledWith(t, [][]string{
{"checkout", "work/org/repo1", testsupport.Pwd()},
Expand All @@ -291,10 +292,10 @@ func TestItClonesReposInMultipleOrgs(t *testing.T) {
assert.NoError(t, err)

fakeGitHub.AssertCalledWith(t, [][]string{
{"work/orgA/repo1"},
{"work/orgA", "orgA/repo1"},
{"work/orgB/repo2"},
{"work/orgB", "orgB/repo2"},
{"user_can_push", "work/orgA/repo1"},
{"clone", "work/orgA", "orgA/repo1"},
{"user_can_push", "work/orgB/repo2"},
{"clone", "work/orgB", "orgB/repo2"},
})
fakeGit.AssertCalledWith(t, [][]string{
{"checkout", "work/orgA/repo1", testsupport.Pwd()},
Expand All @@ -314,10 +315,10 @@ func TestItClonesReposFromOtherHosts(t *testing.T) {
assert.NoError(t, err)

fakeGitHub.AssertCalledWith(t, [][]string{
{"work/orgA/repo1"},
{"work/orgA", "mygitserver.com/orgA/repo1"},
{"work/orgB/repo2"},
{"work/orgB", "orgB/repo2"},
{"user_can_push", "work/orgA/repo1"},
{"clone", "work/orgA", "mygitserver.com/orgA/repo1"},
{"user_can_push", "work/orgB/repo2"},
{"clone", "work/orgB", "orgB/repo2"},
})
fakeGit.AssertCalledWith(t, [][]string{
{"checkout", "work/orgA/repo1", testsupport.Pwd()},
Expand All @@ -339,15 +340,60 @@ func TestItSkipsCloningIfAWorkingCopyAlreadyExists(t *testing.T) {
assert.Contains(t, out, "Forking and cloning org/repo1")

fakeGitHub.AssertCalledWith(t, [][]string{
{"work/org", "org/repo2"},
{"work/org/repo2", "org/repo2"},
{"fork_and_clone", "work/org", "org/repo2"},
{"get_default_branch", "work/org/repo2", "org/repo2"},
})
fakeGit.AssertCalledWith(t, [][]string{
{"checkout", "work/org/repo2", testsupport.Pwd()},
{"pull", "--ff-only", "work/org/repo2", "upstream", "main"},
})
}

func TestItForksIfUserHasNoPushPermission(t *testing.T) {
fakeGitHub := github.NewFakeGitHub(func(command github.Command, args []string) (bool, error) {
switch command {
case github.IsPushable:
return false, nil
case github.ForkAndClone:
return true, nil
case github.GetDefaultBranchName:
return true, nil
default:
return false, errors.New("unexpected command")
}
}, func(workingDir string) (interface{}, error) {
return nil, errors.New("unexpected call")
})

// fakeGitHub := github.NewAlwaysSucceedsFakeGitHub()
gh = fakeGitHub
fakeGit := git.NewAlwaysSucceedsFakeGit()
g = fakeGit

testsupport.PrepareTempCampaign(false, "org/repo1", "org/repo2")

out, err := runCloneCommand()
assert.NoError(t, err)
assert.Contains(t, out, "Forking and cloning org/repo1")
assert.Contains(t, out, "Forking and cloning org/repo2")
assert.Contains(t, out, "turbolift clone completed (2 repos cloned, 0 repos skipped)")

fakeGitHub.AssertCalledWith(t, [][]string{
{"user_can_push", "work/org/repo1"},
{"fork_and_clone", "work/org", "org/repo1"},
{"get_default_branch", "work/org/repo1", "org/repo1"},
{"user_can_push", "work/org/repo2"},
{"fork_and_clone", "work/org", "org/repo2"},
{"get_default_branch", "work/org/repo2", "org/repo2"},
})
fakeGit.AssertCalledWith(t, [][]string{
{"checkout", "work/org/repo1", testsupport.Pwd()},
{"pull", "--ff-only", "work/org/repo1", "upstream", "main"},
{"checkout", "work/org/repo2", testsupport.Pwd()},
{"pull", "--ff-only", "work/org/repo2", "upstream", "main"},
})
}

func runCloneCommand() (string, error) {
cmd := NewCloneCmd()
outBuffer := bytes.NewBufferString("")
Expand Down
29 changes: 14 additions & 15 deletions cmd/create_prs/create_prs_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,14 @@ package create_prs

import (
"bytes"
"testing"

"github.com/stretchr/testify/assert"

"github.com/skyscanner/turbolift/internal/git"
"github.com/skyscanner/turbolift/internal/github"
"github.com/skyscanner/turbolift/internal/prompt"
"github.com/skyscanner/turbolift/internal/testsupport"
"github.com/stretchr/testify/assert"
"testing"
)

func TestItWarnsIfDescriptionFileTemplateIsUnchanged(t *testing.T) {
Expand Down Expand Up @@ -127,8 +129,8 @@ func TestItLogsCreatePrErrorsButContinuesToTryAll(t *testing.T) {
assert.Contains(t, out, "2 errored")

fakeGitHub.AssertCalledWith(t, [][]string{
{"work/org/repo1", "PR title"},
{"work/org/repo2", "PR title"},
{"create_pull_request", "work/org/repo1", "PR title"},
{"create_pull_request", "work/org/repo2", "PR title"},
})
}

Expand All @@ -148,8 +150,8 @@ func TestItLogsCreatePrSkippedButContinuesToTryAll(t *testing.T) {
assert.Contains(t, out, "0 OK, 2 skipped")

fakeGitHub.AssertCalledWith(t, [][]string{
{"work/org/repo1", "PR title"},
{"work/org/repo2", "PR title"},
{"create_pull_request", "work/org/repo1", "PR title"},
{"create_pull_request", "work/org/repo2", "PR title"},
})
}

Expand All @@ -167,8 +169,8 @@ func TestItLogsCreatePrsSucceeds(t *testing.T) {
assert.Contains(t, out, "2 OK, 0 skipped")

fakeGitHub.AssertCalledWith(t, [][]string{
{"work/org/repo1", "PR title"},
{"work/org/repo2", "PR title"},
{"create_pull_request", "work/org/repo1", "PR title"},
{"create_pull_request", "work/org/repo2", "PR title"},
})
}

Expand All @@ -188,8 +190,8 @@ func TestItLogsCreateDraftPr(t *testing.T) {
assert.Contains(t, out, "2 OK, 0 skipped")

fakeGitHub.AssertCalledWith(t, [][]string{
{"work/org/repo1", "PR title"},
{"work/org/repo2", "PR title"},
{"create_pull_request", "work/org/repo1", "PR title"},
{"create_pull_request", "work/org/repo2", "PR title"},
})
}

Expand All @@ -213,8 +215,8 @@ func TestItCreatesPrsFromAlternativeDescriptionFile(t *testing.T) {
assert.Contains(t, out, "2 OK, 0 skipped")

fakeGitHub.AssertCalledWith(t, [][]string{
{"work/org/repo1", "custom PR title"},
{"work/org/repo2", "custom PR title"},
{"create_pull_request", "work/org/repo1", "custom PR title"},
{"create_pull_request", "work/org/repo2", "custom PR title"},
})
}

Expand All @@ -223,7 +225,6 @@ func runCommand() (string, error) {
outBuffer := bytes.NewBufferString("")
cmd.SetOut(outBuffer)
err := cmd.Execute()

if err != nil {
return outBuffer.String(), err
}
Expand All @@ -236,7 +237,6 @@ func runCommandWithAlternativeDescriptionFile(fileName string) (string, error) {
outBuffer := bytes.NewBufferString("")
cmd.SetOut(outBuffer)
err := cmd.Execute()

if err != nil {
return outBuffer.String(), err
}
Expand All @@ -249,7 +249,6 @@ func runCommandDraft() (string, error) {
outBuffer := bytes.NewBufferString("")
cmd.SetOut(outBuffer)
err := cmd.Execute()

if err != nil {
return outBuffer.String(), err
}
Expand Down
24 changes: 12 additions & 12 deletions cmd/updateprs/updateprs_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ func TestItLogsClosePrErrorsButContinuesToTryAll(t *testing.T) {
assert.Contains(t, out, "2 errored")

fakeGitHub.AssertCalledWith(t, [][]string{
{"work/org/repo1", filepath.Base(tempDir)},
{"work/org/repo2", filepath.Base(tempDir)},
{"close_pull_request", "work/org/repo1", filepath.Base(tempDir)},
{"close_pull_request", "work/org/repo2", filepath.Base(tempDir)},
})
}

Expand All @@ -46,8 +46,8 @@ func TestItClosesPrsSuccessfully(t *testing.T) {
assert.NotContains(t, out, "error")

fakeGitHub.AssertCalledWith(t, [][]string{
{"work/org/repo1", filepath.Base(tempDir)},
{"work/org/repo2", filepath.Base(tempDir)},
{"close_pull_request", "work/org/repo1", filepath.Base(tempDir)},
{"close_pull_request", "work/org/repo2", filepath.Base(tempDir)},
})
}

Expand All @@ -65,8 +65,8 @@ func TestNoPRFound(t *testing.T) {
assert.Contains(t, out, "0 OK, 2 skipped")

fakeGitHub.AssertCalledWith(t, [][]string{
{"work/org/repo1", filepath.Base(tempDir)},
{"work/org/repo2", filepath.Base(tempDir)},
{"close_pull_request", "work/org/repo1", filepath.Base(tempDir)},
{"close_pull_request", "work/org/repo2", filepath.Base(tempDir)},
})
}

Expand Down Expand Up @@ -102,8 +102,8 @@ func TestItLogsUpdateDescriptionErrorsButContinuesToTryAll(t *testing.T) {
assert.Contains(t, out, "2 errored")

fakeGitHub.AssertCalledWith(t, [][]string{
{"work/org/repo1", "PR title", "PR body"},
{"work/org/repo2", "PR title", "PR body"},
{"update_pr_description", "work/org/repo1", "PR title", "PR body"},
{"update_pr_description", "work/org/repo2", "PR title", "PR body"},
})
}

Expand All @@ -122,8 +122,8 @@ func TestItUpdatesDescriptionsSuccessfully(t *testing.T) {
assert.Contains(t, out, "2 OK, 0 skipped")

fakeGitHub.AssertCalledWith(t, [][]string{
{"work/org/repo1", "Updated PR title", "Updated PR body"},
{"work/org/repo2", "Updated PR title", "Updated PR body"},
{"update_pr_description", "work/org/repo1", "Updated PR title", "Updated PR body"},
{"update_pr_description", "work/org/repo2", "Updated PR title", "Updated PR body"},
})
}

Expand All @@ -142,8 +142,8 @@ func TestItUpdatesDescriptionsFromAlternativeFile(t *testing.T) {
assert.Contains(t, out, "2 OK, 0 skipped")

fakeGitHub.AssertCalledWith(t, [][]string{
{"work/org/repo1", "custom PR title", "custom PR body"},
{"work/org/repo2", "custom PR title", "custom PR body"},
{"update_pr_description", "work/org/repo1", "custom PR title", "custom PR body"},
{"update_pr_description", "work/org/repo2", "custom PR title", "custom PR body"},
})
}

Expand Down
Loading

0 comments on commit 3822206

Please sign in to comment.