From afde9e42c75d21d85ff4346f0a83f24458c2f2cf Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 27 Apr 2020 16:03:04 +0100 Subject: [PATCH 1/6] Make the PushCreate test declarative Reduce the code duplication in the PushCreate test and switch to a declarative format. Signed-off-by: Andrew Thornton --- integrations/git_test.go | 61 +++++++++++++--------------------------- 1 file changed, 19 insertions(+), 42 deletions(-) diff --git a/integrations/git_test.go b/integrations/git_test.go index 69253d39781b2..a30dc191c6755 100644 --- a/integrations/git_test.go +++ b/integrations/git_test.go @@ -499,63 +499,40 @@ func doMergeFork(ctx, baseCtx APITestContext, baseBranch, headBranch string) fun func doPushCreate(ctx APITestContext, u *url.URL) func(t *testing.T) { return func(t *testing.T) { defer PrintCurrentTest(t)() + + // create a context for a currently non-existent repository ctx.Reponame = fmt.Sprintf("repo-tmp-push-create-%s", u.Scheme) u.Path = ctx.GitPath() + // Create a temporary directory tmpDir, err := ioutil.TempDir("", ctx.Reponame) assert.NoError(t, err) - err = git.InitRepository(tmpDir, false) - assert.NoError(t, err) + // Assert that cloning from a non-existent repository does not create it + t.Run("FailToCloneFromNonExistentRepository", doGitCloneFail(tmpDir, u)) - _, err = os.Create(filepath.Join(tmpDir, "test.txt")) - assert.NoError(t, err) + // Now create local repository to push as our test and set its origin + t.Run("InitTestRepository", doGitInitTestRepository(tmpDir)) + t.Run("AddRemote", doGitAddRemote(tmpDir, "origin", u)) - err = git.AddChanges(tmpDir, true) - assert.NoError(t, err) - - err = git.CommitChanges(tmpDir, git.CommitChangesOptions{ - Committer: &git.Signature{ - Email: "user2@example.com", - Name: "User Two", - When: time.Now(), - }, - Author: &git.Signature{ - Email: "user2@example.com", - Name: "User Two", - When: time.Now(), - }, - Message: fmt.Sprintf("Testing push create @ %v", time.Now()), - }) - assert.NoError(t, err) + // Disable "Push To Create" and attempt to push + setting.Repository.EnablePushCreateUser = false + t.Run("FailToPushAndCreateTestRepository", doGitPushTestRepositoryFail(tmpDir, "origin", "master")) - _, err = git.NewCommand("remote", "add", "origin", u.String()).RunInDir(tmpDir) - assert.NoError(t, err) + // Enable "Push To Create" and push + setting.Repository.EnablePushCreateUser = true + t.Run("SuccessfullyPushAndCreateTestRepository", doGitPushTestRepository(tmpDir, "origin", "master")) + // Now add a remote that is invalid to "Push To Create" invalidCtx := ctx invalidCtx.Reponame = fmt.Sprintf("invalid/repo-tmp-push-create-%s", u.Scheme) u.Path = invalidCtx.GitPath() + t.Run("AddInvalidRemote", doGitAddRemote(tmpDir, "invalid", u)) - _, err = git.NewCommand("remote", "add", "invalid", u.String()).RunInDir(tmpDir) - assert.NoError(t, err) - - // Push to create disabled - setting.Repository.EnablePushCreateUser = false - _, err = git.NewCommand("push", "origin", "master").RunInDir(tmpDir) - assert.Error(t, err) - - // Push to create enabled - setting.Repository.EnablePushCreateUser = true - - // Invalid repo - _, err = git.NewCommand("push", "invalid", "master").RunInDir(tmpDir) - assert.Error(t, err) - - // Valid repo - _, err = git.NewCommand("push", "origin", "master").RunInDir(tmpDir) - assert.NoError(t, err) + // Fail to "Push To Create" the invalid + t.Run("FailToPushAndCreateInvalidTestRepository", doGitPushTestRepositoryFail(tmpDir, "invalid", "master")) - // Fetch repo from database + // Finally, fetch repo from database and ensure the correct repository has been created repo, err := models.GetRepositoryByOwnerAndName(ctx.Username, ctx.Reponame) assert.NoError(t, err) assert.False(t, repo.IsEmpty) From 9fde16e53d99222fb1a52adb47bd42a4dede4cf9 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 27 Apr 2020 16:08:36 +0100 Subject: [PATCH 2/6] Delete the test repo at the end Signed-off-by: Andrew Thornton --- integrations/git_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/integrations/git_test.go b/integrations/git_test.go index a30dc191c6755..8842e884de951 100644 --- a/integrations/git_test.go +++ b/integrations/git_test.go @@ -507,6 +507,7 @@ func doPushCreate(ctx APITestContext, u *url.URL) func(t *testing.T) { // Create a temporary directory tmpDir, err := ioutil.TempDir("", ctx.Reponame) assert.NoError(t, err) + defer os.RemoveAll(tmpDir) // Assert that cloning from a non-existent repository does not create it t.Run("FailToCloneFromNonExistentRepository", doGitCloneFail(tmpDir, u)) From 9274b147515e2ec525afe8cc9155c7e3d1a53d5e Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 27 Apr 2020 16:16:06 +0100 Subject: [PATCH 3/6] slight reorder Signed-off-by: Andrew Thornton --- integrations/git_test.go | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/integrations/git_test.go b/integrations/git_test.go index 8842e884de951..c4dc2dda9ed3e 100644 --- a/integrations/git_test.go +++ b/integrations/git_test.go @@ -509,9 +509,6 @@ func doPushCreate(ctx APITestContext, u *url.URL) func(t *testing.T) { assert.NoError(t, err) defer os.RemoveAll(tmpDir) - // Assert that cloning from a non-existent repository does not create it - t.Run("FailToCloneFromNonExistentRepository", doGitCloneFail(tmpDir, u)) - // Now create local repository to push as our test and set its origin t.Run("InitTestRepository", doGitInitTestRepository(tmpDir)) t.Run("AddRemote", doGitAddRemote(tmpDir, "origin", u)) @@ -520,10 +517,21 @@ func doPushCreate(ctx APITestContext, u *url.URL) func(t *testing.T) { setting.Repository.EnablePushCreateUser = false t.Run("FailToPushAndCreateTestRepository", doGitPushTestRepositoryFail(tmpDir, "origin", "master")) - // Enable "Push To Create" and push + // Enable "Push To Create" setting.Repository.EnablePushCreateUser = true + + // Assert that cloning from a non-existent repository does not create it and that it definitely wasn't create above + t.Run("FailToCloneFromNonExistentRepository", doGitCloneFail(tmpDir, u)) + + // Then "Push To Create" t.Run("SuccessfullyPushAndCreateTestRepository", doGitPushTestRepository(tmpDir, "origin", "master")) + // Finally, fetch repo from database and ensure the correct repository has been created + repo, err := models.GetRepositoryByOwnerAndName(ctx.Username, ctx.Reponame) + assert.NoError(t, err) + assert.False(t, repo.IsEmpty) + assert.True(t, repo.IsPrivate) + // Now add a remote that is invalid to "Push To Create" invalidCtx := ctx invalidCtx.Reponame = fmt.Sprintf("invalid/repo-tmp-push-create-%s", u.Scheme) @@ -532,12 +540,6 @@ func doPushCreate(ctx APITestContext, u *url.URL) func(t *testing.T) { // Fail to "Push To Create" the invalid t.Run("FailToPushAndCreateInvalidTestRepository", doGitPushTestRepositoryFail(tmpDir, "invalid", "master")) - - // Finally, fetch repo from database and ensure the correct repository has been created - repo, err := models.GetRepositoryByOwnerAndName(ctx.Username, ctx.Reponame) - assert.NoError(t, err) - assert.False(t, repo.IsEmpty) - assert.True(t, repo.IsPrivate) } } From 339619d6ab892d8f06e748b1244183e5915756f8 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 27 Apr 2020 16:32:35 +0100 Subject: [PATCH 4/6] Remove duplication in doMergeFork too Signed-off-by: Andrew Thornton --- integrations/git_test.go | 89 ++++++++++++++++++---------------------- 1 file changed, 40 insertions(+), 49 deletions(-) diff --git a/integrations/git_test.go b/integrations/git_test.go index c4dc2dda9ed3e..c3d7cc55a26a0 100644 --- a/integrations/git_test.go +++ b/integrations/git_test.go @@ -433,66 +433,57 @@ func doMergeFork(ctx, baseCtx APITestContext, baseBranch, headBranch string) fun defer PrintCurrentTest(t)() var pr api.PullRequest var err error + + // Create a test pullrequest t.Run("CreatePullRequest", func(t *testing.T) { pr, err = doAPICreatePullRequest(ctx, baseCtx.Username, baseCtx.Reponame, baseBranch, headBranch)(t) assert.NoError(t, err) }) - t.Run("EnsureCanSeePull", func(t *testing.T) { - req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) - ctx.Session.MakeRequest(t, req, http.StatusOK) - req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d/files", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) - ctx.Session.MakeRequest(t, req, http.StatusOK) - req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d/commits", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) - ctx.Session.MakeRequest(t, req, http.StatusOK) - }) + + // Ensure the PR page works + t.Run("EnsureCanSeePull", doEnsureCanSeePull(baseCtx, pr)) + + // Then get the diff string var diffStr string t.Run("GetDiff", func(t *testing.T) { req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d.diff", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) resp := ctx.Session.MakeRequest(t, req, http.StatusOK) diffStr = resp.Body.String() }) + + // Now: Merge the PR & make sure that doesn't break the PR page or change its diff t.Run("MergePR", doAPIMergePullRequest(baseCtx, baseCtx.Username, baseCtx.Reponame, pr.Index)) - t.Run("EnsureCanSeePull", func(t *testing.T) { - req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) - ctx.Session.MakeRequest(t, req, http.StatusOK) - req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d/files", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) - ctx.Session.MakeRequest(t, req, http.StatusOK) - req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d/commits", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) - ctx.Session.MakeRequest(t, req, http.StatusOK) - }) - t.Run("EnsureDiffNoChange", func(t *testing.T) { - req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d.diff", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) - resp := ctx.Session.MakeRequest(t, req, http.StatusOK) - assert.Equal(t, diffStr, resp.Body.String()) - }) + t.Run("EnsureCanSeePull", doEnsureCanSeePull(baseCtx, pr)) + t.Run("EnsureDiffNoChange", doEnsureDiffNoChange(baseCtx, pr, diffStr)) + + // Then: Delete the head branch & make sure that doesn't break the PR page or change its diff t.Run("DeleteHeadBranch", doBranchDelete(baseCtx, baseCtx.Username, baseCtx.Reponame, headBranch)) - t.Run("EnsureCanSeePull", func(t *testing.T) { - req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) - ctx.Session.MakeRequest(t, req, http.StatusOK) - req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d/files", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) - ctx.Session.MakeRequest(t, req, http.StatusOK) - req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d/commits", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) - ctx.Session.MakeRequest(t, req, http.StatusOK) - }) - t.Run("EnsureDiffNoChange", func(t *testing.T) { - req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d.diff", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) - resp := ctx.Session.MakeRequest(t, req, http.StatusOK) - assert.Equal(t, diffStr, resp.Body.String()) - }) - t.Run("DeleteRepository", doAPIDeleteRepository(ctx)) - t.Run("EnsureCanSeePull", func(t *testing.T) { - req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) - ctx.Session.MakeRequest(t, req, http.StatusOK) - req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d/files", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) - ctx.Session.MakeRequest(t, req, http.StatusOK) - req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d/commits", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) - ctx.Session.MakeRequest(t, req, http.StatusOK) - }) - t.Run("EnsureDiffNoChange", func(t *testing.T) { - req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d.diff", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) - resp := ctx.Session.MakeRequest(t, req, http.StatusOK) - assert.Equal(t, diffStr, resp.Body.String()) - }) + t.Run("EnsureCanSeePull", doEnsureCanSeePull(baseCtx, pr)) + t.Run("EnsureDiffNoChange", doEnsureDiffNoChange(baseCtx, pr, diffStr)) + + // Delete the head repository & make sure that doesn't break the PR page or change its diff + t.Run("DeleteHeadRepository", doAPIDeleteRepository(ctx)) + t.Run("EnsureCanSeePull", doEnsureCanSeePull(baseCtx, pr)) + t.Run("EnsureDiffNoChange", doEnsureDiffNoChange(baseCtx, pr, diffStr)) + } +} + +func doEnsureCanSeePull(ctx APITestContext, pr api.PullRequest) func(t *testing.T) { + return func(t *testing.T) { + req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d", url.PathEscape(ctx.Username), url.PathEscape(ctx.Reponame), pr.Index)) + ctx.Session.MakeRequest(t, req, http.StatusOK) + req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d/files", url.PathEscape(ctx.Username), url.PathEscape(ctx.Reponame), pr.Index)) + ctx.Session.MakeRequest(t, req, http.StatusOK) + req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d/commits", url.PathEscape(ctx.Username), url.PathEscape(ctx.Reponame), pr.Index)) + ctx.Session.MakeRequest(t, req, http.StatusOK) + } +} + +func doEnsureDiffNoChange(ctx APITestContext, pr api.PullRequest, diffStr string) func(t *testing.T) { + return func(t *testing.T) { + req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d.diff", url.PathEscape(ctx.Username), url.PathEscape(ctx.Reponame), pr.Index)) + resp := ctx.Session.MakeRequest(t, req, http.StatusOK) + assert.Equal(t, diffStr, resp.Body.String()) } } @@ -523,7 +514,7 @@ func doPushCreate(ctx APITestContext, u *url.URL) func(t *testing.T) { // Assert that cloning from a non-existent repository does not create it and that it definitely wasn't create above t.Run("FailToCloneFromNonExistentRepository", doGitCloneFail(tmpDir, u)) - // Then "Push To Create" + // Then "Push To Create"x t.Run("SuccessfullyPushAndCreateTestRepository", doGitPushTestRepository(tmpDir, "origin", "master")) // Finally, fetch repo from database and ensure the correct repository has been created From 160c1cd72d07f446f526e17198390d0ea1c1079c Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 27 Apr 2020 16:56:52 +0100 Subject: [PATCH 5/6] fix test Signed-off-by: Andrew Thornton --- integrations/git_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/integrations/git_test.go b/integrations/git_test.go index c3d7cc55a26a0..f441f3c4d3150 100644 --- a/integrations/git_test.go +++ b/integrations/git_test.go @@ -512,7 +512,10 @@ func doPushCreate(ctx APITestContext, u *url.URL) func(t *testing.T) { setting.Repository.EnablePushCreateUser = true // Assert that cloning from a non-existent repository does not create it and that it definitely wasn't create above - t.Run("FailToCloneFromNonExistentRepository", doGitCloneFail(tmpDir, u)) + cloneTmpDir, err := ioutil.TempDir("", ctx.Reponame) + assert.NoError(t, err) + defer os.RemoveAll(cloneTmpDir) + t.Run("FailToCloneFromNonExistentRepository", doGitCloneFail(cloneTmpDir, u)) // Then "Push To Create"x t.Run("SuccessfullyPushAndCreateTestRepository", doGitPushTestRepository(tmpDir, "origin", "master")) From 0ab3863f604f29b9f87860dd85ca4cdad8b016c4 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 27 Apr 2020 17:08:24 +0100 Subject: [PATCH 6/6] create own tmp dir for doGitCloneFail - to reduce gotchas with its use Signed-off-by: Andrew Thornton --- integrations/git_helper_for_declarative_test.go | 9 ++++++--- integrations/git_test.go | 5 +---- integrations/ssh_key_test.go | 10 +++------- 3 files changed, 10 insertions(+), 14 deletions(-) diff --git a/integrations/git_helper_for_declarative_test.go b/integrations/git_helper_for_declarative_test.go index 5838b9e512156..13b3e92c14b3f 100644 --- a/integrations/git_helper_for_declarative_test.go +++ b/integrations/git_helper_for_declarative_test.go @@ -115,10 +115,13 @@ func doGitClone(dstLocalPath string, u *url.URL) func(*testing.T) { } } -func doGitCloneFail(dstLocalPath string, u *url.URL) func(*testing.T) { +func doGitCloneFail(u *url.URL) func(*testing.T) { return func(t *testing.T) { - assert.Error(t, git.Clone(u.String(), dstLocalPath, git.CloneRepoOptions{})) - assert.False(t, com.IsExist(filepath.Join(dstLocalPath, "README.md"))) + tmpDir, err := ioutil.TempDir("", "doGitCloneFail") + assert.NoError(t, err) + defer os.RemoveAll(tmpDir) + assert.Error(t, git.Clone(u.String(), tmpDir, git.CloneRepoOptions{})) + assert.False(t, com.IsExist(filepath.Join(tmpDir, "README.md"))) } } diff --git a/integrations/git_test.go b/integrations/git_test.go index f441f3c4d3150..7dd0bc056f226 100644 --- a/integrations/git_test.go +++ b/integrations/git_test.go @@ -512,10 +512,7 @@ func doPushCreate(ctx APITestContext, u *url.URL) func(t *testing.T) { setting.Repository.EnablePushCreateUser = true // Assert that cloning from a non-existent repository does not create it and that it definitely wasn't create above - cloneTmpDir, err := ioutil.TempDir("", ctx.Reponame) - assert.NoError(t, err) - defer os.RemoveAll(cloneTmpDir) - t.Run("FailToCloneFromNonExistentRepository", doGitCloneFail(cloneTmpDir, u)) + t.Run("FailToCloneFromNonExistentRepository", doGitCloneFail(u)) // Then "Push To Create"x t.Run("SuccessfullyPushAndCreateTestRepository", doGitPushTestRepository(tmpDir, "origin", "master")) diff --git a/integrations/ssh_key_test.go b/integrations/ssh_key_test.go index 944d2f6bed52b..d445c7f9e5595 100644 --- a/integrations/ssh_key_test.go +++ b/integrations/ssh_key_test.go @@ -113,7 +113,7 @@ func testKeyOnlyOneType(t *testing.T, u *url.URL) { sshURL := createSSHUrl(ctx.GitPath(), u) - t.Run("FailToClone", doGitCloneFail(dstPath, sshURL)) + t.Run("FailToClone", doGitCloneFail(sshURL)) t.Run("CreateUserKey", doAPICreateUserKey(ctx, keyname, keyFile, func(t *testing.T, publicKey api.PublicKey) { userKeyPublicKeyID = publicKey.ID @@ -139,7 +139,7 @@ func testKeyOnlyOneType(t *testing.T, u *url.URL) { sshURL := createSSHUrl(ctx.GitPath(), u) - t.Run("FailToClone", doGitCloneFail(dstPath, sshURL)) + t.Run("FailToClone", doGitCloneFail(sshURL)) // Should now be able to add... t.Run("AddReadOnlyDeployKey", doAPICreateDeployKey(ctx, keyname, keyFile, true)) @@ -204,15 +204,11 @@ func testKeyOnlyOneType(t *testing.T, u *url.URL) { }) t.Run("DeleteUserKeyShouldRemoveAbilityToClone", func(t *testing.T) { - dstPath, err := ioutil.TempDir("", ctx.Reponame) - assert.NoError(t, err) - defer os.RemoveAll(dstPath) - sshURL := createSSHUrl(ctx.GitPath(), u) t.Run("DeleteUserKey", doAPIDeleteUserKey(ctx, userKeyPublicKeyID)) - t.Run("FailToClone", doGitCloneFail(dstPath, sshURL)) + t.Run("FailToClone", doGitCloneFail(sshURL)) }) }) }