-
Notifications
You must be signed in to change notification settings - Fork 877
feat: add unit tests for config package #3095
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Denyme24 <namanraj24@outlook.com>
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
🎉 Welcome to the Kubeflow Trainer! 🎉 Thanks for opening your first PR! We're happy to have you as part of our community 🚀 Here's what happens next:
Join the community:
Feel free to ask questions in the comments if you need any help or clarification! |
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.
Pull request overview
This PR adds comprehensive unit tests for the config package, which previously had zero test coverage. The config package is critical for loading configuration files, applying defaults, and setting up controller-runtime options.
Changes:
- Added 653 lines of unit tests covering configuration loading, validation, defaults application, and controller-runtime options setup
- Fixed a potential nil pointer dereference by adding a default value for
Webhook.Hostindefaults.go - Achieved 98.2% test coverage for the config package
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/config/config_test.go | New comprehensive test suite covering all config package functions including file loading, validation, defaults, and integration tests |
| pkg/apis/config/v1alpha1/defaults.go | Added default value for Webhook.Host to prevent nil pointer dereference |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Yes @andreyvelich , I will address those comments. |
What this PR does / why we need it:
It adds comprehensive unit tests for the
configpackage, which previously had zero test coverage. Theconfigpackage is critical as it handles loading configuration files, applying defaults, and setting up controller-runtime options for the trainer controller manager.Test Coverage Includes
Additional Fix
While writing the tests, I discovered and fixed a missing default value for
Webhook.Hostindefaults.go, which could have led to nil pointer issues at runtime.Coverage
configpackage statementsFixes #3081
Checklist: