chore: Add comprehensive unit tests for Config API #2893
Conversation
Signed-off-by: kapil27 <knema@redhat.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 |
Pull Request Test Coverage Report for Build 18553777112Details
💛 - Coveralls |
Signed-off-by: kapil27 <knema@redhat.com>
andreyvelich
left a comment
andreyvelich
left a comment
There was a problem hiding this comment.
Sorry for the late review @kapil27!
I left a few comments.
/assign @kubeflow/kubeflow-trainer-team
| configapi "github.com/kubeflow/trainer/v2/pkg/apis/config/v1alpha1" | ||
| ) | ||
|
|
||
| // TestValidate provides comprehensive validation testing following Kueue patterns |
There was a problem hiding this comment.
| // TestValidate provides comprehensive validation testing following Kueue patterns |
| } | ||
|
|
||
| // TestValidate_PortBoundaries tests webhook port edge cases | ||
| func TestValidate_PortBoundaries(t *testing.T) { |
There was a problem hiding this comment.
Do we really need this test?
| } | ||
|
|
||
| // TestValidate_ClientConnectionEdgeCases tests QPS and Burst edge cases | ||
| func TestValidate_ClientConnectionEdgeCases(t *testing.T) { |
There was a problem hiding this comment.
Please be consistent with Kueue: https://github.com/kubernetes-sigs/kueue/blob/main/pkg/config/validation_test.go#L939
Use this name: TestValidateClientConnection, and use this test structure:
testCases := map[string]struct {
cfg *configapi.Configuration
wantErr field.ErrorList
}{| return scheme | ||
| } | ||
|
|
||
| func TestLoad_Defaults(t *testing.T) { |
There was a problem hiding this comment.
Please use CamelCase naming everywhere.
| func TestLoad_Defaults(t *testing.T) { | |
| func TestLoadDefaults(t *testing.T) { |
| } | ||
| } | ||
|
|
||
| func TestValidate_ComprehensiveValidation(t *testing.T) { |
There was a problem hiding this comment.
Why do you have this validation tests in the config_test.go ?
|
|
||
| func setupScheme() *runtime.Scheme { | ||
| scheme := runtime.NewScheme() | ||
| _ = clientgoscheme.AddToScheme(scheme) |
There was a problem hiding this comment.
Why do you need clientGoScheme? Please also Fatal the error like here: https://github.com/kubernetes-sigs/kueue/blob/main/pkg/config/config_test.go#L95-L99
| } | ||
| } | ||
|
|
||
| func TestLoad_FromFile(t *testing.T) { |
There was a problem hiding this comment.
Please refactor these tests by following the Kueue approach: https://github.com/kubernetes-sigs/kueue/blob/main/pkg/config/config_test.go#L94
JobSet has similar tests for TestLoad(): https://github.com/kubernetes-sigs/jobset/blob/main/pkg/config/config_test.go
3 participants
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
Fixes #<issue number>, #<issue number>, ...format, will close the issue(s) when PR gets merged):Fixes #2885
Checklist: