Replace the current e2e script with a test suite running e2e tests. #1514

fedepaol · 2019-12-30T13:22:55Z

What is the problem I am trying to address?
Right now the e2e tests are performed by a non trivial bash script, difficult to extend with new tests.
This pr replace it with a go - baked e2e test suite, right now adding the same tests we have in the bash script plus one using a wrong GOPROXY variable.

This should be easier to maintain / extend.

How is the fix applied?

By implementing a new test suite that builds athens and interacts directly with go.

Mention the issue number it fixes or add the details of the changes if it doesn't have a specific issue.

Fixes #1474

ghost · 2020-01-20T21:30:15Z

Assigned myself to remember to try this out and go over it in more detail. But I like it.

fedepaol · 2020-02-05T16:19:40Z

@robjloranger @arschles any chance to give a push to this?

ghost · 2020-02-05T21:26:07Z

I'm so sorry @fedepaol, I missed it again. I've been working too much at my day job. I will do this on the weekend at the latest.

fedepaol · 2020-02-06T08:17:30Z

@robjloranger no problem! I was just going through blocked PRs of mine and I remembered about this. Take your time, and thanks!

ghost · 2020-02-11T17:27:07Z

testing now, finally

ghost

Looks good to me, other than one line ending 😃

ghost · 2020-02-11T19:02:30Z

appveyor.yml

silly newline missing

fedepaol · 2020-02-11T21:42:43Z

Looks good to me, other than one line ending smiley

Thanks for your review 👍

ghost

just waiting for the drone ci to pass

marwan-at-work

Just the one comment, but otherwise nice work :)

marwan-at-work · 2020-02-11T22:54:15Z

.drone.yml

@@ -28,7 +28,7 @@ steps:
  # test
  - ./scripts/check_gofmt.sh
  - go vet ./...
-  - go test -v ./...
+  - go test --tags unittests -v ./...


Just checking: this means that go test ./... will always try to run e2e tests no?
In other words, everytime you want to run the unit tests you have to pass that tag.

If that's true, then it would be more correct to say go test --tags e2e instead . Thoughts?

Good call, if true then defaulting to unit tests and hiding e2e behind a flag would be a better UX.

Good point, and I agree. I will change the build tag to be selective for e2e tests, so the default one is for plain unit tests.

marwan-at-work

Thanks for doing this :) I think the PR can definitely use some tightening up but otherwise awesome work 👍

marwan-at-work · 2020-02-14T05:08:51Z

e2etests/run_athens.go

+	"time"
+)
+
+func buildAthens(goBin string, destPath string, env []string) (string, error) {


nit: goBin, destPath string -- no need to declare the type twice.

marwan-at-work · 2020-02-14T05:09:40Z

e2etests/run_athens.go

+	}
+
+	cmd := exec.Command(goBin, "build", "-o", target, binFolder)
+	cmd.Env = env


I'd add a bytes.Buffer to cmd.Stderr and return it in case of error because then it will explain why go build failed.

marwan-at-work · 2020-02-14T05:10:49Z

e2etests/run_athens.go

