search

taiki-e / cargo-llvm-cov

Cargo subcommand to easily use LLVM source-based code coverage (-C instrument-coverage).
Apache License 2.0
933 stars 57 forks source link

`-failure-mode` option? #90

Closed smoelius closed 3 years ago

smoelius commented 3 years ago

Would you consider a PR to pass a -failure-mode option to llvm-profdata merge? https://llvm.org/docs/CommandGuide/llvm-profdata.html#cmdoption-llvm-profdata-merge-failure-mode

taiki-e commented 3 years ago

Seems to make sense to have such an option.

smoelius commented 3 years ago

Cool. Thanks. I'll try to submit something soon.

smoelius commented 3 years ago

Sorry, I just noticed this: https://github.com/taiki-e/cargo-llvm-cov/blob/5fce8956d921c84e3119302cdc9e4ee006b4445d/src/env.rs#L14-L16 I can still submit a PR if you like. But since what I want can be achieved through CARGO_LLVM_PROFDATA_FLAGS, maybe adding the option isn't necessary?

taiki-e commented 3 years ago

I’m fine with either. The environment variable is to allow us to adjust the behavior of llvm-cov and llvm-profdata without changing the source of cargo-llvm-cov. However, if the flag is useful, it would make sense to add it to cargo-llvm-cov itself so that users can use it easily.

smoelius commented 3 years ago

OK. I'll still submit the PR then.

Question: I was planning to write at least one test to the effect of:

Where do you think a test like that should go?

taiki-e commented 3 years ago

I usually add tests using the following steps:

1. add test crate to tests/fixtures/crates. (or using existing test crate) example 2. add test to tests/test.rs that run target/debug/cargo-llvm-cov executable. example 3. run test. (the resulting coverage will be output to tests/fixtures/coverage-reports.) 4. if check on stderr or stdout, use stdout_contains or stderr_contains. example

Considering what kind of tests are needed for this feature, something similar to merge or clean_ws test will probably be needed.