Skip to content
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

runtime-rs: fix the bug of func count_files #9830

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

Conversation

gaohuatao-1
Copy link
Contributor

When the total number of files observed is greater than limit, return -1 directly. runtime has fixed this bug, it should b ported to runtime-rs.

Fixes:#9829

@justxuewei
Copy link
Member

/test

@justxuewei
Copy link
Member

@gaohuatao-1 Thanks for bringing this to runtime-rs. Could you mind adding some unit tests to the method?

Copy link
Contributor

@Apokleos Apokleos left a comment

Choose a reason for hiding this comment

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

Thx @gaohuatao-1 It looks good for me but lacks the UT of such general function.

@katacontainersbot katacontainersbot added size/small Small and simple task and removed size/tiny Smallest and simplest task labels Jun 13, 2024
@justxuewei
Copy link
Member

/test

Copy link
Member

@justxuewei justxuewei left a comment

Choose a reason for hiding this comment

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

Lgtm, thanks!

When the total number of files observed is greater than limit, return -1 directly.
runtime has fixed this bug, it should b ported to runtime-rs.

Fixes:kata-containers#9829

Signed-off-by: gaohuatao <gaohuatao@bytedance.com>
@gaohuatao-1
Copy link
Contributor Author

@justxuewei @Apokleos I have added a ut to the method. PTAL again, thanks.

@justxuewei
Copy link
Member

/test

@gaohuatao-1
Copy link
Contributor Author

@Apokleos Hi, PTAL, Thanks.

Copy link
Contributor

@Apokleos Apokleos left a comment

Choose a reason for hiding this comment

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

Thx @gaohuatao-1 Compared with CountFiles in kata runtime with golang, this commit looks good to me.
But I try to set the limit < 0 (limit declared as i32), the UT fails. I think the argument limit: i32 in count_files() shoud be change as limit: u32, maybe it's another bug, what do you think ?

@gaohuatao-1
Copy link
Contributor Author

gaohuatao-1 commented Jun 18, 2024

Thx @gaohuatao-1 Compared with CountFiles in kata runtime with golang, this commit looks good to me. But I try to set the limit < 0 (limit declared as i32), the UT fails. I think the argument limit: i32 in count_files() shoud be change as limit: u32, maybe it's another bug, what do you think ?
@Apokleos Thanks for your reply.
If the input param limit of func count_files is smaller than zero, test_count_files is still passed. Of course, you cannot set the local var limit to -1 in func test_count_files(). Could you please post your test code?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test size/small Small and simple task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants