Increate the code coverage of crates/libcontainer/ to 75%

[utam0k]

Why?

Since youki is a container runtime, it is software that should be security-sensitive. Therefore, it should be covered as much as possible by unit tests to avoid bugs as much as possible.
However, because it is a container runtime, it is difficult to write unit tests because it is difficult to handle mocks. In other words, this is a challenge. It is also a chance to design well.
https://app.codecov.io/gh/containers/youki

How to participate in this challenge

Please indicate that you would like to challenge this issue. I will look at the code coverage and recommend where to challenge. If you are new to youki development, it would be helpful if you could describe how familiar you are with Rust and container runtimes.

Notes

• src/capabilities.rs (@tommady)
• src/rootfs.rs (@tommady)
• src/container/container.rs(@tommady)
• src/rootles.rs(@kenoss)

[tommady]

Hi @utam0k ,
may I take this challenge (or part of it)?

I am very inexperienced on both 1. the rust and 2. the container runtime.

about the rust, I only have some small contributions to

• spodifyd
• ta-rs
• gbdt-rs

and a small project I owned
fugle-rs

about the container runtime,
I only know the issue I just solved.

so hope you will accept my self-recommended and give me this chance to learn more 🙇🏻
thank you.

[utam0k]

@tommady Of cource! Can I ask you to try adding tests to it? If you want, I can prepare more difficult parts.
https://codecov.io/gh/containers/youki/src/main/src/capabilities.rs

[tommady]

hi @utam0k please help to review #296
thank you 🙇🏻

If you want, I can prepare more difficult parts.

of course, I'd glade to

[utam0k]

@tommady Can I ask you to update the test coverage for this file?
https://app.codecov.io/gh/containers/youki/blob/main/src/hooks.rs

If you want something more difficult, I can ask for the most difficult, init.rs or intermediate.rs. Please take a look at these files after you have done the previous ones first and let me know if you want to try them. Implementing unit tests for these files will be a tremendous challenge and an exciting one. However, it will probably take time and patience to take on these challenges, so I would like to hear your opinion. Of course, I'm here to help.

[yihuaf]

For hook.rs, there is a test that tests the timeout path, but it's skipped at the moment. The min timeout is 1 second, so the unit test would take at least 1 second to run. I wasn't sure if we should include that. Maybe remove the #[skip] and try again for the coverage first?

[tommady]

hi @yihuaf
I have a chat with @utam0k ,
and we agreed that changing the timeout of oci_spec.Hook

from current
timeout: Option<int64>
to
timeout: Option<time::Duration>

is a better way.

because:

1. the naming and the type will not get confused by other people.
2. this way can make the timeout testing easier because we can define our own Duration

What do you think?

[yihuaf]

Go ahead. SGTM :)

…

On Tue, Sep 14, 2021 at 6:04 PM tommady ***@***.***> wrote: hi @yihuaf <https://github.com/yihuaf> I have a chat with @utam0k <https://github.com/utam0k> , and we agreed that changing the timeout of oci_spec.Hook from current timeout: Option<int64> to timeout: Option<time::Duration> is a better way. because: 1. the naming and the type will not get confused by other people. 2. this way can make the timeout testing easier because we can define our own Duration What do you think? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#279 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEY66HTMUYQUUF2ZRA6LRW3UB7WKXANCNFSM5DSOT2BQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[kenoss]

Hi, I'm a beginner of youki and I'd like to contribute.
I have a chat with @utam0k and he suggested that adding unit tests of rootless.rs will be a good first step for me.
I'll try it if no one is doing it.

[utam0k]

Hi, I'm a beginner of youki and I'd like to contribute.
I have a chat with @utam0k and he suggested that adding unit tests of rootless.rs will be a good first step for me.
I'll try it if no one is doing it.

Sure! I look forward to your PR. I believe the following codes help you.
https://github.com/containers/youki/blob/88879550370e48d1cb20041b3f66b4e714ef9c40/src/container/builder_impl.rs#L197-L290

[kenoss]

Thanks!

[tommady]

