-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Implement coverage output #3782
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking forward to see your report on what the hell is going on with #[coverage(off)]
.
TestMode::Node => node::execute(module, &tmpdir, &args, &tests, coverage)?, | ||
TestMode::Deno => deno::execute(module, &tmpdir, &args, &tests, coverage)?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't really review Node or Deno stuff, so I will be pulling in another maintainer when we get there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the only thing missing.
On that not, are you sure t hat EDIT:
|
(Tested this on my project and got dependency error around |
Yes. I played around with that quite a bit. I discarded all the changes regarding "keep_debug" since it wasn't working. |
@egasimus I recommend using the [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" } |
I've typed up a report on the |
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
Using the |
I think adding some attribution to https://github.com/hknio/code-coverage-for-webassembly would be nice! |
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. |
49d083b
to
945d0aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
8e0d77d
to
2c118b3
Compare
@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. |
@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. |
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. |
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. |
2c118b3
to
219fbd9
Compare
78e5f6d
to
8cdd467
Compare
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? |
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. |
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 I think we will have to switch to using
Unfortunately I (as a maintainer) have no relation to |
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. |
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. |
8ff63f6
to
88d5b68
Compare
f5a7516
to
b7f8bfc
Compare
|
||
[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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to enable it, right?
```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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume {your-dot-wasm-without-extension}
wants the test file, not the library file?
Would be nice if that could be clearer.
So I managed to make it work with I also figured out what the issue is: https://rust.godbolt.org/z/YTdPKqPj3. Would you mind trying it and see if it works for you as well? |
This does indeed work. I tried moving the feature to a
|
The problem is probably that We might want to use an environment variable for If that doesn't work we are not gonna get around having an environment variable for 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. |
I don't think so, since I noticed that the flag is being passed in correctly to |
I had a brief look at why So the issue arises at build time. |
I'm going to close this due to inactivity. This is a highly desired feature and I would be happy to review it if somebody wants to continue the work done here! |
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 incodegen.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 thecoverage
feature ofwasm-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.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 inwasm_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 hasminicov
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
#[coverage(off)]
and link to it where appropriate