refactor and add unit tests for operator reconciler #115

joelanford · 2023-02-02T17:38:22Z

Another facet of #107, covering the following:

move reconciler unit tests to a separate file from the suite. if we add other controllers in the future, we'll likely want separate test files for each controller.
export the reconciler's resolver field to make it easy to swap out for tests (also add a test entity source)
remove the manager setup in the unit tests. this ensures we can test individual runs of the Reconcile function, which is good for two reasons:
- no need to use Eventually, no polling, etc. so it speeds up the tests
- it ensures that we fully understand the individual transitions that occur from a beginning state to an end state when multiple Reconcile calls are necessary. (e.g. r1: create BD, r2: update Op based on BD status change to unpacking, r3: update Op again based on BD status change to installed successfully)
  added more unit tests to increase code coverage.
moved conditions and reasons checks to invariants that run after every test of reconciling an existing Operator object.

Closes #86

tmshort

make test-unit locally succeeds
make e2e locally succeeds
But make test locally fails (even after a rebase to main). It passes on main, however.
(make test should apparently be part of the CI)
It's actually failing on make test-e2e...

tmshort · 2023-02-02T19:31:51Z

controllers/operator_controller.go

 	if err != nil {
-		status = metav1.ConditionTrue
+		status = metav1.ConditionFalse


Shouldn't this remain ConditionTrue as resolution did fail?

ConditionTrue is saying Ready: True. Since resolution failed, shouldn't we be saying Ready: false?

As an aside, I think the structure of how these condition values gets set makes it difficult to reason about. I've fixed this in #107, so as soon as this change lands, I can layer in another PR that simplifies the Reconcile function and makes it more obvious how/when the ready condition is set.

You're right. I was conflating "Resolution failed" with the type.

OK, I think the confusion is that it's not obvious this is for the "Ready" type, since TypeReady is not very local, it's down 30+ lines.

^ Exactly - That problem will be resolved in #118.

tmshort · 2023-02-02T19:39:07Z

main.go

-	// to ensure that exec-entrypoint and run can make use of them.
-	_ "k8s.io/client-go/plugin/pkg/client/auth"
-
+	rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1"


Wondering why this moved up here (i.e. away from the other github.com imports). I have seen go fmt move things around, but it seems fine with this.

Oh yeah, that my IDE running gosimports (https://github.com/rinchsan/gosimports) to automatically organize imports into groups and sort them.

joelanford · 2023-02-02T22:35:57Z

Re: make test, I think that's broken? If I clear out my kind clusters and run make test, it fails on main.

make test only include test-e2e, but not e2e, so the kind cluster setup doesn't happen.

varshaprasad96

/lgtm

varshaprasad96 · 2023-02-03T05:51:10Z

controllers/operator_controller_test.go

+				By("running reconcile")
+				res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: opKey})
+				Expect(res).To(Equal(ctrl.Result{}))
+				// TODO: should resolution failure return an error?


we should probably trigger reconcile again with the error that we are setting in the condition.

+1, I've got that fixed up in #118. When this merges, I'll rebase that one.

varshaprasad96 · 2023-02-03T05:53:19Z

controllers/operator_controller_test.go

+					Expect(cond).NotTo(BeNil())
+					Expect(cond.Status).To(Equal(metav1.ConditionTrue))
+					Expect(cond.Reason).To(Equal(operatorsv1alpha1.ReasonResolutionSucceeded))
+					Expect(cond.Message).To(Equal("resolution was successful"))


(Not related to the PR, but just noticed) We should probably define separate error types and constants for the messages to make it easier for users to check the status message.

I'm not opposed to constants for messages we reuse, but I'm pretty sure condition messages are not considered stable or part of the API a consumer is supposed to depend on.

I agree with Joe. Reason is the programatic interface.
But using constants for messages is not a bad idea, either.

tmshort · 2023-02-03T14:49:05Z

Re: make test, I think that's broken? If I clear out my kind clusters and run make test, it fails on main.

make test only include test-e2e, but not e2e, so the kind cluster setup doesn't happen.

See #119

joelanford · 2023-02-06T18:42:10Z

@tmshort anything else you want to review/discuss on this one?

tmshort · 2023-02-06T20:03:39Z

@tmshort anything else you want to review/discuss on this one?

Nope, I fixed the tests in #119, and got my head around the conditions, I think you're good.

Are you concerned about the go-apidiff failure (assuming not, since this is still in development)?

/lgtm

joelanford · 2023-02-07T04:28:27Z

Not concerned about the apidiff failure. While we're in development, the purpose of that is to inform us of the changes we're making to the public API, and potentially have discussion about it. Honestly, the main thing I'm watching for at this stage is unnecessarily adding new surface area to the public API. Not really at all concerns about removals or changes at this point.

joelanford requested review from dtfranz, varshaprasad96 and tmshort February 2, 2023 19:25

tmshort reviewed Feb 2, 2023

View reviewed changes

joelanford mentioned this pull request Feb 3, 2023

controller code/test refactoring/additions #107

Closed

5 tasks

refactor and add unit tests for operator reconciler

52bbccb

joelanford force-pushed the reconciler-unit-tests branch from b91ac8c to 52bbccb Compare February 3, 2023 02:17

joelanford mentioned this pull request Feb 3, 2023

Reduce reconcile complexity #118

Merged

varshaprasad96 approved these changes Feb 3, 2023

View reviewed changes

openshift-ci bot assigned varshaprasad96 Feb 3, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 3, 2023

openshift-ci bot assigned tmshort Feb 6, 2023

joelanford merged commit e596002 into operator-framework:main Feb 7, 2023

joelanford deleted the reconciler-unit-tests branch February 7, 2023 04:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor and add unit tests for operator reconciler #115

refactor and add unit tests for operator reconciler #115

joelanford commented Feb 2, 2023 •

edited

Loading

tmshort left a comment

tmshort Feb 2, 2023

joelanford Feb 2, 2023

joelanford Feb 2, 2023

tmshort Feb 3, 2023

joelanford Feb 3, 2023

tmshort Feb 2, 2023

joelanford Feb 2, 2023

joelanford commented Feb 2, 2023

varshaprasad96 left a comment

varshaprasad96 Feb 3, 2023

joelanford Feb 3, 2023

varshaprasad96 Feb 3, 2023

joelanford Feb 3, 2023

tmshort Feb 3, 2023 •

edited

Loading

tmshort commented Feb 3, 2023

joelanford commented Feb 6, 2023

tmshort commented Feb 6, 2023

joelanford commented Feb 7, 2023

refactor and add unit tests for operator reconciler #115

refactor and add unit tests for operator reconciler #115

Conversation

joelanford commented Feb 2, 2023 • edited Loading

tmshort 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelanford commented Feb 2, 2023

varshaprasad96 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

tmshort Feb 3, 2023 • edited Loading

Choose a reason for hiding this comment

tmshort commented Feb 3, 2023

joelanford commented Feb 6, 2023

tmshort commented Feb 6, 2023

joelanford commented Feb 7, 2023

joelanford commented Feb 2, 2023 •

edited

Loading

tmshort Feb 3, 2023 •

edited

Loading