Convert hack/e2e.go to a test-infra/kubetest shim #40374

fejta · 2017-01-24T18:38:07Z

Replaces hack/e2e.go for a shim that passes the args to k8s.io/test-infra/kubetest
Adds fejta to hack/OWNERS
Adds e2e_test.go for unit test coverage of the shim.

Usage: go run hack/e2e.go [--get=true] [--old=1d] -- KUBETEST_ARGS
In other words there is are --get and --old shim flags, which control how we upgrade kubetest, and a -- to separate the shim args from the kubetest args, and the existing kubetest args like --down --up, etc. If only KUBETEST_ARGS are used then you can skip the -- (although golang will complain about it).

Once this is ready to go I will update the kubekins-e2e image to copy this file from test-infra: https://github.com/kubernetes/test-infra/blob/master/jenkins/e2e-image/Dockerfile#L70

ref kubernetes/test-infra#1475

k8s-reviewable · 2017-01-24T18:39:13Z

This change is

k8s-github-robot · 2017-01-24T18:40:49Z

[APPROVALNOTIFIER] Needs approval from an approver in each of these OWNERS Files:

hack/OWNERS

We suggest the following people:
cc @ihmccreery
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

fejta · 2017-01-24T21:27:19Z

I realize this has an XXL tab, but it is WAY less scary than it looks: almost all of this is deleting the old e2e.go and replacing it with a few lines of "go get -u k8s.io/test-infra/kubetest && kubetest ARGS"

fejta · 2017-01-24T22:29:32Z

@k8s-bot gci gke e2e test this
@k8s-bot kubemark e2e test this

fejta · 2017-01-25T00:28:52Z

W0124 14:56:27.167] 2017/01/24 14:56:27 e2e.go:946: Running: ./test/kubemark/run-e2e-tests.sh --ginkgo.focus=\[Feature:Empty\] --gather-resource-usage=true --garbage-collector-enabled=true
W0124 14:56:27.167] Project: k8s-jkns-pr-kubemark
W0124 14:56:27.167] Zone: us-central1-f
W0124 14:56:27.167] Trying to find master named 'k6k-e2e-agent-pr-16-0-kubemark-master'
W0124 14:56:27.167] Looking for address 'k6k-e2e-agent-pr-16-0-kubemark-master-ip'
W0124 14:56:27.647] Using master: k6k-e2e-agent-pr-16-0-kubemark-master (external IP: 35.184.81.178)
W0124 14:56:29.773] flag provided but not defined: -v
W0124 14:56:29.774] Usage of /tmp/go-build615123988/command-line-arguments/_obj/exe/e2e:
W0124 14:56:29.774]   -get
W0124 14:56:29.774]     	go get -u kubetest if old or not installed (default true)
W0124 14:56:29.774]   -old duration
W0124 14:56:29.774]     	Consider kubetest old if it exceeds this (default 24h0m0s)
W0124 14:56:29.774] 2017/01/24 14:56:29 e2e.go:40: NOTICE: go run hack/e2e.go is now a shim for test-infra/kubetest
W0124 14:56:29.775] 2017/01/24 14:56:29 e2e.go:41:   Usage: go run hack/e2e.go [--get=true] [--old=24h] -- [KUBETEST_ARGS]
W0124 14:56:29.775] 2017/01/24 14:56:29 e2e.go:42:   The separator is required to use --get or --old flags
W0124 14:56:29.775] 2017/01/24 14:56:29 e2e.go:43:   The -- flag separator also suppresses this message
W0124 14:56:29.776] panic: runtime error: slice bounds out of range
W0124 14:56:29.776] 
W0124 14:56:29.776] goroutine 1 [running]:
W0124 14:56:29.776] panic(0x5a1060, 0xc82001a0d0)
W0124 14:56:29.776] 	/usr/local/go/src/runtime/panic.go:481 +0x3e6
W0124 14:56:29.777] main.main()
W0124 14:56:29.777] 	/workspace/kubernetes/hack/e2e.go:44 +0xbd8
W0124 14:56:29.777] exit status 2
W0124 14:56:29.781] 2017/01/24 14:56:29 e2e.go:948: Step './test/kubemark/run-e2e-tests.sh --ginkgo.focus=\[Feature:Empty\] --gather-resource-usage=true --garbage-collector-enabled=true' finished in 2.747092566s

fejta · 2017-01-25T06:20:10Z

Fixed, apparently I cannot count in golang. args[-5:] does not work as it does in python. Added unit tests for this parsing since apparently I should have in the first place!

spxtr

gofmt your code!

spxtr · 2017-01-25T18:05:10Z

hack/e2e.go

