Add tests for gitutil #587

naxhh · 2020-04-06T18:43:26Z

Fixes #538
Related issue: #536

Provide tests for gitutil as discussed in #538
Because of the nature of EnsureCloned and EnsureUpdated this tests are quite slow compared to the rest of the suite (around 25 seconds) maybe would be good to mark most of them to be avoided in short runs, or mock the Exec calls to avoid doing fetches.

Regarding Exec I see it only used in a test to do a git init && git remote add origin {url} maybe is better to expose a function that does that instead of exposing Exec i didn't perform tests on it since is used everywhere in the rest of the tested code.

Finally I noticed EnsureUpdate does the same as EnsureClone + the update part. Should we keep both exposed?

k8s-ci-robot · 2020-04-06T18:43:37Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: naxhh
To complete the pull request process, please assign corneliusweig
You can assign the PR to them by writing /assign @corneliusweig in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ahmetb · 2020-04-06T19:48:23Z

Regarding Exec I see it only used in a test to do a git init && git remote add origin {url} maybe is better to expose a function that does that instead of exposing Exec i didn't perform tests on it since is used everywhere in the rest of the tested code.

Yeah don't worry about this.

ahmetb · 2020-04-06T19:49:46Z

internal/gitutil/git_test.go

+func assertRepoIsCloned(t *testing.T, repoPath string) {
+	t.Helper()
+
+	ok, err := IsGitCloned(repoPath)


ideally test code shouldn't depend on functionality that's being tested.

in this case, IsGitCloned should have a test.

so please consider actually using ioutil.ReadDir and verify the repoPath has >2 files, one of them being .git/.

Sure. that sounds ok to me. I was doing it this way because i've specific tests to check if IsGitCloned worked

Maybe you can also shell out to git to check if it is a repo. Then we don't even need to assume anything about the internals of git repositories.

Other than that, I think your approach has drawbacks as Ahmet pointed out, but it should be fine. After all, we can assume that IsGitCloned is pretty solid.

ahmetb · 2020-04-06T19:50:17Z

internal/gitutil/git_test.go

+	httpClonePath := tempDir.Path("krew-from-https")
+	localClonePath := tempDir.Path("krew-from-local-path")


do you mind splitting into two tests?

Sure. I kept them together to don't make the process slower. but i'll split them

ahmetb · 2020-04-06T20:03:42Z

internal/gitutil/git_test.go

+	}
+
+	_, err := os.Stat(tempDir.Path("file"))
+


do you mind removing unnecessary empty lines, there are far too many :)

ahmetb · 2020-04-06T20:05:02Z

internal/gitutil/git_test.go

+func TestIsGitClonedWhenPathDoesntExists(t *testing.T) {
+	tempDir, cleanup := testutil.NewTempDir(t)
+	defer cleanup()
+
+	ok, err := IsGitCloned(tempDir.Path("does-not-exist"))
+
+	if err != nil {
+		t.Errorf("expected to finish correctly: %s", err)
+	}
+
+	if ok != false {
+		t.Errorf("expected to return false on a non existing folder")
+	}
+}
+
+func TestIsGitClonedWhenIsFalse(t *testing.T) {
+	tempDir, cleanup := testutil.NewTempDir(t)
+	defer cleanup()
+
+	ok, err := IsGitCloned(tempDir.Root())
+
+	if err != nil {
+		t.Errorf("expected to finish correctly: %s", err)
+	}
+
+	if ok != false {
+		t.Errorf("expected to return false on a folder that's not a git repo")
+	}
+}
+
+func TestIsGitClonedWhenIsTrue(t *testing.T) {
+	tempDir, cleanup := testutil.NewTempDir(t)
+	defer cleanup()
+
+	if err := os.MkdirAll(tempDir.Path(".git"), os.ModePerm); err != nil {
+		t.Fatalf("cannot create directory %q: %s", filepath.Dir(tempDir.Path(".git")), err)
+	}
+
+	ok, err := IsGitCloned(tempDir.Root())
+
+	if err != nil {
+		t.Errorf("expected to finish correctly: %s", err)
+	}
+
+	if ok != true {
+		t.Errorf("expected to return true on a git repo")
+	}
+}


you can actually merge all these into 1 test case very easily. and merge if-if to if-elseif

the two first tests, sure I'll merge.

The last one has a custom mkdirall as part of the setup so I think merging them will make the code less clear? how would you merge this one with the others normally?

ahmetb · 2020-04-06T20:06:37Z

internal/gitutil/git_test.go

