fix: add ut for modelhub.#434
Merged
Merged
Conversation
InftyAI-Agent
added
needs-triage
X1aoZEOuO
force-pushed
the
fix/add-ut-for-modelhub
branch
from
ad18539 to
4a23bbf
Compare
Contributor
Author
|
/kind cleanup |
InftyAI-Agent
added
cleanup
Member
|
/assign |
googs1025
reviewed
Jun 3, 2025
Comment on lines
+104
to
+110
| for _, key := range tc.expectEnvContains { | ||
| found := false | ||
| for _, env := range initContainer.Env { | ||
| if env.Name == key { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| assert.True(t, found, "expected env %s not found", key) | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
Why do we only check the key (the same applies to other tests) but not the value? If I understand correctly, we should also be able to use diff := cmp.Diff to implement this test. In addition, I recommend using cmp.Diff to reduce the time complexity, even though the amount of data in the unit test is very small.
Contributor
Author
There was a problem hiding this comment.
Make sense, I think cmp is better, I will update it quickly, thank you very much.
Contributor
Author
There was a problem hiding this comment.
friendly ping @googs1025 , do you have time to take a look?
X1aoZEOuO
force-pushed
the
fix/add-ut-for-modelhub
branch
from
a2d4b1c to
e70bd18
Compare
googs1025
reviewed
Jun 11, 2025
| }) | ||
|
|
||
| for _, tt := range tests { | ||
| // tt := tt |
Contributor
Author
There was a problem hiding this comment.
Oh, I have removed it, thank you.
Member
|
other part LGTM |
InftyAI-Agent
added
the
lgtm
X1aoZEOuO
force-pushed
the
fix/add-ut-for-modelhub
branch
from
e70bd18 to
d5e1341
Compare
InftyAI-Agent
removed
the
lgtm
Signed-off-by: X1aoZEOuO <nizefeng2002@outlook.com>
X1aoZEOuO
force-pushed
the
fix/add-ut-for-modelhub
branch
from
d5e1341 to
8523390
Compare
Signed-off-by: X1aoZEOuO <nizefeng2002@outlook.com>
X1aoZEOuO
force-pushed
the
fix/add-ut-for-modelhub
branch
from
8523390 to
f88ef2d
Compare
Contributor
Author
|
Please take a look, thank you! @kerthcet |
Member
|
/approve |
InftyAI-Agent
added
the
approved
Member
|
/lgtm |
InftyAI-Agent
added
the
lgtm
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it
Add mode unit tests for backend runtime.
Which issue(s) this PR fixes
Fixes #
Special notes for your reviewer
From:
To:
cc @kerthcet
Does this PR introduce a user-facing change?