+
+func stopAthens() error {
+	cmd := exec.Command("pkill", "athens-proxy")
+	err := cmd.Run()


i'd either just return cmd.Run() or properly populate stdout/stderr

marwan-at-work · 2020-02-14T05:12:21Z

e2etests/run_athens.go

+	cmd := exec.Command(athensBin)
+	cmd.Env = env
+
+	go func() {


why not just go cmd.Run() ?

and if you are trying to run the command in the background, it is more appropriate to use cmd.Start(): https://golang.org/pkg/os/exec/#Cmd.Start

and if you are trying to run the command in the background, it is more appropriate to use cmd.Start(): https://golang.org/pkg/os/exec/#Cmd.Start

Ah, nice!

marwan-at-work · 2020-02-14T05:13:59Z

e2etests/run_athens.go

+	}()
+
+	ticker := time.NewTicker(time.Second)
+	timeout := time.After(2 * time.Minute)


two minutes seems a lil crazy to wait for athens to start 😄 -- let's make it 20 seconds?

Sorry, I am getting deviated to k8s timescales

marwan-at-work · 2020-02-14T05:16:54Z

e2etests/run_athens.go

+	return target, err
+}
+
+func stopAthens() error {


I think a better way to "stopAthens" is to have the runAthensAndWait take a context that you can cancel whenever you want to stop athens and just pass that context into https://golang.org/pkg/os/exec/#CommandContext

which should render this whole function unnecessary.

Did not know about it, thanks!

Well, yes and no, as I am using stopAthens also to remove any dangling instance before running the real tests. So it's ok for the teardown phase, but I'll keep this around as well.

marwan-at-work · 2020-02-14T05:23:54Z

e2etests/all_test.go

+	if err != nil {
+		m.Fail("Failed to stop athens", err)
+	}
+	chmodR(m.tmpDir, 0777)


I think a cleaner way to do this is to just call clean modcache -- I think we should probably do it on the go_get_fetcher as well. But that's another topic :)

I do agree to clear the cache instead of brutally erasing in SetupTest, as there we care only about the cache to be empty.
In TearDown though, I'd like to remove any additional folder that was created by the test suite, and I am afraid clearing the cache is not enough.

@fedepaol right you can still remove the folder, but there won't be a need to chmod anything :)

Yes I left it there as I did not remember why I added originally. I'll try to remove it and see if it works :-)

marwan-at-work · 2020-02-14T05:32:25Z

e2etests/all_test.go

+		m.Fail("Failed to make temp dir", err)
+	}
+
+	m.goModCache = path.Join(m.tmpDir, "pkg", "mod")


I would probably make two temp directories to maintain a nicer separation: one for GOPATH and one outside of GOPATH for happy-path. This way you don't to actually care about the "pkg/mod" part here, and cleaning the cache only needs to take the GOPATH env var and nothing else

arschles

@fedepaol sorry I got sidetracked and didn't review this. It looks great 🎉 🚀 ! @marwan-at-work can we address your comments in a follow-up PR? I can submit it if @fedepaol. I would love to get this into master so that I can expand on the tests...

fedepaol · 2020-02-15T08:47:31Z

@arschles I think I addressed all the comments in my latest push of yesterday.
@marwan-at-work would you mind giving another look?

marwan-at-work

@arschles I added a couple of more comments but hitting approve if we want to tighten things up in a follow up PR -- happy to do it as well.

Thanks again @fedepaol :)

marwan-at-work · 2020-02-15T18:14:44Z

e2etests/all_test.go

+	suite.Suite
+	goBinaryPath   string
+	env            []string
+	tmpDir         string


i'd call it goPath or GOPATH to know what the tmpDir is for :)

marwan-at-work · 2020-02-15T18:19:21Z

e2etests/all_test.go

+	// ignoring error as if no athens is running it fails.
+
+	ctx := context.Background()
+	ctx, m.stopAthens = context.WithCancel(ctx)


Form the context package docs:

Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it. The Context should be the first parameter.

I realize storing the context is necessary here because of testify/suite, but I'd rather we followed best practices instead of having the framework steer us away form that.

That said, we can definitely do it in a follow up PR because it means removing the test suite from the e2e tests :)

Ah right. That would make things less easy. Not even sure if it's worth reverting to the previous stopAthens, as even if we remove testify having a propagated context will bloat the code a bit.

We want e2e tests to be skipped while running normal unit tests.

fedepaol · 2020-02-16T20:07:58Z

@marwan-at-work I applied the change suggested in #1514 (comment) .
I am not sure about the context thing though. We need to spin up athens when the "suite" (or the set of tests) start, and to stop it when the "suite" is complete, so I can't see a clean way to have the context not stored and propagated through functions.
I feel like the best approach is to revert to the previous "stop" function. Wdyt?

marwan-at-work · 2020-02-16T20:35:59Z

@fedepaol IMHO the best way is to just use the standard library because the std lib wouldn't get in your way the same way this test suite does :)

That said, we can keep things the way they are because it works, then remove them and use regular table tests a follow up pr

