Code coverage support for tests (scrypto coverage command) #1640

bbarwikowski-hacken · 2023-10-31T19:04:05Z

Summary

This PR adds new command scrypto coverage which allows to create code coverage report from tests. It was implemented based on https://github.com/hknio/code-coverage-for-webassembly findings.

Please let me know what you think, in same cases I had to use ugly approach like:

/// The maximum invoke payload size.
#[cfg(not(feature = "coverage"))]
pub const MAX_INVOKE_PAYLOAD_SIZE: usize = 1 * 1024 * 1024;
#[cfg(feature = "coverage")]
pub const MAX_INVOKE_PAYLOAD_SIZE: usize = 32 * 1024 * 1024;

If you have better idea how it could be implemented, please let me know.

In the next commit I'll add test_coverage.sh script, the only problem is I'll need to install nightly version of rust and the same version of llvm as nightly rustc is using. Should I add it in Dockerfile or somewhere else?

iamyulong

Really nice work!

I managed to run scrypto new-package hello and scrypto coverage, after following all the hints for dependencies.

A coverage report was successfully generated, and the total coverage percentage seems to be right on index.html. However, the coverage report for the hello/src/lib.rs file is completely missed. It shows zero coverage.

iamyulong · 2023-11-06T09:13:08Z

radix-engine/src/vm/wasm/prepare.rs

-                reason: err.to_string(),
-            }
-        })?;
+        #[cfg(not(feature = "coverage"))]


Out of curiosity, did you run into any issue with gas metering ingestion for the coverage-enabled wasm code?

Yes. When transaction fails for example due to out of gas then dump_coverage cannot be called because it will fail due to out of gas. When this is commented, the function dump_coverage is not using any build-in native functions so it's going to work even in case of transaction failure.
The other option is to disable gas/native functions only when dump_coverage is called.

That makes sense. I was hoping it wasn't an error with the instrumentation itself, otherwise we have a problem. All is good.

iamyulong · 2023-11-06T09:14:44Z

radix-engine/src/transaction/transaction_executor.rs

+            usd_price: USD_PRICE_IN_XRD.try_into().unwrap(),
+            state_storage_price: Decimal::zero(),
+            archive_storage_price: Decimal::zero(),
+        }


Due to the difference in configuration, some tests may fail in coverage mode but not in regular test mode. I guess it's not a big deal for most cases. We will need to make sure the --no-fail-fast flag is provided when running coverage.

iamyulong · 2023-11-06T09:19:41Z

scrypto-unit/src/test_runner.rs

