ROX-30138: Add tests for WithSelector by kurlov · Pull Request #496 · operator-framework/helm-operator-plugins

kurlov · 2025-10-20T11:55:42Z

Follow up for #459

Add test for WithSelector using controller manager to verify the reconciler only processes CRs that match the configured selector.

codecov-commenter · 2025-10-23T08:18:22Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.71%. Comparing base (08ab7fb) to head (c471e97).
⚠️ Report is 128 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (08ab7fb) and HEAD (c471e97). Click for more details.
HEAD has 1 upload less than BASE
Flag BASE (08ab7fb) HEAD (c471e97)
2 1

Additional details and impacted files

@@ Coverage Diff @@## main #496 +/- ##
===========================================
- Coverage 85.06% 74.71% -10.35% 
===========================================
Files 19 31 +12 Lines 1346 2037 +691 ===========================================
+ Hits 1145 1522 +377 - Misses 125 433 +308 - Partials 76 82 +6

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

porridge

This is much better than the previous one, and it will actually fail when we accidentally stop passing the predicate, so it's useful.

However, I'm afraid that this test passing does not guarantee much. AFAICT there is no way to be sure whether the controller will never schedule the unwanted CR for reconciliation, or if it just did not get round to doing that yet. 🤔

Perhaps we can improve our confidence a bit by creating the unlabeled CR before the labeled one?

porridge · 2025-11-04T09:31:26Z

+ controllerName := fmt.Sprintf("%v-controller", strings.ToLower(gvk.Kind))
+ Expect(r.addDefaults(mgr, controllerName)).To(Succeed())
+ r.setupScheme(mgr)
+
+ c, err := controller.New(controllerName, mgr, controller.Options{
+ Reconciler: reconciler,
+ MaxConcurrentReconciles: 1,
+ })
+ Expect(err).ToNot(HaveOccurred())
+ Expect(r.setupWatches(mgr, c)).To(Succeed())


This duplicates a bunch of code from SetupWithManager. I don't think we should be doing this, it makes the test brittle. I guess the reason is that you want to append to reconciledCRs? Perhaps consider using a hook for this instead?

it's a great idea. I changed the test to use hooks

porridge · 2025-11-04T09:33:07Z

