Add mechanism for accurate, isolated unit tests calling Reconcile directly

[nathanperkins]

My team is writing reconciler unit tests which use fake.Client and call Reconcile directly. These tests have been a great benefit to developer velocity because they are easy to write using standard go testing, run quickly and accurately, and don't require dependencies.

In many cases, it is suggested to use envtest rather than the fake client for a few reasons:

• Some features, like SSA (Support server-side apply (client.Apply) in fake Client #2341) are unsupported and unlikely to be supported.
• Output from the fake can vary from real-world outputs and it is suggested to use envtest for highest accuracy.

However, there are some issues:

• Bringing up an environment takes ~5-10s so running many testcases requires reusing the env.
• Some test cases can be isolated by namespaces, but not those that involve cluster-scoped objects.
  • Cluster-scoped objects must be manually cleaned up between cases.
  • Opportunity for test pollution and unusual results if env is not cleaned up properly.
  • Cannot run concurrently.

@vincepri suggests (#2341 (comment)) using the cache's DefaultLabelSelector feature to isolate test cases from each other. I wrote some exploratory code creating wrapped clients which use the DefaultLabelSelector to isolate test cases and automatically add the labels during Create operations. It works well enough, but has some drawbacks:

• Involving the cache means introducing eventual consistency compared to the fake client.
  • Improper use leads to flaky results.
  • When writing unit tests which Reconcile directly, it requires carefully using a non-cached client to create test objects and check outputs, while the cached, isolated client must be created and synced just before calling Reconcile, to ensure that the caches have all the necessary test inputs.
• Requires writing and maintaining two wrapped clients.

I'd like to discuss testing strategies for these cases and see if we can improve. Here are some quick opportunities and thoughts:

• Address feature gaps in the fake client. This is probably not feasible.
• Provide isolated client wrapper code which uses @vincepri's suggestion for cache with DefaultLabelSelector. This requires that test writers account for eventual consistency due to the cache or that they use a non-cached client in the test code and the cached client in the reconciler.
• Provide a mechanism to manually wait for client caches to be fully synced before running Reconcile. I'm not sure if this is feasible.
• Add DefaultLabelSelector support to client.Client and provide an isolated client wrapper which adds the label on Create. This seems to be a decent option.

Whatever option is best, I'd like to help contribute and help document the best approach.

Thank you :)

[sbueringer]

Also sharing here because it's related to the discussion: https://cluster-api.sigs.k8s.io/developer/testing.html

Involving the cache means introducing eventual consistency compared to the fake client.

We had a similar experience in ClusterAPI. On one side it's more realistic that way, on the other side this lead to many flaky unit tests.

[troy0820]

/kind support feature

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle rotten
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue with /reopen
• Mark this issue as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

[k8s-ci-robot]

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue with /reopen
• Mark this issue as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

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.