test/e2e: Refactor the Operator e2e tests to clean up testing resources #2518

timflannagan · 2021-12-10T21:35:38Z

Update test/e2e/operator_test.go and attempt to refactor the current e2e
specs to reduce the number of testing artifacts that are left behind
when individual tests pass/fail.

Signed-off-by: timflannagan [email protected]

Description of the change:

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 /doc
Commit messages sensible and descriptive

timflannagan · 2021-12-10T21:36:38Z

test/e2e/operator_test.go

-		Eventually(func() error {
-			return client.Create(clientCtx, o)
-		}).Should(Succeed())
+	Context("when an Operator resource can select its components by label", func() {


Note: moved this from an It to a Context so we gain access to BeforeEach/AfterEach/etc. blocks. Before, this It spec would leak testing Operator resources when it failed the test case (but cleaned up properly when it passed).

A question and a nit:

Question: Why not start with a When?

nit: We should probably clean up the long list in the comment too. I believe they were written when old tests were being ported over to ginkgo, but time constraint forced to putting these in It blocks and keep those comments for safe measures.

@anik120 +1 for both points.

How easily accessible is the same information in those comments if we were to remove them? Only thing I'd want to make sure of is that we're not losing context in removing them.

Yep agreed - I removed the list of existing comments from this PR and changed the Context -> When container. I'd like to hold off on refactoring that "should surface referenced components in its status" test case as it's not been marked as a flaky when Per had added support for identifying and running flaky tests in their own workflow in #2624.

timflannagan · 2021-12-10T21:37:44Z

test/e2e/operator_test.go

-		Expect(err).ToNot(HaveOccurred())
-		defer w.Stop()
+		It("should not contain copied csv status references", func() {
+			Consistently(o).ShouldNot(ContainCopiedCSVReferences())


I moved this check to it's own It spec as before we were checking whether the operator object had any CSV references before creating that resource, leading to what appeared to be a wasted check.

timflannagan · 2021-12-10T21:38:28Z

test/e2e/operator_test.go

+				}
+				return err
+			}).Should(Succeed())
+		})
 	})

 	Context("when a subscription to a package exists", func() {


Note: we were previously leaking the kiali installation, it's CRDs, and the corresponding Operator resource that was generated despite these test cases passing or failing.

timflannagan · 2021-12-10T21:39:29Z

test/e2e/operator_test.go

+				})
+			}).Should(Succeed())
+
+			By("Deleting the test Operator resource")


It's highly likely that the generated Operator resource will be left behind, despite this check being in place, as there's some caching issues in the current operator controller. I added a top-level DeleteCollection call in the AfterEach block to help alleviate the frequency this happens.

anik120

looks great Tim, have a few very small nits other lgtm

anik120 · 2021-12-16T19:29:27Z

test/e2e/operator_test.go

-		Eventually(func() error {
-			return client.Create(clientCtx, o)
-		}).Should(Succeed())
+	Context("when an Operator resource can select its components by label", func() {


A question and a nit:

Question: Why not start with a When?

nit: We should probably clean up the long list in the comment too. I believe they were written when old tests were being ported over to ginkgo, but time constraint forced to putting these in It blocks and keep those comments for safe measures.

anik120 · 2021-12-16T19:31:20Z

test/e2e/operator_test.go

+			Expect(err).To(BeNil())
+			defer w.Stop()


Suggested change

Expect(err).To(BeNil())

defer w.Stop()

defer w.Stop()

Expect(err).To(BeNil())

Suggested change

Expect(err).To(BeNil())

defer w.Stop()

Expect(err).To(BeNil())

defer w.Stop()

I updated the Context -> When given the comment structure was already there. Do you have any preference when to use a Context vs. When containers? I wasn't able to find any best practices around structuring ginkgo e2e tests so there might be something I'm missing.

test/e2e/operator_test.go

tylerslaton

/lgtm

Small question, but overall the approach looks great. I like the defer pattern you have for cleaning up the resources.

tylerslaton · 2021-12-16T20:05:55Z

test/e2e/operator_test.go

-		nsB.SetName(genName("ns-b-"))
+				return false
+			}))
+			defer w.Stop()


Is this second defer w.Stop() necassary?

I don't think so - I mainly just indented the existing test cases vs. making any heavy modifications.

openshift-ci · 2022-01-27T16:52:16Z

New changes are detected. LGTM label has been removed.

openshift-ci · 2022-01-27T16:52:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: timflannagan

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~~ [timflannagan]

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

Update test/e2e/operator_test.go and attempt to refactor the current e2e specs to reduce the number of testing artifacts that are left behind when individual tests pass/fail. Signed-off-by: timflannagan <[email protected]>

openshift-ci · 2022-04-09T22:19:21Z

@timflannagan: PR needs rebase.

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-ci bot requested review from ankitathomas and hasbro17 December 10, 2021 21:35

timflannagan commented Dec 10, 2021

View reviewed changes

anik120 reviewed Dec 16, 2021

View reviewed changes

tylerslaton reviewed Dec 16, 2021

View reviewed changes

openshift-ci bot assigned tylerslaton Dec 16, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2021

timflannagan mentioned this pull request Jan 27, 2022

Bump k8s dependencies to 1.23 to align with o-f/api #2574

Closed

5 tasks

timflannagan force-pushed the ci/refactor-operator-e2e-tests branch from c813df3 to 2e808cf Compare January 27, 2022 16:52

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 27, 2022

timflannagan force-pushed the ci/refactor-operator-e2e-tests branch 2 times, most recently from 46ff42f to 4bb317b Compare January 27, 2022 21:38

timflannagan force-pushed the ci/refactor-operator-e2e-tests branch from 4bb317b to c3bb990 Compare February 17, 2022 16:48

timflannagan force-pushed the ci/refactor-operator-e2e-tests branch from c3bb990 to 1a21657 Compare March 2, 2022 22:17

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 9, 2022

timflannagan closed this Jul 28, 2022

timflannagan deleted the ci/refactor-operator-e2e-tests branch July 28, 2022 18:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test/e2e: Refactor the Operator e2e tests to clean up testing resources #2518

test/e2e: Refactor the Operator e2e tests to clean up testing resources #2518

timflannagan commented Dec 10, 2021

timflannagan Dec 10, 2021

anik120 Dec 16, 2021

tylerslaton Dec 16, 2021 •

edited

Loading

timflannagan Feb 17, 2022

timflannagan Dec 10, 2021

timflannagan Dec 10, 2021

timflannagan Dec 10, 2021

anik120 left a comment

anik120 Dec 16, 2021

anik120 Dec 16, 2021

timflannagan Feb 17, 2022

tylerslaton left a comment

tylerslaton Dec 16, 2021

timflannagan Dec 16, 2021

openshift-ci bot commented Jan 27, 2022

openshift-ci bot commented Jan 27, 2022

openshift-ci bot commented Apr 9, 2022

test/e2e: Refactor the Operator e2e tests to clean up testing resources #2518

test/e2e: Refactor the Operator e2e tests to clean up testing resources #2518

Conversation

timflannagan commented Dec 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylerslaton Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anik120 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylerslaton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 27, 2022

openshift-ci bot commented Jan 27, 2022

openshift-ci bot commented Apr 9, 2022

tylerslaton Dec 16, 2021 •

edited

Loading