[TEST] Improve unit test coverage for apiserver pkg/model#3495
[TEST] Improve unit test coverage for apiserver pkg/model#3495kevin85421 merged 10 commits intoray-project:masterfrom
Conversation
|
@dentiny PTAL, thanks! |
| }, | ||
| } | ||
|
|
||
| var serveAppStatus = rayv1api.AppStatus{ |
There was a problem hiding this comment.
Two questions:
- I'm wondering why do we need the variable here? All these variables are used once, it's clearer to put it inside of test?
- Even if you insist they're necessary, I would suggest rename them to something like
deployingAppStatus.
There was a problem hiding this comment.
I agree. Placing them inside the test functions would make things much clearer. Thanks a lot for your suggestion!
| assert.Len(t, appStatuses, 1) | ||
|
|
||
| appStatus := appStatuses[0] | ||
| assert.Equal(t, "app0", appStatus.Name) |
There was a problem hiding this comment.
we shouldn't duplicate the code here, otherwise next time we update the app status, we need to change two places.
My suggestion would be, define a variable and put it into serveApplicationStatuses, use serveApplicationStatuses.Status as expected value.
There was a problem hiding this comment.
Fixed! We now define the expected result at the beginning, avoiding hardcoded expected values during assertions.
| serviceEvents := PopulateRayServiceEvent("svc0", events) | ||
| assert.Len(t, serviceEvents, 1) | ||
|
|
||
| serviceEvent := serviceEvents[0] |
There was a problem hiding this comment.
not sure if I suggested before, I would prefer to have an event variable.
event := &corev1.Event{
{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
},
Reason: "test",
Message: "test",
Type: "Normal",
Count: 2,
}so we could reuse the fields inside of it, rather than hand-write ourselves.
The benefit is, if we want to update field value in the future, we only need to update in one place.
There was a problem hiding this comment.
Fixed! All checks now consistently reuse field values from the pre-defined objects. Thanks for catching that.
|
@JiangJiaWei1103 would you mind rebasing with the master branch? Thanks! |
Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>
Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>
Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>
Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>
Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>
Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>
Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>
Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>
Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>
JiangJiaWei1103
force-pushed
the
add-unit-tests-for-pkg-model
branch
from
bdc2375 to
0adf21a
Compare
Done. Thanks a lot! |
3 participants
Changes
FromCrdToAPIServicesFromCrdToAPIServiceTestPopulateServeApplicationStatusTestPopulateServeDeploymentStatusTestPopulateRayServiceEventWhy are these changes needed?
To ensure high confidence in code quality before releasing the APIServer to the public, we aim to improve unit test coverage. Currently, the unit test coverage of pkg/model package can be further improved, as shown below:
Before
After
After adding new tests above, the unit test coverage boosts from 79.3% to 90.1%.
Related issue number
Closes #3316.
Related PR
#3419
Checks