From 1f0b797ddc139c3241c9b9694f0666a28ab41d80 Mon Sep 17 00:00:00 2001 From: zeripath Date: Tue, 28 Apr 2020 09:32:23 +0100 Subject: [PATCH] Make the PushCreate test declarative (#11229) Reduce the code duplication in the PushCreate test and switch to a declarative format. * Instead of explicitly creating the repository re-use functions from the other declarative tests and add comments * Ensure that the test repository is deleted at the end of test * Slightly reorder the sub-tests Also reduce the code duplication in MergeFork and add some comments there too and make doGitCloneFail be self-contained. Signed-off-by: Andrew Thornton art27@cantab.net --- .../git_helper_for_declarative_test.go | 9 +- integrations/git_test.go | 155 +++++++----------- integrations/ssh_key_test.go | 10 +- 3 files changed, 72 insertions(+), 102 deletions(-) diff --git a/integrations/git_helper_for_declarative_test.go b/integrations/git_helper_for_declarative_test.go index 5838b9e51..13b3e92c1 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 69253d397..7dd0bc056 100644 --- a/integrations/git_test.go +++ b/integrations/git_test.go @@ -433,133 +433,104 @@ 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()) } } 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) + defer os.RemoveAll(tmpDir) - err = git.InitRepository(tmpDir, false) - 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 = os.Create(filepath.Join(tmpDir, "test.txt")) - assert.NoError(t, err) - - 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) - - _, err = git.NewCommand("remote", "add", "origin", u.String()).RunInDir(tmpDir) - assert.NoError(t, err) - - invalidCtx := ctx - invalidCtx.Reponame = fmt.Sprintf("invalid/repo-tmp-push-create-%s", u.Scheme) - u.Path = invalidCtx.GitPath() - - _, err = git.NewCommand("remote", "add", "invalid", u.String()).RunInDir(tmpDir) - assert.NoError(t, err) - - // Push to create disabled + // Disable "Push To Create" and attempt to push setting.Repository.EnablePushCreateUser = false - _, err = git.NewCommand("push", "origin", "master").RunInDir(tmpDir) - assert.Error(t, err) + t.Run("FailToPushAndCreateTestRepository", doGitPushTestRepositoryFail(tmpDir, "origin", "master")) - // Push to create enabled + // Enable "Push To Create" setting.Repository.EnablePushCreateUser = true - // Invalid repo - _, err = git.NewCommand("push", "invalid", "master").RunInDir(tmpDir) - assert.Error(t, err) + // Assert that cloning from a non-existent repository does not create it and that it definitely wasn't create above + t.Run("FailToCloneFromNonExistentRepository", doGitCloneFail(u)) - // Valid repo - _, err = git.NewCommand("push", "origin", "master").RunInDir(tmpDir) - assert.NoError(t, err) + // Then "Push To Create"x + t.Run("SuccessfullyPushAndCreateTestRepository", doGitPushTestRepository(tmpDir, "origin", "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) 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) + u.Path = invalidCtx.GitPath() + t.Run("AddInvalidRemote", doGitAddRemote(tmpDir, "invalid", u)) + + // Fail to "Push To Create" the invalid + t.Run("FailToPushAndCreateInvalidTestRepository", doGitPushTestRepositoryFail(tmpDir, "invalid", "master")) } } diff --git a/integrations/ssh_key_test.go b/integrations/ssh_key_test.go index 944d2f6be..d445c7f9e 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)) }) }) }