e2e test utilities update #2799

perdasilva · 2022-06-13T07:53:19Z

Description of the change:
This PR makes the following changes:

Moves the TestContext to use the new E2EClient with resource tracking and clean-up
Adds CRD garbage collection to E2EClient
Refactors the MagicCatalog implementation to remove the superfluous interface adds a factory method to create one directly from the file, obviating the need to create a file provider
Adds a DeterminedE2EClient which wraps calls around an Eventually loop
Adds E2ETestHelper. A dedicated class for OLM resource assertions with some initial assertions for Subscriptions and CatalogSource

Motivation for the change:

E2EClient helps keep the cluster clean throughout the e2e test execution
Having a DeterminedClient avoids the need for tests to wrap client calls in Eventually loops, keeping them leaner and easier to read/understand
E2ETestHelper will eventually remove the disparate helper functions that are peppered throughout the e2e tests, gives a single place to add and find assertions (e.g. assert that a subscription has reached a particular state, a catalog source is ready, etc.)
BugFix: removes resource catching from update call in e2e client. This should not be done since it may be updating a resource that is not managed by the client.

Architectural changes:
None

Testing remarks:
Manually tested against e2e tests

Reviewer Checklist

perdasilva · 2022-06-13T13:49:48Z

test/e2e/util/e2e_determined_client.go

