test: add scheduled-to-bound coverage for TestPickBindingsToRoll

[Akshita kumari (akshita317)]

Description

TestPickBindingsToRoll carries a // TODO: add more tests. This adds five table-driven cases for the scheduled-to-bound transition, which the existing cases cover only thinly, and which left one branch of calculateRealTarget untested entirely.

Cases added:

Case	What it pins
scheduled binding already at the latest snapshot	every existing case starts from an outdated snapshot, so this path was untested
PickFixed placement type	PickFixedPlacementType appeared zero times in this package's tests, so the ClusterNames branch of calculateRealTarget was never taken
multiple scheduled bindings, maxSurge: 0, nothing ready	rollout is allowed
scheduled binding, maxSurge: 0, target already met by ready bound bindings	rollout is held back
multiple scheduled bindings, percentage maxSurge	the surge budget rounds up (50% of 3 → 2, not 1)

The maxSurge: 0 pair is deliberately split into two cases. Scheduled bindings are not added to canBeReadyBindings (controller.go:421-431), so calculateMaxToAdd returns the full target number when nothing is ready yet — maxSurge: 0 only holds a rollout back once ready bindings already meet the target. The two cases document that distinction, which I found surprising when reading the code.

Coverage

                        before    after
calculateRealTarget      70.0%    80.0%

The PickFixed case is the one that moves the needle. pickBindingsToRoll (98.7%), determineBindingsToUpdate (100%) and calculateMaxToAdd (100%) were already covered, so the remaining four cases add behavioural assertions — particularly the percentage rounding — rather than new lines. I would rather state that plainly than imply more than the numbers support.

Expected values were derived from reading pickBindingsToRoll, determineBindingsToUpdate, calculateMaxToAdd and calculateRealTarget before running anything, not adjusted afterwards to match observed output.

Testing

go test ./pkg/controllers/rollout/ -run TestPickBindingsToRoll -count=1 -v

All five new cases pass. All other unit tests in the package pass.

One pre-existing failure, unrelated to this change:

--- FAIL: TestPickBindingsToRoll/test_unscheduled_bindings_with_different_waitTimes_and_check_the_wait_time_is_correct
    pickBindingsToRoll() = waitTime 0s, want 25s

This reproduces identically on unmodified main (I stashed this change and re-ran to confirm), and the CI run for e160c1cd, the commit this branch is based on, is also red. The case builds bindings with time.Now()-relative transition times, so it looks timing-sensitive rather than caused by any recent change. Flagging it here so it is not misattributed to this PR — happy to open a separate issue if that would be useful.

TestAPIs was not run locally as it needs an envtest control plane.

Related

Part of #676. That issue also lists a set of untested paths in pickBindingsToRoll itself — IsBindingDiffReported, deletion timestamps on scheduled bindings, negative maxNumberToRemove, and others. Those are a separate concern from the scheduled-to-bound group and are left for follow-up rather than bundled here, so the // TODO comment stays in place.

[Akshita kumari (akshita317)]

CI note for reviewers: unit-and-integration-tests is red, but not from this change.

ok    github.com/kubefleet-dev/kubefleet/pkg/controllers/rollout           571.040s
FAIL  github.com/kubefleet-dev/kubefleet/pkg/controllers/bindingwatcher    79.814s

pkg/controllers/rollout — the only package this PR touches — passes, including the five new cases. The failure is TestAPIs in pkg/controllers/bindingwatcher, at watcher_integration_test.go:751, which this PR does not touch.

Happy to rebase once that settles if you would rather see a green run before reviewing.

[Simon Waight (sjwaight)]

Wei Weng (@weng271190436) can you please review given original issue was opened by you. Thanks.