Skip to content

fix: add ut for backend runtime. #428

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

X1aoZEOuO
Copy link

@X1aoZEOuO X1aoZEOuO commented May 22, 2025

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:

github.com/inftyai/llmaz/pkg/controller_helper/backendruntime   0.016s  coverage: 35.9% of statements

To:

github.com/inftyai/llmaz/pkg/controller_helper/backendruntime   0.016s  coverage: 69.2% of statements

cc @kerthcet

Does this PR introduce a user-facing change?

Add ut for backend runtime.

@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 May 22, 2025
@InftyAI-Agent InftyAI-Agent requested a review from kerthcet May 22, 2025 03:11
@X1aoZEOuO
Copy link
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 May 22, 2025
@kerthcet
Copy link
Member

Would you like to take a view first @googs1025

@googs1025
Copy link
Member

/cc

sure. I'll take a look today 😄

@InftyAI-Agent InftyAI-Agent requested a review from googs1025 May 22, 2025 03:57
Comment on lines +106 to +107
resourcesNil bool
shmNil bool
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 not only check whether it is nil, but also check whether it is exactly the same?

Copy link
Author

Choose a reason for hiding this comment

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

Make sense, I have changed to real variable.

type want struct {
cmd []string
envs []corev1.EnvVar
lifecycle *corev1.Lifecycle
Copy link
Member

Choose a reason for hiding this comment

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

miss Args param

Copy link
Author

Choose a reason for hiding this comment

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

Got.

},
}

return &BackendRuntimeParser{
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 use NewBackendRuntimeParser ?

Copy link
Author

Choose a reason for hiding this comment

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

Make sense, I will fix it with NewBackendRuntimeParser function.


return &BackendRuntimeParser{
backendRuntime: backend,
models: []*coreapi.OpenModel{{}}, // 这里只需要占位
Copy link
Member

Choose a reason for hiding this comment

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

remove chinese, and can we add OpenModel for test?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I will update it.

Copy link
Member

Choose a reason for hiding this comment

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

No Chinese comments please.

p := tc.parser

if diff := cmp.Diff(tc.want.cmd, p.Command()); diff != "" {
t.Fatalf("Command() mismatch (-want +got):\n%s", diff)
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 keep the error information consistent across each output?
example:

t.Fatalf("Command() mismatch (-want +got):\n%s", diff)

t.Fatalf("Image() = %s, want %s", got, tc.want.image)

Copy link
Author

Choose a reason for hiding this comment

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

Thank you, I will change them to same output.

@kerthcet
Copy link
Member

/approve

Leave the LGTM to @googs1025

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

@kerthcet kerthcet left a comment

Choose a reason for hiding this comment

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

Only one nit. Otherwise LGTM @X1aoZEOuO


return &BackendRuntimeParser{
backendRuntime: backend,
models: []*coreapi.OpenModel{{}}, // 这里只需要占位
Copy link
Member

Choose a reason for hiding this comment

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

No Chinese comments please.

@kerthcet
Copy link
Member

kindly ping @X1aoZEOuO

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. 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
Development

Successfully merging this pull request may close these issues.

4 participants