Bareminimum workqueue test #263

guptaNswati · 2025-02-28T00:59:53Z

This adds a preliminary unit test for pkg/workqueue which initialize the WorkQueue with a default Rate-limiter for handling event failures and retries asynchronously. The tests make sure the items added are valid and processed.

Heres the Test results

go test -v ./...
=== RUN   TestEnqueue
=== RUN   TestEnqueue/EnqueueRaw
=== RUN   TestEnqueue/EnqueueValid
=== RUN   TestEnqueue/EnqueueInvalid
W0228 00:51:17.477414 2902840 workqueue.go:72] unexpected object type string: runtime.Object required
=== RUN   TestEnqueue/NilCallback
E0228 00:51:17.482679 2902840 workqueue.go:99] Failed to reconcile work item: no callback to process work item: &{Object:&Unknown{TypeMeta:TypeMeta{APIVersion:,Kind:,},Raw:nil,ContentEncoding:,ContentType:,} Callback:<nil>}
--- PASS: TestEnqueue (0.02s)
    --- PASS: TestEnqueue/EnqueueRaw (0.01s)
    --- PASS: TestEnqueue/EnqueueValid (0.01s)
    --- PASS: TestEnqueue/EnqueueInvalid (0.00s)
    --- PASS: TestEnqueue/NilCallback (0.01s)
PASS
ok  	github.com/NVIDIA/k8s-dra-driver-gpu/pkg/workqueue	0.018s

copy-pr-bot · 2025-02-28T00:59:56Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

pkg/workqueue/workqueue_test.go

ArangoGutierrez · 2025-02-28T06:48:44Z

/ok to test

ArangoGutierrez · 2025-02-28T06:53:56Z

pkg/workqueue/workqueue_test.go

+	// Create a context with timeout for processing.
+	// use DefaultTypedControllerRateLimiter Base delay: 5ms
+	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Millisecond)
+	defer cancel()
+
+	// Test EnqueueRaw
+	t.Run("EnqueueRaw", func(t *testing.T) {
+		var called int32
+		callback := func(ctx context.Context, obj any) error {
+			atomic.StoreInt32(&called, 1)
+			return nil
+		}
+		wq.EnqueueRaw("AnyObject", callback)
+		wq.processNextWorkItem(ctx)
+
+		if atomic.LoadInt32(&called) != 1 {
+			t.Error("EnqueueRaw callback was not invoked")
+		}
+	})
+	// Test Enqueue with valid and invalid runtime.Object and  nill callback
+	// TODO: Implement a proper claim spec that needs to be processed
+	t.Run("EnqueueValid", func(t *testing.T) {
+		var called int32
+		callback := func(ctx context.Context, obj any) error {
+			_, ok := obj.(runtime.Object)
+			if !ok {
+				t.Errorf("Expected runtime.Object, got %T", obj)
+			}
+			atomic.StoreInt32(&called, 1)
+			return nil
+		}
+		validObj := &runtime.Unknown{}
+		wq.Enqueue(validObj, callback)
+		wq.processNextWorkItem(ctx)
+
+		if atomic.LoadInt32(&called) != 1 {
+			t.Error("Enqueue callback was not invoked")
+		}
+	})
+
+	t.Run("EnqueueInvalid", func(t *testing.T) {
+		callback := func(ctx context.Context, obj any) error { return nil }
+		wq.Enqueue("NotRuntimeObject", callback)
+	})
+
+	t.Run("NilCallback", func(t *testing.T) {
+		validObj := &runtime.Unknown{}
+		wq.Enqueue(validObj, nil)
+		wq.processNextWorkItem(ctx)
+	})


Suggested change

// Create a context with timeout for processing.

// use DefaultTypedControllerRateLimiter Base delay: 5ms

ctx, cancel := context.WithTimeout(context.Background(), 5*time.Millisecond)

defer cancel()

// Test EnqueueRaw

t.Run("EnqueueRaw", func(t *testing.T) {

var called int32

callback := func(ctx context.Context, obj any) error {

atomic.StoreInt32(&called, 1)

return nil

}

wq.EnqueueRaw("AnyObject", callback)

wq.processNextWorkItem(ctx)

if atomic.LoadInt32(&called) != 1 {

t.Error("EnqueueRaw callback was not invoked")

}

})

// Test Enqueue with valid and invalid runtime.Object and nill callback

// TODO: Implement a proper claim spec that needs to be processed

