🌱 Catalog unit test #251

ankitathomas · 2023-06-02T18:25:39Z

This PR refactors the e2e catalog reconcile test to a unit test checking whether reconcile is triggered on catalog events (create, update, delete events). Reconcile is detected by scanning controller logs for logs from the reconcile loop for all operators.

See: #247

internal/controllers/operator_controller_test.go

joelanford · 2023-06-02T20:11:17Z

internal/controllers/operator_controller_test.go

+			l := funcr.New(func(prefix, args string) {
+				if prefix == "operator-controller" &&
+					strings.Contains(args, `"controller"="operator"`) &&
+					strings.Contains(args, `"msg"="ending"`) {
+					// filter for only relevant logs
+					testLogs = append(testLogs, fmt.Sprintf("%s", args))
+				}
+			}, funcr.Options{Verbosity: 1})


TIL, this funcr is super cool.

I'm torn though. On one hand, doing things this way means we can pretty much test our controller setup logic as-is. However it seems very brittle because changing our logs makes our tests for a completely unrelated thing break.

Another option is to slightly restructure the SetupWithManager like this:

// SetupWithManager sets up the controller with the Manager. func (r *OperatorReconciler) SetupWithManager(mgr ctrl.Manager) error { _, err := NewOperatorController(mgr, r) return err } // NewOperatorController sets up a controller for Operator objects using the provided reconciler. func NewOperatorController(mgr ctrl.Manager, r reconcile.Reconciler) (controller.Controller, error) { return ctrl.NewControllerManagedBy(mgr). For(&operatorsv1alpha1.Operator{}). Watches(source.NewKindWithCache(&catalogd.Catalog{}, mgr.GetCache()), handler.EnqueueRequestsFromMapFunc(operatorRequestsForCatalog(context.TODO(), mgr.GetClient(), mgr.GetLogger()))). Owns(&rukpakv1alpha1.BundleDeployment{}). Build(r) }

And then our tests could use NewOperatorController with a custom reconciler that collects the reconcile.Requests and makes assertions based on that. WDYT?

Switching to a fake reconciler does make the tests considerably faster, but this makes me wonder if we can just test the handler function on its own, just testing if it generates the right set of requests when a catalog changes.

That way, we'd be able to adapt the tests easier to a potential future where we only reconcile affected operators on a catalog update.

We could test the handler function on its own as well. In my mind, this test is about making sure that the entire controller setup works like we expect.

internal/controllers/operator_controller_test.go

internal/controllers/catalog_test.go

joelanford · 2023-06-06T02:47:58Z

internal/controllers/catalog_test.go

