The reason this isn't currently supported is because upstream client-go doesn't support this: kubernetes/kubernetes#115598

We could build something downstream I suppose and the create case is going to be simple. I think the Update case is going to be pretty complicated though (if a field that is currently present but not in the submitted applyconfig, should we keep it or not is a non-trivial question to answer), so if possible I'd prefer to wait for upstream.

In the meantime when using todays 0.15 release of controller-runtime, you can use the interceptor to set up a createOrUpdate logic in Patch when an applypatch is submitted that works for your case.

Envtest takes ~5-15s to set up for each test case

Hm, envtest is usually setup once per test package, is there a reason why envtest in this case once for every test case?

Hm, envtest is usually setup once per test package, is there a reason why envtest in this case once for every test case?

I need to review our cases. We want to ensure that our test cases are isolated. Most of the time you can isolate them into unique namespaces but some of the controllers have specific requirements around that. I think we may be able to get around it by specifying which namespace to use as a field on the reconciler struct.

In the meantime when using todays 0.15 release of controller-runtime, you can use the interceptor to set up a createOrUpdate logic in Patch when an applypatch is submitted that works for your case.

Thank you for the response! I will look into this :)

The reason this isn't currently supported is because upstream client-go doesn't support this: kubernetes/kubernetes#115598

Sounds reasonable to me. I mostly wanted to create an issue so that we have something to represent that it would make our lives a bit easier. If the work to benefit ratio doesn't make sense, that's fair.

Given that we can run envtest and isolate cases in namespaces, that is probably the way to go. It's a small bummer that we have to run our unit tests alongside an external dependency which takes some time to start up. We're going to reorganize things a bit and improve our scripts to make this easier for our developers.

Hm, envtest is usually setup once per test package, is there a reason why envtest in this case once for every test case?

Found a case where it is a bit of a drag to use envtest. Any test which involves cluster scoped objects cannot be fully isolated in namespaces, leading to some issues:

• Requires the test case to clean up after itself, resulting in more complex code and longer test execution times.
• Opportunity for test pollution to cause false positive and/or false negatives if clean up isn't fully accounted for.
• Cannot run concurrently.

@nathanperkins could we keep the discussion around if and how and when to use envtest separate? We are aware that this is lacking right now, but this is non-trivial which is the reason upstream hasn't done it. This essentially requires to have the serverside SSA logic in the fake client.

@nathanperkins could we keep the discussion around if and how and when to use envtest separate?

Sure, it's totally understood this is not going to be resolved anytime soon.

I think that people searching for this issue will find it useful if there is clarity on what they can do in the meantime. The discussion on when / how to use envtest effectively instead of the fake client seems useful to that end. Maybe there is a doc or blog post that could be linked?

Found a case where it is a bit of a drag to use envtest. Any test which involves cluster scoped objects cannot be fully isolated in namespaces

Do you have an example handy to show? Just to wrap my head around a bit more 😄

A few thoughts:

• We probably need better documentation / examples on how to use and re-use envtest appropriately.
• Ideally envtest is so easy to integrate and use that very few folks rely on fake client (which imo is generally lacking in terms of full feature support, and gives a false sense of safety in some cases).
• Depending on how the reconcilers are structured, maybe there is a way that a filtered cache/client is passed into each reconciler's test case so it only has a partial view of the objects?

  controller-runtime/pkg/cache/cache.go

  Lines 147 to 153 in 30eae58

  	// DefaultLabelSelector will be used as a label selectors for all object types
  	// unless they have a more specific selector set in ByObject.
  	DefaultLabelSelector labels.Selector
  	// DefaultFieldSelector will be used as a field selectors for all object types
  	// unless they have a more specific selector set in ByObject.
  	DefaultFieldSelector fields.Selector

  • Requires some sort of predetermined convention, in either labels, or annotations, or naming convention.
  • Cache filters would only apply to list/get calls.

Do you have an example handy to show? Just to wrap my head around a bit more 😄

I couldn't show the code without going through a bunch of approvals. I can tell you it's a controller which reconciles on corev1.Service and looks at corev1.Node status to create some internals CRs for network configuration. Our test is using envtest and we can't fully isolate the nodes between test cases without cleaning up the nodes.

Depending on how the reconcilers are structured, maybe there is a way that a filtered cache/client is passed into each reconciler's test case so it only has a partial view of the objects?

Great idea! I'll share this with my teammate and see if it works for our case.

We probably need better documentation / examples on how to use and re-use envtest appropriately.

I agree, sharing some of these patterns would really help. Last I looked at the kubebuilder docs, they have the example which uses ginkgo and gomega and relies on the manager to run reconciliation. I've been finding that it's easier to write exhaustive and accurate test coverage using more traditional table driven tests which call reconcile directly. We still write integration tests with ginkgo and gomega which use the manager, but less exhaustive and focusing more on ensuring the event handlers work correctly.

I'd love to see more discussion in the community about this, whether in docs or blog posts :)

@vincepri, I'm moving discussion of using envtest with isolated unittest cases to #2358

@nathanperkins we have some general guidance here: https://cluster-api.sigs.k8s.io/developer/testing.html (not sure in which issue we want to dig deeper into pro/con of fake client vs envtest)

The reason this isn't currently supported is because upstream client-go doesn't support this: kubernetes/kubernetes#115598

We could build something downstream I suppose and the create case is going to be simple. I think the Update case is going to be pretty complicated though (if a field that is currently present but not in the submitted applyconfig, should we keep it or not is a non-trivial question to answer), so if possible I'd prefer to wait for upstream.

In the meantime when using todays 0.15 release of controller-runtime, you can use the interceptor to set up a createOrUpdate logic in Patch when an applypatch is submitted that works for your case.

For anyone looking for a workaround until the fake Client supports client.Apply:

import (
        "k8s.io/apimachinery/pkg/types"
	"sigs.k8s.io/controller-runtime/pkg/client/fake"
        "sigs.k8s.io/controller-runtime/pkg/client"
)

fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...).WithInterceptorFuncs(interceptor.Funcs{Patch: func(ctx context.Context, clnt client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
		// Apply patches are supposed to upsert, but fake client fails if the object doesn't exist,
		// if an apply patch occurs for an object that doesn't yet exist, create it.
		if patch.Type() != types.ApplyPatchType {
			return clnt.Patch(ctx, obj, patch, opts...)
		}
		check, ok := obj.DeepCopyObject().(client.Object)
		if !ok {
			return errors.New("could not check for object in fake client")
		}
		if err := clnt.Get(ctx, client.ObjectKeyFromObject(obj), check); k8serror.IsNotFound(err) {
			if err := clnt.Create(ctx, check); err != nil {
				return fmt.Errorf("could not inject object creation for fake: %w", err)
			}
		}
		return clnt.Patch(ctx, obj, patch, opts...)
	}}).Build()

/kind support feature
@nathanperkins can we close this issue? I see you moved it to a different issue