Skip to content
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

Fix generic code hit by doctests not correctly remapped #122

Closed
wants to merge 2 commits into from

Conversation

Swatinem
Copy link
Contributor

@Swatinem Swatinem commented Jan 7, 2022

cargo-llvm-cov currently uses --remap-path-prefix internally (not sure for the reason why?).
However that option is not recognized by rustdoc at the moment, which means that generic code that is instantiated in doctests is not counted towards the coverage, as it ends up in a "different" file as far as llvm tools are concerned.

I think in order to fix this, the flag needs to be supported in rustdoc as well, and I am exploring that option right now, so stay tuned.

relates to #2

I opened rust-lang/rust#92648 for the necessary changes in rustdoc, with which I generated the test snapshots in this PR, however I would like to somehow increase the confidence here a bit. Also I have seen that there is some code that does feature detection, and it might be a good idea to do that for this flag as well (and gate the test on the flag being available). Can you maybe point me in the right direction how to best do that?

@taiki-e
Copy link
Owner

taiki-e commented Jan 10, 2022

cargo-llvm-cov currently uses --remap-path-prefix internally (not sure for the reason why?).

IIRC, this was needed because sometimes macros are displayed with absolute paths in HTML reports.

cargo-llvm-cov/src/main.rs

Lines 205 to 206 in 30efe47

// --remap-path-prefix is needed because sometimes macros are displayed with absolute path
rustflags.push_str(&format!(" --remap-path-prefix {}/=", cx.ws.metadata.workspace_root));

I guess the underlying bug has not been fixed yet, but currently, the ignore_filename_regex excludes a few more directories, so I don't think those will be displayed in the report anyway. (That said, this flag may have been necessary to include those macros in the report.)

@Swatinem
Copy link
Contributor Author

Are you suggesting the flag might not be necessary after all?

@taiki-e
Copy link
Owner

taiki-e commented Feb 12, 2022

Are you suggesting the flag might not be necessary after all?

Yes

@Swatinem
Copy link
Contributor Author

uff… I tried to do that, but now all the snapshot tests have randomized paths in them. I give up 😓

@taiki-e
Copy link
Owner

taiki-e commented Mar 1, 2022

(I'm starting to think to provide an option to disable --remap-path-prefix: #140 (comment))

bors bot added a commit that referenced this pull request Apr 8, 2022
141: Make --remap-path-prefix optional r=taiki-e a=taiki-e

Fixes #140 

cc #122

Co-authored-by: Taiki Endo <te316e89@gmail.com>
@taiki-e
Copy link
Owner

taiki-e commented Sep 10, 2022

Closing as rust-lang/rust#92648 has been closed.

Also, --remap-path-prefix is optional since cargo-llvm-cov 0.3 (#141), so the problem may no longer be reproducible without the --remap-path-prefix option.

Thanks anyway for the PR!

@taiki-e taiki-e closed this Sep 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants