NO-JIRA: chore(tests): reduce the code duplication in runtime tests #636
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant Helper as common.go Helpers
participant K8s as Kubernetes API
Test->>Helper: createNotebook(name, namespace)
Helper->>K8s: Construct Notebook resource
Test->>Helper: createRuntimeConfigMap(name, namespace, data)
Helper->>K8s: Construct ConfigMap resource
Test->>Helper: createRuntimeImageStream(name, namespace, tags)
Helper->>K8s: Construct ImageStream resource
Test->>K8s: Apply resources, verify state
Suggested labels
✨ Finishing Touches
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
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #636 +/- ##
===========================================
+ Coverage 58.08% 70.14% +12.06%
===========================================
Files 11 10 -1
Lines 3075 2164 -911
===========================================
- Hits 1786 1518 -268
+ Misses 1132 523 -609
+ Partials 157 123 -34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I guess? it is almost impossible to me to match up the code reorg in the diff view. I'll try fancy diff. |
|
still can't make sense of it even with fancy diff webui tool |
|
@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)
components/odh-notebook-controller/controllers/common.go (1)
10-30: Consider documenting the choice of default container image.The function uses a hardcoded UBI8 image. While this is acceptable for test helpers, consider adding a comment explaining why this specific image was chosen as the default for tests.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/odh-notebook-controller/controllers/common.go(1 hunks)components/odh-notebook-controller/controllers/notebook_controller_test.go(0 hunks)components/odh-notebook-controller/controllers/notebook_runtime_test.go(2 hunks)
💤 Files with no reviewable changes (1)
- components/odh-notebook-controller/controllers/notebook_controller_test.go
🧰 Additional context used
🧠 Learnings (1)
components/odh-notebook-controller/controllers/notebook_runtime_test.go (1)
Learnt from: jiridanek
PR: opendatahub-io/kubeflow#623
File: components/odh-notebook-controller/controllers/notebook_runtime_test.go:179-179
Timestamp: 2025-06-25T06:54:57.600Z
Learning: In the file `components/odh-notebook-controller/controllers/notebook_runtime_test.go`, there is a suggestion to repurpose `testCase.ConfigMap` as `expectedConfigMap` in the test structure to improve clarity by separating input from expected results. This improvement is deferred to be addressed as part of GitHub issue #634.
🔇 Additional comments (6)
components/odh-notebook-controller/controllers/common.go (2)
32-45: LGTM!The ConfigMap helper is well-implemented and suitable for test usage.
47-68: LGTM!The ImageStream helper correctly sets up the required labels and lookup policy for runtime image testing.
components/odh-notebook-controller/controllers/notebook_runtime_test.go (4)
21-28: Good refactoring to follow Go naming conventions.The constants have been properly renamed to use idiomatic Go camelCase instead of underscore_case.
30-73: Excellent refactoring to reduce code duplication.The new helper function effectively consolidates the repeated logic for checking Notebook volumes and mounts. The use of
Eventuallywith proper timeout handling and the boolean parameter for expected state makes the tests more maintainable and readable.
78-136: Well-structured test refactoring with improved cleanup logic.The test has been improved with:
- Simplified test case structure focusing on relevant fields
- Robust cleanup logic that waits for resource deletion
- Effective use of the new helper functions
The addition of
Eventuallychecks for resource deletion in BeforeEach prevents potential race conditions between tests.
138-290: Excellent addition of comprehensive ImageStream test coverage.The new test group provides thorough coverage for ImageStream-driven ConfigMap creation and mounting scenarios. The test cases effectively cover:
- Multiple valid runtime image tags
- Single valid tag
- Irrelevant metadata (negative test case)
The tests properly validate both ConfigMap content and Notebook mounting behavior.
|
/lgtm rabbit is happy, so who am I to argue |
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-merge-robot
added
the
needs-rebase
Description
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit