hack/tests,test/e2e: separate testing and scaffolding of E2E te… #1586

AlexNPavel · 2019-06-22T00:29:37Z

Description of the change: Separate the scaffolding and testing sections of the Golang E2E tests. This change allows us to directly use operator-sdk test local to run the tests directly from a properly scaffolding memcached-operator project.

Motivation for the change: It makes the code cleaner, less hacky, and designed in a way that users can copy our tests directly into the example memcached-operator project without modifications.

At the moment, this means that our go files for testing now have tmpl extensions. To develop with tools, you should use the e2e-go-scaffold.sh script, do you development in the generated project, and then copy back the modified test files into the sdk. This also means that we won't have highlighting in GitHub. We need to keep it like this for now however, as the project refers to github.com/example-inc/memcached-operator, which does not exist and would cause dep ensure and IDE tools to fail. We will be able to fix this once the go mod migration of the SDK is completed: #1566

estroz

/lgtm

openshift-ci-robot · 2019-06-24T19:45:14Z

New changes are detected. LGTM label has been removed.

AlexNPavel · 2019-06-24T19:54:22Z

/hold
Looks like I messed up a rebase and some of Joe's changes to the E2E test disappeared. Fixing now

AlexNPavel · 2019-06-24T19:58:55Z

/hold cancel
Github stores branches that previously existed but were force pushed to, so it was a simple copy paste fix 🙌

lilic

Overall lgtm, tested it locally and it works on mac OSX as well 👍

hack/tests/scaffolding/scaffold-memcached.go

lilic · 2019-06-27T11:48:57Z

test/e2e/_incluster-test-code/memcached_test.go

+
+import (
+	"bytes"
+	goctx "context"


Nit: Any reason for this rename?

We have it renamed as goctx in all of our documentation and sample code for the test framework, so this makes it more consistent. It makes it easier to differentiate between the TestCtx from the framework and context from golang.

test/e2e/_incluster-test-code/memcached_test.go

hack/tests/scaffolding/scaffold-memcached.go

Co-Authored-By: Lili Cosic <[email protected]>

…into go-e2e-refactor

joelanford · 2019-06-28T18:30:07Z

hack/tests/scaffolding/e2e-go-scaffold.sh

+mkdir -p $BASEPROJECTDIR
+
+pushd "$BASEPROJECTDIR"
+go run "$ROOTDIR/hack/tests/scaffolding/scaffold-memcached.go" --local-repo $ROOTDIR --image-name=$IMAGE_NAME --local-image


Not a huge deal, since I could fix this in the go-mod PR, but go run doesn't work well when running from a directory outside the module where the go file being run is.

I solved this in the go-mod PR by running go build before switching to the $BASEPROJECTDIR directory, and then just running the executable after switching.

Ehh.. maybe don't worry about this, as there will be other things to fix anyway.

joelanford

A few nits, questions, and suggestions, but looks good overall.

Do we have developer docs about CI and testing that will need to be updated?

hack/tests/e2e-go.sh

hack/tests/scaffolding/e2e-go-scaffold.sh

joelanford · 2019-06-28T19:08:17Z

hack/tests/scaffolding/scaffold-memcached.go

+		return nil, errors.Wrap(err, "failed to write go.mod before replacing SDK repo")
+	}
+	return modBytes, nil
+}


Most of this go program is running CLI commands and other things that would be much simpler in bash. Would it make sense to port it to bash and incorporate it into e2e-go-scaffold.sh?

Probably not something to do in this PR, but maybe something to consider for a TODO?

I'll add that as a TODO

joelanford · 2019-06-28T19:11:31Z

hack/tests/scaffolding/scaffold-memcached.go

