Discussion for addition of Code coverage in integration tests

containers / youki

A container runtime written in Rust

https://containers.github.io/youki/

Apache License 2.0

6.27k stars 341 forks source link

Discussion for addition of Code coverage in integration tests #316

Open YJDoc2 opened 3 years ago

YJDoc2 commented 3 years ago

Currently we use cargo tests along with cargo-llvm-cov in CI to generate code coverage report. Even though it provides a good way to measure coverage, currently not all functions are tested, and in fact some are very hard to test.

Thus, should we think of using the OCI validation tests as well to generate the data? I asked the maintainer of cargo-llvm-cov if taking coverage report from an external command such as validation tests would be possible, and they stated that even though currently there is no direct way to do this, it should be still possible : https://github.com/taiki-e/cargo-llvm-cov/issues/93

Some things we will need to consider with this are :

to take coverage the code needs to be compiled on nightly , along with extra stuff that is needed to emit code coverage information
currently we use release version to validate, as that will be the final version to be used, so switching to instrumentation version needed for coverage may not be good.
running integration test once again for coverage separately could be a good idea, but currently on average the tests take about 5 minutes to run. This will introduce a separate job in CI of about 5 min duration.
How much difference will this actually make in code coverage report

What are your opinions @utam0k @Furisto @yihuaf ?

utam0k commented 3 years ago

How about using debug builds for PR and other CI, and automatically running tests with release builds on the main branch once daily?

yihuaf commented 3 years ago

How about using debug builds for PR and other CI, and automatically running tests with release builds on the main branch once daily?

I think this is reasonable. I think debug build for CI, validation, and coverage. I don't think we need to run coverage again for release build.

utam0k commented 2 years ago

Rust 1.60 introduced a new feature for test coverage. Can we use it?

YJDoc2 commented 2 years ago

I think we can incorporate that, we might need an additional component to be installed in the CI grcov which converts the data generated to lcov format needed for coverage report. I'm looking into if we can directly generate lcov from cargo. I think the CI action that we use also have updated to the new feature, updating the action version in CI might be able to give us this as well.

taiki-e commented 2 years ago

Rust 1.60 introduced a new feature for test coverage. Can we use it?

cargo-llvm-cov (v0.2+) uses -C instrument-coverage that stabilized in 1.60. (v0.1 uses -Z instrument-coverage that is old version of -C instrument-coverage.

By the way, the feature mentioned in this issue has already been implemented as llvm show-env subcommand, It should be possible to use cargo-llvm-cov for integration testing. like:

source <(cargo llvm-cov show-env --export-prefix) # Set the environment variables needed to get coverage.
cargo llvm-cov clean --workspace # remove artifacts that may affect the coverage results
cargo build # build rust binaries
# commands using binaries in target/debug/*, including `cargo test`
# ...
cargo llvm-cov --no-run --lcov # generate report without tests

YJDoc2 commented 2 years ago

Hey, thanks a lot for this! I'll try to update according to this and adding integration tests into code coverage, although we might need to build them separately to the youki, as we don't want to consider them into code coverage. Thanks :)