Convert package manifest e2e to ginkgo test #1439

harishsurf · 2020-04-13T14:12:33Z

Description of the change:
This change converts packagemanifest_e2e_test.go to leverage ginkgo's BDD approach.
The change also involves adding GinkgoRecover wherever go routines are called

Motivation for the change:

Reviewer Checklist

Implementation matches the proposed design, or proposal is updated to match implementation
Sufficient unit test coverage
Sufficient end-to-end test coverage
Docs updated or added to /docs
Commit messages sensible and descriptive

harishsurf · 2020-04-13T14:58:02Z

test/e2e/catalog_e2e_test.go

@@ -242,10 +242,10 @@ var _ = Describe("Catalog", func() {
 		}

 		// Create the initial catalogsource
-		createInternalCatalogSource(GinkgoT(), c, crc, mainCatalogName, testNamespace, mainManifests, nil, []v1alpha1.ClusterServiceVersion{mainCSV})
+		createInternalCatalogSource(c, crc, mainCatalogName, testNamespace, mainManifests, nil, []v1alpha1.ClusterServiceVersion{mainCSV})


This method uses deprecated internal catalog source type. I was thinking of rewriting this method to use configMap catalog source type as a separate PR. Thoughts?

I think that is a great idea.

harishsurf · 2020-04-13T15:20:13Z

test/e2e/packagemanifest_e2e_test.go

-			_, ok := expectedRelatedImages[v]
-			require.True(GinkgoT(), ok, "Expect this image %s to exist in the related images list\n", v)
-		}
+	Context("Given an operator-registry image", func() {


Not a 100% sure, if the Context wording is accurate. I felt a second look at it would be good.

Until we update this test to build its own registry image during the test run, it makes more sense to me to merge the
"CatalogSource created..." context into this and leave the image reference string as an implementation detail.

harishsurf · 2020-04-21T16:41:43Z

/retest

benluddy · 2020-04-21T17:56:57Z

test/e2e/util_test.go

-	require.NoError(t, extScheme.AddToScheme(scheme))
-	require.NoError(t, k8sscheme.AddToScheme(scheme))
-	require.NoError(t, v1beta1.AddToScheme(scheme))
+	err := extScheme.AddToScheme(scheme)


A slightly more gomega-forward way of writing this might be Expect(extScheme.AddToScheme(scheme)).To(Succeed()).

https://godoc.org/github.com/onsi/gomega#Succeed

benluddy · 2020-04-21T18:05:27Z

test/e2e/packagemanifest_e2e_test.go

+					relatedImages := pm.Status.Channels[0].CurrentCSVDesc.RelatedImages
+					Expect(len(relatedImages)).To(Equal(len(expectedRelatedImages)))
+
+					for _, v := range relatedImages {


This could be:

Expect(relatedImages).To(ConsistOf([]string{ "quay.io/coreos/etcd@sha256:3816b6daf9b66d6ced6f0f966314e2d4f894982c6b1493061502f8c2bf86ac84", "quay.io/coreos/etcd@sha256:49d3d4a81e0d030d3f689e7167f23e120abf955f7d08dbedf3ea246485acee9f", "quay.io/coreos/etcd-operator@sha256:c0301e4686c3ed4206e370b42de5a3bd2229b9fb4906cf85f3f30650424abec2" }))

which would cover this for loop as well as the length assertion on L187.

benluddy · 2020-04-21T18:11:19Z

test/e2e/packagemanifest_e2e_test.go

-			return false, nil
+	Context("Given a ConfigMap containing package manifest, CSV and CRD", func() {
+		When("A CatalogSource is created using the ConfigMap as catalog source type", func() {
+			It("Package manifest loading can be verified", func() {


Spec descriptions need to state the behavior that is being verified!

benluddy · 2020-04-21T18:12:13Z

test/e2e/packagemanifest_e2e_test.go

+				csvJSON, _ := json.Marshal(csv)
+
+				// Wait for package-server to be ready
+				err := wait.Poll(pollInterval, 1*time.Minute, func() (bool, error) {


Can this use Eventually?

benluddy · 2020-04-21T18:13:24Z

test/e2e/packagemanifest_e2e_test.go

+				csv := newCSV(packageStable, testNamespace, "", semver.MustParse("0.1.0"), []apiextensions.CustomResourceDefinition{crd}, nil, namedStrategy)
+				csv.SetLabels(map[string]string{"projected": "label"})
+				csv.Spec.NativeAPIs = []metav1.GroupVersionKind{{Group: "kubenative.io", Version: "v1", Kind: "Native"}}
+				csvJSON, _ := json.Marshal(csv)


I'm sure this isn't new, but the test shouldn't be ignoring the error value returned from Marshal. A nice thing to add (in a separate PR) would be automation for running static analysis tools like errcheck on every change.

benluddy · 2020-04-21T18:14:07Z

test/e2e/packagemanifest_e2e_test.go

-				return true, nil
-			}
-			return false, nil
+	Context("Given a ConfigMap containing package manifest, CSV and CRD", func() {


The ConfigMap mentioned in the container description should be created as part of this container (in a BeforeEach).

Edit: I have the same comment with a few other container nodes, please take a look at those.

benluddy · 2020-04-21T18:17:18Z

test/e2e/packagemanifest_e2e_test.go

+				_, err = fetchCatalogSource(crc, catalogSourceName, testNamespace, catalogSourceRegistryPodSynced)
+				Expect(err).ToNot(HaveOccurred())
+
+				By("querying the package manifest using the package name", func() {


IMO it's clearer not to pass the option function to By, to reduce nesting and avoid giving the impression of a new container node. The only use case I can imagine for the optional function argument would be calling a named function that was used in multiple specs.

benluddy · 2020-04-21T18:28:38Z

test/e2e/packagemanifest_e2e_test.go

-		}
+	Context("Given an operator-registry image", func() {
+		Context("When a CatalogSource is created using gRPC catalog source type", func() {
+			When("Package manifest is available", func() {


I wonder if it would be clearer to change the test from:

poll until list package manifests returns without error

require that get catalog source returns without error

poll until get package manifest returns without error

to:

poll until get package manifest returns without error

It seems to me that we're checking for multiple related conditions, when a failure in any one of them would imply a failure of the primary test condition.

benluddy · 2020-04-21T18:31:55Z

test/e2e/packagemanifest_e2e_test.go

-			_, ok := expectedRelatedImages[v]
-			require.True(GinkgoT(), ok, "Expect this image %s to exist in the related images list\n", v)
-		}
+	Context("Given an operator-registry image", func() {


Until we update this test to build its own registry image during the test run, it makes more sense to me to merge the
"CatalogSource created..." context into this and leave the image reference string as an implementation detail.

Bowenislandsong

NIce work! Just added some nits.

Bowenislandsong · 2020-04-21T18:34:37Z

test/e2e/catalog_e2e_test.go

@@ -242,10 +242,10 @@ var _ = Describe("Catalog", func() {
 		}

 		// Create the initial catalogsource
-		createInternalCatalogSource(GinkgoT(), c, crc, mainCatalogName, testNamespace, mainManifests, nil, []v1alpha1.ClusterServiceVersion{mainCSV})
+		createInternalCatalogSource(c, crc, mainCatalogName, testNamespace, mainManifests, nil, []v1alpha1.ClusterServiceVersion{mainCSV})


I think that is a great idea.

Bowenislandsong · 2020-04-21T18:56:32Z

test/e2e/packagemanifest_e2e_test.go

@@ -3,170 +3,196 @@ package e2e
 import (
 	"context"
 	"encoding/json"
+


nit: dont need new line

Bowenislandsong · 2020-04-21T18:57:06Z

test/e2e/packagemanifest_e2e_test.go

-			},
-		}
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"


nit: these need to be in the second group of imports

benluddy · 2020-05-04T15:39:05Z

test/e2e/packagemanifest_e2e_test.go

-			"quay.io/coreos/etcd@sha256:49d3d4a81e0d030d3f689e7167f23e120abf955f7d08dbedf3ea246485acee9f":          "",
-			"quay.io/coreos/etcd-operator@sha256:c0301e4686c3ed4206e370b42de5a3bd2229b9fb4906cf85f3f30650424abec2": "",
-		}
+		It("lists the CatalogSource contents using the PackageManifest API", func() {


Is this two specs in one ("packagemanifest can be retrieved by name" and "packagemanifest appears in list")? If yes, I think the second one is underspecified -- it would pass if Items is an empty slice or if it contained a packagemanifest other than the expected one.

I split them into two It specs

benluddy · 2020-05-04T15:43:14Z

test/e2e/packagemanifest_e2e_test.go

-		require.NoError(GinkgoT(), err, "error getting package manifest")
-		require.NotNil(GinkgoT(), pm)
-		require.Equal(GinkgoT(), packageName, pm.GetName())
+		It("lists the CatalogSource contents using the PackageManifest API", func() {


Is this precisely stating the behavior the test is intended to verify? It looks like a relatedImages test.

exdx · 2020-05-06T15:17:01Z

/lgtm

exdx · 2020-05-06T15:29:18Z

/test e2e-aws-olm

Bowenislandsong · 2020-05-06T19:27:55Z

/lgtm

benluddy · 2020-05-06T18:33:01Z

test/e2e/packagemanifest_e2e_test.go

 		fetched, err = pmc.OperatorsV1().PackageManifests(namespace).Get(context.TODO(), name, metav1.GetOptions{})
 		if err != nil && !errors.IsNotFound(err) {
 			return true, err
 		}
 		return check(fetched), nil
-	})
+	}).Should(BeTrue())


There's an opportunity here to define custom matchers, which would make the output of test failures more useful. I don't think it needs to block this PR, though.

Noted. I'll look into this and follow it up with another PR

benluddy · 2020-05-06T19:54:11Z

test/e2e/packagemanifest_e2e_test.go

+func containsPackageManifest(pmList []packagev1.PackageManifest, pkgName string) bool {
+	for _, pm := range pmList {
+		if pm.GetName() == pkgName {
+			fmt.Printf("$$$ %v\n", pm.GetName())


Was this left in by mistake?

benluddy · 2020-05-06T20:15:37Z

test/e2e/packagemanifest_e2e_test.go

+func containsPackageManifest(pmList []packagev1.PackageManifest, pkgName string) bool {
+	for _, pm := range pmList {
+		if pm.GetName() == pkgName {
+			fmt.Printf("$$$ %v\n", pm.GetName())


Was this left in by mistake?

benluddy · 2020-05-06T20:17:44Z

test/e2e/packagemanifest_e2e_test.go


 	return fetched, err
 }
+
+func containsPackageManifest(pmList []packagev1.PackageManifest, pkgName string) bool {


This could be expressed by composing the ContainsElement and WithTransform matchers. I suspect this is a common pattern in the tests, so maybe that's something to come back to in the future when considering reusable matchers/fixtures.

harishsurf · 2020-05-07T17:36:51Z

/retest

openshift-bot · 2020-05-18T07:47:14Z

/retest