Scaffolded controller test needs fixing

[janetkuo]

The scaffolded controller test has several issues:

• It uses cached client mgr.GetClient() instead of a client that directly reads from apiserver.
  

  kubebuilder/pkg/scaffold/controller/controllertest.go

  Line 106 in e904181

  c = mgr.GetClient()

• Expect reconcile doesn't really do anything -- it should be removed. We only need to check some condition to happen, instead of a reconcile. Updated: Allow users to test a condition to eventually happen without having to expect Reconcile to happen first.
  

  kubebuilder/pkg/scaffold/controller/controllertest.go

  Line 128 in e904181

  g.Eventually(requests, timeout).Should(gomega.Receive(gomega.Equal(expectedRequest)))

• Logger should be set in TestMain
  

  kubebuilder/pkg/scaffold/controller/controllersuitetest.go

  Line 67 in e904181

  func TestMain(m *testing.M) {

• It doesn't use ginkgo.

@DirectXMan12

[DirectXMan12]

/kind bug
/priority important-soon

[janetkuo]

Actually, we cannot remove the "expect reconcile" part, because the scaffolded test reconciler writes requests to a request channel:

kubebuilder/pkg/scaffold/controller/controllersuitetest.go

Lines 83 to 93 in e904181

	// SetupTestReconcile returns a reconcile.Reconcile implementation that delegates to inner and
	// writes the request to requests after Reconcile is finished.
	func SetupTestReconcile(inner reconcile.Reconciler) (reconcile.Reconciler, chan reconcile.Request) {
	requests := make(chan reconcile.Request)
	fn := reconcile.Func(func(req reconcile.Request) (reconcile.Result, error) {
	result, err := inner.Reconcile(req)
	requests <- req
	return result, err
	})
	return fn, requests
	}

This will cause problems when Reconcile loop fails and requeues. While writing tests, we don't know whether the Reconcile will return an error or not; for example, we cannot expect an object's status to be updated after a single Reconcile -- it may eventually happen, but just not at the first Reconcile.

It's possible to write tests that workaround this issue, by making sure Reconcile never fails in the tests, but that's pretty limiting and makes tests less robust.

cc @mengqiy @droot

[cizixs]

To overcome cached client issue, I added several time.Sleep statement for now. I wonder if there is a non-cached client for mgr?

@janetkuo @DirectXMan12

[mengqiy]

I wonder if there is a non-cached client for mgr?

@cizixs It is supported. Please check kubernetes-sigs/controller-runtime#327.

I can probably use a non-cached client to address the 1st issue mentioned in #639 (comment)

[DirectXMan12]

We no longer scaffold controller tests at the moment. Closing this, we'll keep it in mind as we add those back in.