fix(envtest): ensures consistent env test setup#798
Conversation
This PR simplifies EnvTest setup and ensures webhooks and controller are configured and deployed together. Signed-off-by: Bartosz Majsak <bartosz.majsak@gmail.com>
WalkthroughA new test environment setup utility was introduced to streamline controller and webhook integration tests for the LLM Inference Service. Test suite initializations were refactored to use this utility, replacing manual setup logic. A test case was updated to specify explicit gateway references. Webhook manifest path handling was improved to ensure uniqueness. Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant SetupTestEnv
participant ControllerManager
participant WebhookManager
participant K8sCluster
TestSuite->>SetupTestEnv: Call SetupTestEnv()
SetupTestEnv->>ControllerManager: Register LLMInferenceServiceReconciler
SetupTestEnv->>WebhookManager: Register webhook validators
SetupTestEnv->>K8sCluster: Ensure required resources
SetupTestEnv-->>TestSuite: Return test client
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.2.2)Error: can't load config: can't unmarshal config by viper (flags, file): 1 error(s) decoding:
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/controller/llmisvc/fixture/envtest.go (1)
40-95: Excellent centralized test environment setup implementation.This new
SetupTestEnv()function successfully consolidates all test environment setup logic and achieves the PR's objectives:Strengths:
- Comprehensive setup including timeouts, context management, controllers, and webhooks
- Proper cleanup handling with
DeferCleanupand context cancellation- Configurable timeout via environment variable with sensible default
- Consistent webhook manifest handling using the improved deduplication logic
- Clean separation of controller and webhook setup functions
Minor observation:
The TODO comment on Line 59 indicates the EventRecorder setup is a stub and should be aligned withmain.goin the future. This doesn't impact current functionality but should be addressed eventually.This consolidation eliminates code duplication across test suites and provides a consistent, maintainable foundation for LLMInferenceService testing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/controller/llmisvc/controller_int_test.go(1 hunks)pkg/controller/llmisvc/fixture/envtest.go(1 hunks)pkg/controller/llmisvc/suite_test.go(1 hunks)pkg/controller/llmisvc/webhook/webhook_suite_test.go(1 hunks)pkg/testing/config.go(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: hdefazio
PR: opendatahub-io/kserve#658
File: pkg/controller/v1beta1/inferenceservice/rawkube_controller_test.go:1873-1886
Timestamp: 2025-06-12T22:10:15.375Z
Learning: If a review comment is raised on code that was not introduced or changed by the current PR, and the user points this out, acknowledge it and clarify that no action is required from the PR author.
pkg/controller/llmisvc/suite_test.go (1)
Learnt from: israel-hdez
PR: #738
File: pkg/apis/serving/v1beta1/configmap.go:0-0
Timestamp: 2025-07-17T16:19:36.771Z
Learning: In pkg/apis/serving/v1beta1/configmap.go, the GetStorageInitializerConfigs function was moved from another package to prevent circular imports, which explains why it may contain existing patterns rather than being newly written code.
pkg/controller/llmisvc/controller_int_test.go (1)
Learnt from: bartoszmajsak
PR: #757
File: pkg/controller/llmisvc/router_discovery.go:45-49
Timestamp: 2025-07-21T17:40:11.003Z
Learning: In the KServe LLMInferenceService controller, the DiscoverURLs function in router_discovery.go intentionally returns nil, nil for missing Gateways rather than errors. This allows the reconciler to continue processing and update status conditions through the separate validation logic in ref_validation.go, preventing reconciliation loops that would occur if errors were returned.
pkg/controller/llmisvc/fixture/envtest.go (1)
Learnt from: israel-hdez
PR: #738
File: pkg/apis/serving/v1beta1/configmap.go:0-0
Timestamp: 2025-07-17T16:19:36.771Z
Learning: In pkg/apis/serving/v1beta1/configmap.go, the GetStorageInitializerConfigs function was moved from another package to prevent circular imports, which explains why it may contain existing patterns rather than being newly written code.
🧬 Code Graph Analysis (2)
pkg/controller/llmisvc/suite_test.go (1)
pkg/controller/llmisvc/fixture/envtest.go (1)
SetupTestEnv(40-95)
pkg/controller/llmisvc/webhook/webhook_suite_test.go (1)
pkg/controller/llmisvc/fixture/envtest.go (1)
SetupTestEnv(40-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Build
- GitHub Check: build (3.12)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: precommit-check
- GitHub Check: test
- GitHub Check: build (3.9)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
🔇 Additional comments (5)
pkg/testing/config.go (2)
86-86: LGTM! Comment correction.The comment now correctly describes the
WithWebhooksmethod instead of incorrectly referencing "WithWebhookManifests."
190-199: Excellent deduplication implementation.The webhook manifest path deduplication logic is well-implemented:
- Uses a
seenmap to track existing paths efficiently- Prevents duplicate entries that could cause envtest configuration issues
- Maintains order of existing paths while adding only new ones
This enhancement supports the PR's goal of consistent test environment setup.
pkg/controller/llmisvc/controller_int_test.go (1)
305-312: Good test improvement - explicit gateway references.The test now explicitly specifies gateway references in the
GatewaySpec, making the test more deterministic and clearly aligned with the test case name "should delete managed HTTPRoute when ref is defined."This change improves test clarity by removing reliance on default gateway behavior.
pkg/controller/llmisvc/suite_test.go (1)
37-37: Excellent consolidation of test environment setup.The replacement of complex manual setup with
fixture.SetupTestEnv()perfectly achieves the PR's goal of consistent and simplified test environment configuration. This eliminates code duplication and centralizes the setup logic for better maintainability.The
SetupTestEnv()function properly handles all aspects of environment setup including timeouts, context management, controller/webhook registration, and cleanup.pkg/controller/llmisvc/webhook/webhook_suite_test.go (1)
38-38: Consistent test environment consolidation.Similar to the controller test suite, this change successfully consolidates webhook test environment setup into the centralized
fixture.SetupTestEnv()function. This ensures consistent test environment configuration across both controller and webhook test suites, improving maintainability and reducing code duplication.
| envTest := testing.NewEnvTest(webhookManifests). | ||
| WithWebhooks(webhooks). |
There was a problem hiding this comment.
This is one of the follow-ups I mentioned in the PR description.
| envTest := testing.NewEnvTest(webhookManifests). | |
| WithWebhooks(webhooks). | |
| envTest := testing.NewEnvTest(). | |
| WithWebhooks(webhook, webhookManifests). |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bartoszmajsak, pierDipi 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 |
pierDipi
merged commit 5bd42ef
into
opendatahub-io:release-v0.15
github-project-automation
Bot
moved this from New/Backlog
to Done
in ODH Model Serving Planning
2 participants
This PR simplifies EnvTest setup and ensures webhooks and controller are configured and deployed together.
Follow-ups to make this PR focused:
Summary by CodeRabbit
Tests
Chores