Beginnings of a test framework #149

totherme · 2017-11-21T16:51:31Z

As discussed in #87 and in slack and hangouts.

Known issues:

If etcd starts slowly, the apiserver might not start in a sane state. This will be fixed soon.
If anything is currently listening on any of the ports we use, there will be clashes.

k8s-ci-robot · 2017-11-21T16:51:37Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: totherme
We suggest the following additional approver: pwittrock

Assign the PR to them by writing /assign @pwittrock in a comment when ready.

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

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

OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

apelisse · 2017-11-27T23:36:36Z

This change is

apelisse

I've stopped before that commit: "Implement first command talking to the APIServer". I'll keep reading later.
I think it's looking good, I have minor nits and dumb questions, nothing blocking.

apelisse · 2017-11-28T22:10:41Z

pkg/framework/test/apiserver.go

+}
+
+// Start starts the apiserver, and returns a gexec.Session. To stop it again, call Terminate and Wait on that session.
+func (s APIServer) Start(etcdURL string) (*gexec.Session, error) {


I'm assuming it's part of the plan, but we'll probably want to run these in parallel:

Can the non-insecure port be selected, or is it necessarily 443?

The insecure-port will have to be configurable (and maybe automatically selected for you),

Do you happen to know how long it takes to spawn a new apiserver?

We might end-up with incompatible options for various versions of the apiserver, something to keep in mind.

Nothing to be changed here, just thinking out loud ... :-)

Right now only the etcd port is configurable. However we know that we might need to make other ports configurable or random. We have some stories for that in our backlog to address that and see if and how we can make all the components run multiple times in parallel and play nicely together.

We started tracking the time it takes to bring up and bring down the whole thing, see here.

I see your point with the incompatible options. When this becomes an issue we can introduce some versioning or something else to address that.

apelisse · 2017-11-28T22:13:57Z

pkg/framework/test/apiserver_test.go

