Remove KubeConfig Dependency for Store Tests #2789

diazjf · 2018-07-16T16:30:59Z

Removes the KubeConfig Dependency for the Store Test by using the FakeClient Instead. Unit Tests should not rely on a real KubeConfig.

See #2788

aledbf · 2018-07-16T16:47:23Z

@diazjf to be sure this is ok, please also remove https://github.com/kubernetes/ingress-nginx/blob/master/.travis.yml#L38 (these tests are the only reason why we start a cluster in tests)

codecov-io · 2018-07-16T16:58:47Z

Codecov Report

Merging #2789 into master will decrease coverage by 0.25%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2789      +/-   ##
==========================================
- Coverage   47.58%   47.33%   -0.26%     
==========================================
  Files          75       75              
  Lines        5413     5413              
==========================================
- Hits         2576     2562      -14     
- Misses       2509     2527      +18     
+ Partials      328      324       -4

Impacted Files	Coverage Δ
cmd/nginx/main.go	`6.57% <ø> (-13.82%)`	⬇️
internal/watch/file_watcher.go	`80.76% <0%> (-3.85%)`	⬇️
internal/ingress/controller/store/store.go	`62.68% <0%> (+2.33%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12f7966...c783c08. Read the comment docs.

diazjf · 2018-07-16T17:03:07Z

@aledbf main_test.go still fails since I haven't edited that file. store_test.go should pass. I will update main_test.go in this PR and let you know.

aledbf · 2018-07-16T17:08:04Z

@diazjf thank you for removing this dependency

antoineco · 2018-07-16T17:19:55Z

Excellent 👏 , I also wanted to tackle this, the dependency on a real API was annoying.

antoineco · 2018-07-16T17:24:20Z

internal/ingress/controller/store/store_test.go

@@ -605,37 +619,89 @@ func TestStore(t *testing.T) {
 	// check invalid secret (missing ca)
 }

-func createNamespace(clientSet *kubernetes.Clientset, t *testing.T) string {
+func createNamespace(clientSet *fake.Clientset, t *testing.T) string {


This is still using a specific type, you can replace any Clientset by kubernetes.Interface to make it portable.

I don't think we can cast fake.ClientSet to kubernetes.Clientset.

Not ClientSet, Interface. All REST clients implement it, including fake.

Can you provide me an example of how it should look. Having some issues trying to properly type-cast.

All you have to do is replace clientSet *fake.Clientset with clientSet kubernetes.Interface, where kubernetes is k8s.io/client-go/kubernetes.
There is nothing to cast since you're just calling methods provided by the interface.

Awesome Thanks, I see it now:

fakeclient:

clientset "k8s.io/client-go/kubernetes" clientset.Interface = &Clientset{} # &Clientset{} is: # type Clientset struct { # testing.Fake # discovery *fakediscovery.FakeDiscovery #}

The reason why it works is not the contents of the struct, just the methods the struct implements (in other words, Admissionregistration() ... StorageV1beta1())

antoineco · 2018-07-16T17:26:28Z

internal/ingress/controller/store/store_test.go

 	"sync"
 	"sync/atomic"
 	"testing"
 	"time"

 	"github.com/eapache/channels"
-	"k8s.io/api/extensions/v1beta1"


Nice catch 👍

antoineco · 2018-07-16T17:29:30Z

internal/ingress/controller/store/store_test.go

 	if err != nil {
 		if k8sErrors.IsNotFound(err) {
-			return clientSet.ExtensionsV1beta1().Ingresses(ingress.Namespace).Create(ingress)
+			t.Logf("ingress %v not found, creating", ingress)
+			return clientSet.Extensions().Ingresses(ingress.Namespace).Create(ingress)
 		}
 		return nil, err


You should avoid returning errors in tests and simply call t.Fatal in case an error occurs, like you did above. Call t.Helper() at the beginning of the function in that case.

antoineco · 2018-07-16T20:05:35Z

internal/ingress/controller/store/store_test.go

-func ensureIngress(ingress *extensions.Ingress, clientSet *kubernetes.Clientset) (*extensions.Ingress, error) {
-	s, err := clientSet.ExtensionsV1beta1().Ingresses(ingress.Namespace).Update(ingress)
+func createConfigMap(clientSet *fake.Clientset, ns string, t *testing.T) string {
+	t.Log("creating temporal config map")


This is a helper function, adding t.Helper() here would allow logs to report line numbers accurately.

antoineco · 2018-07-16T20:05:49Z

internal/ingress/controller/store/store_test.go

+	return ing
+}
+
+func deleteIngress(ingress *extensions.Ingress, clientSet *fake.Clientset, t *testing.T) {


This is a helper function, adding t.Helper() here would allow logs to report line numbers accurately.

antoineco · 2018-07-16T20:06:02Z

internal/ingress/controller/store/store_test.go

+	return cm.Name
+}
+
+func deleteConfigMap(cm, ns string, clientSet *fake.Clientset, t *testing.T) {


This is a helper function, adding t.Helper() here would allow logs to report line numbers accurately.

antoineco · 2018-07-16T20:06:17Z

internal/ingress/controller/store/store_test.go

+	t.Logf("temporal configmap %v deleted", cm)
+}
+
+func ensureIngress(ingress *extensions.Ingress, clientSet *fake.Clientset, t *testing.T) *extensions.Ingress {


This is a helper function, adding t.Helper() here would allow logs to report line numbers accurately.

antoineco · 2018-07-16T20:07:14Z

internal/ingress/controller/store/store_test.go

 			ObjectMeta: metav1.ObjectMeta{
 				Name:      "dummy",
 				Namespace: ns,
+				SelfLink:  fmt.Sprintf("/apis/extensions/v1beta1/namespaces/%s/ingresses/dummy", ns),


Isn't this field generated by the API server, and not the user?

I don't think the fakeClient mocks generating this field, when it is not provided in the ingress the following is obtained:

Could not construct reference to: '<ingress-resource>' due to: 'selfLink was empty, can't make reference'

antoineco · 2018-07-16T20:15:06Z

internal/ingress/controller/store/store_test.go

 		if err != nil {
-			t.Errorf("unexpected error creating ingress: %v", err)
+			t.Errorf("unexpected error waiting for secret: %v", err)


(my favorite review comment, sorry for repeating myself) Log messages should start with a capital letter to differentiate them from error messages returned by functions.
Besides errors are never expected, so removing the word "unexpected" from all these log messages makes them shorter and to the point. But that's obviously a nit pick, these messages are not user facing so feel free to skip that comment.

antoineco · 2018-07-16T20:15:28Z

internal/ingress/controller/store/store_test.go

 		}
+		time.Sleep(1 * time.Second)


Do we really need that sleep with the fake client?

Yes, I noticed that if we don't wait a second, the event wont get generated by the time we need it to.

diazjf · 2018-07-16T20:28:49Z

@aledbf @antoineco So I can get store_test.go working without KubeClient, but main_test.go will require some more effort, are you guys opposed to just merging this once all the issues have been resolved, and I can submit another PR in the future for main_test.go?

By main_test.go I mean https://github.com/kubernetes/ingress-nginx/blob/master/cmd/nginx/main_test.go I'm thinking it looks more like a functional test, also I'm wondering where this is used.

diazjf · 2018-07-16T21:33:06Z

So tests are passing!! Give me 👍 and I'll revert .travis and main_test.go back to normal.

@antoineco @aledbf

antoineco · 2018-07-17T04:47:51Z

Awesome work, thanks @diazjf! What issue are you facing exactly with main_test.go? Maybe we can rework the tests because not everything makes sense to test (eg. NewClientSet)

diazjf · 2018-07-17T15:56:26Z

@antoineco thanks for all the help. It seems like createApiserverClient relies on a real Kubernetes Cluster and we may not be able to get by with the fakeClient. I have reworked the tests not to use the ApiServerClient. Let me know what you think.

@aledbf

antoineco · 2018-07-17T15:59:20Z

cmd/nginx/main_test.go

 	if err == nil {
-		t.Fatal("Expected an error creating REST client without an API server URL or kubeconfig file.")
+		t.Fatal("expected an error creating REST client without an API server URL or kubeconfig file.")


Capital letter was fine (log message, cf previous comment).

Will change back.

antoineco · 2018-07-17T16:04:22Z

cmd/nginx/main_test.go

-	}
-
-	_, err = createApiserverClient("", "")
+	_, err := createApiserverClient("", "")


This calls kubernetes.NewForConfig(), which will always fail without API. Not sure if it's really interesting to test createApiserverClient() at all, unless we make it testable (eg. create a fake client if called by tests).

I think it will be a bigger effort to make it more testable, but maybe we can set that for a future release.

I can look into this in the future if its ok.

antoineco · 2018-07-17T16:06:03Z

cmd/nginx/main_test.go

+
+	ns := "test"
+
+	cm := createConfigMap(clientSet, ns, t)


Not sure to understand why this is here?

Since controller.NewNGINXController(conf, nil, fs) make a call to the store. The store sets a configuration and requires a configmap. If it is not created then there will be a segmentation fault.

Removes the KubeConfig Dependency for the Store Test by using the FakeClient Instead. Unit Tests should not rely on a real KubeConfig. Fixes kubernetes#2789

antoineco · 2018-07-17T17:04:32Z

once all the issues have been resolved, and I can submit another PR in the future for main_test.go?

I'm fine refactoring the cmd/nginx tests in a separate PR. @aledbf?

edit: sorry, didn't see the "hold" label had been removed.

antoineco · 2018-07-17T17:04:46Z

/lgtm

k8s-ci-robot · 2018-07-17T17:04:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: antoineco, diazjf

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:

~~OWNERS~~ [antoineco]

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

aledbf · 2018-07-17T17:12:02Z

I'm fine refactoring the cmd/nginx tests in a separate PR. @aledbf?

👍

aledbf · 2018-07-17T17:12:28Z

edit: sorry, didn't see the "hold" label had been removed.

I removed the label after the 0.17.1 release

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 Jul 16, 2018

diazjf force-pushed the mock-kube-in-unit-tests branch from a125e2a to b5f32b4 Compare July 16, 2018 16:33

diazjf mentioned this pull request Jul 16, 2018

Unit Tests Should Not Rely on a Real KubeConfig #2788

Closed

2 tasks

diazjf force-pushed the mock-kube-in-unit-tests branch from b5f32b4 to 786b80b Compare July 16, 2018 16:54

antoineco suggested changes Jul 16, 2018

View reviewed changes

diazjf force-pushed the mock-kube-in-unit-tests branch from b19883f to f890769 Compare July 16, 2018 18:36

antoineco suggested changes Jul 16, 2018

View reviewed changes

diazjf force-pushed the mock-kube-in-unit-tests branch from f890769 to 8fd8984 Compare July 16, 2018 20:23

diazjf force-pushed the mock-kube-in-unit-tests branch 3 times, most recently from cb5e1bc to 753cdc7 Compare July 16, 2018 21:06

aledbf added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 17, 2018

aledbf removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 17, 2018

diazjf force-pushed the mock-kube-in-unit-tests branch from 753cdc7 to 4e71431 Compare July 17, 2018 15:56

antoineco reviewed Jul 17, 2018

View reviewed changes

Remove KubeConfig Dependency for Store Tests

c783c08

Removes the KubeConfig Dependency for the Store Test by using the FakeClient Instead. Unit Tests should not rely on a real KubeConfig. Fixes kubernetes#2789

diazjf force-pushed the mock-kube-in-unit-tests branch from 4e71431 to c783c08 Compare July 17, 2018 16:13

k8s-ci-robot assigned antoineco Jul 17, 2018

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 17, 2018

k8s-ci-robot merged commit 96c289b into kubernetes:master Jul 17, 2018

Remove KubeConfig Dependency for Store Tests #2789

Remove KubeConfig Dependency for Store Tests #2789

Conversation

diazjf commented Jul 16, 2018

aledbf commented Jul 16, 2018

codecov-io commented Jul 16, 2018 • edited Loading

Codecov Report

diazjf commented Jul 16, 2018

aledbf commented Jul 16, 2018

antoineco commented Jul 16, 2018

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

antoineco Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

diazjf Jul 16, 2018 • 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

antoineco Jul 16, 2018 • 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

antoineco Jul 16, 2018 • 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

diazjf commented Jul 16, 2018

diazjf commented Jul 16, 2018

antoineco commented Jul 17, 2018

diazjf commented Jul 17, 2018

antoineco Jul 17, 2018 • 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

antoineco commented Jul 17, 2018 • edited Loading

antoineco commented Jul 17, 2018

k8s-ci-robot commented Jul 17, 2018

aledbf commented Jul 17, 2018

aledbf commented Jul 17, 2018

codecov-io commented Jul 16, 2018 •

edited

Loading

antoineco Jul 16, 2018 •

edited

Loading

diazjf Jul 16, 2018 •

edited

Loading

antoineco Jul 16, 2018 •

edited

Loading

antoineco Jul 16, 2018 •

edited

Loading

antoineco Jul 17, 2018 •

edited

Loading

antoineco commented Jul 17, 2018 •

edited

Loading