Update instrument-coverage.md

[gftea]

explicitly set environment variable LLVM_PROFILE_FILE="default_%m.profraw" when starting cargo test, otherwise, only one default.profraw is generated and will be overwritten if run unnittests and integration tests at the same time.

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

[ehuss]

Thanks! Can you say more about under which circumstances it will only generate a single file? My understanding is that the default will disambiguate each file such as default_11566440541585291497_0_48944.profraw. The default should be set here.

[gftea]

my local env has rustc 1.63, and I run the tests with below commands

RUSTFLAGS="-C instrument-coverage -C codegen-units=1" cargo test -p amqprs --all-features --tests

the integration tests overwrite the unittests data, so read the coverage data using object file of unittests, it shows all zeros.

Once I set LLVM_PROFILE_FILE="default_%m.profraw" , then I get both coverage data for unittests and integration tests

[ehuss]

I believe it was changed in 1.65 via #100384. Can you try with a newer version?

[gftea]

even it works in later rustc version, it is still important to have that information otherwise users follow the guideline will encounter issues

[ehuss]

I think it would be great to make it a little clearer what the default is for LLVM_PROFILE_FILE if it is not specified. I don't think the documentation should encourage changing the default as a matter of course, as the default should be serviceable for most situations. The documentation already includes a note just below recommending using %m and %p. We generally don't document the behavior of older versions of the compiler, as it would eventually be overwhelming to have every part annotated with every change. However, in this situation, it might be worthwhile to mention that the default changed in 1.65. Perhaps something like "The default for LLVM_PROFILE_FILE is default_%m_%p.profraw (versions prior to 1.65 had a default of default.profraw)."

[gftea]

I agree with your points to be more specific, but for user on older verson than 1.65, it will cause incorrect behaviors without explicitly set the LLVM_PROFILE_FILE.
If user testing MSRV older than 1.65, then it will have false coverage reports according to guideline.
maybe we can add additonal info about the defaults, but I think it is better to have it explicitly set.

[ehuss]

I disagree, as I think it is best for the default experience to not imply that LLVM_PROFILE_FILE is necessary. We generally don't provide instructions on how to use older versions of Rust. It is unfortunate that older versions of Rust did not provide a better default (and was not well documented). The suggested text I provided should provide enough information if someone is using an older version.

[gftea]

@ehuss , can you review the latest changes based on your suggestion

[rustbot]

Could not assign reviewer from: ehuss.
User(s) ehuss are either the PR author or are already assigned, and there are no other candidates.
Use r? to specify someone else to assign.

[ehuss]

I posted some edits to clarify some of the wording and to use a consistent formatting. I also squashed the commits.

@bors r+ rollup

[bors]

📌 Commit c1f1f60 has been approved by ehuss

It is now in the queue for this repository.

[gftea]

I posted some edits to clarify some of the wording and to use a consistent formatting. I also squashed the commits.

thanks!