+}
+
+func (m *DeterminedE2EClient) Update(context context.Context, obj k8scontrollerclient.Object, options ...k8scontrollerclient.UpdateOption) error {
+	m.keepTrying(func() error {


should we check for conflict failures and get the latest resource version before trying again?

Yes, I think so, otherwise this will be pretty brittle.

We could use the SSA client, or maybe an merge update strategy would work?

test/e2e/magic_catalog.go

njhale

/lgtm

openshift-bot · 2022-06-13T17:54:05Z

/retest-required

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

exdx

Very nice. I had some nits, nothing blocking. I saw @njhale lgtm'd so it's fine to go in as-is.

I respun CI, I think there may be another flake.

exdx · 2022-06-13T17:59:13Z

test/e2e/util/e2e_client.go


 	"github.com/onsi/ginkgo/v2"
-	k8serror "k8s.io/apimachinery/pkg/api/errors"
+	extensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"


nit: we use apiextensionsv1 for this import elsewhere in the codebase

exdx · 2022-06-13T18:03:26Z

test/e2e/util/e2e_client.go

+
+		Logf("deleting %s/%s", namespace, obj.GetName())
+		if err := k8scontrollerclient.IgnoreNotFound(m.Delete(context.Background(), obj)); err != nil {
+			Logf("error deleting object %s/%s: %s", namespace, obj.GetName(), obj)


nit: should the last argument here be the error instead of the obj?

exdx · 2022-06-13T18:06:06Z

test/e2e/util/e2e_client.go

+				if err := m.Client.Delete(context.Background(), &crd); err != nil {
+					return err


CRDs may take some time to delete, especially on openshift, if there are CRs associated with that CRD as well. Would it make sense to block after Deletion and wait for a apierrors.NotFound(m.Client.Get(crd))?

exdx · 2022-06-13T18:10:22Z

test/e2e/util/e2e_determined_client.go

+}
+
+func (m *DeterminedE2EClient) Update(context context.Context, obj k8scontrollerclient.Object, options ...k8scontrollerclient.UpdateOption) error {
+	m.keepTrying(func() error {


Yes, I think so, otherwise this will be pretty brittle.

We could use the SSA client, or maybe an merge update strategy would work?

openshift-ci · 2022-06-13T18:11:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: exdx, njhale, perdasilva

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,perdasilva]

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

exdx · 2022-06-14T00:29:51Z

/lgtm

openshift-bot · 2022-06-14T00:30:08Z

/retest-required

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

exdx · 2022-06-14T00:30:35Z

Should we squash the commits before merging?

openshift-bot · 2022-06-14T00:42:09Z

/retest-required

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

openshift-bot · 2022-06-14T00:54:09Z

/retest-required

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

openshift-bot · 2022-06-14T01:06:08Z

/retest-required

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

openshift-bot · 2022-06-14T01:18:08Z

/retest-required

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

openshift-bot · 2022-06-14T01:30:08Z

/retest-required

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

openshift-bot · 2022-06-14T01:42:13Z

/retest-required

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

timflannagan · 2022-06-14T01:48:41Z

Holding so the bot doesn't go crazy retesting.

/hold

* Update unpack job security Signed-off-by: perdasilva <perdasilva@redhat.com> * Refactor catsrc pod creation to use security package Signed-off-by: perdasilva <perdasilva@redhat.com>

…method to create from file Signed-off-by: perdasilva <perdasilva@redhat.com>

…llection Signed-off-by: perdasilva <perdasilva@redhat.com>

Signed-off-by: perdasilva <perdasilva@redhat.com>

exdx

/lgtm

exdx · 2022-06-14T18:25:12Z

pkg/controller/security/security.go

+// ApplyPodSpecSecurity applies the standard security profile to a pod spec
+func ApplyPodSpecSecurity(spec *corev1.PodSpec) {
+	var containerSecurityContext = &corev1.SecurityContext{
+		Privileged:               pointer.Bool(privileged),


This pointer.T() pattern is nice

all @timflannagan

exdx · 2022-06-14T18:34:03Z

test/e2e/util/gomega/catalogsource_matchers.go

+		util.Logf("expecting catalog source last connection state '%s' to be '%s'", actual.Status.GRPCConnectionState.LastObservedState, s.Expected)
+		return s.EqualMatcher.Match(actual.Status.GRPCConnectionState.LastObservedState)
+	default:
+		return false, fmt.Errorf("actual %v is not a subscription", actual)


Suggested change

return false, fmt.Errorf("actual %v is not a subscription", actual)

return false, fmt.Errorf("actual %v is not a catalogsource", actual)

exdx · 2022-06-14T18:36:36Z

test/e2e/util/gomega/catalogsource_matchers.go

+	*matchers.EqualMatcher
+}
+
+func (s *CatalogSourceGrpcConnectionLastConnectionStateMatcher) FailureMessage(actual interface{}) (message string) {


Could we use client.Object instead of the interface{} type as an argument? Since we know we are dealing with a kube object.

This is the Matcher interface =S would be nice tho!

perdasilva · 2022-06-14T19:29:49Z

/hold cancel

openshift-ci bot requested review from dinhxuanvu and oceanc80 June 13, 2022 07:53

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

perdasilva force-pushed the e2e_util_update branch 2 times, most recently from f5a8d7d to de529f0 Compare June 13, 2022 11:25

perdasilva commented Jun 13, 2022

View reviewed changes

njhale reviewed Jun 13, 2022

View reviewed changes

test/e2e/magic_catalog.go Show resolved Hide resolved

njhale approved these changes Jun 13, 2022

View reviewed changes

openshift-ci bot assigned njhale Jun 13, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 13, 2022

exdx approved these changes Jun 13, 2022

View reviewed changes

perdasilva force-pushed the e2e_util_update branch from c407449 to 0760106 Compare June 13, 2022 18:35

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

perdasilva force-pushed the e2e_util_update branch from 0760106 to 57c5357 Compare June 13, 2022 18:42

openshift-ci bot assigned exdx Jun 14, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 14, 2022

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 14, 2022

perdasilva force-pushed the e2e_util_update branch from 57c5357 to 20b3c6d Compare June 14, 2022 15:54

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

perdasilva added 6 commits June 14, 2022 19:03

Update unpack job pod security (operator-framework#2793)

ee04368

* Update unpack job security Signed-off-by: perdasilva <perdasilva@redhat.com> * Refactor catsrc pod creation to use security package Signed-off-by: perdasilva <perdasilva@redhat.com>

Refactor MagicCatalog removing superfluous interface and add factory …

3bda1d7

…method to create from file Signed-off-by: perdasilva <perdasilva@redhat.com>

Switch TestContext client to be the e2e client and add crd garbage co…

fdf9787

…llection Signed-off-by: perdasilva <perdasilva@redhat.com>

Add determined e2e client that retries on failure

e588a8d

Signed-off-by: perdasilva <perdasilva@redhat.com>

Small fixes

61b498b

Signed-off-by: perdasilva <perdasilva@redhat.com>

Add olm gomega assertions and matchers

527290f

Signed-off-by: perdasilva <perdasilva@redhat.com>

perdasilva force-pushed the e2e_util_update branch from 20b3c6d to 527290f Compare June 14, 2022 17:03

exdx reviewed Jun 14, 2022

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 14, 2022

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 14, 2022

openshift-ci bot merged commit a538831 into operator-framework:master Jun 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

e2e test utilities update #2799

e2e test utilities update #2799

perdasilva commented Jun 13, 2022 •

edited

Loading

perdasilva Jun 13, 2022

exdx Jun 13, 2022

njhale left a comment

openshift-bot commented Jun 13, 2022

exdx left a comment

exdx Jun 13, 2022

exdx Jun 13, 2022

exdx Jun 13, 2022

exdx Jun 13, 2022

openshift-ci bot commented Jun 13, 2022

exdx commented Jun 14, 2022

openshift-bot commented Jun 14, 2022

exdx commented Jun 14, 2022 •

edited

Loading

openshift-bot commented Jun 14, 2022

openshift-bot commented Jun 14, 2022

openshift-bot commented Jun 14, 2022

openshift-bot commented Jun 14, 2022

openshift-bot commented Jun 14, 2022

openshift-bot commented Jun 14, 2022

timflannagan commented Jun 14, 2022

exdx left a comment

exdx Jun 14, 2022

perdasilva Jun 14, 2022

exdx Jun 14, 2022

exdx Jun 14, 2022

perdasilva Jun 14, 2022

perdasilva commented Jun 14, 2022

		if err := m.Client.Delete(context.Background(), &crd); err != nil {
		return err

	return false, fmt.Errorf("actual %v is not a subscription", actual)
	return false, fmt.Errorf("actual %v is not a catalogsource", actual)

e2e test utilities update #2799

e2e test utilities update #2799

Conversation

perdasilva commented Jun 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njhale left a comment

Choose a reason for hiding this comment

openshift-bot commented Jun 13, 2022

exdx 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

Choose a reason for hiding this comment

openshift-ci bot commented Jun 13, 2022

exdx commented Jun 14, 2022

openshift-bot commented Jun 14, 2022

exdx commented Jun 14, 2022 • edited Loading

openshift-bot commented Jun 14, 2022

openshift-bot commented Jun 14, 2022

openshift-bot commented Jun 14, 2022

openshift-bot commented Jun 14, 2022

openshift-bot commented Jun 14, 2022

openshift-bot commented Jun 14, 2022

timflannagan commented Jun 14, 2022

exdx 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perdasilva commented Jun 14, 2022

perdasilva commented Jun 13, 2022 •

edited

Loading

exdx commented Jun 14, 2022 •

edited

Loading