Implement coverage output #3782

aDogCalledSpot · 2024-01-14T12:55:58Z

This follows up from #3774.

Fixes #2276.
Fixes #3774.

Overview

wasm-pack can set WASM_BINDGEN_TEST_COVERAGE to allow generating coverage data. WASM_BINDGEN_TEST_PROFRAW_OUT takes a path to specify where the generated .profraw file should be saved.

Test runners dump their coverage output after all tests have run. For browser tests, the data is sent to the server which can then dump the data into a profraw. For node/deno tests, the data is dumped directly to a file.

Merging the profraw files into a profdata and generating human-readable output (usually HTML) from the profdata is left to the user.

Changes

Interpreter changes

Generating profiling information causes i64s to be placed in the describe blocks that the interpreter parses. I tried adding #[coverage(off)] to these blocks to stop the profiling code from being generated but it didn't seem to be sufficient. I've kept comments about where I added these annotations in the diff here in case anyone has a suggestion of where else I could try adding them. All my attempts are in codegen.rs.

#[coverage(off)] isn't stabilized but I'm willing to open a stabilization PR if we are sure that it fixes our issues here. I'll be sure to document what I found more precisely in a separate issue to ensure the efforts aren't lost.

Until then, I added a coverage flag which allows skipping the instructions which are causing issues. If the flag isn't set, behavior is the same as before.

Dumping the coverage

The coverage is dumped through the __wbgtest_cov_dump() command which requires enabling the coverage feature of wasm-bindgen-test.

Test runner changes

Browsers

The clients call __wbgtest_cov_dump and send the resulting data to the server. The server has an additional endpoint which writes coverage to a file.

Node/Deno

The output is gathered and is dumped to a file directly.

Usage

It's easiest to test if you also use my fork of wasm-pack (I'll follow-up with a PR there once we're happy with how everything works here).

Enable the coverage feature of the wasm-bindgen-test dependency.

# Build for use with minicov
RUSTFLAGS="-Cinstrument-coverage -Zno-profiler-runtime --emit=llvm-ir" wasm-pack test --chrome --headless --coverage --profraw-out wtest.profraw
# Generate a {file_name}.o which contains the mapping of wasm instructions
# to source code lines.
clang target/wasm32-unknown-unknown/debug/deps/{file_name}.ll -Wno-override-module -c -o {file_name}.o
# Merge all profraws into a profdata
llvm-profdata merge --sparse wtest.profraw -o coverage.profdata
# Build HTML output from the profdata
llvm-cov show --instr-profile=coverage.profdata {file_name}.o --format=html --output-dir=coverage/ --sources .

Possible mistakes you can make:

So there are three things the user has to get right:

RUSTFLAGS need to be set correctly when building in order to generate profiling information (profiling)
WASM_BINDGEN_TEST_COVERAGE environment variable needs to be set (flag)
coverage feature in wasm_bindgen_test needs to enabled (feature)

Here's the output of all the ways you can mix these up:

No profiling, no feature, no flag:
Fine, just as before

No profiling, no feature, flag:
Prints error saying that coverage was supposed to be dumped but feature disabled

No profiling, feature, no flag:
Fine, just as before. Note that you might have issues if you have a crate depending on wasm-bindgen-test that also has minicov as a dependency since you'll have multiple definitions of functions. This is why we have the feature.

