Convert gc_e2e_test.go to ginkgo #1429

harishsurf · 2020-04-06T22:35:34Z

Description of the change:
This change converts gc_e2e_test.go to leverage ginkgo's BDD approach

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-06T22:53:52Z

test/e2e/gc_e2e_test.go

+
+						//By("check for dependent (should be deleted since last blocking owner was deleted", func() {
+						// check for dependent (should be deleted since last blocking owner was deleted)
+						cf, err := kubeClient.KubernetesInterface().CoreV1().ConfigMaps(testNamespace).Get(dependent.GetName(), metav1.GetOptions{})


The test ensures that the dependent configMap is not deleted if one of its owner reference is deleted.

Problem: The assertion at line 266 fails because err is nil

What I found:
When ownerB CSV is deleted, an empty configMap struct is returned in the older version. (line 262)
See below output:
Current version:

###B &ConfigMap{ObjectMeta:{dependent openshift-operators /api/v1/namespaces/openshift-operators/configmaps/dependent 412d8073-d3ce-4d11-9a77-7876f0e8e062 161740 0 2020-04-06 18:25:30 -0400 EDT <nil> <nil> map[] map[] [

Older version:

###B &ConfigMap{ObjectMeta:{ 0 0001-01-01 00:00:00 +0000 UTC <nil> <nil> map[] map[] [] [] []},Data:map[string]string{},BinaryData:map[string][]byte{},}cleaning up any remaining non persistent resources...

The cf returned here has value whereas in the older version of the file, cf returns an empty ConfigMap struct as seen above

Because of this, at line 266
Current version:

Error is <nil> and value is <nil>

vs
Older version:

Error type *errors.StatusError and value is configmaps "dependent" not found

I wonder if that has anything to do with your foreground deletion option. Since the object in the configmap returned is the dependent.

Figured out the issue. It was related to the placement of the deletion code inside of It block. Thanks for looking into this!

ecordell · 2020-04-08T21:20:54Z

test/e2e/gc_e2e_test.go

