Skip to content

fix: add ut for modelhub. #434

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 13, 2025
Merged

fix: add ut for modelhub. #434

merged 2 commits into from
Jun 13, 2025

Conversation

X1aoZEOuO
Copy link
Contributor

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:

ok      github.com/inftyai/llmaz/pkg/controller_helper/modelsource      0.016s  coverage: 70.0% of statements

To:

ok      github.com/inftyai/llmaz/pkg/controller_helper/modelsource      0.015s  coverage: 78.6% of statements

cc @kerthcet

Does this PR introduce a user-facing change?

Add ut for modelhub.

@InftyAI-Agent InftyAI-Agent added needs-triage Indicates an issue or PR lacks a label and requires one. needs-priority Indicates a PR lacks a label and requires one. do-not-merge/needs-kind Indicates a PR lacks a label and requires one. labels Jun 2, 2025
@InftyAI-Agent InftyAI-Agent requested review from kerthcet June 2, 2025 03:19
@X1aoZEOuO X1aoZEOuO force-pushed the fix/add-ut-for-modelhub branch from ad18539 to 4a23bbf Compare June 2, 2025 03:20
@X1aoZEOuO
Copy link
Contributor Author

/kind cleanup

@InftyAI-Agent InftyAI-Agent added cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed do-not-merge/needs-kind Indicates a PR lacks a label and requires one. labels Jun 2, 2025
@googs1025
Copy link
Member

/assign
I will take a look this week after the festival

googs1025
googs1025 reviewed Jun 3, 2025
View reviewed changes
pkg/controller_helper/modelsource/modelhub_test.go Outdated
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)
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense, I think cmp is better, I will update it quickly, thank you very much.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

friendly ping @googs1025 , do you have time to take a look?

@rudeigerc rudeigerc mentioned this pull request Jun 6, 2025
Merged
@X1aoZEOuO X1aoZEOuO requested a review from googs1025 June 7, 2025 13:11
@X1aoZEOuO X1aoZEOuO force-pushed the fix/add-ut-for-modelhub branch from a2d4b1c to e70bd18 Compare June 9, 2025 00:36
googs1025
googs1025 reviewed Jun 11, 2025
View reviewed changes
pkg/controller_helper/modelsource/modelhub_test.go Outdated
})

for _, tt := range tests {
// tt := tt
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I have removed it, thank you.

@googs1025
Copy link
Member

other part LGTM
/lgtm

@InftyAI-Agent InftyAI-Agent added the lgtm Looks good to me, indicates that a PR is ready to be merged. label Jun 11, 2025
@X1aoZEOuO X1aoZEOuO force-pushed the fix/add-ut-for-modelhub branch from e70bd18 to d5e1341 Compare June 12, 2025 01:59
@InftyAI-Agent InftyAI-Agent removed the lgtm Looks good to me, indicates that a PR is ready to be merged. label Jun 12, 2025
fix: add ut for modelhub.
6ec49e8 
Signed-off-by: X1aoZEOuO <nizefeng2002@outlook.com>
@X1aoZEOuO X1aoZEOuO force-pushed the fix/add-ut-for-modelhub branch from d5e1341 to 8523390 Compare June 12, 2025 03:10
fix: use cmp to check env values.
f88ef2d 
Signed-off-by: X1aoZEOuO <nizefeng2002@outlook.com>
@X1aoZEOuO X1aoZEOuO force-pushed the fix/add-ut-for-modelhub branch from 8523390 to f88ef2d Compare June 12, 2025 13:18
@X1aoZEOuO
Copy link
Contributor Author

Please take a look, thank you! @kerthcet

@kerthcet
Copy link
Member

/approve

@InftyAI-Agent InftyAI-Agent added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 13, 2025
@googs1025
Copy link
Member

/lgtm
thanks @X1aoZEOuO 😄

@InftyAI-Agent InftyAI-Agent added the lgtm Looks good to me, indicates that a PR is ready to be merged. label Jun 13, 2025
@InftyAI-Agent InftyAI-Agent merged commit d8fc1c4 into InftyAI:main Jun 13, 2025
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@googs1025 googs1025 googs1025 left review comments

@kerthcet kerthcet Awaiting requested review from kerthcet

Assignees

@googs1025 googs1025

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Looks good to me, indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a label and requires one. needs-triage Indicates an issue or PR lacks a label and requires one.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@X1aoZEOuO @googs1025 @kerthcet @InftyAI-Agent