Add E2E tests #32

xmudrii · 2018-06-26T11:34:00Z

This PR adds E2E tests for the EtcdProxyController.

The E2E tests assumes you have a working Kubernetes cluster and EtcdProxy Controller deployed. The hack/run-e2e.sh script, which is used by CI, deploys the controller from the deployment manifest located in the artifacts directory.

By default, tests use the kubeconfig located in $HOME/.kube/config. The kubeconfig path can be changed by modifying the kubeconfigPath variable in the test/e2e/e2e_test.go file.

The tests are located in the test/e2e/e2e_test.go file. Currently, we have one test function which tests:

Is EtcdStorage creation successful,
Are EtcdStorage conditions set as intended,
Is ReplicaSet with the expected name deployed,
Is expected number of pods available (running),
Is Service with the expected name created.

We're testing two cases for now:

EtcdStorage is valid,
EtcdStorage name is too long.

I'm also working on testing are all resources going to be deleted once the EtcdStorage object is deleted.

xmudrii

I've left some questions.

xmudrii · 2018-06-26T11:39:24Z

Makefile

-verify-ci: install-tools ## Run code checks
-	PKGS="${GOFILES}" GOFMT="gofmt" ./hack/verify-ci.sh
+.PHONY: test-e2e
+test-e2e: ## Run E2E tests. Requires working Kubernetes cluster and a Kubeconfig file.


This should be updated to use the run-e2e.sh script.

A follow-up question: do we need to leave a note that this is potentially destructive operation and to be sure you're running it on the correct cluster?

A follow-up question: do we need to leave a note that this is potentially destructive operation and to be sure you're running it on the correct cluster?

note that your tests may be destructive. We can assume these people know what they're doing and get what they deserve.

Added a note and fixed the command.

xmudrii · 2018-06-26T11:40:22Z

test/e2e/e2e_test.go

+)
+
+// kubeconfigPath is path to the kubernetes configuration file.
+var kubeconfigPath = filepath.Join(os.Getenv("HOME"), "/.kube/config")


Is there a better way for providing kubeconfigPath? Maybe with an environment variable, or this is good enough?

Is there a better way for providing kubeconfigPath? Maybe with an environment variable, or this is good enough?

We respect KUBECONFIG in most (all?) kube binaries. I'd use that.

This is a great idea, updated the PR to use it.

xmudrii · 2018-06-26T11:40:51Z

test/e2e/e2e_test.go

