E2E: handling extra files #69103

pohly · 2018-09-26T15:31:46Z

What this PR does / why we need it:

There is an on-going effort to make test/e2e.test self-contained by building all files that are needed at runtime into the binary via bindata. Tests which support that do so by retrieving files from the test/e2e/generated package. The downside of that approach is that such tests only work in Kubernetes itself. As described in issue #66649, making tests reusable outside of Kubernetes is useful, for example for testing other CSI drivers.

This PR adds an API (test/e2e/framework/testfiles) that decouples tests from the actual code that retrieves files. In the test/e2e test suite, access via --repo-root is kept in addition to retrieval from bindata, so everything works as before.

The goal is not to get rid of --repo-root. However, by converting some tests to use the new API, those tests no longer depend on --repo-root, so we get a step closer to that goal - if it still is a goal. Some tests depend on extra files (for example, test/e2e/framework/ingress/ingress_utils.go for providing the optional secret.yaml) which cannot be built into the binary.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Partial fix #66649 (main goal)
Partial fix #23987 (side effect)

Special notes for your reviewer:

This depends on PR #68187 getting merged first. The first commit here is from that PR.

This is a subset of PR #68483.

Release note:

NONE

/sig testing

timothysc · 2018-09-26T15:39:15Z

/ok-to-test

timothysc

Minor comments + 2 commits, please.

1 for changes
1 for auto-generated.

timothysc · 2018-09-26T15:58:13Z

test/e2e/framework/testfiles/testfiles.go

+	"github.com/onsi/ginkgo"
+)
+
+var filesources = make([]FileSource, 0, 5)


Why not just make it a nil slice?

No particular reason, I just thought that pre-allocating some memory might be useful. But I agree, that's something that the first append() can also do, so I'll make it nil.

timothysc · 2018-09-26T17:08:39Z

test/e2e/framework/testfiles/testfiles.go

