[Feature] Add unit test for update service request validation #3546
Conversation
Signed-off-by: LeoLiao123 <leoyeepaa@gmail.com>
Signed-off-by: LeoLiao123 <leoyeepaa@gmail.com>
|
@dentiny PTAL |
| @@ -171,7 +171,7 @@ func ValidateCreateServiceRequest(request *api.CreateRayServiceRequest) error { | |||
|
|
|||
| func ValidateUpdateServiceRequest(request *api.UpdateRayServiceRequest) error { | |||
| if request.Name == "" { | |||
| return util.NewInvalidInputError("Service name is empty. Please specify a valid value.") | |||
| return util.NewInvalidInputError("Name is empty. Please specify a valid value.") | |||
There was a problem hiding this comment.
The original message is identical to the one on line 189 and may cause confusion when debugging.
apiserver/pkg/server/serve_server.go
Outdated
| @@ -190,7 +190,7 @@ func ValidateUpdateServiceRequest(request *api.UpdateRayServiceRequest) error { | |||
| } | |||
|
|
|||
| if request.Service.User == "" { | |||
| return util.NewInvalidInputError("User who create the Service is empty. Please specify a valid value.") | |||
| return util.NewInvalidInputError("User who update the Service is empty. Please specify a valid value.") | |||
There was a problem hiding this comment.
To avoid confusion with ValidateCreateServiceRequest, I changed the message to refer to an 'update' instead.
…ones. Signed-off-by: LeoLiao123 <leoyeepaa@gmail.com>
Signed-off-by: LeoLiao123 <leoyeepaa@gmail.com>
Signed-off-by: LeoLiao123 <leoyeepaa@gmail.com>
|
Hi @dentiny, I've re-triggered the CI and it has passed. Please take a look. Thanks! |
Signed-off-by: LeoLiao123 <leoyeepaa@gmail.com>
| } | ||
| // Execute tests sequentially | ||
| for _, tc := range tests { | ||
| tc := tc // capture range variable |
There was a problem hiding this comment.
@dentiny I initially followed the previous test pattern, but after experimenting I found that capturing isn’t necessary to iterate all test cases, so I’ve removed it in my latest commit!
exp screenshot :
There was a problem hiding this comment.
As a side note, I was curious and looked into why this pattern existed. I found that before Go 1.22 it was indeed necessary to recapture the range variable this way, but it has since been fixed. Reference
There was a problem hiding this comment.
gotcha, I think it's capture by ref vs capture by value
Signed-off-by: LeoLiao123 <leoyeepaa@gmail.com>
|
just fyi, I usually re-request review after address all comments:
feel free to ping me directly (on slack), and sorry for the latency |
|
Got it! Thanks for your thorough explanation and the review! |
Why are these changes needed?
See #3539
Related issue number
Closes #3539
Manual test
Checks