-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
[Proposal] Capture coverage information in wasm-pack test #3774
Comments
Pinging @daxpedda |
Yo, exciting stuff! Is this already in a state where I can try it out? |
Not yet, the questions covered really stop this from being viable until they're resolved. I'll open a PR once it's in a more testable state. |
Would really like to figure out why
As you said: most of this should be moved to the runner. I don't think we need to use an "append" endpoint at all, we can just collect all this data in the runner and when it's done send it to the server. This looks good, I'm unsure how we will exactly expose it. Passing an experimental flag to A good entry in the Thank you for doing this. Looking forward to the PR! |
I'll document what I've tried and what I found (and how I found what I found) thoroughly in a separate issue and link TODOS in the code to the issue so that it can easily be picked up again. The issue could be as simple as that I didn't add the annotation in enough places but it could also be more complicated.
Agreed, I've updated the proposal. It's now working in chrome and firefox in both headless and not headless and node. I'm not sure how to invoke Deno as the runner but the code is the same as for node.
Does I would suggest an additional Will also add an experimental feature for coverage to Correct RUSTFLAGS, opt-in in the CLI and opt-in in the Cargo.toml should be sufficient for confirming that someone wants to try this.
Definitely. Near impossible to use otherwise 😅 |
Verifying the RUSTFLAGS on every run seems a bit excessive.If we are already printing a warning about using the coverage feature we could simply print a hint linking to the docs I'm going to add for in case they run into trouble. And the docs can say that if their case isn't covered they should open an issue here. |
Call it
Sounds good!
The flag has to be added in
Not sure what you mean, but I originally meant to make sure that users set the right |
The test runner currently gets all its arguments via environment variables, so I would just follow suit there and add take care of handling the command line args in wasm-pack. So
Yeah, you simply wouldn't find any coverage data. I'm personally not a fan of going through all the |
The wasm-bindgen/crates/test/src/rt/mod.rs Lines 295 to 314 in adcf778
And passed from here: wasm-bindgen/crates/cli/src/bin/wasm-bindgen-test-runner/main.rs Lines 198 to 215 in adcf778
So In any case, now that I think about it maybe we should use an environmental variable anyway and stick with only supporting the same args as the native Cargo test runner. Ideally in the future, we would figure out a way if coverage is enabled or not without any environment variable or flag at all. Not sure if there is currently a way to do this at all. Apologies btw, I don't have anything to do with
I think I have expressed myself very poorly here. What I meant to say is that we should check what happens if users enable coverage but don't pass the right How we would do that I have no clue, so if you have any ideas it would be nice to discuss that. If all that happens is that there is no coverage data, then that's ideal, no need to do anything here! But again: this is probably something we can leave for a follow-up. |
The line is commented with wasm-bindgen/crates/test/src/rt/mod.rs Lines 293 to 294 in adcf778
and is followed by wasm-bindgen/crates/test/src/rt/mod.rs Lines 296 to 300 in adcf778
and the test-runner main also says: wasm-bindgen/crates/cli/src/bin/wasm-bindgen-test-runner/main.rs Lines 51 to 52 in adcf778
So I think it's best to leave it as an environment variable for now.
That's fine. The wasm-pack changes are just parsing a flag. Everything else depends on what happens in wasm-bindgen. Getting the interface correct here is the most important - regardless of wasm-pack.
Ideally the profiler is built into the .wasm and is supported by LLVM. Then you could just use grcov to take care of everything since executing the file will simply passively generate .profraw files. (Ideally llvm-cov would also be able to parse the DWARF info out of the .wasm since it's actually in there saving us yet another step).
I understood that. I guess I'm the one who's being cryptic. Conveniently or infuriatingly, depending on how you want to look at it, you don't really get much output at all when trying to generate coverage data. If you have all the stuff in wasm-bindgen set up correctly but only forget to compile with the correct |
These comments are unfortunately outdated, we have since then already started supporting some flags. E.g.
Indeed we should still stick with an environment variable here to support the same CLI as the native Cargo test runner. That is until rust-lang/rfcs#3287 is implemented.
I see! Yeah that honestly sounds fine to me, I will leave if you want to add a warning here or not to you. |
So there are three things the user has to get right:
Here's the output of all the ways you can mix these up: No profiling, no feature, no flag: No profiling, no feature, flag: No profiling, feature, no flag: No profiling, feature, flag: Profiling, no feature, no flag: Profiling, no feature, flag: Profiling, feature, no flag: Profiling, feature, flag: (*) 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. |
Thank you for putting this together! |
Hell yeah, let's go! I just compiled RUSTFLAGS="-Cinstrument-coverage -Zno-profiler-runtime --emit=llvm-ir" ../wasm-pack-coverage/target/debug/wasm-pack test --chrome --headless --coverage --profraw-out wtest.profraw Missing feature [dev-dependencies]
wasm-bindgen-test = { git = "https://github.com/aDogCalledSpot/wasm-bindgen", branch = "coverage", features = [ "coverage" ] }
[dependencies]
wasm-bindgen = { git = "https://github.com/aDogCalledSpot/wasm-bindgen", branch = "coverage" }
# my other deps:
js-sys = "0.3.64"
tinyrand = "0.5.0"
lazy_static = "1.4.0"
derivative = "2.2.0" I think that's about right? Now running the same command gets me as far as this dependency error:
(Previously, I was also using Am I doing something wrong? From the looks of it we also need a fork of |
Try this: [dev-dependencies]
wasm-bindgen-test = { version = "0.3", features = ["coverage"] }
[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" }
wasm-bindgen-shared = { git = "https://github.com/aDogCalledSpot/wasm-bindgen", branch = "coverage" } |
This follows up on my work in: #2276
I have a very rough proof of concept and would like to discuss further implementation details before moving towards a PR.
The rough idea
profiler_builtins
aren't implemented for wasm, so we need to useminicov
minicov::capture_coverage
after the run of every testBuilding the code
If
minicov
is linked, thenRUSTFLAGS="-Cinstrument-coverage -Zno-profiler-runtime --emit=llvm-ir" wasm-pack test
is sufficient for getting the standard compiled wasm from the calledcargo build
.However,
wasm-interpreter
then runs over the file and tries to fetch all thewbgt_describe
s. Profiling information is already included in these. I made some attempts at using#[coverage(off)]
to avoid the generation of calls to the profiler in these functions but was unsuccessful. We can retry this approach if we have the rest working until then I propose the following workaround:The workaround
All fields related to profiling use i64. Since the interpreter disallows anything other than i32, we can ignore the i64s if we want to capture coverage (add some flag). Otherwise panic as usual.
This means we add the workaround for the instructions i64 const, i64 store, i64 load and i64 add.
Generating the profraw
Using
minicov::capture_coverage
works fine for getting the data for a single test. Printing the data to a file however doesn't work from a browser. Since the browser already connects to a small rouille server, I suggest creating two extra endpoints in that server:/__coverage/append
expects theVec<u8>
generated by minicov in the request bodyappends it to a veccoverage
should be called after every test/__coverage/dump
Vec<u8>
generated by minicov in the request bodyThe code is pretty straightforward here.
Merging the profraws and generating the HTML
I personally don't think we should be taking care of this. That way it should be possible to merge the coverage of normal
#[test]
unit tests as well, so that#[wasm_bindgen_test]
only needs to be used for code that runs only on wasm. I haven't been able to confirm this yet but I'll be looking into it.Open questions
Reporting the coverage to the server
The following code works:
Ideally, everything from the line with
let mut coverage...
would be handled automatically. I tried adding all of it toContext::execute
but for some reason when I remove the code from the test itself I get the error:TypeError: The specifier “env” was a bare specifier, but was not remapped to anything. Relative module specifiers must start with “./”, “../” or “/”.
The line causing the issue is immediately the first line of the generated JS file:
import * as __wbg_star0 from 'env';
The final dump can probably just be called with fetch from the test harness once all tests have completed.
Retrieving the server URL
We only know that the port is 8000 when running non-headless. In other cases we need to retrieve it and add it to the code here. This is probably easier if we do all the HTTP requests from the runner itself instead of from expanded tests, since the runner knows the server URL.
Gating coverage execution
Wanting to collect coverage now changes code in following places:
The server doesn't really matter since the endpoints will never be called.
The interpreter will need some sort of flag to inform it that it shouldn't ignore the i64s it comes across. We can probably just add that as a top level CLI argument.
Same as above for the runner.
The proc macro is the most difficult, if we can move all of the responsibility to the runner, then we have nothing to worry about. If some logic dependent on whether we want coverage or not remains in the expanded code then we will probably have to set an environment variable from the CLI tool to generate the correct code which is pretty ugly.
You can find my changes to this repo here: https://github.com/aDogCalledSpot/wasm-bindgen/tree/coverage
The text was updated successfully, but these errors were encountered: