Reconsider deprecation of pkg/client/fake

[detiber]

Currently pkg/client/fake is marked as Deprecated and that pkg/envtest should be used for all testing by v1.0.0, however there are use cases where envtest is not a proper replacement for the fake client.

For example, if using Test Driven Development(TDD) to build functionality with a controller-runtime based controller it is quite painful to debug issues with the controller implementation. Rather than just running the tests using a debugger such as delve, it becomes necessary to either debug the controller implementation using printf-style debugging or one would have to somehow wire up a remote debugger.

Instead, I believe a better approach would be to document the limitations and use cases for using the fake client rather than removing it.

[detiber]

/assign @DirectXMan12
/cc @ncdc @vincepri

[detiber]

As an aside, I'm happy to sign up to help maintain and support the fake client if it is a matter of needing someone to do so.

[sebgl]

For what it's worth we rely a lot on the fake client in https://github.com/elastic/cloud-on-k8s.

With a quick and dirty grep in the codebase, I can count 162 initializations of the fake client in our unit tests, that we aim to run very quickly (a few seconds) - and in parallel.

Doing the same by spawning a local apiserver + etcd env test would make running unit tests much longer and happened to be a bit unreliable in busy CI environments.

Obviously this does not prevent us from also running integration/E2E tests with a real Kubernetes environment.

[DirectXMan12]

We talked about this a bit at the in-person meeting @ kubecon. The notes are on the kuebuilder ML for reference.

TL;DR:

• It needs to be audited for shortcomings compared to a real client, and those need to be well documented
• It needs clearer documentation on what it should and shouldn't be -- where do we draw the line and say "use a real API server"?
• It needs folks to support it
• If we're gonna do reactors in the public API let's do it in a usable way (i.e. not the ones from k/XYZ). I still don't really want to, but it's a common request, so we might need to

[dsharp-pivotal]

It sounds like you are reconsidering deprecating fakeClient, which is great. But we wanted to put on the record why fakeClient is important to us.

Removal of fakeClient would be very disruptive to our team (Greenplum for Kubernetes). We do extensive test driven development, and currently have 108 test cases that use either pkg/client/fake or the similar client-go fakes (for our older controller, which we are working on migrating to kubebuilder).

• envtest takes about 5 seconds to start up (see below). We would have run our controller unit tests about 20x in that time. Short (<<1sec) test iteration times are critical to effective TDD. A single test case that takes 5 seconds is unacceptable.
• We are disinterested in introducing an apiserver. Our controller's interface is the Client. We only want to test our controller's business logic, and not get bogged down in details of how a (semi-)real apiserver works. If we didn't have fakeClient, we would have to write our own stubs, mocks, and fakes. envtest is fine for integration tests, but it's not appropriate for unit testing. I could argue that introducing envtest makes a test not a unit test.
• envtest requires many round trips to the apiserver for a test scenario to get set up and then run the controller. After each request, it takes an unpredictable time for the system to settle back into a quiescent state. This makes it difficult to know when the desired state should be expected to be achieved, and therefore when it's safe to make assertions that will always pass (non-flakey tests). We can use gomega.Eventually(), but this contributes to making tests slow.
• envtest makes test concurrency difficult. You could use careful naming of objects to try to isolate them from each other, but this is error prone. You could set up separate envtests in a BeforeEach(), but this requires workarounds to assign server ports to each environment.
• Reactors: We need to inject errors to test that our controller can handle them. We can't simulate all of these errors by putting objects in an apiserver. We have found reactors to be a convenient way to do that. We wrote a reactive.Client that wraps any client.Client to add reactors back to fakeClient. We did at one point wrap envtest's client in reactive.Client, but as we added tests about errors using reactors, we realized our test runtimes were going to increase far too much. Switching to the fake client allowed us to cover all the error cases we wanted while keeping test times manageable. (If there's interest, we could contribute reactive.Client(). It would take some polish--some methods we don't use are not implemented.)

https://engineering.pivotal.io/post/gp4k-kubebuilder-tdd/