+)
+
+var _ = Describe("Operator Controller Catalog Reconciler", func() {
+	When("a catalog changes on cluster", func() {


Based on the previous suggestion, WDYT about adding an extra layer of hierarchy under the Describe("SetupWithManager") block. This way it'll be setup a bit easer to handle more watches or other configurations we add in the future.

BeforeEach() // setup and start the controller+manager When("Operator events occur") It("reconciles self") // do Operator create, update, delete When("Catalog events occur") BeforeEach() // create a few operators It("reconciles all Operators") // do Catalog create, update, delete AfterEach() // delete the operators AfterEach() // cancel the context to stop the manager

I've been thinking about this, but do we want to test controller-runtime objects? Testing for reconcile on catalog events makes sense with catalogs since we're using a custom handler, but with operators, it feels like we're testing whether the manager works, that should be on controller-runtime.

If we are however adding tests for operator events, then we'd also need to add tests for bundledeployment events to see if reconciles happen for the operators in the owner references, which also seems unnecessary to test.

It isn't really testing controller-runtime. It's testing our specific watch setup. I agree that almost all of the code that actually runs is controller-runtime code, but it's all triggered based on our setup of the manager.

I think there's pretty good long term value here from a regression standpoint, especially if we can get the framework setup while things are simple. Anecdotally, the rukpak watches are actually fairly complex due to the way its APIs reference provisioner classes. There has been discussion in the context of oeprator-controller to implement things like upgrade policies via referenced objects, so I think it is inevitable that our watches become more complex over time.

I think @joelanford's suggestion here makes a lot of sense for testing that the watches we are setting up are getting triggered as expected. That being said, I don't think it should block this PR specifically unless @ankitathomas really felt like including it. At least the foundation is laid by this PR and we can always increase the robustness of our tests in follow ups

internal/controllers/catalog_test.go

testdata/crds/catalogd.operatorframework.io_catalogs.yaml

joelanford

Looks really good. Just a few (hopefully easy to implement) suggestions that I think will make things more clear and result in fewer lines of code.

everettraven

Overall looks great! I just have one nit but IMO shouldn't block this PR. Awesome job @ankitathomas - I definitely learned some things we should add in catalogd :)

internal/controllers/catalog_test.go

everettraven · 2023-06-08T14:06:34Z

internal/controllers/catalog_test.go

+)
+
+var _ = Describe("Operator Controller Catalog Reconciler", func() {
+	When("a catalog changes on cluster", func() {


I think @joelanford's suggestion here makes a lot of sense for testing that the watches we are setting up are getting triggered as expected. That being said, I don't think it should block this PR specifically unless @ankitathomas really felt like including it. At least the foundation is laid by this PR and we can always increase the robustness of our tests in follow ups

internal/controllers/catalog_test.go

testdata/crds/catalogd.operatorframework.io_catalogs.yaml

Signed-off-by: Ankita Thomas <ankithom@redhat.com>

everettraven

Added another nit and agree with @joelanford's suggestions

everettraven · 2023-06-09T13:23:27Z

cmd/manager/main.go

-			olm.NewOLMVariableSource(mgr.GetClient()),
-		),
-	}).SetupWithManager(mgr); err != nil {
+			olm.NewOLMVariableSource(mgr.GetClient())),


Nit: Keep the solver.NewDeppySolver closing parenthesis on it's own line. IMO it makes it easier to see where the call stops at a glance

everettraven · 2023-06-09T19:23:19Z

internal/controllers/catalog_test.go

+			Expect(err).To(BeNil())
+
+			opNames = []string{"prometheus", "project-quay"}
+			reqChan = make(chan string, 4*len(opNames))


Do we still need the buffer to be 4*len(opNames)? I don't think its a huge deal, but IIUC we could probably reduce this to just len(opNames) since a receiver will be using the channel after each "reconcile" that occurs.

everettraven · 2023-06-09T19:26:13Z

internal/controllers/catalog_test.go

+			err = controllers.SetupWithManager(fakeReconciler, mgr)
+			Expect(err).To(BeNil())


Nit (stolen from a nit @joelanford left on one of my PRs but I really liked it so 🤷 ): For functions that just return an error, I think something like:

Suggested change

err = controllers.SetupWithManager(fakeReconciler, mgr)

Expect(err).To(BeNil())

Expect(controllers.SetupWithManager(fakeReconciler, mgr)).To(Succeed())

looks a bit nicer

everettraven · 2023-06-09T19:33:38Z

internal/controllers/catalog_test.go

+		})
+		It("reconciles all affected operators on cluster", func() {
+			By("creating a new catalog")
+			catalog := &catalogd.Catalog{ObjectMeta: metav1.ObjectMeta{Name: "t"}, Spec: catalogd.CatalogSpec{Source: catalogd.CatalogSource{Type: catalogd.SourceTypeImage, Image: &catalogd.ImageSource{}}}}


Nit: Make this take up multiple lines - this is a really long line and is more difficult to follow than if it was on multiple lines

internal/controllers/catalog_test.go

Signed-off-by: Ankita Thomas <ankithom@redhat.com>

openshift-merge-robot · 2023-06-24T20:31:59Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

perdasilva · 2023-07-18T12:52:26Z

@ankitathomas can you please rebase?

ankitathomas requested review from joelanford, tmshort and everettraven June 2, 2023 18:25

joelanford reviewed Jun 2, 2023

View reviewed changes

internal/controllers/operator_controller_test.go Outdated Show resolved Hide resolved

joelanford reviewed Jun 2, 2023

View reviewed changes

internal/controllers/operator_controller_test.go Outdated Show resolved Hide resolved

ankitathomas force-pushed the catalog-unit-test branch from 75b496c to 54b6ab2 Compare June 5, 2023 17:51

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 5, 2023

ankitathomas force-pushed the catalog-unit-test branch from 232308d to 88e2d32 Compare June 5, 2023 18:22

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 5, 2023

ankitathomas force-pushed the catalog-unit-test branch 4 times, most recently from ebddb6d to 46a214a Compare June 5, 2023 22:16

ankitathomas requested a review from joelanford June 5, 2023 22:16

ankitathomas force-pushed the catalog-unit-test branch 2 times, most recently from 5765192 to 38247ae Compare June 5, 2023 22:24

joelanford reviewed Jun 6, 2023

View reviewed changes

internal/controllers/catalog_test.go Outdated Show resolved Hide resolved

joelanford reviewed Jun 6, 2023

View reviewed changes

internal/controllers/catalog_test.go Outdated Show resolved Hide resolved

joelanford reviewed Jun 6, 2023

View reviewed changes

internal/controllers/catalog_test.go Outdated Show resolved Hide resolved

joelanford reviewed Jun 6, 2023

View reviewed changes

testdata/crds/catalogd.operatorframework.io_catalogs.yaml Show resolved Hide resolved

joelanford reviewed Jun 6, 2023

View reviewed changes

ankitathomas force-pushed the catalog-unit-test branch from 38247ae to 263c046 Compare June 6, 2023 21:37

ankitathomas requested a review from joelanford June 7, 2023 16:16

ankitathomas force-pushed the catalog-unit-test branch from 263c046 to f93ddb6 Compare June 8, 2023 07:33

joelanford mentioned this pull request Jun 8, 2023

operator_controller_test: ensure we cleanup after each spec #257

Merged

4 tasks

everettraven previously approved these changes Jun 8, 2023

View reviewed changes

joelanford reviewed Jun 8, 2023

View reviewed changes

internal/controllers/catalog_test.go Outdated Show resolved Hide resolved

joelanford reviewed Jun 8, 2023

View reviewed changes

internal/controllers/catalog_test.go Outdated Show resolved Hide resolved

joelanford reviewed Jun 8, 2023

View reviewed changes

internal/controllers/catalog_test.go Outdated Show resolved Hide resolved

joelanford reviewed Jun 8, 2023

View reviewed changes

testdata/crds/catalogd.operatorframework.io_catalogs.yaml Outdated Show resolved Hide resolved

ankitathomas added 2 commits June 8, 2023 14:06

refactor catalogsource e2e test to unit test

0ed3ede

Signed-off-by: Ankita Thomas <ankithom@redhat.com>

remove e2e catalog reconcile test

48b3c3f

Signed-off-by: Ankita Thomas <ankithom@redhat.com>

ankitathomas force-pushed the catalog-unit-test branch 3 times, most recently from 13928f3 to 7a0c2f8 Compare June 8, 2023 21:53

everettraven reviewed Jun 9, 2023

View reviewed changes

ankitathomas force-pushed the catalog-unit-test branch 2 times, most recently from 79f38db to d695d11 Compare June 9, 2023 18:26

everettraven reviewed Jun 9, 2023

View reviewed changes

fake reconciler for catalog sync tests

c2819f4

Signed-off-by: Ankita Thomas <ankithom@redhat.com>

ankitathomas force-pushed the catalog-unit-test branch from d695d11 to c2819f4 Compare June 9, 2023 22:14

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2023

ankitathomas dismissed everettraven’s stale review via c2819f4 November 2, 2023 15:03

ncdc changed the title ~~Catalog unit test~~ 🌱 Catalog unit test Nov 6, 2023

ankitathomas closed this Dec 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Catalog unit test #251

🌱 Catalog unit test #251

ankitathomas commented Jun 2, 2023

joelanford Jun 2, 2023

ankitathomas Jun 5, 2023

joelanford Jun 8, 2023

joelanford Jun 6, 2023

ankitathomas Jun 6, 2023 •

edited

Loading

joelanford Jun 7, 2023

everettraven Jun 8, 2023

joelanford left a comment

everettraven left a comment

everettraven Jun 8, 2023

everettraven left a comment

everettraven Jun 9, 2023

everettraven Jun 9, 2023

everettraven Jun 9, 2023

everettraven Jun 9, 2023

openshift-merge-robot commented Jun 24, 2023

perdasilva commented Jul 18, 2023

		err = controllers.SetupWithManager(fakeReconciler, mgr)
		Expect(err).To(BeNil())

	err = controllers.SetupWithManager(fakeReconciler, mgr)
	Expect(err).To(BeNil())
	Expect(controllers.SetupWithManager(fakeReconciler, mgr)).To(Succeed())

🌱 Catalog unit test #251

🌱 Catalog unit test #251

Conversation

ankitathomas commented Jun 2, 2023

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

ankitathomas Jun 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelanford left a comment

Choose a reason for hiding this comment

everettraven left a comment

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

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-merge-robot commented Jun 24, 2023

perdasilva commented Jul 18, 2023

ankitathomas Jun 6, 2023 •

edited

Loading