+var kubeconfigPath = filepath.Join(os.Getenv("HOME"), "/.kube/config")
+
+// newKubeClient create new Kubernetes client instance based on kubeconfig file.
+func newKubeClient(kubeconfigPath string) (*kubernetes.Clientset, error) {


Is it okay to leave those functions in this file, or should I create a new file such as util.go?

Is it okay to leave those functions in this file, or should I create a new file such as util.go?

don't create a "util" anything until absolutely necessary. Let's say 1000 lines. They generally become a bucket of stuff.

xmudrii · 2018-06-26T11:41:37Z

test/e2e/e2e_test.go

+	return clientset.NewForConfig(config)
+}
+
+func replicasetName(etcdstorageName string) string {


My idea was to have a helper functions for calculating names, instead of hardcoding them in test cases, to make potential changes easier.

My idea was to have a helper functions for calculating names, instead of hardcoding them in test cases, to make potential changes easier.

I generally question the utility of one liners. Do you think it's likely we'll change our minds? How many places would we really have to update it? Should such a change be hard since we're changing external behavior?

Do you think it's likely we'll change our minds?

I don't think we'll ever change those. Even if we do in some case, that would be just a few lines to update. Let's get rid of those functions.

xmudrii · 2018-06-26T11:48:24Z

test/e2e/e2e_test.go

+	return fmt.Sprintf("etcd-%s", etcdstorageName)
+}
+
+func TestDeployEtcdStorage(t *testing.T) {


Is it okay to have all these checks in this function, or I should split them to multiple tests? In this function we're testing is the EtcdStorage instance deployed, and are all relevant resources deployed.

TODO: add godoc comment

Added godoc comment.

xmudrii · 2018-06-26T11:52:15Z

test/e2e/e2e_test.go

+		}
+
+		// Only continue if etcdStorage is expected to be valid.
+		if tc.etcdStorageValid {


I have a problem here.

We have defined that EtcdStorages name length should be less than 59 characters (64 which is max for Services, -5 for etcd- prefix). I added a test case for that, but, this is going to fail on Create.

But that failure is expected, so I was not sure how to stop there. I added etcdStorageValid bool, and if it is false, error on Create is going to be ignored, and we're not going to continue.

Is there a better way to mark test as passed, than this one? There is 'FailNow(), which fails, but I don't want fail.
There is also SkipNow(), but it marks test as skipped, and I was not sure do we want that.

xmudrii · 2018-06-26T11:52:44Z

test/e2e/e2e_test.go

+		// Only continue if etcdStorage is expected to be valid.
+		if tc.etcdStorageValid {
+			// It takes some time for the ReplicaSet and Service to be created, so we need to poll.
+			err = wait.Poll(500*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) {


TODO: check for error.

xmudrii · 2018-06-26T11:52:48Z

test/e2e/e2e_test.go

+				t.Fatalf("expected replicaset '%s', but got: %v", tc.expectedReplicaSetName, err)
+			}
+			// Wait for pods to become ready.
+			err = wait.Poll(500*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) {


TODO: check for error.

xmudrii · 2018-06-26T11:52:59Z

test/e2e/e2e_test.go

+			}
+
+			// Check is ReplicaSet deployed.
+			rs, err := client.AppsV1().ReplicaSets("kube-apiserver-storage").Get(tc.expectedReplicaSetName, metav1.GetOptions{})


This is not needed, we have it in wait.Poll.

deads2k · 2018-06-26T11:58:06Z

.travis.yml

+      script: make test-e2e
+      before_script:
+        # Download kubectl, which is a requirement for using minikube and running e2e tests.
+        - curl -Lo kubectl https://storage.googleapis.com/kubernetes-release/release/${KUBERNETES_VERSION}/bin/linux/amd64/kubectl && chmod +x kubectl && sudo mv kubectl /usr/local/bin/


@soltysh @liggitt next release I'm solving my oc problems like this! :)

I object 😝

deads2k · 2018-06-26T11:59:05Z

.travis.yml

+        # Update the kubeconfig to use the minikube cluster.
+        - minikube update-context
+        # Wait for Kubernetes to be up and ready.
+        - JSONPATH='{range .items[*]}{@.metadata.name}:{range @.status.conditions[*]}{@.type}={@.status};{end}{end}'; until kubectl get nodes -o jsonpath="$JSONPATH" 2>&1 | grep -q "Ready=True"; do sleep 1; done


In addition, I'd suggest waiting for the server's /healthz to report true.

Added a TODO note. I'll try to get it for this PR.

deads2k · 2018-06-26T12:12:27Z

test/e2e/e2e_test.go

+	return kubernetes.NewForConfig(config)
+}
+
+func newEtcdProxyClinet(kubeconfigPath string) (*clientset.Clientset, error) {


deads2k · 2018-06-26T14:01:36Z

test/e2e/e2e_test.go

+
+// newEtcdProxyClient creates new client for etcdstorage resources.
+func newEtcdProxyClient() (*clientset.Clientset, error) {
+	config, err := clientcmd.BuildConfigFromFlags("", os.Getenv("KUBECONFIG"))


I would have one helper to create the config and then do the NewForConfig call inline. I also tolerate NewForConfigOrDie in tests.

deads2k · 2018-06-26T14:02:00Z

test/e2e/e2e_test.go

+			etcdStorageValid:       true,
+		},
+		{
+			name: "test etcdstorage creation - name too long",


this should be a unit test

deads2k · 2018-06-26T14:02:57Z

test/e2e/e2e_test.go

+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			// Create an EtcdStorage instance.
+			es, err := etcdproxyClient.EtcdV1alpha1().EtcdStorages().Create(tc.etcdStorage)


missing deletion/cleanup

deads2k · 2018-06-26T14:04:12Z

This is fine, but in general I expect my e2e tests to tell a story. In kube, they alias kubectl and use an exec helper to instantiate manifests and check status on that.

The ideal e2e test would be creating your sample manifests. If you want to simply create a bash script you call to do that for now, that would be fine, but I think it is important to do.

xmudrii · 2018-06-27T11:35:13Z

@deads2k I've addressed comments from yesterday and added an E2E story for the sample-apiserver. Compared to EtcdStorage story, which is using a Bash script + Go, the sample-apiserver story is only using the bash script.

The tests for sample-apiserver are checking is the pod ready. This is probably not enough, but I don't have idea what else could I check. If you have some ideas, let me know. :)

Also, I've disabled E2E tests in Travis as they're failing. The tests are passing locally, so for now, let's leave it off if you agree.

deads2k · 2018-06-27T12:15:37Z

hack/run-e2e.sh

+    READY_REPLICAS=$(kubectl get rs apiserver -o jsonpath="{.status.readyReplicas}")
+done
+
+echo '* Starting sample-apiserver e2e tests.'


followup issue to actually create a resource.

deads2k · 2018-06-27T12:16:11Z

test/e2e/e2e_test.go

+	// Initialize clients.
+	cfg, err := clientcmd.BuildConfigFromFlags("", os.Getenv("KUBECONFIG"))
+	if err != nil {
+		t.Fatalf("unable to create kubernetes config from provided kubeconfig file: %v", err)


nit: I generally just use t.Fatal(err). You get a line number, so you can track it back easily enough.

deads2k · 2018-06-27T12:17:27Z

Also, I've disabled E2E tests in Travis as they're failing. The tests are passing locally, so for now, let's leave it off if you agree.

Yeah, that's fine. Don't forget to run your e2e and tell us in the description whether it worked or not.

Add Minikube cluster to Travis-CI

51040ea

xmudrii added the do-not-merge/wip Indicates Work-In-Progress Pull Requests. label Jun 26, 2018

xmudrii assigned deads2k Jun 26, 2018

xmudrii force-pushed the e2e-tests branch from 3c5c8f0 to d40d04c Compare June 26, 2018 11:36

xmudrii commented Jun 26, 2018

View reviewed changes

deads2k reviewed Jun 26, 2018

View reviewed changes

xmudrii force-pushed the e2e-tests branch 2 times, most recently from 467d4e3 to d80f700 Compare June 26, 2018 15:46

E2E tests testing are EtcdStorage resources deployed

a1be2a8

xmudrii force-pushed the e2e-tests branch from e76b0f8 to a07b620 Compare June 27, 2018 11:28

E2E tests testing sample-apiserver manifests. Address code review.

c41c9f3

xmudrii force-pushed the e2e-tests branch from a07b620 to c41c9f3 Compare June 27, 2018 11:30

xmudrii changed the title ~~[WIP] Add E2E tests~~ Add E2E tests Jun 27, 2018

xmudrii added ready-for-review Indicates that a Pull Request is ready for review. and removed do-not-merge/wip Indicates Work-In-Progress Pull Requests. labels Jun 27, 2018

xmudrii added this to the Milestone 2 milestone Jun 27, 2018

deads2k reviewed Jun 27, 2018

View reviewed changes

deads2k approved these changes Jun 27, 2018

View reviewed changes

deads2k merged commit 0133578 into master Jun 27, 2018

xmudrii deleted the e2e-tests branch June 27, 2018 12:20

xmudrii mentioned this pull request Jun 27, 2018

Add deploying Flunder resource to the sample-apiserver E2E story and fix minor bugs #33

Merged

This was referenced Jun 27, 2018

Add E2E tests #24

Closed

Enable E2E tests in Travis-CI using Minikube and Kubernetes v1.10.0 #39

Merged

Test all Kubernetes components work lilic/travis-minikube#9

Closed

Add E2E tests #32

Add E2E tests #32

Conversation

xmudrii commented Jun 26, 2018

xmudrii 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

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

deads2k commented Jun 26, 2018

xmudrii commented Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jun 27, 2018

xmudrii commented Jun 27, 2018 •

edited

Loading