func BenchmarkEnvtest(b *testing.B) {
	for i := 0; i < b.N; i++ {
		testEnv := &envtest.Environment{}
		if _, err := testEnv.Start(); err != nil {
			b.Error(err)
		}
		if err := testEnv.Stop(); err != nil {
			b.Error(err)
		}
	}
}

$ go test -bench . -benchtime 10x
BenchmarkEnvtest-8   	      10	6014350966 ns/op

[steveQuadros]

Similarly my team has test coverage metrics that need to be hit, often times getting over that hump means covering a few lines that handle errors on client.List, client.Get, etc. These are generally impossible to test with testenv as far as I can see (since we can't manually send a request to reconcile for a Get failure to reconcile; we can't test our error handling). This would be very helpful to us, and I'd be on board for helping to support client/fake as well.

[vincepri]

/kind design

We're holding off deprecating the fake client for the time being. We'll need design documents and folks that can maintain the client going forward.

[vincepri]

/priority important-longterm

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[detiber]

/remove-lifecycle stale

[akutz]

FWIW, I have written several test frameworks/harnesses that execute unit tests with fake.Client and integration tests with envtest. The test function(s) are the same for each test type, but in unit tests the test function receives a fake client and in integration testing a real client via envtest.

I find fake.Client supports creating fail-fast test patterns that can be re-used for more resource intensive integration tests. I would hate to see fake.Client go away.

[DirectXMan12]

So I think at the moment, the consensus that we have (which probably needs to be written in the deprecation comment) is that we're gonna keep fake client around, but we want to overhaul it a bit to be a little less clunky in certain areas.

[horis233]

When I transferring fake.Client to envtest, I notice I can use the fake.Client to mock the status of the testing data.

For example, my controller can react differently to the status of the k8s job. Then, by using the fake.Client, I can mock the k8s job status to failed, running or completed.

But envtest seems can't do that, when I create a k8s job, the environment will empty its status, which impacts my original test cases.

The above finding is based on my testing. If the envtest can cover this case as well, please correct me.

[JoelSpeed]

You should be able to set the status within envtest, however you will need to use the Status() client to modify the status because envtest honours subresources where the fake client doesn't.

Eg Call client.Create(...) to create the object, then obj.Status = <desired status>, then client.Status().Update(...) to get the status updated.

[horis233]

@JoelSpeed
Thanks, It is very useful. The method works for me.

[alukyan]

Please do not deprecate pkg/client/fake as using real environment for unit tests creates lots of issues with writing unit tests. Our current project, one of which is CRD controller currently has over 700+ unit tests and their execution time is about 12 seconds.
I can run the whole suite locally on my machine many times per day to verify I am not breaking anything. Using envtest will impact this experience greatly.
It is also difficult to write unit tests for specific corner case conditions (like error conditions) that are not common - recreating those corner case conditions in real environment is challenging task. We really do not want to test our error handling paths in code in production scenarios. That is why we write a lot of unit tests to get us 100% code coverage or close to it

[ktarplee]

It appears fake client does not support listing object with options client.MatchingFields and client.MatchingFieldsSelector. If this is true then maybe the "limitations" in the doc should be expanded to include these missing features in the fake client.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

[bboreham]

/remove-lifecycle stale

I'm using fake-client to synthesise particular ordering of events arriving; a real api-server cannot do this.

[coderanger]

I think we should probably close this issue, the Fake client isn't deprecated anymore and the new builder syntax has fixed some of the worst problems. I think any remaining issues should move to their own tickets so users aren't misled by the history here.

[bboreham]

Can you post a link to the commit/PR where it was un-deprecated please?

[vincepri]

We're still looking for the fake client codebase, if anyone is interested please open a PR with a new OWNERS file

cc @alvaroaleman @DirectXMan12

[alvaroaleman]

This was undeprecated in #1101, I don't think there is anything left to do here. As Vince mentioned, if you are interested in owning some or all of the fake client, please add yourself to an OWNERS file there.

[detiber]

I went ahead and started a PR to add an OWNERS file and related OWNERS_ALIASES for the fake client here: #1494

I started by adding just myself, but happy to add others who may be interested in helping maintain the fake client as well.