Merge pull request #2131 from DirectXMan12/docs/book-test-client-advice

📖 Fix advice on clients in tests, ensure that book gets tested

Makefile

@@ -88,7 +88,7 @@ golangci-lint:
 ##@ Tests
 
 .PHONY: test
-test: test-unit test-integration test-testdata ## Run the unit and integration tests (used in the CI)
+test: test-unit test-integration test-testdata test-book ## Run the unit and integration tests (used in the CI)
 
 .PHONY: test-unit
 test-unit: ## Run the unit tests
@@ -119,3 +119,7 @@ test-e2e-local: ## Run the end-to-end tests locally
 .PHONY: test-e2e-ci
 test-e2e-ci: ## Run the end-to-end tests (used in the CI)`
 	./test/e2e/ci.sh
+
+.PHONY: test-book
+test-book: ## Run the cronjob tutorial's unit tests to make sure we don't break it
+	cd ./docs/book/src/cronjob-tutorial/testdata/project && make test

docs/book/src/cronjob-tutorial/testdata/project/controllers/suite_test.go

@@ -115,14 +115,16 @@ var _ = BeforeSuite(func() {
 		The only difference is that the manager is started in a separate goroutine so it does not block the cleanup of envtest
 		when you’re done running your tests.
 
-		It is not recommended to use the manager client in tests because it is not strongly consistent. Indeed, the manager
-		client is designed to do the "right thing" for controllers by default which is to read from caches. The best solution
-		is to instantiate a new client using client.New for tests (as k8sClient above). It will provide a client that reads
-		directly from the API meaning that you can write tests expecting read-after-write consistency.
-
-		However, keep in mind that you should not do this in the controller's conciliation loop (read an object after you have
-		written it). Kubernetes favors an approach where you first do some reads, process and then do some writes and return.
-		This way, you let the queue take care of the next cycle of readings if they are necessary.
+		Note that we set up both a "live" k8s client, separate from the manager.  This is because when making assertions in
+		tests, you generally want to assert against the live state of the API server.  If you used the client from the
+		manager (`k8sManager.GetClient`), you'd end up asserting against the contents of the cache instead, which is slower
+		and can introduce flakiness into your tests.  We could use the manager's `APIReader` to accomplish the same thing,
+		but that would leave us with two clients in our test assertions and setup (one for reading, one for writing), and
+		it'd be easy to make mistakes.
+
+		Note that we keep the reconciler running against the manager's cache client, though -- we want our controller to
+		behave as it would in production, and we use features of the cache (like indicies) in our controller which aren't
+		available when talking directly to the API server.
 	*/
 
 	k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{

test.sh

@@ -14,11 +14,6 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-source test/common.sh
-
-header_text "Running kubebuilder unit tests"
-go test -race -v ./pkg/...
-
-./test/integration.sh
-
-./test/testdata/test.sh
+# prow calls this file currently, but we can just use `make test` to test
+# the set of things we want.
+make test