+// doesn't have to implement error checking.
+func ReadOrDie(filePath string) []byte {
+	if len(filesources) == 0 {
+		ginkgo.Fail(fmt.Sprintf("No file sources registered (yet?), cannot retrieve test file %s.", filePath))


Do we need a dependence on ginkgo in this utility functions, or should we return an error and fail outside?

After looking at the call sites I think you wrap everything into an error handler and pass in a closure to call the ginkgo with the error without you having to take on owning ginkgo details in this sub-package.

This is part of test/e2e/framework, which already is tied to Ginkgo, so it seemed okay to use that also here. It makes the API a bit simpler. What about just calling panic? The effect will be the same (also get's caught by Ginkgo and logged).

It's not just ReadOrDie, Exists also fails on errors to simplify the call sites.

Passing via a well defined, simple closure it not too pesky and would meet the goal of minimizing the proliferation of ginkgo dependencies and being about to have the failure be clearly visible from the call sites.

pohly · 2018-09-27T11:35:26Z

@timothysc I force-pushed two commits, one for the source code changes and one for the Bazel BUILD files. I kept the test/e2e/examples.go changes because if we merge all of this at once, we do not need PR #68187 anymore.

This version uses panic in test/e2e/framework/testfiles instead of Ginkgo. I'd prefer to keep the API simple, but don't feel strongly about it and can also introduce an additional parameter which handles the errors.

Here's output for a test that triggers such a panic because the file name was changed such that the file isn't found. I've shortened the list of files. The original error lists all of the builtin files:

•! Panic in Spec Setup (BeforeEach) [6.161 seconds]
[sig-cli] Kubectl client
/nvme/gopath/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/e2e/kubectl/framework.go:22
  [k8s.io] Update Demo
  /nvme/gopath/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/e2e/framework/framework.go:694
    should do a rolling update of a replication controller  [Conformance] [BeforeEach]
    /nvme/gopath/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/e2e/framework/framework.go:699

    Test Panicked
    Test file "test/fixtures/doc-yaml/user-guide/update-demo/nautilus-xxx-rc.yaml.in" was not found.
    Test files are expected in "/nvme/gopath/src/k8s.io/kubernetes/cluster/..".
    The following files are built into the test executable via gobindata. For questions on maintaining gobindata, contact the sig-testing group.
       test/e2e/testing-manifests/flexvolume/dummy
       test/e2e/testing-manifests/flexvolume/dummy-attachable
....
       test/images/webhook/patch_test.go
       test/images/webhook/pods.go
       test/images/webhook/scheme.go
    
    /usr/lib/go-1.10/src/runtime/panic.go:502

    Full Stack Trace
    	/usr/lib/go-1.10/src/runtime/panic.go:502 +0x229
    k8s.io/kubernetes/test/e2e/framework/testfiles.ReadOrDie(0xc421f40410, 0x45, 0x2, 0xc421f40410, 0x45)
    	/nvme/gopath/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/e2e/framework/testfiles/testfiles.go:90 +0x2e6
    k8s.io/kubernetes/test/e2e/kubectl.glob..func2.6.1()
    	/nvme/gopath/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/e2e/kubectl/kubectl.go:295 +0xad
    k8s.io/kubernetes/vendor/github.com/onsi/ginkgo/internal/leafnodes.(*runner).runSync(0xc420f4f800, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
    	/nvme/gopath/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/e2e/e2e.go:306 +0x1fc
    k8s.io/kubernetes/test/e2e.TestE2E(0xc42056fa40)
    	/nvme/gopath/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/e2e/e2e_test.go:65 +0x2b
    testing.tRunner(0xc42056fa40, 0x45c7fe8)
    	/usr/lib/go-1.10/src/testing/testing.go:777 +0xd0
    created by testing.(*T).Run
    	/usr/lib/go-1.10/src/testing/testing.go:824 +0x2e0

timothysc · 2018-09-27T14:23:34Z

test/e2e/framework/testfiles/testfiles.go

+		error += filesource.DescribeFiles()
+		error += "\n"
+	}
+	panic(error)


panic is probably not the right solution but a simple callback parameter for the error. Or to change the signatures outside to handle the error.

Okay, I'll add that.

pohly · 2018-09-27T15:13:55Z

@timothysc I pushed a version which takes a callback that gets invoked on failures. To make the API still easy to use, the signature is exactly that of ginkgo.Fail. Otherwise callers would have to use a closure.

Some callers can handle an error and don't use Ginkgo. For those I added a testfiles.Read which returns the error.

pohly · 2018-09-27T15:50:15Z

/retest

timothysc

/lgtm
/approve

Thanks for doing this!

pohly · 2018-10-01T19:45:30Z

/kind feature

fejta-bot · 2018-10-02T03:49:59Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

The new test/e2e/framework/testfiles package makes it possible to write tests that do not depend on a specific way of providing additional test files at runtime. Such tests and the framework are then more easily reused in other test suites. In the test/e2e suite file access is enabled based on the existing "repo-root" command line parameter and the built-in bindata. Tests using the new API will first check for files under "repo-root" and then fall back to the builtin data. This way, users of a test binary can modify those files without having to rebuild the binary. "repo-root" is still needed because at least some tests check for additional files (secret.yaml, via ingress_utils.go) that are not part of the upstream source code and thus may or may not be built into a test binary. Tests using bindata or repo-root directly get modified to use the new API, or removed when they are obsolete: test/e2e/examples.go depended on files that were removed in kubernetes#61246 and thus can no longer be run in Kubernetes. Moving the tests to kubernetes/examples is tracked in kubernetes/examples#214. The file removal did not break the automated E2E testing probably because the tests are under the Feature:Example tag and thus not enabled during normal CI runs. Removing also the obsolete tests makes it simpler to rework the "repo-root" setting because less code uses it. Related-to: kubernetes#66649 and kubernetes#23987

Generated via hack/update-bazel.sh.

pohly · 2018-10-02T08:21:59Z

/retest

pohly · 2018-10-02T10:12:05Z

@timothysc I had to rebase because 60f736e introduced a new direct call to the generated package. Please add lgtm again.

There's also some test failure in pull-kubernetes-integration which I don't understand ("test-cmd run_rs_tests" fails) - a flake or does this PR really cause that test to fail now?

pohly · 2018-10-02T11:58:52Z

/retest

timothysc · 2018-10-04T13:26:15Z

/lgtm

k8s-ci-robot · 2018-10-04T13:26:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pohly, timothysc

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~test/OWNERS~~ [timothysc]

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

pohly · 2018-10-04T15:39:54Z

/retest

pohly · 2018-10-04T18:07:44Z

/retest

k8s-ci-robot requested review from adohe-zz and jsafrane September 26, 2018 15:32

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 26, 2018

This was referenced Sep 26, 2018

E2E settings #69105

Merged

e2e refactor #68483

Merged

timothysc suggested changes Sep 26, 2018

View reviewed changes

pohly force-pushed the testfiles branch from 7250427 to 6c8e1f9 Compare September 27, 2018 11:04

pohly force-pushed the testfiles branch from 573a9a0 to 20f8dfb Compare September 27, 2018 14:20

timothysc reviewed Sep 27, 2018

View reviewed changes

pohly force-pushed the testfiles branch from 20f8dfb to fab34f7 Compare September 27, 2018 15:11

pohly force-pushed the testfiles branch from fab34f7 to 318aa5e Compare September 28, 2018 06:25

timothysc approved these changes Oct 1, 2018

View reviewed changes

k8s-ci-robot assigned timothysc Oct 1, 2018

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 1, 2018

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 1, 2018

pohly mentioned this pull request Oct 1, 2018

e2e/examples: remove obsolete tests #68187

Closed

pohly added 2 commits October 2, 2018 08:31

e2e: update bazel BUILD files

20c9549

Generated via hack/update-bazel.sh.

pohly force-pushed the testfiles branch from 318aa5e to 20c9549 Compare October 2, 2018 06:33

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 2, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2018

k8s-ci-robot merged commit 166490a into kubernetes:master Oct 4, 2018

pohly mentioned this pull request Oct 10, 2018

REQUEST: New membership for pohly kubernetes/org#155

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

E2E: handling extra files #69103

E2E: handling extra files #69103

pohly commented Sep 26, 2018 •

edited

timothysc commented Sep 26, 2018

timothysc left a comment

timothysc Sep 26, 2018

pohly Sep 27, 2018

timothysc Sep 26, 2018

timothysc Sep 26, 2018

pohly Sep 27, 2018

timothysc Sep 27, 2018

pohly commented Sep 27, 2018

timothysc Sep 27, 2018

pohly Sep 27, 2018

pohly commented Sep 27, 2018

pohly commented Sep 27, 2018

timothysc left a comment

pohly commented Oct 1, 2018

fejta-bot commented Oct 2, 2018

pohly commented Oct 2, 2018

pohly commented Oct 2, 2018

pohly commented Oct 2, 2018

timothysc commented Oct 4, 2018

k8s-ci-robot commented Oct 4, 2018

pohly commented Oct 4, 2018

pohly commented Oct 4, 2018

E2E: handling extra files #69103

E2E: handling extra files #69103

Conversation

pohly commented Sep 26, 2018 • edited

timothysc commented Sep 26, 2018

timothysc 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

pohly commented Sep 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly commented Sep 27, 2018

pohly commented Sep 27, 2018

timothysc left a comment

Choose a reason for hiding this comment

pohly commented Oct 1, 2018

fejta-bot commented Oct 2, 2018

pohly commented Oct 2, 2018

pohly commented Oct 2, 2018

pohly commented Oct 2, 2018

timothysc commented Oct 4, 2018

k8s-ci-robot commented Oct 4, 2018

pohly commented Oct 4, 2018

pohly commented Oct 4, 2018

pohly commented Sep 26, 2018 •

edited