+	tmplFiles := map[string]string{
+		filepath.Join(localSDKPath, "example/memcached-operator/memcached_controller.go.tmpl"): "pkg/controller/memcached/memcached_controller.go",
+		filepath.Join(localSDKPath, "test/e2e/_incluster-test-code/main_test.go"):              "test/e2e/main_test.go",
+		filepath.Join(localSDKPath, "test/e2e/_incluster-test-code/memcached_test.go"):         "test/e2e/memcached_test.go",


Is there a reason for prefixing the incluster-test-code package name with an underscore? Does that hint to the go toolchain to ignore it?

Yes, underscores tell the go toolchain (tools like go vet and go mod) to ignore that directory (or file), which we need to do since we are importing a non-existent project. This wasn't a problem before because we had the .tmpl file extension, but that makes the files much more difficult to work on and review since that removes the code highlighting, which is more important now that this is our main test file.

Is it possible to do the same with example/memcached-operator/memcached_controller.go.tmpl? i.e example/_memcached-operator/memcached_controller.go.

It would be so much better to look at with syntax highlighting especially since we link to it in our README.

hack/tests/scaffolding/scaffold-memcached.go

hasbro17

LGTM after nits.

Co-Authored-By: Haseeb Tariq <[email protected]>

AlexNPavel · 2019-06-28T23:05:46Z

@joelanford I looked at the docs, and it looks like we don't actually describe how we scaffold the project, so the docs are still mostly accurate. They are missing the metrics tests, but I feel like that should be a separate PR, since it's not directly related to the changes here.

AlexNPavel added area/testing Issue related to testing the operator-sdk and subcomponents refactoring labels Jun 22, 2019

AlexNPavel requested review from joelanford, shawn-hurley, lilic, hasbro17, theishshah and estroz June 22, 2019 00:29

openshift-ci-robot assigned estroz Jun 24, 2019

estroz approved these changes Jun 24, 2019

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 24, 2019

hack/tests,test/e2e: separate testing and scaffolding of E2E tests

c500238

AlexNPavel force-pushed the go-e2e-refactor branch from 20d22e0 to c500238 Compare June 24, 2019 19:45

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 24, 2019

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 24, 2019

test/e2e/_incluster-test-code: fix rebase

794cb2d

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jun 24, 2019

Merge branch 'master' into go-e2e-refactor

1a548c0

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 25, 2019

AlexNPavel added 2 commits June 26, 2019 09:56

hack/tests/scaffolding: fix travis

1abfa9c

hack/tests/scaffolding: remove unused var and add TODOs

0fea55f

AlexNPavel force-pushed the go-e2e-refactor branch from a1f3bf1 to 0fea55f Compare June 26, 2019 18:23

lilic approved these changes Jun 27, 2019

View reviewed changes

Apply suggestions from code review

079fd63

Co-Authored-By: Lili Cosic <[email protected]>

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2019

AlexNPavel added 2 commits June 28, 2019 11:07

Merge branch 'master' into go-e2e-refactor

f0fcb1a

Merge branch 'go-e2e-refactor' of github.com:AlexNPavel/operator-sdk …

009746e

…into go-e2e-refactor

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 28, 2019

AlexNPavel added 2 commits June 28, 2019 11:10

hack/tests/scaffolding: remove extra line

638dd9a

test/e2e/main_test.go: remove unused flags

0115e04

joelanford reviewed Jun 28, 2019

View reviewed changes

hack/tests/scaffolding: do go build instead of run

ba2c657

joelanford reviewed Jun 28, 2019

View reviewed changes

hack/tests/scaffolding: use mktemp dir

a683413

hasbro17 reviewed Jun 28, 2019

View reviewed changes

hack/tests/scaffolding/scaffold-memcached.go Outdated Show resolved Hide resolved

hasbro17 reviewed Jun 28, 2019

View reviewed changes

hack/tests/scaffolding/scaffold-memcached.go Outdated Show resolved Hide resolved

hasbro17 reviewed Jun 28, 2019

View reviewed changes

hack/tests/scaffolding/scaffold-memcached.go Outdated Show resolved Hide resolved

hasbro17 reviewed Jun 28, 2019

View reviewed changes

hack/tests/scaffolding/scaffold-memcached.go Outdated Show resolved Hide resolved

AlexNPavel added 2 commits June 28, 2019 14:09

hack/tests/scaffolding: add TODO

9c9f636

hack/tests/scaffolding: remove unnecessary error check

25c0720

hasbro17 approved these changes Jun 28, 2019

View reviewed changes

Apply suggestions from code review

462c399

Co-Authored-By: Haseeb Tariq <[email protected]>

AlexNPavel changed the title ~~hack/tests,test/e2e: separate testing and scaffolding of E2E tests~~ hack/tests,test/e2e: separate testing and scaffolding of E2E te… Jun 28, 2019

AlexNPavel merged commit 4d7b853 into operator-framework:master Jun 28, 2019

AlexNPavel deleted the go-e2e-refactor branch June 28, 2019 23:27

hack/tests,test/e2e: separate testing and scaffolding of E2E te… #1586

hack/tests,test/e2e: separate testing and scaffolding of E2E te… #1586

Uh oh!

Conversation

AlexNPavel commented Jun 22, 2019

Uh oh!

estroz left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Jun 24, 2019

Uh oh!

AlexNPavel commented Jun 24, 2019

Uh oh!

AlexNPavel commented Jun 24, 2019

Uh oh!

lilic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelanford left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hasbro17 left a comment

Choose a reason for hiding this comment

Uh oh!

AlexNPavel commented Jun 28, 2019

Uh oh!

Uh oh!