+
+					When("removing one of the owner", func() {
+
+						It("deleting ownerA CSV in the foreground", func() {


nit:

Suggested change

It("deleting ownerA CSV in the foreground", func() {

It("deletes ownerA CSV in the foreground", func() {

ecordell · 2020-04-08T21:21:30Z

test/e2e/gc_e2e_test.go

+					Expect(err).NotTo(HaveOccurred(), "dependent could not be created")
+				})
+
+				Context("owner reference behavior can be verified", func() {


shouldn't this describe the context more? something about the foreground propogation policy?

I have added it :)

ecordell · 2020-04-08T21:25:02Z

test/e2e/gc_e2e_test.go

+	//				CSV-A     CSV-B                        CSV-B
+	//				   \      /      --Delete CSV-A-->       |
+	//				   ConfigMap						 ConfigMap
+	Describe("Multiple OwnerReferences will not be garbage collected", func() {


Suggested change

Describe("Multiple OwnerReferences will not be garbage collected", func() {

Describe("Resources with Multiple OwnerReferences", func() {

harishsurf · 2020-04-13T13:02:19Z

/retest

harishsurf · 2020-04-21T17:27:06Z

/retest

test/e2e/gc_e2e_test.go

harishsurf · 2020-04-28T20:20:15Z

/retest

harishsurf · 2020-04-29T17:19:39Z

/retest

harishsurf · 2020-04-29T17:27:22Z

/retest

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

/lgtm

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

/retest

awgreene

No blocking nits - feel free to update the PR as you see fit.

test/e2e/gc_e2e_test.go

awgreene · 2020-05-07T13:24:55Z

/approve

shawn-hurley · 2020-05-08T13:23:34Z

Adding a valid bug because making CI tests more reliable.

harishsurf · 2020-05-08T14:52:36Z

/test e2e-gcp

openshift-bot · 2020-05-08T14:56:11Z

/retest

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

openshift-bot · 2020-05-08T15:35:15Z

/retest

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

shawn-hurley · 2020-05-08T16:56:31Z

/hold

I am worried that we are adding flakes to CI

the test failures appear to be related to this change can someone look?

harishsurf · 2020-05-11T15:22:12Z

/hold

I am worried that we are adding flakes to CI

the test failures appear to be related to this change can someone look?

@shawn-hurley, I ran some analysis(with Ben's help) on our ci-failures using prow's history. Could you take a look at this gist and see if it has enough evidence to have this and other ginkgo PRs merged?
cc @benluddy @ecordell

shawn-hurley · 2020-05-11T17:11:13Z

/hold cancel

shawn-hurley · 2020-05-11T17:11:19Z

/retest

openshift-bot · 2020-05-11T17:15:01Z

/retest

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

openshift-bot · 2020-05-11T19:12:03Z

/retest

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

openshift-bot · 2020-05-11T19:25:00Z

/retest

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

openshift-bot · 2020-05-11T21:21:59Z

/retest

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

openshift-bot · 2020-05-11T21:34:59Z

/retest

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

openshift-bot · 2020-05-11T23:44:58Z

/retest

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

openshift-bot · 2020-05-12T02:58:40Z

/retest

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

openshift-bot · 2020-05-12T03:11:40Z

/retest

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

openshift-bot · 2020-05-12T04:55:43Z

/retest

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

openshift-bot · 2020-05-12T05:08:38Z

/retest

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

openshift-bot · 2020-05-12T06:39:39Z

/retest

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

openshift-bot · 2020-05-12T08:36:40Z

/retest

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

openshift-bot · 2020-05-12T08:49:42Z

/retest

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

openshift-bot · 2020-05-12T10:59:39Z

/retest

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

openshift-bot · 2020-05-12T11:12:39Z

/retest

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

openshift-bot · 2020-05-12T12:44:23Z

/retest

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

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 6, 2020

openshift-ci-robot requested review from awgreene and benluddy April 6, 2020 22:35

harishsurf commented Apr 6, 2020

View reviewed changes

harishsurf force-pushed the e2e-gc branch 2 times, most recently from fbee673 to 182efde Compare April 7, 2020 14:30

ecordell reviewed Apr 8, 2020

View reviewed changes

harishsurf force-pushed the e2e-gc branch from 182efde to 63ae87b Compare April 11, 2020 23:29

harishsurf changed the title ~~WIP: Convert gc_e2e_test.go to ginkgo~~ Convert gc_e2e_test.go to ginkgo Apr 13, 2020

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Apr 13, 2020

harishsurf force-pushed the e2e-gc branch from 63ae87b to eff36ad Compare April 17, 2020 17:15

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 17, 2020

harishsurf force-pushed the e2e-gc branch 2 times, most recently from 17aa209 to 1afae36 Compare April 21, 2020 13:07

benluddy reviewed Apr 21, 2020

View reviewed changes

harishsurf force-pushed the e2e-gc branch from 1afae36 to 75ede5e Compare April 28, 2020 17:45

harishsurf force-pushed the e2e-gc branch from 75ede5e to c59cd5e Compare April 30, 2020 18:07

harishsurf mentioned this pull request Apr 30, 2020

[WIP] test PR #1495

Closed

5 tasks

openshift-ci-robot assigned exdx May 6, 2020

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

awgreene requested changes May 7, 2020

View reviewed changes

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

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

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

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

test/e2e/gc_e2e_test.go Show resolved Hide resolved

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 7, 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 8, 2020

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 8, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 11, 2020

openshift-merge-robot merged commit d23b7ab into operator-framework:master May 12, 2020

harishsurf deleted the e2e-gc branch May 19, 2020 14:38


		When("removing one of the owner", func() {

		It("deleting ownerA CSV in the foreground", func() {

	It("deleting ownerA CSV in the foreground", func() {
	It("deletes ownerA CSV in the foreground", func() {

	Describe("Multiple OwnerReferences will not be garbage collected", func() {
	Describe("Resources with Multiple OwnerReferences", func() {

Convert gc_e2e_test.go to ginkgo #1429

Convert gc_e2e_test.go to ginkgo #1429

Uh oh!

Conversation

harishsurf commented Apr 6, 2020

Uh oh!

harishsurf Apr 6, 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!

Bowenislandsong Apr 7, 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!

harishsurf Apr 7, 2020

Choose a reason for hiding this comment

Uh oh!

ecordell Apr 8, 2020

Choose a reason for hiding this comment

Uh oh!

ecordell Apr 8, 2020

Choose a reason for hiding this comment

Uh oh!

harishsurf Apr 11, 2020

Choose a reason for hiding this comment

Uh oh!

ecordell Apr 8, 2020

Choose a reason for hiding this comment

Uh oh!

harishsurf commented Apr 13, 2020

Uh oh!

harishsurf commented Apr 21, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

harishsurf commented Apr 28, 2020

Uh oh!

harishsurf commented Apr 29, 2020

Uh oh!

harishsurf commented Apr 29, 2020

Uh oh!

exdx commented May 6, 2020

Uh oh!

exdx commented May 6, 2020

Uh oh!

awgreene left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

awgreene commented May 7, 2020

Uh oh!

shawn-hurley commented May 8, 2020

Uh oh!

harishsurf commented May 8, 2020

Uh oh!

openshift-bot commented May 8, 2020

Uh oh!

openshift-bot commented May 8, 2020

Uh oh!

shawn-hurley commented May 8, 2020

Uh oh!

harishsurf commented May 11, 2020

Uh oh!

shawn-hurley commented May 11, 2020

Uh oh!

shawn-hurley commented May 11, 2020

Uh oh!

openshift-bot commented May 11, 2020

Uh oh!

openshift-bot commented May 11, 2020

harishsurf Apr 6, 2020 •

edited

Loading

Bowenislandsong Apr 7, 2020 •

edited

Loading