No profiling, feature, flag:
Prints error saying that empty coverage data was received. Says what RUSTFLAGS to use (I wanted to have this as a warning but warnings don't seem to be forwarded to the command line in headless mode)

Profiling, no feature, no flag:
wasm-bindgen-interpreter will panic. Prints a hint. (*)

Profiling, no feature, flag:
TypeError: The specifier "env" was a bare specifier
(The JS interpreter can't find the calls to the profiling information)(**)

Profiling, feature, no flag:
wasm-bindgen-interpreter will panic (somewhere else for some reason). Prints a hint.(***)

Profiling, feature, flag:
You get coverage!

(*) If we don't need the workaround in the interpreter, I'm pretty sure this would just take longer to compile but run fine

(**) A better message here would be great but I don't know how to detect this since the problem occurs at link time but it is only printed at runtime.

(***) If we don't need the workaround in the interpreter, this would probably also be fine.

TODOs

Write an issue documenting the issues with #[coverage(off)] and link to it where appropriate
Write an entry to the book documenting this usage and the possible mistakes you can make and link to it where appropriate

daxpedda

Looking forward to see your report on what the hell is going on with #[coverage(off)].

crates/backend/src/codegen.rs

daxpedda · 2024-01-15T09:39:35Z

crates/cli/src/bin/wasm-bindgen-test-runner/main.rs

+        TestMode::Node => node::execute(module, &tmpdir, &args, &tests, coverage)?,
+        TestMode::Deno => deno::execute(module, &tmpdir, &args, &tests, coverage)?,


I can't really review Node or Deno stuff, so I will be pulling in another maintainer when we get there.

I think this is the only thing missing.

crates/cli/src/bin/wasm-bindgen-test-runner/main.rs

crates/cli/src/bin/wasm-bindgen-test-runner/server.rs

crates/test/Cargo.toml

crates/test/src/lib.rs

daxpedda · 2024-01-15T10:57:24Z

On that not, are you sure t hat llvm-cov can't get the debug data from the Wasm file?
Right now the wasm-bindgen-test-runner actually throws away all the debug info, so did you try keeping it and see how llvm-cov reacts then?

EDIT:

wasm-bindgen/crates/cli/src/bin/wasm-bindgen-test-runner/main.rs

Line 192 in 8797e93

.keep_debug(false)

egasimus · 2024-01-15T11:31:10Z

(Tested this on my project and got dependency error around console_error_panic_hook which still seems to depend on the main wasm-bindgen repo, see #3774 (comment))

aDogCalledSpot · 2024-01-15T11:59:03Z

On that not, are you sure t hat llvm-cov can't get the debug data from the Wasm file? Right now the wasm-bindgen-test-runner actually throws away all the debug info, so did you try keeping it and see how llvm-cov reacts then?

EDIT:

wasm-bindgen/crates/cli/src/bin/wasm-bindgen-test-runner/main.rs

Line 192 in 8797e93

.keep_debug(false)

Yes. I played around with that quite a bit. I discarded all the changes regarding "keep_debug" since it wasn't working.

daxpedda · 2024-01-15T12:02:39Z

(Tested this on my project and got dependency error around console_error_panic_hook which still seems to depend on the main wasm-bindgen repo, see #3774 (comment))

@egasimus I recommend using the patch section instead of overwriting dependencies directly.
E.g. I used the following:

[patch.crates-io]
wasm-bindgen-test = { git = "https://github.com/aDogCalledSpot/wasm-bindgen", branch = "coverage" }
wasm-bindgen = { git = "https://github.com/aDogCalledSpot/wasm-bindgen", branch = "coverage" }
js-sys = { git = "https://github.com/aDogCalledSpot/wasm-bindgen", branch = "coverage" }
web-sys = { git = "https://github.com/aDogCalledSpot/wasm-bindgen", branch = "coverage" }
wasm-bindgen-futures = { git = "https://github.com/aDogCalledSpot/wasm-bindgen", branch = "coverage" }

aDogCalledSpot · 2024-01-16T11:09:51Z

I've typed up a report on the #[coverage(off)] stuff. I'll turn it into an issue once this is ready to merge otherwise: https://gist.github.com/aDogCalledSpot/509309bb2aa4b251ce4d497bca9baf40

njelich · 2024-01-16T12:10:42Z

Really glad that the wasmcov method is spreading :) Would be nice if we got an attribution in contributions using the workaround.

aDogCalledSpot · 2024-01-16T12:39:19Z

Really glad that the wasmcov method is spreading :) Would be nice if we got an attribution in contributions using the workaround.

I honestly wasn't aware of wasmcov until now. Looking at it now I also don't feel as if it addresses the main contributions of this PR:

Skipping over the issues in the interpreter (as unelegant as it may be)
Sending the coverage data to the server and writing it to a profraw from there

Using the .ll file to extract the debug info was taken from https://github.com/hknio/code-coverage-for-webassembly which appears to predate wasmcov by a few weeks.

daxpedda · 2024-01-16T12:43:27Z

Using the .ll file to extract the debug info was taken from https://github.com/hknio/code-coverage-for-webassembly which appears to predate wasmcov by a few weeks.

I think adding some attribution to https://github.com/hknio/code-coverage-for-webassembly would be nice!

aDogCalledSpot · 2024-01-16T12:46:48Z

Looking more closely it seems as if the auhor of the page I linked is affiliated with wasmcov (all working for the same company).

There isn't any code specific to the workaround in this PR since this would be a manual step done after running the tests. I'll add an attribution to the documentation in the book where I explain how to use this feature.

daxpedda

Apologies, most of my comments are just cleanup.

I think it would be good to also wire up the strategy I talked about in https://gist.github.com/aDogCalledSpot/509309bb2aa4b251ce4d497bca9baf40?permalink_comment_id=4833636#gistcomment-4833636, even if we can't solve it before a merge, presumably getting #[coverage(off)] on some functions is still a plus.

crates/cli/src/bin/wasm-bindgen-test-runner/main.rs

crates/cli/src/bin/wasm-bindgen-test-runner/server.rs

crates/test/src/rt/mod.rs

crates/wasm-interpreter/src/lib.rs

crates/test/src/coverage.rs

crates/wasm-interpreter/src/lib.rs

njelich · 2024-01-16T14:47:50Z

@aDogCalledSpot You know, it was a few weeks between when we came up with it, and when we gave it a name :p Thanks for the attribution :) Glad to be useful to the community at large.

njelich · 2024-01-16T15:00:23Z

@aDogCalledSpot Btw, for a lot of the stuff that needs to be done (merging prof raw files, processing, etc - I am exposing those functions in wasmcov - its meant to be an easy to use library, so maybe you will find it useful for wasm-bindgen?)

Same goes for the initial environment setup.

aDogCalledSpot · 2024-01-16T15:12:54Z

I'm afraid I'm a bit lost with the feature. Even in a minimal example I can't seem to make it do what I would want to:

Do I understand correctly that you would want this to compile:

#[allow_internal_unstable(coverage_attribute)]
#[coverage(off)]
pub fn foo() -> u32 {
    5
}

because this still complains about coverage being an experimental feature.

aDogCalledSpot · 2024-01-16T15:28:02Z

@aDogCalledSpot Btw, for a lot of the stuff that needs to be done (merging prof raw files, processing, etc - I am exposing those functions in wasmcov - its meant to be an easy to use library, so maybe you will find it useful for wasm-bindgen?)

I think it's out of scope for wasm-bindgen. I don't think there should be any more happening here in terms of test than native cargo test can do. I'm looking to implement more of the handling in cargo-llvm-cov instead.

crates/backend/src/codegen.rs

aDogCalledSpot · 2024-01-18T08:50:16Z

Thanks for elaborating ❤️

It works when using the nightly compiler now but the CI uses stable which is causing it to fail on the clippy tests. Should we make clippy use the nightly compiler in the CI?

aDogCalledSpot · 2024-01-18T08:57:41Z

For the book I'll cover it quite briefly and refer to the wasm-pack docs where I'll cover it in more depth since that's what seems to be done for the other arguments too.

daxpedda · 2024-01-18T09:14:58Z

It works when using the nightly compiler now but the CI uses stable which is causing it to fail on the clippy tests. Should we make clippy use the nightly compiler in the CI?

Actually this is a bit problematic now that I think about it ... we might not want to do it with a crate feature after all, because --all-features worked before but now it wouldn't.

I think we will have to switch to using cfg(web_sys_unstable_test_coverage), similarly to how cfg(web_sys_unstable_apis) is used now.

For the book I'll cover it quite briefly and refer to the wasm-pack docs where I'll cover it in more depth since that's what seems to be done for the other arguments too.

Unfortunately I (as a maintainer) have no relation to wasm-pack so I don't want to rely on it that much (maybe other maintainers can chime in on this), but would prefer if we have a complete documentation in wasm-bindgen, it doesn't have to be in depth.

crates/backend/src/lib.rs

crates/test/src/lib.rs

aDogCalledSpot · 2024-01-18T10:51:58Z

I think we will have to switch to using cfg(web_sys_unstable_test_coverage), similarly to how cfg(web_sys_unstable_apis) is used now.

I feel like this adds a really large hurdle for the very small benefit of not generating the coverage data for these functions which also doesn't solve the issues it's supposed to. By using the feature flag you can already opt into getting the profraw even if the macros generate coverage output.

I would prefer to drop this commit from the PR and instead add a reference to the commit in my gist/follow-up issue. That way anyone working on this issue can check it out and compile with nightly and then easily continue where I left off.

daxpedda · 2024-01-18T10:56:35Z

I would prefer to drop this commit from the PR and instead add a reference to the commit in my gist/follow-up issue.

Keep in mind that when we solve this we will still have to switch away from the crate feature breaking everybodies workflow.

Considering this is an unstable feature I'm fine with that though.

guide/src/wasm-bindgen-test/coverage.md

daxpedda · 2024-01-19T11:24:18Z

guide/src/wasm-bindgen-test/coverage.md

+
+[1]: usage.html#appendix-using-wasm-bindgen-test-without-wasm-pack
+
+- `WASM_BINDGEN_UNSTABLE_TEST_COVERAGE` to generate a single `.profraw` in your current working directory.


This is to enable it, right?

daxpedda · 2024-01-19T11:25:49Z

guide/src/wasm-bindgen-test/coverage.md

+```sh
+RUSTFLAGS="-Cinstrument-coverage -Zno-profiler-runtime --emit=llvm-ir" wasm-pack test --coverage --profraw-out cov_data/
+# Generate the debug info on the host
+clang target/wasm32-unknown-unknown/debug/deps/{your-dot-wasm-without-extension}.ll -Wno-override-module -c -o wasm.o


I assume {your-dot-wasm-without-extension} wants the test file, not the library file?
Would be nice if that could be clearer.

daxpedda · 2024-01-19T12:18:29Z

So I managed to make it work with #[coverage(off)]: daxpedda@a81f944.
I probably went a bit overboard with tagging functions, but at least all fn describe*() functions have to be tagged.
The commit obviously can't be applied as-is because I didn't guard the #![feature(coverage_attributes)] nor the #[coverage(off)] attributes.

I also figured out what the issue is: https://rust.godbolt.org/z/YTdPKqPj3.
Apparently #[coverage(off)] doesn't account for inlining functions.
(see rust-lang/rust#84605 (comment))

Would you mind trying it and see if it works for you as well?
I'm happy to make a PR on your branch and fix it up properly.

aDogCalledSpot · 2024-01-19T15:26:27Z

This does indeed work. I tried moving the feature to a cfg option like web_sys_unstable_apis but I'm unsure about the usage because passing the cfg option to RUSTFLAGS doesn't seem to pass it through when specifying a different target. I can't get it to work for web_sys_unstable_apis either.

RUSTFLAGS="--cfg=unstable_coverage" cargo build --target wasm32-unknown-unknown doesn't work
RUSTFLAGS="--cfg=unstable_coverage" cargo build works
CARGO_BUILD_TARGET="wasm32-unknown-unknown" RUSTFLAGS="--cfg=unstable_coverage" cargo build doesn't work either.

daxpedda · 2024-01-19T15:33:32Z

The problem is probably that wasm-bindgen-test-runner doesn't read RUSTFLAGS, so it has to be an environment variable.

We might want to use an environment variable for wasm-bindgen-macro as well?

If that doesn't work we are not gonna get around having an environment variable for wasm-bindgen-test-runner and a cfg for wasm-bindgen-macro.

I know all this is a bit unfortunate, but I can't come up with a better solution, I'm all ears if you have one.

aDogCalledSpot · 2024-01-22T12:57:58Z

The problem is probably that wasm-bindgen-test-runner doesn't read RUSTFLAGS, so it has to be an environment variable.

I don't think so, since RUSTFLAGS="cfg=unstable_coverage" cargo build --target wasm32-unknown-unknown shouldn't run wasm-bindgen-test-runner.

I noticed that the flag is being passed in correctly to wasm-bindgen-test which pulls in wasm-bindgen as a dependency (also sees the cfg) which, in turn, pulls in wasm-bindgen-macro which doesn't see the cfg. The whole path down from there doesn't appear to recognize the flag.

aDogCalledSpot · 2024-01-27T10:38:29Z

I had a brief look at why llvm-cov can't find the debug info. The issue isn't the DWARF info. llvm-dwarfdump hello_world.wasm works fine. The problem is that LLVM looks for a section called __llvm_prf_names and doesn't find it in the wasm.

So the issue arises at build time.

daxpedda · 2024-03-15T07:40:50Z

I'm going to close this due to inactivity.
It is going to continue to be tracked in #2276.

This is a highly desired feature and I would be happy to review it if somebody wants to continue the work done here!

This was referenced Jan 14, 2024

[Proposal] Capture coverage information in wasm-pack test #3774

Open

Support for Wasm? taiki-e/cargo-llvm-cov#221

Open

[Proposal] Add support for wasm-pack test taiki-e/cargo-llvm-cov#337

Open

daxpedda reviewed Jan 15, 2024

View reviewed changes

aDogCalledSpot force-pushed the coverage branch 2 times, most recently from 49d083b to 945d0aa Compare January 16, 2024 12:54

aDogCalledSpot requested a review from daxpedda January 16, 2024 12:55

daxpedda requested changes Jan 16, 2024

View reviewed changes

aDogCalledSpot force-pushed the coverage branch 5 times, most recently from 8e0d77d to 2c118b3 Compare January 16, 2024 14:27

aDogCalledSpot added 2 commits January 16, 2024 15:55

Add console_warn and console_error macros

b2d43e9

Implement coverage output

c850010

Address requested changes

76c48f1

aDogCalledSpot force-pushed the coverage branch from 2c118b3 to 219fbd9 Compare January 16, 2024 16:52

daxpedda reviewed Jan 17, 2024

View reviewed changes

crates/backend/src/codegen.rs Outdated Show resolved Hide resolved

aDogCalledSpot force-pushed the coverage branch from 78e5f6d to 8cdd467 Compare January 18, 2024 08:37

daxpedda reviewed Jan 18, 2024

View reviewed changes

crates/backend/src/lib.rs Outdated Show resolved Hide resolved

crates/test/src/lib.rs Show resolved Hide resolved

aDogCalledSpot and others added 2 commits January 18, 2024 11:56

Ensure interoperability with normal cargo test

85c93f0

add profraw prefix

88d5b68

aDogCalledSpot force-pushed the coverage branch 2 times, most recently from 8ff63f6 to 88d5b68 Compare January 18, 2024 10:59

cargo fmt

e8f74a2

aDogCalledSpot mentioned this pull request Jan 18, 2024

Add coverage handling rustwasm/wasm-pack#1362

Draft

daxpedda reviewed Jan 19, 2024

View reviewed changes

guide/src/wasm-bindgen-test/coverage.md Outdated Show resolved Hide resolved

Document coverage in the guide

b7f8bfc

aDogCalledSpot force-pushed the coverage branch from f5a7516 to b7f8bfc Compare January 19, 2024 10:43

daxpedda reviewed Jan 19, 2024

View reviewed changes

daxpedda mentioned this pull request Jan 19, 2024

Tracking issue for function attribute #[coverage] rust-lang/rust#84605

Open

daxpedda mentioned this pull request Jan 25, 2024

Release v0.2.91 #3812

Closed

daxpedda added the waiting for author Waiting for author to respond label Feb 28, 2024

daxpedda closed this Mar 15, 2024

daxpedda mentioned this pull request Mar 15, 2024

Add support to execute tests in workerd #3891

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement coverage output #3782

Implement coverage output #3782

aDogCalledSpot commented Jan 14, 2024 •

edited

daxpedda left a comment

daxpedda Jan 15, 2024

aDogCalledSpot Jan 19, 2024

daxpedda commented Jan 15, 2024 •

edited

egasimus commented Jan 15, 2024

aDogCalledSpot commented Jan 15, 2024

daxpedda commented Jan 15, 2024

aDogCalledSpot commented Jan 16, 2024

njelich commented Jan 16, 2024

aDogCalledSpot commented Jan 16, 2024

daxpedda commented Jan 16, 2024

aDogCalledSpot commented Jan 16, 2024 •

edited

daxpedda left a comment

njelich commented Jan 16, 2024 •

edited

njelich commented Jan 16, 2024 •

edited

aDogCalledSpot commented Jan 16, 2024 •

edited

aDogCalledSpot commented Jan 16, 2024

aDogCalledSpot commented Jan 18, 2024 •

edited

aDogCalledSpot commented Jan 18, 2024 •

edited

daxpedda commented Jan 18, 2024

aDogCalledSpot commented Jan 18, 2024

daxpedda commented Jan 18, 2024 •

edited

daxpedda Jan 19, 2024

daxpedda Jan 19, 2024 •

edited

daxpedda commented Jan 19, 2024 •

edited

aDogCalledSpot commented Jan 19, 2024

daxpedda commented Jan 19, 2024

aDogCalledSpot commented Jan 22, 2024

aDogCalledSpot commented Jan 27, 2024

daxpedda commented Mar 15, 2024

		TestMode::Node => node::execute(module, &tmpdir, &args, &tests, coverage)?,
		TestMode::Deno => deno::execute(module, &tmpdir, &args, &tests, coverage)?,


		[1]: usage.html#appendix-using-wasm-bindgen-test-without-wasm-pack

		- `WASM_BINDGEN_UNSTABLE_TEST_COVERAGE` to generate a single `.profraw` in your current working directory.

Implement coverage output #3782

Implement coverage output #3782

Conversation

aDogCalledSpot commented Jan 14, 2024 • edited

Overview

Changes

Interpreter changes

Dumping the coverage

Test runner changes

Browsers

Node/Deno

Usage

Possible mistakes you can make:

TODOs

daxpedda left a comment

Choose a reason for hiding this comment

daxpedda Jan 15, 2024

Choose a reason for hiding this comment

aDogCalledSpot Jan 19, 2024

Choose a reason for hiding this comment

daxpedda commented Jan 15, 2024 • edited

egasimus commented Jan 15, 2024

aDogCalledSpot commented Jan 15, 2024

daxpedda commented Jan 15, 2024

aDogCalledSpot commented Jan 16, 2024

njelich commented Jan 16, 2024

aDogCalledSpot commented Jan 16, 2024

daxpedda commented Jan 16, 2024

aDogCalledSpot commented Jan 16, 2024 • edited

daxpedda left a comment

Choose a reason for hiding this comment

njelich commented Jan 16, 2024 • edited

njelich commented Jan 16, 2024 • edited

aDogCalledSpot commented Jan 16, 2024 • edited

aDogCalledSpot commented Jan 16, 2024

aDogCalledSpot commented Jan 18, 2024 • edited

aDogCalledSpot commented Jan 18, 2024 • edited

daxpedda commented Jan 18, 2024

aDogCalledSpot commented Jan 18, 2024

daxpedda commented Jan 18, 2024 • edited

daxpedda Jan 19, 2024

Choose a reason for hiding this comment

daxpedda Jan 19, 2024 • edited

Choose a reason for hiding this comment

daxpedda commented Jan 19, 2024 • edited

aDogCalledSpot commented Jan 19, 2024

daxpedda commented Jan 19, 2024

aDogCalledSpot commented Jan 22, 2024

aDogCalledSpot commented Jan 27, 2024

daxpedda commented Mar 15, 2024

aDogCalledSpot commented Jan 14, 2024 •

edited

daxpedda commented Jan 15, 2024 •

edited

aDogCalledSpot commented Jan 16, 2024 •

edited

njelich commented Jan 16, 2024 •

edited

njelich commented Jan 16, 2024 •

edited

aDogCalledSpot commented Jan 16, 2024 •

edited

aDogCalledSpot commented Jan 18, 2024 •

edited

aDogCalledSpot commented Jan 18, 2024 •

edited

daxpedda commented Jan 18, 2024 •

edited

daxpedda Jan 19, 2024 •

edited

daxpedda commented Jan 19, 2024 •

edited