🌱 Remove ginkgo from e2e tests #545

tmshort · 2023-11-16T20:24:27Z

Fixes #196

Description

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

codecov · 2023-11-16T20:29:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d02bca5) 83.72% compared to head (d84e6de) 83.72%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #545   +/-   ##
=======================================
  Coverage   83.72%   83.72%           
=======================================
  Files          20       20           
  Lines         811      811           
=======================================
  Hits          679      679           
  Misses         91       91           
  Partials       41       41

Flag	Coverage Δ
e2e	`63.62% <ø> (ø)`
unit	`78.54% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

stevekuznetsov

Looks good! Just a couple questions

stevekuznetsov · 2023-11-19T15:28:46Z

Makefile

@@ -105,16 +105,16 @@ test: manifests generate fmt vet test-unit test-e2e #HELP Run all tests.
 .PHONY: e2e
 FOCUS := $(if $(TEST),-v -focus "$(TEST)")
 E2E_FLAGS ?= ""
-e2e: $(GINKGO) #EXHELP Run the e2e tests.
-	$(GINKGO) --tags $(GO_BUILD_TAGS) $(E2E_FLAGS) -trace -progress $(FOCUS) test/e2e
+e2e: $(SETUP_ENVTEST) #EXHELP Run the e2e tests.


Thank you for making this self-contained, nobody has to remember how to use setup-envtest anymore!

stevekuznetsov · 2023-11-19T15:29:07Z

Makefile

-e2e: $(GINKGO) #EXHELP Run the e2e tests.
-	$(GINKGO) --tags $(GO_BUILD_TAGS) $(E2E_FLAGS) -trace -progress $(FOCUS) test/e2e
+e2e: $(SETUP_ENVTEST) #EXHELP Run the e2e tests.
+	eval $$($(SETUP_ENVTEST) use -p env $(ENVTEST_VERSION)) && go test -tags $(GO_BUILD_TAGS) -v $(shell go list ./test/e2e)


Any specific reason to use $(shell go list ./test/e2e) instead of ./test/e2e/...?

I think I tried ./test/e2e/... (or maybe it was without the ...?) and it didn't like it, possibly because go test didn't like the syntax (it's been over a week). But there's no reason not to use that vs calling out to the $(shell)

stevekuznetsov · 2023-11-19T15:31:11Z

test/operator-framework-e2e/operator_framework_test.go

-	RunSpecs(t, "Operator Framework E2E Suite")
-}
+	t.Parallel()
+	cfg := ctrl.GetConfigOrDie()


Isn't the ctrl.GetConfigOrDie() helper for in-cluster config when running in a Pod? Why is it in use here?

I believe it will also build the configuration from the KUBECONFIG when run outside of a cluster

And, I believe this E2E is run in a pod, even so, it's working as desired.

As long as you can run it locally that's fine

test/operator-framework-e2e/operator_framework_test.go

stevekuznetsov · 2023-11-19T15:32:31Z

test/operator-framework-e2e/operator_framework_test.go

 			ObjectMeta: metav1.ObjectMeta{
-				Name: "catalog" + rand.String(10),
+				Name: "plainv0",


Why not prefer to use GenerateName or random names for parallel runs?

Looks like these names are used in a testing log message to signal we are testing for specific bundle types

Although, I do wonder if it would be more appropriate to create a different testing struct (similar to what we do for table tests) to have the test name that is running be more descriptive. Maybe something along the lines of:

{ name: "Operator references a plain+v0 bundle" operator: &operatorv1alpha1.Operator{...} }

These tests are already run in parallel.
Really no need to use random names when the names are already different, because as @everettraven points out, the name is used as part of the log output.

IF this test were more complicated, I'd go with what @everettraven is suggesting, but there's only two test cases. If this were to increase to 5 or more, then, yes, a more comprehensive table test structure would be appropriate.

test/e2e/install_test.go

Signed-off-by: Todd Short <tshort@redhat.com>

tmshort · 2023-11-27T19:05:13Z

@stevekuznetsov @everettraven updated with all checks passing.

everettraven · 2023-11-27T19:10:48Z

test/e2e/install_test.go


 	// Get all namespaces
 	namespaces := corev1.NamespaceList{}
-	if err := c.List(ctx, &namespaces); err != nil {
-		GinkgoWriter.Printf("Failed to list namespaces %w", err)
+	if err := c.List(context.Background(), &namespaces); err != nil {


Would it be worth creating a single context to reuse in all these calls instead of using context.Background() in every call?

Not sure it matters much since there will never be a context in test code

everettraven

These changes look reasonable to me.

tmshort requested a review from a team as a code owner November 16, 2023 20:24

tmshort mentioned this pull request Nov 17, 2023

🌱 Remove ginkgo from internal/controller unit tests #541

Merged

4 tasks

tmshort force-pushed the test-e2e branch 2 times, most recently from ea859a2 to d84e6de Compare November 17, 2023 18:19

stevekuznetsov reviewed Nov 19, 2023

View reviewed changes

everettraven reviewed Nov 22, 2023

View reviewed changes

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

🌱 Remove ginkgo from e2e tests

a6f27ac

Signed-off-by: Todd Short <tshort@redhat.com>

tmshort force-pushed the test-e2e branch from d84e6de to a6f27ac Compare November 27, 2023 15:49

everettraven reviewed Nov 27, 2023

View reviewed changes

everettraven approved these changes Nov 27, 2023

View reviewed changes

stevekuznetsov approved these changes Nov 27, 2023

View reviewed changes

stevekuznetsov added this pull request to the merge queue Nov 27, 2023

Merged via the queue into operator-framework:main with commit 9e5adb0 Nov 27, 2023
12 checks passed

tmshort deleted the test-e2e branch November 29, 2023 19:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Remove ginkgo from e2e tests #545

🌱 Remove ginkgo from e2e tests #545

tmshort commented Nov 16, 2023

codecov bot commented Nov 16, 2023 •

edited

Loading

stevekuznetsov left a comment

stevekuznetsov Nov 19, 2023

stevekuznetsov Nov 19, 2023

tmshort Nov 27, 2023 •

edited

Loading

stevekuznetsov Nov 19, 2023

everettraven Nov 22, 2023

tmshort Nov 27, 2023

stevekuznetsov Nov 27, 2023

stevekuznetsov Nov 19, 2023

everettraven Nov 22, 2023

everettraven Nov 22, 2023

tmshort Nov 27, 2023

tmshort commented Nov 27, 2023

everettraven Nov 27, 2023

stevekuznetsov Nov 27, 2023

everettraven left a comment

🌱 Remove ginkgo from e2e tests #545

🌱 Remove ginkgo from e2e tests #545

Conversation

tmshort commented Nov 16, 2023

Description

Reviewer Checklist

codecov bot commented Nov 16, 2023 • edited Loading

Codecov Report

stevekuznetsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmshort Nov 27, 2023 • 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

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

tmshort commented Nov 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

everettraven left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 16, 2023 •

edited

Loading

tmshort Nov 27, 2023 •

edited

Loading