🐛 improve event recording logic and test maintainbility #1376
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: haoqing0110 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughExtracted PlacementDecision event emission into a new helper and updated create/update flows to call it; events now use the PlacementDecision as the event source, score messages are deterministically ordered and truncated, and new unit tests and test helpers were added for event verification. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1376 +/- ##
==========================================
+ Coverage 61.71% 61.79% +0.07%
==========================================
Files 226 226
Lines 18610 18616 +6
==========================================
+ Hits 11486 11504 +18
+ Misses 5904 5894 -10
+ Partials 1220 1218 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/assign @qiujian16 |
|
|
||
| // update the event with warning | ||
| // Record events only when there are changes (status updated or labels updated) | ||
| if labelUpdated { |
There was a problem hiding this comment.
why do we need to record event when label is changed? Is anything different in event under this case?
There was a problem hiding this comment.
PlacementDecision label contains the index and decision group info; there's a possibility that select clusters do not change, but only the label (decision group name) changes, and this should be traced by event.
There was a problem hiding this comment.
but we do not record decision group name in the event I think?
There was a problem hiding this comment.
Yes, we might need a different event to record the label changes.
There was a problem hiding this comment.
if we do not have different events for now, could we just avoid event recording when label changes.
There was a problem hiding this comment.
Got it, removed the logic to call recordDecisionEvents when label changes.
haoqing0110
force-pushed
the
refactor/improve-event-recording-logic
branch
2 times, most recently
from
bcfb01e to
57891e6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/placement/controllers/scheduling/scheduling_controller.go`:
- Around line 724-766: The ScoreUpdate event is emitted even when clusterScores
is empty, producing noisy empty events; in recordDecisionEvents check for empty
scores (e.g., if len(clusterScores) == 0 or len(sortedClusterNames) == 0 after
computing sortedClusterNames from sets.KeySet(clusterScores)) and skip emitting
the ScoreUpdate Eventf via c.eventsRecorder.Eventf; leave the DecisionUpdate
event behavior unchanged so only the score-specific Eventf call (the one using
scoreStr) is suppressed when there are no cluster scores.
🧹 Nitpick comments (2)
pkg/placement/controllers/scheduling/scheduling_controller_event_test.go (2)
98-114: Missing test case: warning status with no decision change should not emit events.The current "warning status event" test combines a warning status with a decision change. Consider adding a test where
statusisWarningbut decisions are unchanged — this would verify that warning status alone doesn't trigger spurious events.
150-165:collectEventsmay leave unexpected extra events undetected in the channel.If the production code emits more events than
expectedCount, this function stops collecting after hitting the expected count, and the surplus is silently ignored. TheverifyEventscheck at line 169 only validateslen(events) == expectEventCounton what was collected.Consider draining the channel briefly after the main collection loop to detect unexpected extra events:
Suggested improvement
func collectEvents(recorder *kevents.FakeRecorder, expectedCount int, timeout time.Duration) []string { var events []string deadline := time.After(timeout) for i := 0; i < expectedCount; i++ { select { case event := <-recorder.Events: events = append(events, event) case <-deadline: return events } } + // Drain any unexpected extra events + for { + select { + case event := <-recorder.Events: + events = append(events, event) + case <-time.After(100 * time.Millisecond): + return events + } + } + return events }This way,
verifyEventswill catch unexpected surplus events via the count check.
haoqing0110
force-pushed
the
refactor/improve-event-recording-logic
branch
from
57891e6 to
061d2ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/placement/controllers/scheduling/scheduling_controller_event_test.go`:
- Around line 150-165: collectEvents currently returns immediately when
expectedCount==0 and therefore misses spurious events; modify collectEvents (the
function and its use of recorder.Events and expectedCount) to always wait for
the configured timeout and drain any events emitted during that period (or after
collecting the expectedCount) so tests catch unexpected extras — implement a
drain loop using the same deadline (select on recorder.Events vs deadline)
either when expectedCount==0 or after the main collection loop to append any
remaining events before returning.
🧹 Nitpick comments (2)
pkg/placement/controllers/scheduling/scheduling_controller_event_test.go (1)
98-114: "warning status event" expects 2 events butexpectWarning: true— verify the Warning event is one of the 2, not a 3rd.
expectEventCount: 2meanscollectEventswill collect at most 2 events.verifyEventsthen checks forDecisionUpdate,ScoreUpdate, andWarning. Since the Warning check usescontainsEventType(events, "Warning")which is a substring search on the same 2 collected events, this works if the DecisionUpdate event string contains "Warning" (which it will, since the event type iscorev1.EventTypeWarning). This is correct but somewhat implicit — worth a brief inline comment for future maintainers.pkg/placement/controllers/scheduling/scheduling_controller_test.go (1)
1446-1479: Assertion helpers are useful and correct.Minor inconsistency:
assertClusterSetBindingNameschecks length separately then deletes from a set, whileassertClusterSetNamesandassertClusterNamesusesets.Equal. Consider unifying the approach for consistency, but this is a nitpick.Optional: align assertClusterSetBindingNames with the other helpers
func assertClusterSetBindingNames(t *testing.T, bindings []*clusterapiv1beta2.ManagedClusterSetBinding, expectedNames []string) { - expectedBindingNames := sets.NewString(expectedNames...) - if len(bindings) != expectedBindingNames.Len() { - t.Errorf("expected %d bindings but got %d", expectedBindingNames.Len(), len(bindings)) - } - for _, binding := range bindings { - expectedBindingNames.Delete(binding.Name) - } - if expectedBindingNames.Len() > 0 { - t.Errorf("expected bindings: %s", strings.Join(expectedBindingNames.List(), ",")) - } + actual := sets.NewString() + for _, binding := range bindings { + actual.Insert(binding.Name) + } + expected := sets.NewString(expectedNames...) + if !actual.Equal(expected) { + t.Errorf("expected bindings %v, but got %v", expected.List(), actual.List()) + } }
| // collectEvents collects events from FakeRecorder with a timeout | ||
| func collectEvents(recorder *kevents.FakeRecorder, expectedCount int, timeout time.Duration) []string { | ||
| var events []string | ||
| deadline := time.After(timeout) | ||
|
|
||
| for i := 0; i < expectedCount; i++ { | ||
| select { | ||
| case event := <-recorder.Events: | ||
| events = append(events, event) | ||
| case <-deadline: | ||
| return events | ||
| } | ||
| } | ||
|
|
||
| return events | ||
| } |
There was a problem hiding this comment.
collectEvents won't detect spurious events when expectedCount is 0.
When expectedCount is 0, the loop body never executes, so the function returns immediately without checking whether any unexpected events were emitted. This means the "labels changed only" and "no changes" test cases would still pass even if events were erroneously recorded — defeating the purpose of testing the core bug fix (issue #1323).
Consider adding a brief drain after collecting the expected events to catch unexpected extras:
Proposed fix
func collectEvents(recorder *kevents.FakeRecorder, expectedCount int, timeout time.Duration) []string {
var events []string
deadline := time.After(timeout)
for i := 0; i < expectedCount; i++ {
select {
case event := <-recorder.Events:
events = append(events, event)
case <-deadline:
return events
}
}
+ // Drain any unexpected extra events within a short window
+ drainDeadline := time.After(100 * time.Millisecond)
+ for {
+ select {
+ case event := <-recorder.Events:
+ events = append(events, event)
+ case <-drainDeadline:
+ return events
+ }
+ }
+
return events
}🤖 Prompt for AI Agents
In `@pkg/placement/controllers/scheduling/scheduling_controller_event_test.go`
around lines 150 - 165, collectEvents currently returns immediately when
expectedCount==0 and therefore misses spurious events; modify collectEvents (the
function and its use of recorder.Events and expectedCount) to always wait for
the configured timeout and drain any events emitted during that period (or after
collecting the expectedCount) so tests catch unexpected extras — implement a
drain loop using the same deadline (select on recorder.Events vs deadline)
either when expectedCount==0 or after the main collection loop to append any
remaining events before returning.
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]> Signed-off-by: Qing Hao <[email protected]>
haoqing0110
force-pushed
the
refactor/improve-event-recording-logic
branch
from
061d2ce to
f015d36
Compare
2 participants
🤖 Generated with Claude Code
Summary
Related issue(s)
Fixes #1323
Summary by CodeRabbit
Bug Fixes
Tests