hi, as discussed with @utam0k
I'll be going to add unit tests for

1. src/rootfs.rs
2. src/container/container.rs

thanks!

[hle0]

Hi, I'd love to try my hand at this. I don't have experience with youki, but I do have a tiny bit of experience with rust (few dozen hours maybe, a lot of that is documentation reading). Is there anything I could do? Thank you!

Edit: I forgot to add, I don't know too much about container runtimes themselves, but I do know about running containers in practice (e.g. using docker, how container images are built in layers) and a bit about some syscalls/kernel facilities like seccomp, cgroups, etc.

[yihuaf]

Hi, I'd love to try my hand at this. I don't have experience with youki, but I do have a tiny bit of experience with rust (few dozen hours maybe, a lot of that is documentation reading). Is there anything I could do? Thank you!

Edit: I forgot to add, I don't know too much about container runtimes themselves, but I do know about running containers in practice (e.g. using docker, how container images are built in layers) and a bit about some syscalls/kernel facilities like seccomp, cgroups, etc.

Welcome :) Since you are new, I would suggest take a look at https://app.codecov.io/gh/containers/youki and see where the coverage is low. Then read the code and see if you can help writing tests. Also let us know if any comments is not enough to understand what the code is doing.

Once you are done with that, then let us know what function you would like to help write the test for. I would suggest writing one test at a time since we want to keep the PR as simple as possible. Let us know how we can help here, or on Discord :)

[utam0k]

@hle0 Hi! welcome. I'm sorry for the delay in getting back to you. How about this file? If you feel that this is too easy, please contact me. I can prepare a more difficult one for you.

[dolmen]

Coverage had dropped to 68%. Merging pull request that decrease code coverage should be blocked.

[utam0k]

Coverage had dropped to 68%. Merging pull request that decrease code coverage should be blocked.

It is possible for code coverage to drop as new files are added. There is a trade-off between that and increased test coverage. There are a few files that can easily increase code coverage. If you do that, you can easily reach 70%.
However, this issue is a means to an end, not an end in itself, to increase test coverage. The goal is to refactor the structure to be more testable. Think of 75% as just an indicator. There is no need to stop PR.
I certainly hope that you will make an effort to have unit tests added.

[chinzhiweiblank]

Hi! I would like to try my hand at this. I am intermediate in Rust. I don't know about container runtimes but I know about how containers work (cgroups, capability, unshare). Is there anything you could let me do? Thank you

[utam0k]

@chinzhiweiblank Thanks for your interest! I'm happy to hear that.
How about AppArmor for starters? If you want something more difficult, by all means, let me know.
https://app.codecov.io/gh/containers/youki/blob/main/crates/libcontainer/src/apparmor.rs
https://github.com/containers/youki/blob/main/crates/libcontainer/src/apparmor.rs

[chinzhiweiblank]

@chinzhiweiblank Thanks for your interest! I'm happy to hear that. How about AppArmor for starters? If you want something more difficult, by all means, let me know. https://app.codecov.io/gh/containers/youki/blob/main/crates/libcontainer/src/apparmor.rs https://github.com/containers/youki/blob/main/crates/libcontainer/src/apparmor.rs

No problem. I'll take this up.

[utam0k]

@chinzhiweiblank Thank you. I'm looking forward to seeing your PR.

[yihuaf]

Since I am responsible for many of the decreasing the coverage in the recent PRs, I want to take a look at this next.

[utam0k]

Hi @yihuaf Thanks for your hands-up, but if you are okay, I want you to try #1348.
#279 is the issue for beginners, but if you have a lot interested in #279, please continue.

[yihuaf]

@utam0k Understood. I will take a look at #1348. For #279, there are a number of fixes I already noticed, so I will fix them quickly. I won't go into it too much at this moment then.

[utam0k]

@yihuaf The reason why I'm asking you to do #1348 is that this feature is a new challenge in the Cloud Native community. I asked you to do this issue because I trust your imagination and implementation skills.
Of course, you don't need to hurry and don't feel too much pressure. I hope you will enjoy it.