+                });
+                let definition = manifest_decode(&definition).unwrap_or_else(|err| {
+                    panic!(
+                        "Failed to parse manifest definition from path {:?} - {:?}",


Minor: manifest definition -> package definition

iamyulong · 2023-11-06T09:36:52Z

simulator/src/scrypto/cmd_coverage.rs

+        if !is_nightly {
+            println!("Coverage tool requries nightly version of rust toolchain");
+            println!("You can install it by using the following commands:");
+            println!("rustup default nightly");


Wonder if it's possible to use nightly for coverage only?

Yes, it could use "cargo +nightly" when building package, but there may be some issues when someone is using multiple nightly versions. Anyway, I added this in the next commit.

bbarwikowski-hacken · 2023-11-10T12:33:41Z

I managed to run scrypto new-package hello and scrypto coverage, after following all the hints for dependencies.

A coverage report was successfully generated, and the total coverage percentage seems to be right on index.html. However, the coverage report for the hello/src/lib.rs file is completely missed. It shows zero coverage.

@iamyulong This should not happened, in my case helo/src/lib.rs has correct coverage report. Did you update dependencies in cargo.toml to use version with coverage? If you did and it didn't work then please contact me to find out why it happened.

bbarwikowski-hacken · 2023-11-10T13:14:37Z

simulator/src/scrypto/cmd_coverage.rs

+
+        // Build package
+        let path = self.path.clone().unwrap_or(current_dir().unwrap());
+        let (wasm_path, _) = build_package(&path, false, false, true, Level::Trace, true)


@iamyulong This approach, with building package before running tests and then reusing it makes tests way faster. Here's my recommendation regarding scrypto test:

Build all packages in advance, including dependencies (this may be hard when it comes to dependencies)

Store information what has been build in ENV, you can also store checksum and build options

In test_runner.rs try to use cached version, if it doesn't exist then build it and eventually cache it somehow (this may be hard because requires synchronization)

This is generally a good idea. 👍

I believe the cargo build automatically caches previous builds and avoids re-compiling if not updates have been made, right? However, we can potentially speed up tests by caching package publishing processing and genesis bootstrap.

This is definitely something that we need to measure and improve.

Yeah it does, but it's slow and requires other tests to wait when using multiple workers/threads

iamyulong · 2023-11-10T13:30:15Z

Hi @bbarwikowski-hacken, thanks for sharing the screenshot!

I actually got the same HTML report as yours. I think I was confused by it only showing the hit counts, rather than highlighting covered lines green and uncovered lines red. Any chance we can improve this a bit? Not a blocker and I think the PR is ready for merging. :)

bbarwikowski-hacken · 2023-11-10T13:34:25Z

Hi @bbarwikowski-hacken, thanks for sharing the screenshot!

I actually got the same HTML report as yours. I think I was confused by it only showing the hit counts, rather than highlighting covered lines green and uncovered lines red. Any chance we can improve this a bit? Not a blocker and I think the PR is ready for merging. :)

The problem is that hello package has 100% code coverage so there's nothing to highlight 😆 This is how it looks in case of not covered code.

Added initial coverage support (scrypto coverage command)

552d292

bbarwikowski-hacken changed the title ~~Initial code coverage support for tests (scrypto coverage command)~~ Code coverage support for tests (scrypto coverage command) Oct 31, 2023

bbarwikowski-hacken marked this pull request as ready for review October 31, 2023 19:12

bbarwikowski-hacken marked this pull request as draft October 31, 2023 19:13

iamyulong reviewed Nov 6, 2023

View reviewed changes

bbarwik added 2 commits November 10, 2023 13:43

Improved scrypto coverage command

0b1cfa7

Added test for scrypto coverage command

c9e251d

bbarwikowski-hacken marked this pull request as ready for review November 10, 2023 13:01

bbarwikowski-hacken requested a review from iamyulong November 10, 2023 13:06

bbarwikowski-hacken commented Nov 10, 2023

View reviewed changes

iamyulong approved these changes Nov 10, 2023

View reviewed changes

iamyulong merged commit 7c7389d into radixdlt:develop Nov 10, 2023
24 of 25 checks passed

0xOmarA mentioned this pull request Jan 21, 2024

Coverage reports can't be generated on MacOS hknio/wasmcov#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code coverage support for tests (scrypto coverage command) #1640

Code coverage support for tests (scrypto coverage command) #1640

bbarwikowski-hacken commented Oct 31, 2023 •

edited

iamyulong left a comment

iamyulong Nov 6, 2023

bbarwik Nov 10, 2023 •

edited

iamyulong Nov 10, 2023

iamyulong Nov 6, 2023

iamyulong Nov 6, 2023

iamyulong Nov 6, 2023

bbarwik Nov 10, 2023 •

edited

bbarwikowski-hacken commented Nov 10, 2023

bbarwikowski-hacken Nov 10, 2023

iamyulong Nov 10, 2023

bbarwikowski-hacken Nov 10, 2023

iamyulong commented Nov 10, 2023

bbarwikowski-hacken commented Nov 10, 2023 •

edited

Code coverage support for tests (scrypto coverage command) #1640

Code coverage support for tests (scrypto coverage command) #1640

Conversation

bbarwikowski-hacken commented Oct 31, 2023 • edited

Summary

iamyulong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbarwik Nov 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbarwik Nov 10, 2023 • edited

Choose a reason for hiding this comment

bbarwikowski-hacken commented Nov 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamyulong commented Nov 10, 2023

bbarwikowski-hacken commented Nov 10, 2023 • edited

bbarwikowski-hacken commented Oct 31, 2023 •

edited

bbarwik Nov 10, 2023 •

edited

bbarwik Nov 10, 2023 •

edited

bbarwikowski-hacken commented Nov 10, 2023 •

edited