Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

E2E: handling extra files #69103

Merged
merged 2 commits into from Oct 4, 2018

Conversation

@pohly
Copy link
Contributor

commented Sep 26, 2018

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

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

/ok-to-test

This was referenced Sep 26, 2018

@timothysc
Copy link
Member

left a comment

Minor comments + 2 commits, please.

1 for changes
1 for auto-generated.

"github.com/onsi/ginkgo"
)

var filesources = make([]FileSource, 0, 5)

This comment has been minimized.

Copy link
@timothysc

timothysc Sep 26, 2018

Member

Why not just make it a nil slice?

This comment has been minimized.

Copy link
@pohly

pohly Sep 27, 2018

Author Contributor

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.

// 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))

This comment has been minimized.

Copy link
@timothysc

timothysc Sep 26, 2018

Member

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

This comment has been minimized.

Copy link
@timothysc

timothysc Sep 26, 2018

Member

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 comment has been minimized.

Copy link
@pohly

pohly Sep 27, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@timothysc

timothysc Sep 27, 2018

Member

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 pohly force-pushed the pohly:testfiles branch from 7250427 to 6c8e1f9 Sep 27, 2018

@pohly

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2018

@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

@pohly pohly force-pushed the pohly:testfiles branch from 573a9a0 to 20f8dfb Sep 27, 2018

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

This comment has been minimized.

Copy link
@timothysc

timothysc Sep 27, 2018

Member

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.

This comment has been minimized.

Copy link
@pohly

pohly Sep 27, 2018

Author Contributor

Okay, I'll add that.

@pohly pohly force-pushed the pohly:testfiles branch from 20f8dfb to fab34f7 Sep 27, 2018

@pohly

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2018

/retest

@pohly pohly force-pushed the pohly:testfiles branch from fab34f7 to 318aa5e Sep 28, 2018

@timothysc
Copy link
Member

left a comment

/lgtm
/approve

Thanks for doing this!

@pohly

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2018

/kind feature

@fejta-bot

This comment has been minimized.

Copy link

commented Oct 2, 2018

/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.

pohly added some commits Aug 31, 2018

e2e: abstract access to additional files
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
#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: #66649 and #23987
e2e: update bazel BUILD files
Generated via hack/update-bazel.sh.

@pohly pohly force-pushed the pohly:testfiles branch from 318aa5e to 20c9549 Oct 2, 2018

@k8s-ci-robot k8s-ci-robot removed the lgtm label Oct 2, 2018

@pohly

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2018

/retest

@pohly

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2018

/retest

@timothysc

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Oct 4, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2018

[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:

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

@pohly

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2018

/retest

1 similar comment
@pohly

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2018

/retest

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

18 checks passed

cla/linuxfoundation pohly authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details

@pohly pohly referenced this pull request Oct 10, 2018

Closed

REQUEST: New membership for pohly #155

6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.