Proposal: a way to inject errors into fake Client for testing

[jpatel-pivotal]

It would be extremely helpful if the fake Client in controller-runtime had a way to inject errors that get returned to help with testing a controller's reconcile logic.

Consider the following psuedo-code below for example:

myObjectBeingReconciled, err := r.Get(ctx, req.NamespacedName)

	if instanceNotFound(err) {
		return noRequeue()
	} else if err != nil {
		return requeue(err)
	}

Currently there is no easy way to unit test the handled vs. unhandled error case shown above with the fake Client. I propose that the fake Client could have a field like errorsToReturn map[errorKey]error that could be used to hold errors to return for testing as shown below:

type errorKey struct {
	action         string
	resourceSchema schema.GroupVersionKind
	resourceKey    client.ObjectKey
}
type fakeClient struct {
	tracker     versionedTracker
	scheme      *runtime.Scheme
	errorsToReturn map[errorKey]error
}

Error injection could occur when the fakeClient is initialized with a constructor like func NewFakeClientWithInjectedErrors(clientScheme *runtime.Scheme, errToReturn map[errorKey]error, initObjs ...runtime.Object) client.Client

OR

via an exported method like func (c *fakeClient) InjectActionWithError(errorKey errorKey, injectedError error).

Using the InjectActionWithError() we could now inject a instanceNotFound error as well as a generic test error like errors.New("unhandled error case")to be returned for myObjectBeingReconciled so that we can get to 100% test coverage on the psuedo-code shown above.

//InjectActionWithError injects errorKey and error into errorsToReturn map for testing error cases.
func (c *fakeClient) InjectActionWithError(errorKey errorKey, injectedError error) {
	c.errToReturn[errorKey] = injectedError
}

The fake Client has to check the errorsToReturn map and see if a particular action is meant to return an error. If so, the injected error can be returned. If no injected errors were meant to be returned for this particular action, then it performs the intended action as before. An example get implementation would look like:

func (c *fakeClient) Get(ctx context.Context, key client.ObjectKey, obj runtime.Object) error {
	err = c.getInjectedError(errorKey)
	if err != nil {
                // Injected error matched so we return it
		return err
	}
        // This particular get action was not meant to return an error
	// Do the actual get as before
}

//A helper method to get injected errors when errorKey is found in errorsToReturn map for testing error cases.
func (c *fakeClient) getInjectedError(errKey errorKey) error {
	return c.errToReturn[errKey]
}

This is briefly discussed in #1031

[berlin-ab]

1. I don't think the fake client is the appropriate place to introduce a stubbed error. The fake client, as I understand it, is meant to be a somewhat realistic stand-in for a real-world environment.

2. I've been wishing for another type of test double to stand in for the controller runtime client.Client. I'd like that test double to be less knowledgeable than the fake client. This new client should provide the ability to provide canned responses to the client.Client calls, and should be able to observe interactions with the client.Client (for example: Patch was called N times with these arguments)

[vincepri]

/kind design
/milestone Next

[jpatel-pivotal]

@berlin-ab I think what you are looking for is a thing that would allow you to spy on actions performed on different objects. So for e.g. if there was a struct with fields below:

// very pseudo code to get the idea across
struct {
    actionWasCalled bool
    actionCallCount int
    actionWasCalledWith obj properties
    stubActionError error injection
    options that were passed in to the action
}

Then you could unit test if your reconciler called Get|Create|Patch (some action) N times with some options on objects with property a,b in state z. Currently the fakeClient does make actual changes to the objects in cache and the thing you describe should not really implement the behavior of the action (create,get,delete, patch....) but instead allow you to spy on the action. Feels like another use case: Should be able to spy on actions for testing.

[berlin-ab]

You're right. I want what you're looking for - an ability to stub responses from a client (errors, in your case), but also the ability to observe the interactions with the client. That request should probably be a separate proposal. I would prefer that the fake client not do those things, but instead a new client be written that can stub and spy calls to the client.

[vincepri]

It'd be nice to see a full proposal that goes into some amount of review before proceeding with any code changes

[thetechnick]

Hey there! 👋

Adding my two cents into this conversation: We had this exact issue when writing our tests.
We tried to do setup all our tests with fakeclient and assert on the state in the fakeclient after invoking a function, but especially with errors or in more complex scenarios it became very messy or impossible to just use the fakeclient alone.

After sticking our heads together we finally realized that we don't even need to use the fakeclient for everything we do and our lives will be way simpler by just using good old mocks:

https://github.com/kubermatic/kubecarrier/blob/master/pkg/testutil/mockclient/client.go
Example Usage

---

From my understanding the fakeclient is a just a stateful stub, something you can fulfill dependencies with and do simple assertions, without needing a full blown apiserver.

That's why we (as in my team), will only use fakeclient for "happy path" tests (where it is really awesome), while doing error and regression tests with mocks, so we have full control over the client return values.

I guess every tool has it's job and I don't think fakeclient needs to be able to do everything, so I would rather just document "How to test operators" and where fakeclient fits in and where other tools might come into play.
We could also think about choosing a preferred mocking framework and maintain mock presets for it.

[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

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

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 rotten