+	tempDir, cleanup := testutil.NewTempDir(t)
+	defer cleanup()
+
+	if err := EnsureUpdated("https://github.com/kubernetes-sigs/krew.git", tempDir.Root()); err != nil {


since this test is using the network, it's quite bad for a unit test (which is supposed to run fast).

if possible, we should be using a local directory as the remote repo. for that, we need to init a dir, maybe add a file and make 1 or 2 commits, then clone that dir.

That's why I was asking if I should mock the Exec call. to make them faster and don't depend on git and other stuff.

I can make all tests use a local git repo and only the one about https working to use the url if that sounds ok

Mocking Exec should not be necessary. I think it's fine to clone a local repository (see other comment above).
Also, I don't think that cloning from a remote repo is something that we should test in unit tests.

ahmetb · 2020-04-06T20:07:36Z

thanks for this work, it is SUPER appreciated.
coincidentally today I took a pass on removing git dependency from gitutil (excl. tests) using go-git.v5 and the results look promising. 👍

ahmetb · 2020-04-07T00:20:18Z

Finally I noticed EnsureUpdate does the same as EnsureClone + the update part. Should we keep both exposed?

I think it depends on the usage outside the pkg. If it's not used outside the pkg, you can unexport.

naxhh · 2020-04-07T15:14:15Z

Finally I noticed EnsureUpdate does the same as EnsureClone + the update part. Should we keep both exposed?

I think it depends on the usage outside the pkg. If it's not used outside the pkg, you can unexport.

It's being used so I'll keep them. Today or tomorrow I'll upload the fixes. thanks for your time.

naxhh · 2020-04-07T15:15:28Z

Btw the ci failed on the linting part. but the error doesn't give me a hint if the problem is mine or is unrelated

level=warning msg="[runner] Can't run linter goanalysis_metalinter: deadcode: analysis skipped: errors in package: [/home/runner/work/krew/krew/cmd/krew/main.go:20:2: could not import sigs.k8s.io/krew/cmd/krew/cmd (/home/runner/work/krew/krew/cmd/krew/cmd/search.go:50:21: IndexURI not declared by package constants)]"

In local i didn't see this error at any point

ahmetb · 2020-04-07T16:09:07Z

We changed the const to DefaultIndexUri. Not sure why the compilation hasn’t failed instead.

ahmetb · 2020-04-07T19:37:33Z

@naxhh sorry it appears that a previous patch broke the code. I sent #590 to fix it. I still do not know why that PRs tests have not failed, this one’s currently mysterious.

ahmetb · 2020-04-07T21:49:08Z

Please rebase on top of master now.

corneliusweig

Hey @naxhh, thanks for taking this on. This is much needed and not completely straightforward to set up. I very much like what you test. But I think we need to revisit how it's best done. I left a few comments inline, but one overarching theme is that these tests rely a lot on cloning from a remote repository.
This is not ideal, because as unit tests, they are expected to run very fast, and not have external dependencies. To overcome this, I suggest you add a test tool to set up a git repository and add a util to add trivial commits. This will be much faster than cloning from remote, and also is more self-contained.

corneliusweig · 2020-04-11T20:51:17Z

internal/gitutil/git_test.go

@@ -0,0 +1,213 @@
+// Copyright 2019 The Kubernetes Authors.


Suggested change

// Copyright 2019 The Kubernetes Authors.

// Copyright 2020 The Kubernetes Authors.

😉

corneliusweig · 2020-04-11T20:57:47Z

internal/gitutil/git_test.go

+	if err := os.MkdirAll(tempDir.Path(".git"), os.ModePerm); err != nil {
+		t.Fatalf("cannot create directory %q: %s", filepath.Dir(tempDir.Path(".git")), err)
+	}


Maybe this is nit-picking. But you are setting the test up in a way that tests your (and git's) implementation. Could you come up with a way where you actually create a git repo in that directory? That way, you would test the behaviour of IsGitCloned in a more reliable way.

corneliusweig · 2020-04-11T21:03:58Z

internal/gitutil/git_test.go

+func assertRepoIsCloned(t *testing.T, repoPath string) {
+	t.Helper()
+
+	ok, err := IsGitCloned(repoPath)


Maybe you can also shell out to git to check if it is a repo. Then we don't even need to assume anything about the internals of git repositories.

Other than that, I think your approach has drawbacks as Ahmet pointed out, but it should be fine. After all, we can assume that IsGitCloned is pretty solid.

corneliusweig · 2020-04-11T21:07:29Z

internal/gitutil/git_test.go

+	if err := EnsureCloned("https://github.com/kubernetes-sigs/krew.git", httpClonePath); err != nil {
+		t.Errorf("http clone expected to finish correctly: %s", err)
+	}
+
+	if err := EnsureCloned(httpClonePath, localClonePath); err != nil {
+		t.Errorf("folder clone expected to finish correctly: %s", err)
+	}


IMO one of the two cases is enough. Otherwise we are testing if git accepts HTTP URLs or local file paths.

corneliusweig · 2020-04-11T21:12:17Z

internal/gitutil/git_test.go

+	httpClonePath := tempDir.Path("krew-from-https")
+	localClonePath := tempDir.Path("krew-from-local-path")
+
+	if err := EnsureCloned("https://github.com/kubernetes-sigs/krew.git", httpClonePath); err != nil {


Cloning krew is not a good idea IMO. It is far too large to be cloned for an ordinary unit test.

We should really strive to not have to clone from remote too often. There are basically two ways to approach this:

Create a util similar to

krew/integration_test/testutil_test.go

Line 265 in b2b8a40

func (it *ITest) initializeIndex() {

(or extract it to a common place).

Set up a repo by shelling out to git and clone that repo like localClonePath.

It looks to me like 2) is more straightforward.

(same in other test cases)

corneliusweig · 2020-04-11T21:17:22Z

internal/gitutil/git_test.go

+
+	tempDir.Write("file", []byte("create a file to ensure is not overwriten in the next clone"))
+
+	if err := EnsureCloned("https://github.com/kubernetes-sigs/krew.git", tempDir.Root()); err != nil {


Suggested change

if err := EnsureCloned("https://github.com/kubernetes-sigs/krew.git", tempDir.Root()); err != nil {

if err := EnsureCloned("https://github.com/kubernetes-sigs/krew.git", tempDir.Root()); err == nil {

I would expect that the second invocation provokes an error. Are you sure this is correct?

i'll double check but I think it just returned without an error.

corneliusweig · 2020-04-11T21:21:04Z

internal/gitutil/git_test.go

+		t.Errorf("clone expected to finish correctly: %s", err)
+	}
+
+	tempDir.Write("file", []byte("create a file to ensure is not overwriten in the next clone"))


This is not needed to provoke an error in git. Consider removing it.

corneliusweig · 2020-04-11T21:23:13Z

internal/gitutil/git_test.go

+	tempDir, cleanup := testutil.NewTempDir(t)
+	defer cleanup()
+
+	if err := EnsureCloned("https://github.com/kubernetes-sigs/krew.git", tempDir.Root()); err != nil {


If you continue to use the same repo in all tests, please extract the URL to a package-local constant.

corneliusweig · 2020-04-11T21:24:34Z

internal/gitutil/git_test.go

+	tempDir, cleanup := testutil.NewTempDir(t)
+	defer cleanup()
+
+	if err := EnsureUpdated("https://github.com/kubernetes-sigs/krew.git", tempDir.Root()); err != nil {


Mocking Exec should not be necessary. I think it's fine to clone a local repository (see other comment above).
Also, I don't think that cloning from a remote repo is something that we should test in unit tests.

corneliusweig · 2020-04-11T21:26:00Z

internal/gitutil/git_test.go

+	lastCommitID, err := Exec(tempDir.Root(), "rev-parse", "HEAD")
+
+	if err != nil {


Please remove the blank line. The error check really belongs to the previous line and should be grouped with it. (Same in other places)

ahmetb · 2020-04-29T18:00:03Z

@naxhh Gently pinging, when you have time PTAL at the comments.

ahmetb · 2020-05-06T16:09:04Z

@naxhh Ping again.

naxhh · 2020-05-06T16:52:19Z

Sorry I've been busy changing jobs. I'll try to make some time and finish this

fejta-bot · 2020-08-04T17:49:10Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

corneliusweig · 2020-08-15T21:12:11Z

@naxhh Polite ping. Would be really great to have these tests.

fejta-bot · 2020-09-14T21:51:45Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

fejta-bot · 2020-10-14T22:34:39Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

k8s-ci-robot · 2020-10-14T22:34:47Z

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Add tests for gitutil

7673962

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 6, 2020

k8s-ci-robot requested review from corneliusweig and soltysh April 6, 2020 18:43

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 6, 2020

ahmetb reviewed Apr 6, 2020

View reviewed changes

corneliusweig suggested changes Apr 11, 2020

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 4, 2020

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Sep 14, 2020

k8s-ci-robot closed this Oct 14, 2020

		httpClonePath := tempDir.Path("krew-from-https")
		localClonePath := tempDir.Path("krew-from-local-path")

	// Copyright 2019 The Kubernetes Authors.
	// Copyright 2020 The Kubernetes Authors.


		tempDir.Write("file", []byte("create a file to ensure is not overwriten in the next clone"))

		if err := EnsureCloned("https://github.com/kubernetes-sigs/krew.git", tempDir.Root()); err != nil {

		lastCommitID, err := Exec(tempDir.Root(), "rev-parse", "HEAD")

		if err != nil {

Add tests for gitutil #587

Add tests for gitutil #587

Conversation

naxhh commented Apr 6, 2020

k8s-ci-robot commented Apr 6, 2020

ahmetb commented Apr 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmetb Apr 6, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmetb commented Apr 6, 2020

ahmetb commented Apr 7, 2020

naxhh commented Apr 7, 2020

naxhh commented Apr 7, 2020 • edited

ahmetb commented Apr 7, 2020

ahmetb commented Apr 7, 2020

ahmetb commented Apr 7, 2020

corneliusweig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmetb commented Apr 29, 2020

ahmetb commented May 6, 2020

naxhh commented May 6, 2020

fejta-bot commented Aug 4, 2020

corneliusweig commented Aug 15, 2020

fejta-bot commented Sep 14, 2020

fejta-bot commented Oct 14, 2020

k8s-ci-robot commented Oct 14, 2020

ahmetb Apr 6, 2020 •

edited

naxhh commented Apr 7, 2020 •

edited