-	if _, err := f.Write(out); err != nil {
-		log.Fatalf("Error writing XML data: %s", err)
+func parse(args []string)flags {
+	//


spxtr · 2017-01-25T18:17:00Z

hack/e2e.go

-
-func (b bash) Up() error {
-	return finishRunning(exec.Command("./hack/e2e-internal/e2e-up.sh"))
+// Struct that allows unit tests to override functionality.


I usually see this done with a global variable for small packages. Something like var osStat = os.Stat and then in tests override it.

That sounds like a race condition if tests run concurrently

Unit tests can only run concurrently if they call t.Parallel().

Is this something you want me to add in this PR?

spxtr · 2017-01-25T18:18:42Z

hack/e2e.go

-		if err != nil {
-			return nil, err
+// Try to find kubetest, either GOPATH/bin/kubetest or PATH
+func (t tester) look() (string, error) {


look isn't a very descriptive name... I'd suggest findKubetest or something like that.

spxtr · 2017-01-25T18:19:17Z

hack/e2e.go

+// Upgrade if kubetest does not exist or has not been updated today
+func (t tester) getKubetest(get bool, old time.Duration) (string, error) {
+	// Find kubetest installation
+	p, err := t.look()



remove this newline

spxtr · 2017-01-25T18:21:44Z

hack/e2e.go

+	old := fs.Duration("old", oldDefault, "Consider kubetest old if it exceeds this")
+	var a []string
+	if err := fs.Parse(args[1:]); err != nil {
+		log.Print("NOTICE: go run hack/e2e.go is now a shim for test-infra/kubetest")


Do we plan on eventually removing hack/e2e.go, or will this be sticking around indefinitely?

Removing eventually

spxtr · 2017-01-25T18:23:58Z

hack/e2e_test.go

+		returnError error
+	}{
+		{ // 0: Pass when on GOPATH/bin
+			false,


I prefer explicitly stating the struct members that you're setting in this kind of thing, since it's super hard to read these entries when it's just a list of values.

Please elaborate, table driven testing is explicitly what our style guide recommends:
https://github.com/kubernetes/community/blob/master/contributors/devel/testing.md#unit-tests

I agree that table driven testing is illegible.

Just learned about annoyingStruct{foo: 1, bar: false} thanks!

fejta · 2017-01-25T20:49:32Z

@k8s-bot gci gke e2e test this

fejta · 2017-01-25T20:52:53Z

@k8s-bot kops aws e2e test this

spxtr · 2017-01-26T22:20:14Z

$ go run hack/e2e.go -help
Usage of /tmp/go-build302892506/command-line-arguments/_obj/exe/e2e:
  -get
    	go get -u kubetest if old or not installed (default true)
  -old duration
    	Consider kubetest old if it exceeds this (default 24h0m0s)
2017/01/26 14:18:52 e2e.go:50: NOTICE: go run hack/e2e.go is now a shim for test-infra/kubetest
2017/01/26 14:18:52 e2e.go:51:   Usage: go run hack/e2e.go [--get=true] [--old=24h0m0s] -- [KUBETEST_ARGS]
2017/01/26 14:18:52 e2e.go:52:   The separator is required to use --get or --old flags
2017/01/26 14:18:52 e2e.go:53:   The -- flag separator also suppresses this message
2017/01/26 14:18:52 e2e.go:139: Updating kubetest binary...
^Csignal: interrupt

That's not what I wanted to happen...

fejta · 2017-01-30T21:36:45Z

Zach and Kris, any thoughts?

zmerlynn · 2017-01-30T23:01:58Z

LGTM, seems fine. Can you prepare a PR to update https://github.com/kubernetes/community/blob/master/contributors/devel/e2e-tests.md?

krousey · 2017-01-31T19:12:56Z

hack/e2e.go

@@ -14,968 +14,144 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

-// e2e.go runs the e2e test suite. No non-standard package dependencies; call with "go run".
+// User-interface for test-infra/kubetest/e2e.go
+// Equivalent to go get -u k8s.io/test-infra/kubetest/e2e.go && kubetest "${@}"


go get -u k8s.io/test-infra/kubetest && kubetest "${@}"

krousey · 2017-01-31T19:17:16Z

hack/e2e.go

-	}
-	if _, err := f.Write(out); err != nil {
-		log.Fatalf("Error writing XML data: %s", err)
+func parse(args []string) (flags, error) {


Why are you doing it this way instead of just having flag.Bool var blocks and calling flag.Parse in main?

To assist with unit testing

I looked at the unit test, and it seems to mostly test that the flag package works properly.

Actually I added this unit test because it is non-trivial and I messed it up multiple times. These unit tests are what give me confidence it does the right thing. Specifically that go run hack/e2e.go --get=false --blah --foo sends --blah and --foo to kubetest.

k8s-github-robot · 2017-02-03T23:05:56Z

Automatic merge from submit-queue (batch tested with PRs 40696, 39914, 40374)

fejta assigned spiffxp and spxtr Jan 24, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 24, 2017

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Jan 24, 2017

fejta mentioned this pull request Jan 24, 2017

Pull e2e.go from test-infra kubernetes/test-infra#1681

Merged

fejta added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jan 24, 2017

fejta force-pushed the e2e branch 2 times, most recently from fcece64 to da6ec5a Compare January 24, 2017 21:15

fejta assigned fejta and unassigned spiffxp and spxtr Jan 25, 2017

fejta force-pushed the e2e branch from da6ec5a to 3cf32a6 Compare January 25, 2017 06:18

fejta assigned spiffxp and spxtr and unassigned fejta Jan 25, 2017

spxtr reviewed Jan 25, 2017

View reviewed changes

fejta force-pushed the e2e branch 2 times, most recently from a18a222 to 20efaf3 Compare January 25, 2017 20:12

fejta force-pushed the e2e branch from 20efaf3 to 7406921 Compare January 27, 2017 22:52

fejta assigned krousey and zmerlynn Jan 30, 2017

k8s-github-robot assigned jbeda and unassigned spiffxp, krousey, zmerlynn and spxtr Jan 30, 2017

apelisse assigned spiffxp, krousey, zmerlynn and spxtr Jan 31, 2017

k8s-github-robot assigned lavalamp Jan 31, 2017

fejta unassigned jbeda and lavalamp Jan 31, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 31, 2017

fejta force-pushed the e2e branch from 7406921 to 25141a6 Compare January 31, 2017 18:54

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 31, 2017

krousey reviewed Jan 31, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 2, 2017

krousey approved these changes Feb 2, 2017

View reviewed changes

Convert hack/e2e.go to a test-infra/kubetest shim

2fffe48

fejta force-pushed the e2e branch from 25141a6 to 2fffe48 Compare February 3, 2017 01:43

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 3, 2017

fejta added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 3, 2017

k8s-github-robot merged commit c78745e into kubernetes:master Feb 3, 2017

fejta deleted the e2e branch February 3, 2017 23:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert hack/e2e.go to a test-infra/kubetest shim #40374

Convert hack/e2e.go to a test-infra/kubetest shim #40374

fejta commented Jan 24, 2017 •

edited

k8s-reviewable commented Jan 24, 2017

k8s-github-robot commented Jan 24, 2017

fejta commented Jan 24, 2017

fejta commented Jan 24, 2017

fejta commented Jan 25, 2017 •

edited

fejta commented Jan 25, 2017 •

edited

spxtr left a comment

spxtr Jan 25, 2017

fejta Jan 25, 2017

spxtr Jan 25, 2017

fejta Jan 25, 2017

spxtr Jan 25, 2017

fejta Jan 26, 2017

spxtr Jan 25, 2017

fejta Jan 25, 2017

spxtr Jan 25, 2017

fejta Jan 25, 2017

spxtr Jan 25, 2017

fejta Jan 25, 2017

spxtr Jan 25, 2017

fejta Jan 25, 2017

fejta Jan 25, 2017

fejta commented Jan 25, 2017

fejta commented Jan 25, 2017

spxtr commented Jan 26, 2017

fejta commented Jan 30, 2017

zmerlynn commented Jan 30, 2017

krousey Jan 31, 2017

fejta Feb 3, 2017

krousey Jan 31, 2017

fejta Jan 31, 2017

krousey Jan 31, 2017

fejta Feb 1, 2017

k8s-github-robot commented Feb 3, 2017

Convert hack/e2e.go to a test-infra/kubetest shim #40374

Convert hack/e2e.go to a test-infra/kubetest shim #40374

Conversation

fejta commented Jan 24, 2017 • edited

k8s-reviewable commented Jan 24, 2017

k8s-github-robot commented Jan 24, 2017

fejta commented Jan 24, 2017

fejta commented Jan 24, 2017

fejta commented Jan 25, 2017 • edited

fejta commented Jan 25, 2017 • edited

spxtr 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

fejta commented Jan 25, 2017

fejta commented Jan 25, 2017

spxtr commented Jan 26, 2017

fejta commented Jan 30, 2017

zmerlynn commented Jan 30, 2017

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

k8s-github-robot commented Feb 3, 2017

fejta commented Jan 24, 2017 •

edited

fejta commented Jan 25, 2017 •

edited

fejta commented Jan 25, 2017 •

edited