+ Eventually(func() []string {
+ reconciledCRsMutex.Lock()
+ defer reconciledCRsMutex.Unlock()
+ return reconciledCRs


I think that since this returns a slice pointing at the same backing array, the caller will race with other accesses 🤔
Perhaps instead check the existence of desired element in the Eventually, in the scope of the lock, and then make Should just check for true return value?

I might not understand the comment correctly but there is mutex in write/read to prevent data race. And the Eventually() matches synchronously after the function returns but before the mutex is unlocked. Also tests run with -race flag and during the development I had a few data race issues but all of them resolved. So I believe it should be fine.
In addition the using of slice/map is more accurate (not perfect though) than bool var because bool in Eventually() will match even when reconciler reconciled two different label selectors

the Eventually() matches synchronously after the function returns but before the mutex is unlocked.
I don't think this is true. Mutex is released when the func completes, and then the Eventually works on a copy of the returned slice with the same backing array as the mutex-protected slice. This program shows the data corruption that can happen. After running it a few times I got it to produce:
I don't think I get the point behind slice vs bool.. you can have the bool mean anything you need inside the func...

I think I got your point and moved assertions inside Eventually() and just check the predicate with .Should(BeTrue())

Co-authored-by: Marcin Owsiany <marcin@owsiany.pl>

kurlov · 2025-11-27T07:45:08Z

 	var preds []predicate.Predicate

-	if r.labelSelector.Size() > 0 {
+	if len(r.labelSelector.MatchLabels) > 0 || len(r.labelSelector.MatchExpressions) > 0 {


Minor thing, I guess it's a bit better instead of pb generated method

Better in what sense? I don't think computational performance is important in this (initialization) phase. The goal of using Size() was to make it robust against addition of new fields to the metav1.LabelSelector size.

I thought that Size() will be > 0 for empty LabelSelector{} struct but it's not the case. Changed back to Size()

kurlov · 2025-11-27T07:46:03Z

+ })
+ })
+
+ It("should reconcile CRs independently when using two managers with different label selectors", func() {


Also added a test with two managers to simulate the original bug when two managers with different label selectors reconciles each other CRs

porridge · 2025-11-27T10:09:17Z

 	var preds []predicate.Predicate

-	if r.labelSelector.Size() > 0 {
+	if len(r.labelSelector.MatchLabels) > 0 || len(r.labelSelector.MatchExpressions) > 0 {


Better in what sense? I don't think computational performance is important in this (initialization) phase. The goal of using Size() was to make it robust against addition of new fields to the metav1.LabelSelector size.

porridge · 2025-11-27T10:44:35Z

+ Eventually(func() []string {
+ reconciledCRsMutex.Lock()
+ defer reconciledCRsMutex.Unlock()
+ return reconciledCRs


the Eventually() matches synchronously after the function returns but before the mutex is unlocked.
I don't think this is true. Mutex is released when the func completes, and then the Eventually works on a copy of the returned slice with the same backing array as the mutex-protected slice. This program shows the data corruption that can happen. After running it a few times I got it to produce:
I don't think I get the point behind slice vs bool.. you can have the bool mean anything you need inside the func...

porridge · 2025-11-27T10:56:18Z

+ }
+ return nil
+ })
+ _ = setupManagerWithSelectorAndPostHook(ctx, postHook, anotherMatchingLabels)


I'm wondering... now that we have all these managers and reconcilers, how do we make sure they do not affect other Describes? Or that we don't get affected by other ones? I cannot see them being stopped directly anywhere. Do they run indefinitely? 🤔

I added managers shut down. Also after experiencing a few issues with cache/leftover CRs I decided to not share CRs between tests and even name them differently (there is CR cleanup but still it's really easy to get not obvious issues). It is a few repeated lines in the tests but I believe it is worth it. It will make life a bit easier for person who will touch these tests in the future.

porridge

👍🏻

perdasilva · 2025-12-04T09:25:10Z

+ })
+
+ By("verifying only the labeled CR was reconciled", func() {
+ Eventually(func() bool {


is it possible that the non-labeled object gets reconciled straight after and we don't catch it?

Yes, unfortunately there does not seem to be a way to know whether controller-runtime will never do something, or just didn't get round to doing it yet :-(

It might be worth calling this out in a comment somewhere. In case we hit a flake, we can get the context.

I don't think that non-labeled object gets reconciled. I saw the thread Marcin shared but in this test there is a labeled manager so even if non-labeled object gets a reconciliation attempt it will not be in the reconciledCRs slice because of the filtering by watchers.
@porridge would you agree with that or I missing something?

@kurlov I think @perdasilva 's question was about the fact that if there is a bug which will cause the reconcile for the non-labeled object to run after all, but with a delay of, say, 3 seconds, then this test will not be able to catch this.

I think it would be fine to just add a comment to that effect and we move forward with the PR. I don't think it would be be highly likely to happen, and I'm struggling to think of another test strategy to avoid this.

Added a comment

thank you! merging this in. Thanks for both the fix and the patience!

Add tests for WithSelector
24bd217

openshift-ciBot requested review from SimonBaeumer and everettraven October 20, 2025 11:55

github-actionsBot added area/sdk area/testing labels Oct 20, 2025

Fix lint
25d253a

Small fix
d486710

porridge suggested changes Nov 4, 2025
View reviewed changes

kurlovand others added 5 commits November 4, 2025 15:19

Apply suggestions from code review
23cd0b6
Co-authored-by: Marcin Owsiany <marcin@owsiany.pl>

Fix indentation
0a43969

Use hooks and add another test with two managers
072866a

Fix linting
25d4810

Improve assertion
ae66a85

kurlov commented Nov 27, 2025
View reviewed changes

kurlov requested a review from porridge November 27, 2025 07:49

porridge reviewed Nov 27, 2025
View reviewed changes

kurlov added 2 commits December 1, 2025 10:34

Add managers cleanup and change Eventually asserting for mutex objects
bd631b6

return protobuf Size() method for label setup check
9149213

kurlov requested a review from porridge December 1, 2025 09:44

porridge approved these changes Dec 1, 2025
View reviewed changes

perdasilva reviewed Dec 4, 2025
View reviewed changes

grokspawn mentioned this pull request Dec 4, 2025
📖 Clarify single controller assumption in OLM v1 design operator-framework/operator-controller#2372
Merged
4 tasks

perdasilva reviewed Dec 5, 2025
View reviewed changes

Comment threadpkg/reconciler/reconciler_test.go

Add a comment that unlabeled CR might be reonciled
c471e97

kurlov requested a review from perdasilva December 12, 2025 05:03

perdasilva approved these changes Dec 16, 2025
View reviewed changes

perdasilva added this pull request to the merge queue Dec 16, 2025

Merged via the queue into operator-framework:main with commit d5d6714Dec 16, 2025
6 checks passed

Uh oh!

Conversation

kurlov commented Oct 20, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Oct 23, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

porridge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kurlovDec 1, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kurlovDec 1, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

porridge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kurlovDec 5, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kurlov commented Oct 20, 2025•
edited
Loading

codecov-commenter commented Oct 23, 2025•
edited
Loading

kurlovDec 1, 2025•
edited
Loading

kurlovDec 1, 2025•
edited
Loading

kurlovDec 5, 2025•
edited
Loading