fedepaol · 2020-02-16T20:56:42Z

@fedepaol IMHO the best way is to just use the standard library because the std lib wouldn't get in your way the same way this test suite does :)

That said, we can keep things the way they are because it works, then remove them and use regular table tests a follow up pr

Ok, we can discuss those details later then. Thanks again for your review!

fedepaol · 2020-02-18T19:07:07Z

So this can be merged right?

arschles · 2020-02-19T23:38:50Z

@fedepaol thank you * 💯 for your hard work on this PR!

mplachter · 2020-02-25T12:59:14Z

Heads up this did break the docker-e2e-tests. I'm currently fixing to test the new redis changes will get a MR up to fix the docker-e2e-tests (this might take me a bit just a heads up if someone wants to fix it before me go-ahead)

test_e2e.sh doesn't exist anymore.

  teste2e:
    build:
      context: .
      dockerfile: Dockerfile.test
      args:
        GOLANG_VERSION: 1.13
    command: ["./scripts/test_e2e.sh"]

arschles · 2020-02-25T19:39:06Z

@mplachter would it help if I go ahead and fix docker-e2e-tests in a separate PR?

fedepaol requested a review from a team as a code owner December 30, 2019 13:22

fedepaol force-pushed the e2e branch from b463c20 to 384abc7 Compare December 30, 2019 17:26

ghost self-assigned this Jan 20, 2020

ghost reviewed Feb 11, 2020

View reviewed changes

fedepaol force-pushed the e2e branch from dceb653 to b0eee6b Compare February 11, 2020 21:42

ghost approved these changes Feb 11, 2020

View reviewed changes

marwan-at-work requested changes Feb 11, 2020

View reviewed changes

fedepaol force-pushed the e2e branch from b0eee6b to 5723201 Compare February 12, 2020 20:01

marwan-at-work requested changes Feb 14, 2020

View reviewed changes

Replace the current e2e script with a test suite running e2e tests.

a3ec68f

fedepaol force-pushed the e2e branch from 2a16fc2 to 5c5c8de Compare February 14, 2020 23:13

arschles approved these changes Feb 15, 2020

View reviewed changes

marwan-at-work approved these changes Feb 15, 2020

View reviewed changes

fedepaol mentioned this pull request Feb 15, 2020

Is it worth using testcontainers to set up our unit, integration or e2e tests? #1500

Open

Add a build tag to skip e2e while running unit tests.

135c0fb

We want e2e tests to be skipped while running normal unit tests.

fedepaol force-pushed the e2e branch from 5c5c8de to 135c0fb Compare February 16, 2020 20:05

arschles merged commit 27f3683 into gomods:master Feb 19, 2020

arschles mentioned this pull request Feb 19, 2020

Expand the e2e tests #1474

Closed

arschles mentioned this pull request Feb 20, 2020

In tests, replace go mod download with a mock executable #1449

Closed

Replace the current e2e script with a test suite running e2e tests. #1514

Replace the current e2e script with a test suite running e2e tests. #1514

Conversation

fedepaol commented Dec 30, 2019

ghost commented Jan 20, 2020

fedepaol commented Feb 5, 2020

ghost commented Feb 5, 2020

fedepaol commented Feb 6, 2020

ghost commented Feb 11, 2020

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedepaol commented Feb 11, 2020

ghost left a comment

Choose a reason for hiding this comment

marwan-at-work left a comment

Choose a reason for hiding this comment

marwan-at-work Feb 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marwan-at-work 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

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

arschles left a comment

Choose a reason for hiding this comment

fedepaol commented Feb 15, 2020

marwan-at-work 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

fedepaol commented Feb 16, 2020

marwan-at-work commented Feb 16, 2020

fedepaol commented Feb 16, 2020

fedepaol commented Feb 18, 2020

arschles commented Feb 19, 2020

mplachter commented Feb 25, 2020 • edited Loading

arschles commented Feb 25, 2020

marwan-at-work Feb 11, 2020 •

edited

Loading

mplachter commented Feb 25, 2020 •

edited

Loading