test(e2e): delete all subs, installplans, catalogs, and csvs in test teardown #1635

njhale · 2020-07-13T20:08:19Z

Replace the existing test cleaner, which preserved test resources on
failure, with a tear down function that always deletes
non-essential Subscriptions, InstallPlans, CatalogSources, and
ClusterServiceVersions in the given test namespace.

openshift-ci-robot · 2020-07-13T20:08:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: njhale

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

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

ecordell · 2020-07-13T21:01:43Z

test/e2e/util_test.go

 }

+// func cleanupOLM(namespace string) {


probably just delete this?

ecordell · 2020-07-13T21:03:39Z

test/e2e/util_test.go

+)
+
+// TearDown deletes all OLM resources in the corresponding namespace and at the cluster scope.
+func TearDown(namespace string) {


it would be useful if there were a way to control this (i.e. via an env var) so that locally I can quickly switch cleanup off to look at the cluster state.

And I think really what we want is a sort of "must-gather" per test. Doesn't need to block this PR but I'd be interested in something that say, read all of this content off the cluster and saved it to configmaps before deleting everything.

@benluddy and I were just talking about this over Slack.

He recommended implementing a Reporter to do this. We were thinking that for an initial pass we could export all resources in the operators.coreos.com API group to a tar file hosted in cloud storage at the end of each failed spec, then the reporter could post a link to each export in the suite summary.

I could probably take a crack at a custom reporter for this PR, providing you agree it satisfies your feedback.

That sounds good, but it would be nice if we can make it work across github actions, openshift ci, and locally.

That sounds good, but it would be nice if we can make it work across github actions, openshift ci, and locally.

It may be good enough to accept a template string via environment variables to construct those URLs and set it appropriately in the corresponding config.

benluddy · 2020-07-13T20:23:07Z

test/e2e/catalog_e2e_test.go

@@ -31,7 +31,7 @@ import (
 var _ = Describe("Catalog", func() {
 	It("loading between restarts", func() {

-		defer cleaner.NotifyTestComplete(true)
+		defer TearDown(testNamespace)


Can these all turn into one AfterEach since you're touching them anyway? I see at least a couple of the defers appear partway through a spec, but if this cleanup behavior is supposed to be idempotent, that seems like an overcomplication.

Can these all turn into one AfterEach since you're touching them anyway?

Yeah, can do.

but if this cleanup behavior is supposed to be idempotent, that seems like an overcomplication.

I agree, but I think we need to consider cleaning these up carefully though. Not all tests use testNamespace, but instead generate one or more namespaces on the fly, which is what I think some of these defer statements service. I n general, I think we should standardize on one of the two above patterns across all of our tests. Suggestions?

Can we use the TestContext to provide namespaces to tests? We'd be able to make cleanup, randomization, etc. transparent to individual tests.

…teardown Replace the existing test cleaner, which preserved test resources on failure, with a tear down function that always deletes non-essential Subscriptions, InstallPlans, CatalogSources, and ClusterServiceVersions in the given test namespace.

ecordell · 2020-07-14T16:03:12Z

/lgtm

openshift-bot · 2020-07-14T16:35:06Z

/retest