t.Run("EnqueueValid", func(t *testing.T) {

var called int32

callback := func(ctx context.Context, obj any) error {

_, ok := obj.(runtime.Object)

if !ok {

t.Errorf("Expected runtime.Object, got %T", obj)

}

atomic.StoreInt32(&called, 1)

return nil

}

validObj := &runtime.Unknown{}

wq.Enqueue(validObj, callback)

wq.processNextWorkItem(ctx)

if atomic.LoadInt32(&called) != 1 {

t.Error("Enqueue callback was not invoked")

}

})

t.Run("EnqueueInvalid", func(t *testing.T) {

callback := func(ctx context.Context, obj any) error { return nil }

wq.Enqueue("NotRuntimeObject", callback)

})

t.Run("NilCallback", func(t *testing.T) {

validObj := &runtime.Unknown{}

wq.Enqueue(validObj, nil)

wq.processNextWorkItem(ctx)

})

ctx := context.Background()

tests := []struct {

name string

obj any

callback func(context.Context, any) error

called *int32

}{

{

name: "EnqueueRaw",

obj: "AnyObject",

callback: func(ctx context.Context, obj any) error {

atomic.StoreInt32(&called, 1)

return nil

},

called: new(int32),

},

{

name: "EnqueueValid",

obj: &runtime.Unknown{},

callback: func(ctx context.Context, obj any) error {

if _, ok := obj.(runtime.Object); !ok {

t.Errorf("Expected runtime.Object, got %T", obj)

}

atomic.StoreInt32(&called, 1)

return nil

},

called: new(int32),

},

{

name: "EnqueueInvalid",

obj: "NotRuntimeObject",

callback: func(ctx context.Context, obj any) error { return nil },

},

{

name: "NilCallback",

obj: &runtime.Unknown{},

},

}

for _, tt := range tests {

t.Run(tt.name, func(t *testing.T) {

wq.Enqueue(tt.obj, tt.callback)

wq.processNextWorkItem(ctx)

if tt.called != nil && atomic.LoadInt32(tt.called) != 1 {

t.Error("Callback was not invoked")

}

})

}

This is a more compact and readable form factor.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

pkg/workqueue/workqueue_test.go

elezar · 2025-02-28T07:37:13Z

pkg/workqueue/workqueue_test.go

+		if atomic.LoadInt32(&called) != 1 {
+			t.Error("Enqueue callback was not invoked")
+		}


Could you comment on why it's required to use atomics in the context of the tests?

I read that this is a standard go practice to use atomics to test callback functions

Copilot

PR Overview

This pull request adds a preliminary unit test for the WorkQueue package to verify that items are enqueued and processed with a default rate limiter and provide appropriate error handling in cases such as invalid types or missing callbacks.

Added tests for different enqueue scenarios including raw data, valid objects, invalid type, and nil callbacks.
Validates that callback execution is triggered correctly under controlled conditions.

Reviewed Changes

File	Description
pkg/workqueue/workqueue_test.go	Added tests for WorkQueue functionality covering various enqueuing scenarios

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

pkg/workqueue/workqueue_test.go

PR Overview

This PR introduces a preliminary unit test for the workqueue package to validate that items are enqueued and processed correctly, including edge cases such as invalid objects and nil callbacks.

Adds table-driven tests for multiple scenarios (valid, invalid, nil callback).
Initializes the workqueue with a default rate limiter and verifies callback invocation where applicable.

Reviewed Changes

File	Description
pkg/workqueue/workqueue_test.go	Adds preliminary unit tests for the pkg/workqueue module

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

pkg/workqueue/workqueue_test.go:38

Consider increasing the timeout duration to reduce the risk of test flakiness in slower environments.

ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond)

pkg/workqueue/workqueue_test.go:84

[nitpick] Consider explicitly validating that no callback is invoked when a nil callback is provided to improve test robustness.

name: "NilCallback",

Signed-off-by: Swati Gupta <[email protected]>

ArangoGutierrez

test list form factor

ArangoGutierrez · 2025-03-06T14:21:43Z

pkg/workqueue/workqueue_test.go

+	// Create a WorkQueue using the default rate limiter.
+	defaultRateLimiter := DefaultControllerRateLimiter()
+	wq := New(defaultRateLimiter)
+
+	require.NotNil(t, wq)
+	require.NotNil(t, wq.queue)
+
+	// Create a context with timeout for processing.
+	// use DefaultTypedControllerRateLimiter Base delay: 5ms
+	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Millisecond)
+	defer cancel()
+
+	// Test EnqueueRaw
+	t.Run("EnqueueRaw", func(t *testing.T) {
+		var called int32
+		callback := func(ctx context.Context, obj any) error {
+			atomic.StoreInt32(&called, 1)
+			return nil
+		}
+		wq.EnqueueRaw("AnyObject", callback)
+		wq.processNextWorkItem(ctx)
+
+		if atomic.LoadInt32(&called) != 1 {
+			t.Error("EnqueueRaw callback was not invoked")
+		}
+	})
+	// Test Enqueue with valid and invalid runtime.Object and  nil callback
+	// TODO: Implement a proper claim spec that needs to be processed
+	t.Run("EnqueueValid", func(t *testing.T) {
+		var called int32
+		callback := func(ctx context.Context, obj any) error {
+			_, ok := obj.(runtime.Object)
+			if !ok {
+				t.Errorf("Expected runtime.Object, got %T", obj)
+			}
+			atomic.StoreInt32(&called, 1)
+			return nil
+		}
+		validObj := &runtime.Unknown{}
+		wq.Enqueue(validObj, callback)
+		wq.processNextWorkItem(ctx)
+
+		if atomic.LoadInt32(&called) != 1 {
+			t.Error("Enqueue callback was not invoked")
+		}
+	})
+
+	t.Run("EnqueueInvalid", func(t *testing.T) {
+		callback := func(ctx context.Context, obj any) error { return nil }
+		wq.Enqueue("NotRuntimeObject", callback)
+	})
+
+	t.Run("NilCallback", func(t *testing.T) {
+		validObj := &runtime.Unknown{}
+		wq.Enqueue(validObj, nil)
+		wq.processNextWorkItem(ctx)
+	})


Suggested change

// Create a WorkQueue using the default rate limiter.

defaultRateLimiter := DefaultControllerRateLimiter()

wq := New(defaultRateLimiter)

require.NotNil(t, wq)

require.NotNil(t, wq.queue)

// Create a context with timeout for processing.

// use DefaultTypedControllerRateLimiter Base delay: 5ms

ctx, cancel := context.WithTimeout(context.Background(), 5*time.Millisecond)

defer cancel()

// Test EnqueueRaw

t.Run("EnqueueRaw", func(t *testing.T) {

var called int32

callback := func(ctx context.Context, obj any) error {

atomic.StoreInt32(&called, 1)

return nil

}

wq.EnqueueRaw("AnyObject", callback)

wq.processNextWorkItem(ctx)

if atomic.LoadInt32(&called) != 1 {

t.Error("EnqueueRaw callback was not invoked")

}

})

// Test Enqueue with valid and invalid runtime.Object and nil callback

// TODO: Implement a proper claim spec that needs to be processed

t.Run("EnqueueValid", func(t *testing.T) {

var called int32

callback := func(ctx context.Context, obj any) error {

_, ok := obj.(runtime.Object)

if !ok {

t.Errorf("Expected runtime.Object, got %T", obj)

}

atomic.StoreInt32(&called, 1)

return nil

}

validObj := &runtime.Unknown{}

wq.Enqueue(validObj, callback)

wq.processNextWorkItem(ctx)

if atomic.LoadInt32(&called) != 1 {

t.Error("Enqueue callback was not invoked")

}

})

t.Run("EnqueueInvalid", func(t *testing.T) {

callback := func(ctx context.Context, obj any) error { return nil }

wq.Enqueue("NotRuntimeObject", callback)

})

t.Run("NilCallback", func(t *testing.T) {

validObj := &runtime.Unknown{}

wq.Enqueue(validObj, nil)

wq.processNextWorkItem(ctx)

})

wq := New(DefaultControllerRateLimiter())

require.NotNil(t, wq)

require.NotNil(t, wq.queue)

ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond)

defer cancel()

tests := []struct {

name string

obj any

callback func(context.Context, any) error

validate func(*testing.T, *int32)

processNextWorkItem bool

}{

{

name: "EnqueueRaw",

obj: "AnyObject",

callback: func(ctx context.Context, obj any) error {

atomic.StoreInt32(obj.(*int32), 1)

return nil

},

validate: func(t *testing.T, called *int32) {

if atomic.LoadInt32(called) != 1 {

t.Error("EnqueueRaw callback was not invoked")

}

},

processNextWorkItem: true,

},

{

name: "EnqueueValid",

obj: &runtime.Unknown{},

callback: func(ctx context.Context, obj any) error {

if _, ok := obj.(runtime.Object); !ok {

t.Errorf("Expected runtime.Object, got %T", obj)

}

atomic.StoreInt32(new(int32), 1)

return nil

},

validate: func(t *testing.T, called *int32) {

// No validation needed for runtime.Object type

},

processNextWorkItem: true,

},

{

name: "EnqueueInvalid",

obj: "NotRuntimeObject",

callback: func(ctx context.Context, obj any) error { return nil },

validate: nil,

processNextWorkItem: false,

},

{

name: "NilCallback",

obj: &runtime.Unknown{},

callback: nil,

validate: nil,

processNextWorkItem: true,

},

}

for _, tt := range tests {

t.Run(tt.name, func(t *testing.T) {

var called int32

if tt.name == "EnqueueRaw" {

wq.EnqueueRaw(&called, tt.callback)

} else {

wq.Enqueue(tt.obj, tt.callback)

}

if tt.processNextWorkItem {

wq.processNextWorkItem(ctx)

if tt.validate != nil {

tt.validate(t, &called)

}

}

})

}

I have tested this suggestion it works

IMO, it may be a bit of an overkill to add the extra validate func and bool just to make the test compact. If its not strongly desired, I would like to stick to the current format.

@ArangoGutierrez how do we move forward with this? :)

ArangoGutierrez requested review from Copilot, klueska and ArangoGutierrez February 28, 2025 06:35

ArangoGutierrez assigned guptaNswati Feb 28, 2025

ArangoGutierrez requested changes Feb 28, 2025

View reviewed changes

pkg/workqueue/workqueue_test.go Outdated Show resolved Hide resolved

ArangoGutierrez requested changes Feb 28, 2025

View reviewed changes

Copilot AI reviewed Feb 28, 2025

View reviewed changes

elezar reviewed Feb 28, 2025

View reviewed changes

pkg/workqueue/workqueue_test.go Outdated Show resolved Hide resolved

elezar reviewed Feb 28, 2025

View reviewed changes

ArangoGutierrez requested a review from Copilot February 28, 2025 11:47

Copilot AI reviewed Feb 28, 2025

View reviewed changes

pkg/workqueue/workqueue_test.go Outdated Show resolved Hide resolved

guptaNswati force-pushed the unit-test-for-api branch from d6dae83 to 7c8dde8 Compare March 4, 2025 01:21

ArangoGutierrez requested a review from Copilot March 4, 2025 10:02

Copilot AI reviewed Mar 4, 2025

View reviewed changes

guptaNswati force-pushed the unit-test-for-api branch from 7c8dde8 to 60f3084 Compare March 4, 2025 21:33

Bareminimum workqueue test

Loading
Loading status checks…

053866d

Signed-off-by: Swati Gupta <[email protected]>

guptaNswati force-pushed the unit-test-for-api branch from 60f3084 to 053866d Compare March 4, 2025 23:04

ArangoGutierrez requested changes Mar 6, 2025

View reviewed changes

guptaNswati requested review from elezar and ArangoGutierrez March 7, 2025 18:07

guptaNswati changed the title ~~Draft: Bareminimum workqueue test~~ Bareminimum workqueue test Mar 10, 2025

guptaNswati requested a review from jgehrcke March 10, 2025 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bareminimum workqueue test #263

Bareminimum workqueue test #263

guptaNswati commented Feb 28, 2025

copy-pr-bot bot commented Feb 28, 2025

ArangoGutierrez commented Feb 28, 2025

ArangoGutierrez Feb 28, 2025

klueska Feb 28, 2025

elezar Feb 28, 2025

guptaNswati Mar 3, 2025

Copilot AI left a comment

ArangoGutierrez left a comment

ArangoGutierrez Mar 6, 2025

guptaNswati Mar 7, 2025

guptaNswati Mar 10, 2025

guptaNswati Mar 10, 2025

Bareminimum workqueue test #263

Are you sure you want to change the base?

Bareminimum workqueue test #263

Conversation

guptaNswati commented Feb 28, 2025

copy-pr-bot bot commented Feb 28, 2025

ArangoGutierrez commented Feb 28, 2025

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

Copilot AI left a comment

Choose a reason for hiding this comment

PR Overview

Reviewed Changes

Choose a reason for hiding this comment

PR Overview

Reviewed Changes

ArangoGutierrez 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