Implement a fake status client for testing

[tomasaschan]

What this PR does / why we need it:
This adds a FakeStatusClient with noop implementations of Update and Patch, to enable doing things like client.Status().Update(...) in methods being tested.

I don't know if there was any special reason this was not already in place, other than "it hadn't been needed yet" - if there are any special considerations that need to be taken care of when updating status, this PR might not be good enough. But given that FakeClient.Update and similar methods also just return nil, this should probably be OK for most use cases.

[k8s-ci-robot]

Hi @tomasaschan. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[camilamacedo86]

It is not recommended the usage of the fakeClient from controller runtime (submariner-io/submariner-operator#1236)
It would be removed but the community ask for not to break their tests. (kubernetes-sigs/controller-runtime#768)

The recommendation is to use ENV Test instead to test the controllers. WDYT about we change this project to start to use EnvTest instead?

[tomasaschan]

@camilamacedo86 Where can I read more about using ENV Test instead?

Note that the FakeClient struct is used by the test helpers in this project as well (which is how I found it), so maybe a good way to start down that path would be if you could show how those test helpers can be rewritten using the recommended approach?

[tomasaschan]

@camilamacedo86 Also, see #1101 where the FakeClient was un-deprecated.

[camilamacedo86]

Hi @tomasaschan,

It was undeprecated, however, still not recommended. See: https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/client/fake/doc.go#L30-L36

Also, this project is used in the kubebuilder as well. We can scaffold by using the declarative plugin. See the sample: https://github.com/kubernetes-sigs/kubebuilder/tree/master/testdata/project-v3-addon. Kubebuilder scaffold the suite test for the controllers using the ENV Test, see: https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v3-addon/controllers/suite_test.go

In this way, if this project is providing test features, could we try to move forward here to use env test instead? WDYT? Could you please confirm and share where the fake client here would be used?

/ok-to-test

[camilamacedo86]

/ok-to-test

[tomasaschan]

@camilamacedo86 Two things 🙂

1. This PR is literally 10 lines of code, extending existing functionality by a little to avoid having to panic("not implemented"). No matter the status of the functionality I'm extending here, maybe this is the wrong place to decide whether to uproot the entire testing strategy for this library and its downstreams?

2. using testenv like in the examples you posted is significantly more heavyweight, both on terms of dependencies (tests fail to start if etcd is not on PATH!) and performance (we have some testenv tests in our projects - they typically take 10+ seconds per test, compared with tests using FakeClient which run tens of tests per second...). In other words, these two approaches solve different problems in different ways, and deprecating one in favor of the other will end up dropping support for some use cases.

[justinsb]

This does seem consistent with what we have already, so I am going to approve.

I don't think bringing up a kube-apiserver can ever be the only test strategy; fault injection, or checking the actual calls made, is very difficult without a mock client.

Personally, I have found the tests that bring up kube-apiserver seem to primarily testing kubebuilder & controller-runtime, and not the actual operator. I would prefer to see a lot more golden testing for operators - they are much more understandable, and tend to be much higher performance also :-)

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, tomasaschan

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 [justinsb]

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