Client tests #43

seans3 · 2018-06-15T23:17:09Z

First pass at tests for client.go
Currently 79.6% coverage

seans3 · 2018-06-15T23:17:51Z

/assign droot
/assign pwittrock

pwittrock · 2018-06-16T02:53:44Z

@seans3 error should be legit now. Looks like a linting error.

droot

@seans3 changes are looking good, have some comments.

droot · 2018-06-18T17:33:28Z

pkg/client/client_test.go

+		dgvk := dgv.WithKind("Deployment")
+		ngvk := ngv.WithKind("Node")
+		scheme.AddKnownTypeWithName(dgvk, dep)
+		scheme.AddKnownTypeWithName(ngvk, node)


client users should not be required to register core types, by default, client should use scheme from the core.

droot · 2018-06-18T17:42:36Z

pkg/client/client_test.go

@@ -134,16 +169,25 @@ var _ = Describe("Client", func() {
 			close(done)
 		})

-		It("should create a new object from an unstructured object", func() {
+		It("should create a new object from an unstructured object", func(done Done) {


Most of the time, our users are going to be using the core/custom types but unstructured. So not sure if unstructured test really helps ?

Phil asked to do the unstructured, which gets slightly better code coverage. So let's go with it for now.

droot · 2018-06-18T17:47:36Z

pkg/client/client_test.go

-			// TODO(seans3): implement these
+			By("creating client with empty Scheme")
+			emptyScheme := runtime.NewScheme()
+			cl, err := client.New(cfg, client.Options{Scheme: emptyScheme})


We can make it more closer to the actual user's usecase where: we create the cl without specifying the scheme so that client is initialized with default scheme which understands the core-type resources and then use a custom struct (we can define one in this test file) for Create method and it should fail.

Its good to check that the provided scheme is actually used though :)

I'll add a TODO for this. I don't want this PR to take much longer, so I'll address further checks in a future PR.

droot · 2018-06-18T17:48:59Z

pkg/client/client_test.go

 			dep.Annotations = map[string]string{"foo": "bar"}
 			err = cl.Update(context.TODO(), dep)
 			Expect(err).NotTo(HaveOccurred())

-			By("writing the result back to the go struct")
+			By("validating updated Deployment exists")
 			actual, err := clientset.AppsV1().Deployments(ns).Get(dep.Name, metav1.GetOptions{})
 			Expect(err).NotTo(HaveOccurred())


We aren't validating if the object got updated here.

droot · 2018-06-18T17:49:52Z

pkg/client/client_test.go

 			actual, err := clientset.AppsV1().Deployments(ns).Get(dep.Name, metav1.GetOptions{})
 			Expect(err).NotTo(HaveOccurred())
 			Expect(actual).NotTo(BeNil())

 			close(done)
 		})

-		It("should update an existing new object from an unstructured object", func() {
-			// TODO(seans3): implement these
+		It("should update an existing new object from an unstructured object", func(done Done) {


As I said earlier, not sure if unstructured tests help our usecase ?

Phil asked to do the unstructured, which gets slightly better code coverage. So let's go with it for now.

droot · 2018-06-18T17:50:28Z

pkg/client/client_test.go

+
+			By("fetching newly created unstructured Deployment")
+			actual, err := clientset.AppsV1().Deployments(ns).Get(dep.Name, metav1.GetOptions{})
+			Expect(err).NotTo(HaveOccurred())


We aren't verifying the update actually happened.

Doesn't this line do that:

Expect(actual.Annotations["foo"]).To(Equal("bar"))

I just pushed this commit to address Sunil's concern.

droot · 2018-06-18T17:52:28Z

pkg/client/client_test.go

+		It("should fail if the object cannot be mapped to a GVK", func(done Done) {
+			By("creating client with empty Scheme")
+			emptyScheme := runtime.NewScheme()
+			cl, err := client.New(cfg, client.Options{Scheme: emptyScheme})


same comment as earlier about not specifying the scheme.

I think it makes it easier to check for things not in the Scheme if an empty one is passed in

For this one, I'll keep the empty scheme.

pwittrock · 2018-06-18T22:05:33Z

pkg/client/client_test.go

+
+			By("creating the pod, since required field Containers is empty")
+			err = cl.Create(context.TODO(), pod)
+			Expect(err).To(HaveOccurred())


Would you also check the error message to make sure it is actually the expected message and not something unrelated?

Removing this test. The local versus travis tests vary.

Locally: Pod "pod-11" is invalid: spec.containers: Required value
Travis: pods "pod-11" is forbidden: error looking up service account default/default: serviceaccount "default" not found

For now I'm removing the substring test.

pwittrock · 2018-06-19T00:35:21Z

/lgtm

pwittrock · 2018-06-19T00:35:24Z

/approve

k8s-ci-robot · 2018-06-19T00:51:35Z

New changes are detected. LGTM label has been removed.

seans3 · 2018-06-19T01:22:01Z

/lgtm

k8s-ci-robot · 2018-06-19T01:22:01Z

@seans3: you cannot LGTM your own PR.

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Client tests

Change watching events to use interface instead of struct

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 15, 2018

k8s-ci-robot assigned droot and pwittrock Jun 15, 2018

pwittrock closed this Jun 16, 2018

pwittrock reopened this Jun 16, 2018

droot reviewed Jun 18, 2018

View reviewed changes

seans3 force-pushed the client-tests branch from cacda0d to 6c3b351 Compare June 18, 2018 21:17

pwittrock reviewed Jun 18, 2018

View reviewed changes

seans3 force-pushed the client-tests branch from 7f6861a to 65d39d5 Compare June 19, 2018 00:24

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

pwittrock added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 19, 2018

First version of client tests

3e2d6d2

seans3 force-pushed the client-tests branch from 65d39d5 to 3e2d6d2 Compare June 19, 2018 00:51

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

pwittrock added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 19, 2018

k8s-ci-robot merged commit 89a112f into kubernetes-sigs:master Jun 19, 2018

justinsb pushed a commit to justinsb/controller-runtime that referenced this pull request Dec 7, 2018

Merge pull request kubernetes-sigs#43 from seans3/client-tests

a4f35e8

Client tests

DirectXMan12 pushed a commit that referenced this pull request Jan 31, 2020

Merge pull request #43 from pwittrock/master

ed1c5d9

Change watching events to use interface instead of struct

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client tests #43

Client tests #43

seans3 commented Jun 15, 2018

seans3 commented Jun 15, 2018

pwittrock commented Jun 16, 2018

droot left a comment

droot Jun 18, 2018

seans3 Jun 18, 2018

droot Jun 18, 2018

seans3 Jun 18, 2018

droot Jun 18, 2018

pwittrock Jun 18, 2018

seans3 Jun 18, 2018

droot Jun 18, 2018

droot Jun 18, 2018

seans3 Jun 18, 2018

droot Jun 18, 2018

pwittrock Jun 18, 2018

seans3 Jun 18, 2018

droot Jun 18, 2018

pwittrock Jun 18, 2018

seans3 Jun 18, 2018

pwittrock Jun 18, 2018

seans3 Jun 18, 2018

seans3 Jun 18, 2018

pwittrock commented Jun 19, 2018

pwittrock commented Jun 19, 2018

k8s-ci-robot commented Jun 19, 2018

seans3 commented Jun 19, 2018

k8s-ci-robot commented Jun 19, 2018

Client tests #43

Client tests #43

Conversation

seans3 commented Jun 15, 2018

seans3 commented Jun 15, 2018

pwittrock commented Jun 16, 2018

droot 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

pwittrock commented Jun 19, 2018

pwittrock commented Jun 19, 2018

k8s-ci-robot commented Jun 19, 2018

seans3 commented Jun 19, 2018

k8s-ci-robot commented Jun 19, 2018