Test suite design violates recommended practices for the framework

[lllamnyp]

Summary

Subsequent test cases fail if a prior test in the suite had failed.

Background

Many of the tests work with a mock-up of a running kubernetes API server propped up by the envtest package. The suite of tests for the StatefulSet factory is one such example.

The linked set of test cases

• Takes an EtcdCluster definition
• Calculates a StatefulSet manifest from this definition (using the function CreateOrUpdateStatefulSet).
• Applies this StatefulSet to a mock k8s cluster (this is also done inside the function CreateOrUpdateStatefulSet).
• Reads the StatefulSet back from the cluster, verifying that the created StatefulSet matches what was expected to be generated.
• Deletes the StatefulSet.

The code responsible for this is of the form

It("should successfully create the statefulset with empty spec", func() {
	// sts is declared inside the It() clause which is an antipattern
	sts := &appsv1.StatefulSet{}

	// etcdCluster is defined outside of the It() clause but may be mutated before
	// creating a StatefulSet. Unlike the previous step, this is a correct pattern.
	// etcdCluster.Spec.SomeField = someValue

	// The StatefulSet is calculated and applied
	err := CreateOrUpdateStatefulSet(ctx, etcdcluster, k8sClient, k8sClient.Scheme())
	Expect(err).NotTo(HaveOccurred())

	// The StatefulSet is retrieved from the k8s API server
	err = k8sClient.Get(ctx, typeNamespacedName, sts)
	Expect(err).NotTo(HaveOccurred())

	// The retrieved StatefulSet is verified against the expected value
	Expect(sts.Spec.Replicas).To(Equal(etcdcluster.Spec.Replicas))

	// If any single one of the prior Expect() clauses fails, this is never executed and the
	// cluster is never cleaned up!
	Expect(k8sClient.Delete(ctx, sts)).To(Succeed())
})

Since there is a chance, that k8sClient.Delete(ctx, sts) will not be executed, a subsequent test might attempt to apply a new StatefulSet to the cluster with the same name/namespace, but with differing parameters, some of which might be immutable. This will immediately fail a test case that would have otherwise succeeded.

Root cause

The main cause of this problem is that parts of the setup and cleanup that are meant to be done before and after the test cases are, in fact, part of the test cases themselves. In particular, the k8sClient.Delete() should always run after each test case and should thus be in an AfterEach closure. An AfterEach() closure must be outside of the It() closure, but this means that the sts variable needs to be known outside of the context of the It() closures, i.e. like this:

Context("When ensuring a statefulset", func() {
	etcdcluster := &etcdaenixiov1alpha1.EtcdCluster{}
	sts := &appsv1.StatefulSet{}
	AfterEach(func() {
		k8sClient.Delete(ctx, sts)
	})
	It("should create the statefulset", func() {
		// statements
	})
	// other test cases
})

and NOT like this:

Context("When ensuring a statefulset", func() {
	etcdcluster := &etcdaenixiov1alpha1.EtcdCluster{}
	It("should create the statefulset", func() {
		sts := &appsv1.StatefulSet{}
		// statements
		k8sClient.Delete(ctx, sts)
	})
	// other test cases
})

References

The Ginkgo documentation is helpful, but the following two sections are particularly useful:

• Mental Model: How Ginkgo Traverses the Spec Hierarchy
• Avoid Spec Pollution: Don't Initialize Variables in Container Nodes

Additional remarks

It would be helpful to separate testing of generating a StatefulSet, applying it to a cluster and validating the result that was read back from the cluster. It would enable tests to capture an incorrectly generated spec well before spinning up even a mock k8s cluster and applying the spec to it. It is not that useful to verify that something can be applied to a cluster, as compared to verifying, that the correct thing was generated in the first place.