-
Notifications
You must be signed in to change notification settings - Fork 40k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(dra): improve assert event for unit test for controller #127726
Conversation
/kind cleanup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ok-to-test
@pohly /PTAL thanks |
cc := ctrl.(*controller) | ||
// We need to mock the event recorder to test the controller's event. | ||
fakeRecorder := record.NewFakeRecorder(100) | ||
cc.eventRecorder = fakeRecorder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so that explains the client-go/tools/record/event.go
failure, right?
Good find, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
// expectedEvent is a slice of strings representing expected events. | ||
// Each string in the slice should follow the format: "EventType Reason Message". | ||
// - "Warning Failed processing failed" | ||
expectedEvent []string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there no unit tests which emit events? Can you add one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If no events are generated, then why did I see the event.go
error when writing this code initially? I need to look into this a bit more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps I was trying to test events, noticed that problem, and then added that TODO instead. Not sure anymore, it was two years ago 😅
The reason why many controllers, also elsewhere, test their syncKey
or sync<Something>
methods is that the sync
method needs a workqueue and might block. It's hard to make it process exactly one item. I think it's time to write a mock implementation of TypedRateLimitingInterface
, if there isn't one already... I'll take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was a little confused when I saw this TODO, because it seems that there is currently no code that needs to cover the test of the event. 😄
Do we need this part of the code? Because we are not sure whether the event will be sent to the controller in the future, but it is lossless for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Below is what that could look like.
@googs1025 if this makes sense to you, can you add this mock_queue_test.go
with me as author in a separate commit and then adapt the controller_test.go
and controller.go
accordingly?
diff --git a/staging/src/k8s.io/dynamic-resource-allocation/controller/controller.go b/staging/src/k8s.io/dynamic-resource-allocation/controller/controller.go
index 2a0ae0b2a79..44f6f2f1e9f 100644
--- a/staging/src/k8s.io/dynamic-resource-allocation/controller/controller.go
+++ b/staging/src/k8s.io/dynamic-resource-allocation/controller/controller.go
@@ -328,7 +328,7 @@ func (ctrl *controller) Run(workers int) {
}
for i := 0; i < workers; i++ {
- go wait.Until(ctrl.sync, 0, stopCh)
+ go wait.Until(func() { ctrl.sync(ctrl.queue) }, 0, stopCh)
}
<-stopCh
@@ -344,12 +344,12 @@ var errRequeue = errors.New("requeue")
var errPeriodic = errors.New("periodic")
// sync is the main worker.
-func (ctrl *controller) sync() {
- key, quit := ctrl.queue.Get()
+func (ctrl *controller) sync(queue workqueue.TypedRateLimitingInterface[string]) {
+ key, quit := queue.Get()
if quit {
return
}
- defer ctrl.queue.Done(key)
+ defer queue.Done(key)
logger := klog.LoggerWithValues(ctrl.logger, "key", key)
ctx := klog.NewContext(ctrl.ctx, logger)
diff --git a/staging/src/k8s.io/dynamic-resource-allocation/controller/controller_test.go b/staging/src/k8s.io/dynamic-resource-allocation/controller/controller_test.go
index a8d2f24748d..4747a6ff348 100644
--- a/staging/src/k8s.io/dynamic-resource-allocation/controller/controller_test.go
+++ b/staging/src/k8s.io/dynamic-resource-allocation/controller/controller_test.go
@@ -22,6 +22,7 @@ import (
"fmt"
"testing"
+ "github.com/google/go-cmp/cmp"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
@@ -340,16 +341,13 @@ func TestController(t *testing.T) {
) {
t.Fatal("could not sync caches")
}
- _, err := ctrl.(*controller).syncKey(ctx, test.key)
- if err != nil && test.expectedError == "" {
- t.Fatalf("unexpected error: %v", err)
- }
- if err == nil && test.expectedError != "" {
- t.Fatalf("did not get expected error %q", test.expectedError)
- }
- if err != nil && err.Error() != test.expectedError {
- t.Fatalf("expected error %q, got %q", test.expectedError, err.Error())
+
+ var m Mock[string]
+ m.SyncOne(test.key, ctrl.(*controller).sync)
+ if diff := cmp.Diff(Mock[string]{}, m); diff != "" {
+ t.Fatalf("unexpected state of mock work queue after sync (-want, +got):\n%s", diff)
}
+ // TODO: replace test.expectError with expectations for the state of the work queue.
claims, err := kubeClient.ResourceV1alpha3().ResourceClaims("").List(ctx, metav1.ListOptions{})
require.NoError(t, err, "list claims")
var expectedClaims []resourceapi.ResourceClaim
diff --git a/staging/src/k8s.io/dynamic-resource-allocation/controller/mock_queue_test.go b/staging/src/k8s.io/dynamic-resource-allocation/controller/mock_queue_test.go
new file mode 100644
index 00000000000..08474b0652c
--- /dev/null
+++ b/staging/src/k8s.io/dynamic-resource-allocation/controller/mock_queue_test.go
@@ -0,0 +1,165 @@
+/*
+Copyright 2024 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package controller
+
+import (
+ "slices"
+ "time"
+
+ "k8s.io/client-go/util/workqueue"
+)
+
+// TODO (pohly): move this to k8s.io/client-go/util/workqueue/workqueue.go
+// if it turns out to be generally useful. Doc comments are already written
+// as if the code was there.
+
+// MockQueue is an implementation of [TypedRateLimitingInterface] which
+// can be used to test a function which pulls work items out of a queue
+// and processes them.
+//
+// A null instance is directly usable. The usual usage is:
+//
+// var m workqueue.Mock[string]
+// m.SyncOne("some-item", func(queue workqueue.TypedRateLimitingInterface[string]) { ... } )
+// if diff := cmp.Diff(workqueue.Mock[string]{}, m); diff != "" {
+// t.Errorf("unexpected state of mock work queue after sync (-want, +got):\n%s", diff)
+// }
+//
+// All slices get reset to nil when they become empty, so there are no spurious
+// differences because of the nil vs. empty slice.
+type Mock[T comparable] struct {
+ // Ready contains the items which are ready for processing.
+ Ready []T
+
+ // InFlight contains the items which are currently being processed (= Get
+ // was called, Done not yet).
+ InFlight []T
+
+ // MismatchedDone contains the items for which Done was called without
+ // a matching Get.
+ MismatchedDone []T
+
+ // Later contains the items which are meant to be added to the queue after
+ // a certain delay (= AddAfter was called for them).
+ Later []MockDelayedItem[T]
+
+ // Failures contains the items and their retry count which failed to be
+ // processed (AddRateLimited called at least once, Forget not yet).
+ // The retry count is always larger than zero.
+ Failures map[T]int
+
+ // ShutDownCalled tracks how often ShutDown got called.
+ ShutDownCalled int
+
+ // ShutDownWithDrainCalled tracks how often ShutDownWithDrain got called.
+ ShutDownWithDrainCalled int
+}
+
+// MockDelayedItem is an item which was queue for later processing.
+type MockDelayedItem[T comparable] struct {
+ Item T
+ Duration time.Duration
+}
+
+// SyncOne adds the item to the work queue and calls sync.
+// That sync function can pull one or more items from the work
+// queue until the queue is empty. Then it is told that the queue
+// is shutting down, which must cause it to return.
+//
+// The test can then retrieve the state of the queue to check the result.
+func (m *Mock[T]) SyncOne(item T, sync func(workqueue.TypedRateLimitingInterface[T])) {
+ m.Ready = append(m.Ready, item)
+ sync(m)
+}
+
+// Add implements [TypedInterface].
+func (m *Mock[T]) Add(item T) {
+ m.Ready = append(m.Ready, item)
+}
+
+// Len implements [TypedInterface].
+func (m *Mock[T]) Len() int {
+ return len(m.Ready)
+}
+
+// Get implements [TypedInterface].
+func (m *Mock[T]) Get() (item T, shutdown bool) {
+ if len(m.Ready) == 0 {
+ shutdown = true
+ return
+ }
+ item = m.Ready[0]
+ m.Ready = m.Ready[1:]
+ if len(m.Ready) == 0 {
+ m.Ready = nil
+ }
+ m.InFlight = append(m.InFlight, item)
+ return item, false
+}
+
+// Done implements [TypedInterface].
+func (m *Mock[T]) Done(item T) {
+ index := slices.Index(m.InFlight, item)
+ if index < 0 {
+ m.MismatchedDone = append(m.MismatchedDone, item)
+ }
+ m.InFlight = slices.Delete(m.InFlight, index, index+1)
+ if len(m.InFlight) == 0 {
+ m.InFlight = nil
+ }
+}
+
+// Shutdown implements [TypedInterface].
+func (m *Mock[T]) ShutDown() {
+ m.ShutDownCalled++
+}
+
+// ShutDownWithDrain implements [TypedInterface].
+func (m *Mock[T]) ShutDownWithDrain() {
+ m.ShutDownWithDrainCalled++
+}
+
+// Len implements [TypedInterface].
+func (m *Mock[T]) ShuttingDown() bool {
+ return m.ShutDownCalled > 0 || m.ShutDownWithDrainCalled > 0
+}
+
+// AddAfter implements [TypedDelayingInterface.AddAfter]
+func (m *Mock[T]) AddAfter(item T, duration time.Duration) {
+ m.Later = append(m.Later, MockDelayedItem[T]{Item: item, Duration: duration})
+}
+
+// AddRateLimited implements [TypedRateLimitingInterface.AddRateLimited].
+func (m *Mock[T]) AddRateLimited(item T) {
+ if m.Failures == nil {
+ m.Failures = make(map[T]int)
+ }
+ m.Failures[item]++
+}
+
+// Forget implements [TypedRateLimitingInterface.Forget].
+func (m *Mock[T]) Forget(item T) {
+ if m.Failures == nil {
+ return
+ }
+ delete(m.Failures, item)
+}
+
+// NumRequeues implements [TypedRateLimitingInterface.NumRequeues].
+func (m *Mock[T]) NumRequeues(item T) int {
+ return m.Failures[item]
+}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I will take look tonight
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have merged #127789, so unit testing can cover the event generation part, and we will have some case for event generation
/triage accepted |
ca5775b
to
25ae942
Compare
25ae942
to
6219445
Compare
/test pull-kubernetes-integration |
/test pull-kubernetes-integration |
/test pull-kubernetes-integration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
Thanks!
LGTM label has been added. Git tree hash: ffc5eac67223eea11b28f171e5852189c7cb653b
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: googs1025, pohly The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Although we don’t have any expected events to assert yet, we will need to verify this part in subsequent iterations.
Which issue(s) this PR fixes:
Fixes # None
Special notes for your reviewer:
None
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: