forked from kubeflow/kubeflow
-
Notifications
You must be signed in to change notification settings - Fork 45
Closed
Labels
good first issueGood for newcomersGood for newcomers
Description
Problem
The notebook controller tests in components/odh-notebook-controller/controllers/notebook_controller_test.go
contain 15 instances of local context.Background()
calls that should be replaced with the global context defined in suite_test.go
.
Current State
- Tests create individual contexts with
ctx := context.Background()
in eachIt
block - Global context is properly set up in
suite_test.go
withctx, cancel = context.WithCancel(context.Background())
inBeforeSuite
- This creates architectural inconsistency and bypasses proper cancellation capabilities
Benefits of Using Global Context
- Proper cancellation support - Global context has
context.WithCancel()
for cleanup when tests are interrupted - Consistency - All tests use the same context management approach
- Resource management - Global context is properly managed in test lifecycle
- Performance - Eliminates unnecessary context creation overhead
Scope
Replace all 15 instances of:
It("Should do something", func() {
ctx := context.Background()
// test code...
})
With:
It("Should do something", func() {
// Use global ctx directly
Expect(cli.Create(ctx, resource)).Should(Succeed())
// test code...
})
References
- Original discussion: RHOAIENG-23751: Add finalizer to clean up OAuthClients on notebook deletion #605 (comment)
- Related PR: RHOAIENG-23751: Add finalizer to clean up OAuthClients on notebook deletion #605
Acceptance Criteria
- Remove all 15 instances of local
context.Background()
creation - Verify all tests use the global
ctx
variable consistently - Ensure test functionality remains unchanged
- Update any similar patterns in other test files if found
Metadata
Metadata
Assignees
Labels
good first issueGood for newcomersGood for newcomers