Convert e2e go tests to ginkgo - using ginkgo convert utility #1372
Conversation
There was a problem hiding this comment.
The initial conversion looks good!
There are a couple of failures in the e2e tests that we should double check.
Otherwise it would be great if we could spend an hour or two slightly re-naming the tests so that that output is a little more readable.
Right now I see things like:
Subscription Tests [It] subscription updates existing install plan
but part of BDD is to make the test plan read a little more naturally, in this case something like:
Subscription [It] updates an existing install plan
Small tweaks like that can make it really clear what's happening and why.
We can also be way more generous with the use of Context/It/etc. The majority of the comments in the e2e suite can probably be lines that show up as a block description.
test/e2e/user_defined_sa_test.go
Outdated
| @@ -19,172 +19,175 @@ import ( | |||
| "k8s.io/kubernetes/pkg/apis/rbac" | |||
| ) | |||
|
|
|||
| func TestUserDefinedServiceAccountWithNoPermission(t *testing.T) { | |||
| defer cleaner.NotifyTestComplete(t, false) | |||
| var _ = Describe("Testing with Ginkgo", func() { | |||
There was a problem hiding this comment.
Can we figure out better names for these top-level Describes? This one could be "User Defined ServiceAccount" and the first It could be with no permission
There was a problem hiding this comment.
@ecordell, Part of the reason for not using different Containers / Spec combinations in this PR is:
- It will introduce a lot of changes in a single PR (difficult to comprehend and review) and this change would take a lot of time.
- Some of the tests, from what I understand could be restructured.
What I thought would be a better approach is to have an initial working ginkgo conversion in place and then have separate PRs to do incremental changes.
But, for this PR, I will work with someone and have a better Describe/ It names for the tests.
openshift-ci-robot
added
the
needs-rebase
There was a problem hiding this comment.
It's a big diff, and a mostly-mechanical conversion, so I only mentioned a handful of things that stood out to me.
Evan's point about BDD and comprehensibility is a really important one -- I would prefer a test with a completely arcane implementation that tests a clearly-defined behavior versus a test that is straightforward about what it does but opaque about why it does what it does.
Be careful about long-lived feature branches! Over time, without occasionally rebasing, it can become harder to correctly resolve merge conflicts when you finally go to reintegrate your branch.
test/e2e/packagemanifest_e2e_test.go
Outdated
| type packageManifestCheckFunc func(*packagev1.PackageManifest) bool | ||
|
|
||
| func packageManifestHasStatus(pm *packagev1.PackageManifest) bool { | ||
| // as long as it has a package name we consider the status non-empty | ||
|
|
||
| if pm == nil || pm.Status.PackageName == "" { |
There was a problem hiding this comment.
Wouldn't mind seeing the function body rewritten to return pm != nil && pm.Status.PackageName != "", if you agree.
test/e2e/subscription_e2e_test.go
Outdated
| }) | ||
| It( | ||
|
|
||
| // TestSubscriptionStatusMissingTargetCatalogSource ensures that a Subscription has the appropriate status condition when |
There was a problem hiding this comment.
It's unusual to have this chunk of single-line comments appear inside of a method invocation. Line 585 jumped out at me because I was surprised to see a string literal immediately following these lines of comments. How about putting them immediately above the It?
test/e2e/subscription_e2e_test.go
Outdated
| }) | ||
| It( | ||
|
|
||
| // TestSubscriptionInstallPlanStatus ensures that a Subscription has the appropriate status conditions for possible referenced |
There was a problem hiding this comment.
I have the same comment here as the spec above, do you mind changing these (unless you disagree)?
openshift-ci-robot
removed
the
needs-rebase
|
/retest It looks like this may be a pre-existing flake. The last PR the touched this exhibited the same failure, but eventually passed and merged (https://openshift-gce-devel.appspot.com/pr/operator-framework_operator-lifecycle-manager/1334). If it eventually passes here, then I would like to merge this and address the flake in a follow-up to limit the number of times this change needs to be rebased. |
|
/retest |
|
/retest |
1 similar comment
|
/retest |
|
/test e2e-gcp |
openshift-ci-robot
added
the
needs-rebase
openshift-ci-robot
removed
the
needs-rebase
|
/test e2e-aws-console-olm |
|
/retest |
|
/lgtm Good job staying on top of this conversion! Looking forward to seeing the next steps. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
Description of the change:
This change converts all the e2e tests written in vanilla go to ginkgo test. This is a first step that does basic ginkgo conversion and ensure that all the e2e tests are run successfully. There will be separate PR per test to do incremental changes to achieve full-fledge BDD driven testing.
Motivation for the change:
Reviewer Checklist
/docs