+	Context("when given a path to a non-executable", func() {
+		It("fails with a helpful error", func() {
+			apiServer := APIServer{
+				Path: "./apiserver.go",


if we ever use bazel, that's probably going to break. IOW, it creates a weird implicit dependency on that file being here. Maybe create a tempfile?

Good point -- we can use a temporary file here.

Is this also an issue with any use of gexec.Build ?

apelisse · 2017-11-28T22:17:39Z

pkg/framework/test/apiserver_test.go

+
+	Context("when given a path to a binary that runs for a long time", func() {
+		It("can start and stop that binary", func() {
+			pathToFakeAPIServer, err := gexec.Build("k8s.io/kubectl/pkg/framework/test/assets/fakeapiserver")


I wonder if this is testing gexec or the framework. The coupling with main.go below is kind of awkward.

When you say "the coupling with main.go below is kind of awkward", we think you're primarily referring to the duplication between the arguments in test/apiserver.go and fakeapiserver/apiserver.go. We totally agree that this is awkward.

That fake is only used by this one test -- so if we delete this test, we can delete that fake. That might be a good thing -- especially since the wiring which that test was originally written to drive out is now covered by a higher level integration test. The non-happy-path cases in this file cover code that's not covered anywhere else, so we'd definitely like to keep those.

From a "tests as docs" viewpoint, it feels like a little bit of a pity to leave that file as an incomplete doc -- describing only the error cases... But maybe that's actually just fine? We might have a bit of a think about whether we can write any useful happy-path test (motivated primarily by wanting to keep some executable docs for that path) that doesn't use that fake.

apelisse · 2017-11-28T22:20:39Z

pkg/framework/test/assets/fakeetcd/etcd.go

+)
+
+func main() {
+	expectedArgs := []string{


Same comment as for the apiserver. This feels kind of awkward and out of place.

apelisse · 2017-11-28T22:30:59Z

pkg/framework/test/fixtures.go

@@ -0,0 +1,83 @@
+package test


Thoughts about fixtures.

In kubernetes, the etcd server is mostly hidden behind the apiserver. I'm wondering if the abstraction should be:

apiserver := APIServer{Path: "/bin/apiserver", Etcd: Etcd{Path: "/bin/etcd"}} // The test doesn't really need etcd // Maybe going further, and completely hiding the apiserver: kubectl := Kubectl{Path: "/bin/kubectl", apiserver: APIServer{Path: "/bin/apiserver", Etcd: Etcd{Path: "/bin/etcd"}}} // We have a kubectl that is connected to the apiserver that is connected to etcd. // I don't have to KNOW anything about apiserver, and how it's connected. If I need, I can keep a handle to the APIServer object, but I don't have to.

IOW: explicit dependency between the two components. The connection between the two would be somewhat hidden from the user.

Interested in your opinion?

A couple of random thoughts we just discussed:

We tried to make the interface for the processes we start in our framework rather generic. That should make it relatively easy to introduce new processes (Scheduler, Controller(Manager), ...) somewhen in the future.

We see that the APIServer depends on Etcd -- from that point of view it might be a good idea to make this dependency explicit as you suggested.

I believe we can run the APIServer without Etcd. When Etcd eventually comes up APIServer will connect and everything will just work. We might have test which actually test with a missing Etcd and/or an dis- and reappearing Etcd.

( What if I'd like to run my tests against a already running instance of Etcd on a different machine or so?[1] )

So we are not against making the dependency explicit, we think both ways have their pros and cons. I -- personally -- would opt for keeping it as is for now, but that's just a gut feeling. We are happy to discuss that in more depth!

[1] That just came to our minds -- we know that is not supported or even planned to be supported by our framework. However we thought that might be a valuable use case to keep in mind ...

apelisse · 2017-11-29T01:00:49Z

pkg/framework/test/integration/integration_test.go

+
+var _ = Describe("Integration", func() {
+	It("Successfully manages the fixtures lifecycle", func() {
+		assetsDir, ok := os.LookupEnv("KUBE_ASSETS_DIR")


Are you planning on having a generic way to give the binaries? KUBE_TEST_APISERVER and KUBE_TEST_ETCD, so that my test can easily switch the underlying binary?

Yes. We have that in the back of our heads. We have one story in our backlog for that.

apelisse · 2017-11-29T01:01:35Z

pkg/framework/test/integration/integration_test.go

+)
+
+var _ = Describe("Integration", func() {
+	It("Successfully manages the fixtures lifecycle", func() {


Would it make sense to have a benchmark test to see how long it takes to spawn and stop a fixture? I think that's a metric we'll be interested in.

We introduced that in "Track the time it takes to start&stop the fixtures"

You can find the timings in our pipeline:

https://wings.concourse.ci/teams/k8s-c10s/pipelines/kubectl/jobs/test-dev-branch/builds/23#L5a1d829b:71

https://wings.concourse.ci/teams/k8s-c10s/pipelines/kubectl/jobs/test-dev-branch/builds/22#L5a1d90c7:27

https://wings.concourse.ci/teams/k8s-c10s/pipelines/kubectl/jobs/test-dev-branch/builds/20#L5a1d8f04:71

https://wings.concourse.ci/teams/k8s-c10s/pipelines/kubectl/jobs/test-dev-branch/builds/16#L5a1558c6:24

https://wings.concourse.ci/teams/k8s-c10s/pipelines/kubectl/jobs/test-dev-branch/builds/15#L5a1558c3:20

https://wings.concourse.ci/teams/k8s-c10s/pipelines/kubectl/jobs/test-dev-branch/builds/14#L5a15589a:19

https://wings.concourse.ci/teams/k8s-c10s/pipelines/kubectl/jobs/test-dev-branch/builds/13#L5a1558b8:62

https://wings.concourse.ci/teams/k8s-c10s/pipelines/kubectl/jobs/test-dev-branch/builds/12#L5a1558b5:65

BTW: You can run our tests via ./scripts/run-tests.sh; If you run GINKGO_PERFORMANCE=1 ./scripts/run-tests.sh it will also do some benchmarking.

That's super exciting to see! The numbers are not that exciting. Let's see later what we can do.

apelisse · 2017-11-29T01:02:36Z

pkg/framework/test/etcd.go

 	args := []string{
+		"--debug",


I have the feeling that this is going to be painful to maintain?

If this is only about using --debug or not, we can totally remove that line. If it's about management of arguments (potentially for different versions, ...), we believe we have already covered that in the other comment.

apelisse · 2017-11-29T01:08:32Z

pkg/framework/test/temp_dir_manager.go

+// Create knows how to create a temporary directory and how to keep track of it.
+func (t *TempDirManager) Create() (string, error) {
+	if t.dir == "" {
+		dir, err := t.Maker("", "kube-test-framework")


kube-test-framework- ? (extra-dash)

apelisse · 2017-11-29T01:11:48Z

pkg/framework/test/integration/integration_test.go

@@ -27,11 +27,11 @@ var _ = Describe("Integration", func() {

 		Eventually(func() bool {
 			return isSomethingListeningOnPort(2379)
-		}, 5*time.Second).Should(BeTrue(), "Expected Etcd to listen on 2379")
+		}, 25*time.Second).Should(BeTrue(), "Expected Etcd to listen on 2379")


I foresee ❄️ :-)

Yeah ... we need to figure out how to handle that properly -- we dropped a story in our backlog for that.

We use [ginkgo](http://onsi.github.io/ginkgo/) and [gomega](http://onsi.github.io/gomega/) for testing. We generate some boilerplate with: ``` mkdir integration cd integration ginkgo bootstrap ginkgo generate integration ``` We use [gexec](http://onsi.github.io/gomega/#gexec-testing-external-processes) to compile and run the CLI under test, and to inspect its output. We use `dep ensure` to ensure that all our dependencies are properly vendored. From now on, this will be our workflow with every commit.

As a separate commit, to make review easier.

We use [ginkgo](http://onsi.github.io/ginkgo/) and [gomega](http://onsi.github.io/gomega/) for testing. We generate some boilerplate with: ``` mkdir integration cd integration ginkgo bootstrap ginkgo generate integration ``` We use [gexec](http://onsi.github.io/gomega/#gexec-testing-external-processes) to compile and run the CLI under test, and to inspect its output. We use `dep ensure` to ensure that all our dependencies are properly vendored. From now on, this will be our workflow with every commit.

As a separate commit, to make review easier.

To start an apiserver: ``` apiServer := APIServer{Path: "/path/to/my/apiserver/binary"} session, err := apiServer.Start("tcp://whereever.is.my.etcd:port") Expect(err).NotTo(HaveOccurred()) ``` When you're done testing against that apiserver: ``` session.Terminate().Wait() ``` ...or if you prefer: ``` gexec.Terminate() ``` ...which will terminate not only this apiserver, but also all other command sessions you started in this test.

This can be started and stopped the same way as the apiserver.

Create a new set of test fixtures by doing: ``` f := test.NewFixtures("/path/to/etcd", "/path/to/apiserver") ``` Before running your integration tests, start all your fixtures: ``` err := f.Start() Expect(err).NotTo(HaveOccurred()) ``` Now that you have started your etcd and apiserver, you'll find the apiserver listening locally on the default port. When you're done with your testing, stop and clean up: ``` err := f.Stop() Expect(err).NotTo(HaveOccurred()) ```

We're using concourse because we happen to have a concourse deployment available. You can look at it here: https://wings.concourse.ci/teams/k8s-c10s/

We're not exercising the test framework yet, but it's in place. Our democli expects its test assets to be in `./assets/bin`. We have a script `./scripts/download-binaries.sh` which will populate that directory from a google storage bucket. Once those assets are in place, you can run tests with `./scripts/run-tests.sh`.

os.tempDir() gives the path to the temporary directory, it does not create a random temporary directory.

Using tcp:// does not work.

Testing the lifecycle of our fixtures with the real binaries. Test if we can start the fixtures, the porcesses actually listen on the ports and if we tear down all the parts successfully again.

- Store stdout,stderr in private buffers - Configure the etcURL on construction instead of at start time - Handle the creation of the temporary directory (for the data directory) internally

- Store stdout,stderr in private buffers - Configure the etcURL on construction instead of at start time

Instead of the separate {Etcd,APIServer}StartStopper use the unified interface FixtureProcess

We are now returning an error instead of using an Expectation inline.

- Start() should only return when the process is actually up and listening - It may take some time to tear down a process, so we increased the timeout for Stop() (to some random number) - We make sure Std{Out,Err} is properly initialized, we should not rely on Ginkgo/Gomega to do that for us

... as they have been unified into the FixtureProcess interface and thus they are not needed anymore.

We use the standard go client for kubernetes `client-go`. To vendor it and all its denpendecies we use ``` dep ensure -add k8s.io/client-go@5.0.0 ``` We create a new cobra command with ``` cobra add listPods ``` Note: The new command in cmd/listPods.go uses [the "magic" function init()](https://golang.org/ref/spec#Package_initialization) to register itself.

As a separate commit, to make review easier.

Performance tests are now skipped by default, but run in CI.

While doing that we found that we needed to refactor the fakes to handle command line arguments which are not known up front; we do this by using regular expresseions.

[#153243856]

[#153246098]

- Use a `.gitignore` local to the test framework - Remove some local only things (`.idea`) from the `.gitignore` and push that to `.git/info/exclude`

Actually now put the dependencies into the global `vendor` direcotry.

Eventually we want our framework to work nicely with just `go test`. To get there we need to - inject KUBE_ASSETS_DIR - make the framework work when run multiple times in parallel (port collitions, expose bound ports the the subject under test, ...) We decided to make sure our tests are run in sequence (and not in parallel to any other thing using etcd, for that matter) by making this explicit in the `pre-commit.sh` - for now. As soon as we are there, we can rollback the change to the `pre-commit.sh` end have the test framework be tested the same as everything else. [#153248975]

apelisse · 2017-11-29T21:30:20Z

pkg/framework/test/democli/cmd/listPods.go

+}
+
+func runGetPods() {
+	config, _ := clientcmd.BuildConfigFromFlags("http://localhost:8080", "")


Should this be a parameter instead?

Definitely. We want to have our framework generate some sort of config (the framework will be the thing that determines the port eg. the APIServer is listening on) and somehow expose that to the clients resp. the component under test. Until we have that and somewhat in the spirit of TDD we just did the simplest thing possible to make our stuff work. Next iterations will most definitely change that.

apelisse · 2017-11-29T21:30:57Z

pkg/framework/test/democli/integration/integration_test.go

@@ -20,4 +20,11 @@ var _ = Describe("Integration", func() {
 		Expect(session.Out).To(gbytes.Say(helpfulMessage))
 	})

+	It("can get a list of pods", func() {
+		command := exec.Command(pathToDemoCommand, "listPods")


APIServer coordinates should be passed here.

apelisse · 2017-11-29T21:32:33Z

pkg/framework/test/integration/integration_test.go

@@ -40,6 +32,15 @@ var _ = Describe("Integration", func() {
 		By("Ensuring APIServer is not listening anymore")
 		Expect(isAPIServerListening()).To(BeFalse(), "Expected APIServer not to listen anymore")
 	})
+
+	Measure("It should be fast to bring up and tear down the fixtures", func(b Benchmarker) {


Cool! I didn't know that!

apelisse · 2017-11-29T21:35:48Z

pkg/framework/test/etcd.go

 }

-// DataDirManager knows how to create and destroy Etcd's data directory.
-type DataDirManager interface {
+type dataDirManager interface {


Or you can move that to an internal packages if you want to reuse it somewhere else.

apelisse · 2017-11-29T21:36:22Z

pkg/framework/test/assets/fakeapiserver/apiserver.go

 	"time"
 )

 func main() {
-	expectedArgs := []string{


This list is going to be a nightmare to maintain. Isn't it already?

apelisse · 2017-11-29T21:38:23Z

bin/pre-commit.sh

@@ -19,7 +19,12 @@ go vet -all ./...
 rc=$((rc || $?))

 echo "Running go test"
-go test -v ./...
+go list ./... | grep -vF pkg/framework/test | xargs go test -v


I think there should be some sort of "default" value for KUBE_ASSERT_DIR and maybe we can download the binaries before running go test?

Agreed. I think this also ties back to making our think work well with go test. As soon as we have that we can retire our test wrapper script pkg/framework/test/scripts/run-tests.sh and think about better ways of handling the test binaries/assets resp. their location. We in fact have all those things in our backlog.

apelisse

This is good for now.

Things that are really important to improve:

Fix the go test hack,
Easier to inject fixtures components (apiserver, etcd, ...) or sane default,

Thanks!

totherme · 2017-11-30T11:28:21Z

Thanks @apelisse for all your comments! We've opened issues to track your two main concerns: #152 and #153 . We'll start work on those now.

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

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 21, 2017

totherme force-pushed the test-framework branch 2 times, most recently from 42b51c7 to 6a2d02b Compare November 27, 2017 20:59

totherme force-pushed the test-framework branch from 59630f0 to 6c49758 Compare November 28, 2017 16:45

apelisse reviewed Nov 29, 2017

View reviewed changes

totherme and others added 22 commits November 29, 2017 12:08

Add dependencies

bedfd1e

As a separate commit, to make review easier.

Ignore goland's .idea directory

c7957c3

Add dependencies

782bee8

As a separate commit, to make review easier.

Add etcd launcher to test framework

6776524

This can be started and stopped the same way as the apiserver.

Add CI for local development

1714b31

We're using concourse because we happen to have a concourse deployment available. You can look at it here: https://wings.concourse.ci/teams/k8s-c10s/

Use ioutils.tempDir() to create temporary directories

4e870ff

os.tempDir() gives the path to the temporary directory, it does not create a random temporary directory.

Use http:// as etcd URL scheme

0dfe20e

Using tcp:// does not work.

Make Stop() wait for the processes to exit

8a92d31

Add fixtures tintegration tests

e7bb1e8

Testing the lifecycle of our fixtures with the real binaries. Test if we can start the fixtures, the porcesses actually listen on the ports and if we tear down all the parts successfully again.

Rework fakes to allow additional arguments

a04f002

Refactor Etcd

2fd15f8

- Store stdout,stderr in private buffers - Configure the etcURL on construction instead of at start time - Handle the creation of the temporary directory (for the data directory) internally

Refactor APIServer

940bec8

- Store stdout,stderr in private buffers - Configure the etcURL on construction instead of at start time

Unify fixture processes

ddd0a56

Instead of the separate {Etcd,APIServer}StartStopper use the unified interface FixtureProcess

Extract TempDirManager

d99f3dc

We are now returning an error instead of using an Expectation inline.

Allow more time to bring up the fixtures

e2f4cd5

Refactor away anonymous functions in integration test

5d670c4

totherme and others added 14 commits November 29, 2017 12:08

Remove {Etcd,APIServer}StartStopper interfaces

2e5a832

... as they have been unified into the FixtureProcess interface and thus they are not needed anymore.

Add dependencies

eca5aac

As a separate commit, to make review easier.

Track the time it takes to start&stop the fixtures

e2a217b

Start the fixture processes in parallel

8523ad3

Add test option to run performance tests

63de385

Performance tests are now skipped by default, but run in CI.

Use a temporary directory for the APIServer's certs

7df93be

While doing that we found that we needed to refactor the fakes to handle command line arguments which are not known up front; we do this by using regular expresseions.

Make Etcd's DataDirManager interface private

f5b7280

Drop privilages before running tests in CI

ff9f027

Update comments on Start/Stop of the processes

b7cfe0f

[#153243856]

Create nicer names for temporary directories

c4e57df

[#153246098]

Change .gitignore strategy

d88f71c

- Use a `.gitignore` local to the test framework - Remove some local only things (`.idea`) from the `.gitignore` and push that to `.git/info/exclude`

Switch to repo-global vendoring

d89f4c1

Switch to repo-global vendoring, move dependencies

849d4f9

Actually now put the dependencies into the global `vendor` direcotry.

totherme force-pushed the test-framework branch from 25badce to 849d4f9 Compare November 29, 2017 13:33

hoegaarden added 2 commits November 29, 2017 15:20

Make the framework test bash3 compatible

4d4026f

totherme force-pushed the test-framework branch from 3df0191 to 4d4026f Compare November 29, 2017 16:56

Install ginkgo

a304d58

apelisse reviewed Nov 29, 2017

View reviewed changes

apelisse approved these changes Nov 29, 2017

View reviewed changes

k8s-ci-robot merged commit 4f1e692 into kubernetes:master Nov 29, 2017

cjwagner mentioned this pull request Nov 29, 2017

Merge kubectl approved PRs kubernetes/test-infra#5720

Merged

This was referenced Nov 30, 2017

The Test Framework should not require special treatment in pre-commit.sh #152

Closed

The Test Framework's test fakes shouldn't be so tightly coupled with the implementations they test #153

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Beginnings of a test framework #149

Beginnings of a test framework #149

totherme commented Nov 21, 2017

k8s-ci-robot commented Nov 21, 2017

apelisse commented Nov 27, 2017

apelisse left a comment

apelisse Nov 28, 2017

hoegaarden Nov 29, 2017

apelisse Nov 28, 2017

hoegaarden Nov 29, 2017

apelisse Nov 28, 2017

hoegaarden Nov 29, 2017

apelisse Nov 28, 2017

apelisse Nov 28, 2017

hoegaarden Nov 29, 2017

apelisse Nov 29, 2017

hoegaarden Nov 29, 2017

apelisse Nov 29, 2017

hoegaarden Nov 29, 2017 •

edited

Loading

hoegaarden Nov 29, 2017

apelisse Nov 29, 2017

apelisse Nov 29, 2017

hoegaarden Nov 29, 2017

apelisse Nov 29, 2017

apelisse Nov 29, 2017

hoegaarden Nov 29, 2017

apelisse Nov 29, 2017

hoegaarden Nov 29, 2017 •

edited

Loading

apelisse Nov 29, 2017

apelisse Nov 29, 2017

apelisse Nov 29, 2017

apelisse Nov 29, 2017

apelisse Nov 29, 2017

hoegaarden Nov 29, 2017

apelisse left a comment

totherme commented Nov 30, 2017

Beginnings of a test framework #149

Beginnings of a test framework #149

Conversation

totherme commented Nov 21, 2017

k8s-ci-robot commented Nov 21, 2017

apelisse commented Nov 27, 2017

apelisse 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

hoegaarden Nov 29, 2017 • edited Loading

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

hoegaarden Nov 29, 2017 • edited Loading

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

apelisse left a comment

Choose a reason for hiding this comment

totherme commented Nov 30, 2017

hoegaarden Nov 29, 2017 •

edited

Loading

hoegaarden Nov 29, 2017 •

edited

Loading