-
Notifications
You must be signed in to change notification settings - Fork 149
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
Add a command "cargo grcov" #326
Comments
Would this be as a separate crate? Could we put it in a sub-dir still of this repo? Happy to do an initial PR to get this going if there's appetite? |
The --open would open the html results in a browser. That's kinda what I was thinking. Does it tally with your mozilla-internal spec? |
@gilescope that sounds good to me, it could be part of this repo. An additional feature could be uploading to the coverage services. |
@gilescope would you still be interested in contributing? |
I think it's high time this was done and I've got some time on my hands. I've done similar in other places but really having it in one place would be excellent. I can start with a straw man PR and we can iterate from there? |
(unless you've got a burning desire to have a crack yourself @marco-c - I'm good with that too. I'd be just as happy testing a PR out and giving feebdback. |
@gilescope that'd be great, thanks!
I'm happy to just review ;) |
Ok here's my take on things: I don't think that one command is enough. I think two are required:
By breaking it into two separate commands it enables people to run whatever testing framework(s) they wish in between (for example Once all the tests are run then (Both commands should support --message-format=json for machine readable output like most of the cargo commands do) Ideally for the simplest case something like this should be enough:
Are people comfortable with the above? Have I missed some common usecases that this wouldn't work well for? |
Having two separate commands sounds good to me, as many projects might have different needs than just running I'd only do one thing differently: if |
I like the idea of that. Would one run |
I see these new |
Yep.
This would cover most use cases easily by running
@gilescope you can use LLVM_PROFDATA_FILE as we do in .travis.yml (it's your best bet also to avoid some problems with multiple processes writing to the same profraw file) |
Obviously I'm keen on as little configuration as we can get away with, but I suspect we might need a few arguments. Do we aim for as detailed coverage as we can do (i.e. source level if the llvm_tools rustup component is available/installed)? If we take |
I was thinking the first option, but maybe the second more clearly splits the grcov arguments from the |
What about +nightly ? If it's good enough for cargo then I guess we can have |
@marco-c for tests I'm tempted to use command_access feature on nightly so I can see what the env vars set are (and only assert those bits of the test when run on nightly). But seems the only nice way to do that is to define a 'nightly' feature to know if it's nightly or not. If that's ok let me know. If not I'll ditch command_access and maybe wrap Command instead or maybe switchout the actual command to |
That seems a bit ugly :( |
Agreed - went for lots of quick integration tests, and looks like that's going to be fine as grcov is build on CI before it runs the tests so I can call it. |
It looks good, just make sure you default to have a custom It would be good to add a |
A custom target dir to avoid stomping - that's a great idea.
(To be honest it would be nice if rustc could hold a few different flavours
of builds in the target dir at once. It can handle check and build at the
same time without stomping, but it does definitely stomp at the moment when
we change rustcflags.)
…On Fri, Dec 4, 2020 at 8:28 AM Luca Barbato ***@***.***> wrote:
* `cargo grcov env -- BUILD_COMMAND` to setup the coverage env variables and run a user-defined command to build;
* `cargo grcov build` to setup the coverage env variables and run `cargo build`;
* `cargo grcov test` to setup the coverage env variables, run tests with `cargo test` and then parse resulting coverage artifacts;
* `cargo grcov report -- TEST_COMMAND` to run the user-defined command and then parse any resulting coverage artifacts.
It looks good, just make sure you default to have a custom target-dir so cargo
build and cargo grcov test do not stomp on each other.
It would be good to add a cargo grcov clean that wipes the matching
target-dir or all of them.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#326 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGEJCFWHDIBRIK76EOMRPLSTCMUNANCNFSM4I6BD4WQ>
.
|
Beware: changes in rustcflags would definitely invalidate everything almost all the time. Check is a conceptual subset of build even if right now it isn't exactly the case in practice ( |
True enough. Ideally we use the shiny new source based coverage, given that it's significantly closer to
I have lots of questions. I'm thinking if we run a bunch of tests in different test exes that get created then we would not have one but several. (We can get the json output of the tests so we can figure out the exes but seems messy). Is the location of the exe(s) found in the profraw file? Does grcov definitely only accept one bin for source based coverage? Are there some separate issues that are related that should be mentioned here? |
The workaround solution described at https://doc.rust-lang.org/nightly/unstable-book/compiler-flags/instrument-coverage.html#tips-for-listing-the-binaries-automatically can be used to get the list of binaries. |
My understanding was that
There is no one |
I think this is arguably just a shortcoming of grcov. It should allow Lines 44 to 59 in fb68ecc
That way we could always pass in both |
The multi |
Doesn't that suggest a performance issue in |
Yes, the feature is desirable IMO. My reason for reporting was that I could easily see What ended up working for me on this large project was to first manually use So since my working solution uses all the same pieces as grcov I'm suspicious that the performance issue might be in how grcov calls Lines 65 to 72 in fb68ecc
If I get time I'll see if I can track it down further. |
Regarding binary, see also #535. |
https://github.com/mozilla/productivity-tools-projects/issues/289:
The text was updated successfully, but these errors were encountered: