Modify dynamic_resource_e2e_test to ginkgo #1424

harishsurf · 2020-04-03T14:56:41Z

Description of the change:
This change adds more structure to the tests by leveraging ginkgo's container/It blocks. The changes also cleans up some of the functions in util_test.go. I have posted some questions in the Files changed section for which I need some inputs. Thanks!

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

test/e2e/util_test.go

test/e2e/dynamic_resource_e2e_test.go

harishsurf · 2020-04-03T15:13:21Z

test/e2e/dynamic_resource_e2e_test.go

+						}
+					}
+					//Expect(expectedSteps).To(BeZero(), "Resource steps do not match expected: %#v", expectedSteps)
+					require.Lenf(GinkgoT(), expectedSteps, 0, "Resource steps do not match expected: %#v", expectedSteps)


Forgot to change: This line needs to be replaced with the commented line above

test/e2e/dynamic_resource_e2e_test.go

test/e2e/util_test.go

test/e2e/assets/chart/zz_chart.go

pkg/api/apis/operators/install/install.go

ecordell · 2020-04-03T17:45:20Z

This is on the right track @harishsurf, but we need to work on the test nodes a bit more before merging. This is what it looks like currently:

 Dynamic Resource
  Create namespace and CatalogSource 
    Generate a Subscription
      Resolve prometheus API
        Wait for the expected InstallPlan's execution to either fail or succeed

Some of this is subjective, but in my opinion the nodes we define have two goals:

Be as readable as possible to a wide audience
Define contexts that we can (eventually) re-use between tests

[Describe] Subscriptions create required objects from Catalogs
  [Context] Given a Namespace with an OwnNamespace OperatorGroup
      [Context] when a CatalogSource is created with a bundle that contains prometheus objects
         [It] should install the operator successfully
             [By] creating a subscription to the CatalogSource
         [It] should have created the expected prometheus objects

in the future we may re-use the catalog context by adding more sub-contexts or it blocks - the pattern of creating a catalog source with some content, creating a subscription, and verifying the installed objects is one of the most common patterns we have in the e2e tests.

Bowenislandsong

Great work. Some nits.

Bowenislandsong · 2020-04-03T21:16:09Z

test/e2e/dynamic_resource_e2e_test.go

+
+			deleteOpts = &metav1.DeleteOptions{}
+			// Delete Namespace
+			err := c.KubernetesInterface().CoreV1().Namespaces().Delete(ns.GetName(), deleteOpts)


IIRC, there are efforts to clean the namespace before deleting it. I wonder if you want to add them here before deleting the ns. I understand the current work would delete all resources that are in the ns, but may cause deletion to hang if some resources decide to stick around.

On the worst case, I wonder if adding delediate deletion/gracePeriod=0 would help.

@Bowenislandsong , what resources should be cleaned up before deleting a namespace? I was under the impression that cleaner.NotifyTestComplete(true) which internally calls cleanupOLM deletes OLM CR's.

ahh. I see. Then this is good. Thanks for the explanation!

Bowenislandsong · 2020-04-03T21:23:34Z

test/e2e/dynamic_resource_e2e_test.go

+				Expect(err).NotTo(HaveOccurred())
+
+				// Wait for the CatalogSource to be ready
+				catsrc, err = fetchCatalogSource(crc, catsrc.GetName(), catsrc.GetNamespace(), catalogSourceRegistryPodSynced)


You can change the fetchCatalogSource function name if you feel need to explain what it does.

fetchCatalogSourceOnReady or fetchCatalogSourceWhenAvailable would be a better name?

fetchCatalogSourceOnStatus ? Since you define the status of it to wait on.

Bowenislandsong · 2020-04-03T21:25:06Z

test/e2e/dynamic_resource_e2e_test.go

+
+		AfterEach(func() {
+
+			deleteOpts = &metav1.DeleteOptions{}


either make this global or declare it every time you use it.

Ginkgo randomize executions. This deleteOpts may miss its ordering.

test/e2e/dynamic_resource_e2e_test.go

harishsurf · 2020-04-07T21:02:27Z

/retest

test/e2e/dynamic_resource_e2e_test.go

harishsurf · 2020-04-13T14:07:42Z

/retest

test/e2e/dynamic_resource_e2e_test.go

ecordell

/lgtm

ecordell · 2020-04-14T00:53:51Z

/retest

openshift-ci-robot · 2020-04-27T21:32:32Z

@harishsurf: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

Modify dynamic_resource_e2e_test to ginkgo

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.

openshift-bot · 2020-04-27T21:33:06Z

/retest

Please review the full test history for this PR and help us cut down flakes.

harishsurf · 2020-04-28T14:52:12Z

/retest

harishsurf · 2020-04-29T16:48:48Z

/retest

harishsurf · 2020-04-30T18:20:37Z

/test e2e-aws-olm

harishsurf · 2020-05-08T13:26:41Z

/test e2e-aws-olm

harishsurf · 2020-05-08T16:28:54Z

/retest

harishsurf · 2020-05-12T15:40:37Z

/test e2e-aws-olm

harishsurf · 2020-05-12T21:59:26Z

/test e2e-aws-console-olm

test/e2e/csv_e2e_test.go

awgreene

/approve

A few non-blocking nits.

test/e2e/catalog_e2e_test.go

openshift-ci-robot · 2020-05-12T22:12:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awgreene, benluddy, harishsurf

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [benluddy]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

awgreene · 2020-05-13T19:02:39Z

/lgtm

shawn-hurley · 2020-05-13T19:21:31Z

Overriding valid bug because this is fixing tests

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 3, 2020

openshift-ci-robot requested review from ecordell and exdx April 3, 2020 14:56

harishsurf commented Apr 3, 2020

View reviewed changes

test/e2e/util_test.go Show resolved Hide resolved

harishsurf commented Apr 3, 2020

View reviewed changes

test/e2e/dynamic_resource_e2e_test.go Outdated Show resolved Hide resolved

harishsurf commented Apr 3, 2020

View reviewed changes

benluddy reviewed Apr 3, 2020

View reviewed changes

harishsurf force-pushed the e2e-part1 branch 2 times, most recently from be8d5b1 to ab7a649 Compare April 3, 2020 20:49

Bowenislandsong reviewed Apr 3, 2020

View reviewed changes

harishsurf force-pushed the e2e-part1 branch from ab7a649 to 81f6f29 Compare April 6, 2020 20:48

harishsurf changed the title ~~[WIP] Modify dynamic_resource_e2e_test to ginkgo~~ Modify dynamic_resource_e2e_test to ginkgo Apr 6, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 6, 2020

benluddy reviewed Apr 7, 2020

View reviewed changes

harishsurf force-pushed the e2e-part1 branch from 81f6f29 to caf1eba Compare April 7, 2020 18:20

benluddy reviewed Apr 8, 2020

View reviewed changes

test/e2e/dynamic_resource_e2e_test.go Outdated Show resolved Hide resolved

harishsurf force-pushed the e2e-part1 branch 3 times, most recently from 484f7cf to 8006cb0 Compare April 12, 2020 19:36

benluddy reviewed Apr 13, 2020

View reviewed changes

test/e2e/dynamic_resource_e2e_test.go Outdated Show resolved Hide resolved

harishsurf force-pushed the e2e-part1 branch 2 times, most recently from 91cfe93 to a646d19 Compare April 13, 2020 21:43

ecordell reviewed Apr 13, 2020

View reviewed changes

openshift-ci-robot assigned ecordell Apr 13, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2020

ecordell mentioned this pull request Apr 14, 2020

Admission webhoooks #1436

Merged

openshift-ci-robot removed the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Apr 27, 2020

harishsurf force-pushed the e2e-part1 branch from 53e9f67 to 3303ae7 Compare April 27, 2020 21:45

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 27, 2020

harishsurf force-pushed the e2e-part1 branch 2 times, most recently from c023c02 to 75ad6a1 Compare April 27, 2020 22:04

harishsurf force-pushed the e2e-part1 branch from 75ad6a1 to 45398bf Compare April 29, 2020 14:40

harishsurf force-pushed the e2e-part1 branch 2 times, most recently from 71bf25e to 70a4818 Compare May 7, 2020 16:41

harishsurf force-pushed the e2e-part1 branch from 70a4818 to 7890511 Compare May 12, 2020 19:51

awgreene reviewed May 12, 2020

View reviewed changes

test/e2e/csv_e2e_test.go Show resolved Hide resolved

awgreene reviewed May 12, 2020

View reviewed changes

test/e2e/catalog_e2e_test.go Show resolved Hide resolved

Modify dynamic_resource_e2e_test to ginkgo

4cf3438

harishsurf force-pushed the e2e-part1 branch from 7890511 to 4cf3438 Compare May 13, 2020 16:06

openshift-ci-robot assigned awgreene May 13, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 13, 2020

shawn-hurley added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label May 13, 2020

openshift-merge-robot merged commit 34d6563 into operator-framework:master May 13, 2020

harishsurf deleted the e2e-part1 branch May 19, 2020 14:37

Modify dynamic_resource_e2e_test to ginkgo #1424

Modify dynamic_resource_e2e_test to ginkgo #1424

Uh oh!

Conversation

harishsurf commented Apr 3, 2020

Uh oh!

Uh oh!

Uh oh!

harishsurf Apr 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ecordell commented Apr 3, 2020

Uh oh!

Bowenislandsong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

harishsurf commented Apr 7, 2020

Uh oh!

Uh oh!

harishsurf commented Apr 13, 2020

Uh oh!

Uh oh!

ecordell left a comment

Choose a reason for hiding this comment

Uh oh!

ecordell commented Apr 14, 2020

Uh oh!

openshift-ci-robot commented Apr 27, 2020

Uh oh!

openshift-bot commented Apr 27, 2020

Uh oh!

harishsurf commented Apr 28, 2020

Uh oh!

harishsurf commented Apr 29, 2020

Uh oh!

harishsurf commented Apr 30, 2020

Uh oh!

harishsurf commented May 8, 2020

Uh oh!

harishsurf commented May 8, 2020

Uh oh!

harishsurf commented May 12, 2020

Uh oh!

harishsurf commented May 12, 2020

Uh oh!

Uh oh!

awgreene left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

openshift-ci-robot commented May 12, 2020

Uh oh!

awgreene commented May 13, 2020

harishsurf Apr 3, 2020 •

edited

Loading