Remap instrument-coverage line numbers in doctests #79762
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @estebank |
camelid
added
the
S-waiting-on-review
richkadel
reviewed
Dec 7, 2020
There was a problem hiding this comment.
@Swatinem - This doctest_offset_line() call looks promising!
I know this is a Draft, but I commented on what you have so far.
I assume you'll want to add a test(s) in run-make-fulldeps/coverage/. Just run ./x.py test src/test/run-make-fulldeps/coverage --bless once you've written the new test, and it will generate the coverage results, etc.
| @@ -512,6 +508,8 @@ fn make_code_region( | |||
| } else { | |||
| source_file.lookup_file_pos(span.hi()) | |||
| }; | |||
| let start_line = source_map.doctest_offset_line(&source_file.name, start_line); | |||
There was a problem hiding this comment.
These look like the only pertinent changes in this file. Let me know if I'm missing something.
Very exciting if this works.
Swatinem
force-pushed
the
remap-doctest-coverage
branch
from
51438f9
to
0ce4b51
Compare
|
☔ The latest upstream changes (presumably #79867) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels: |
camelid
added
the
S-waiting-on-author
Swatinem
force-pushed
the
remap-doctest-coverage
branch
2 times, most recently
from
5188cca
to
2a7ef12
Compare
Swatinem
commented
Dec 11, 2020
| .map(|c| if c.is_ascii_alphanumeric() { c } else { '_' }) | ||
| .collect::<String>(); | ||
| let test_id = format!( | ||
| "{file}_{line}_{number}", |
There was a problem hiding this comment.
This basically reverts half of #79413 since rust-lang/cargo#8954 is a proper solution to this. CC @jyn514
There was a problem hiding this comment.
Sure, seems reasonable. I always want to do things through cargo instead of rustdoc where possible because cargo has more information available.
| @@ -32,12 +32,12 @@ Combined regions: | |||
| 10:5 -> 12:6 (count=1) | |||
| Segment at 10:5 (count = 1), RegionEntry | |||
| Segment at 12:6 (count = 0), Skipped | |||
| Emitting segments for function: _RNvXs_Cs4fqI2P2rA04_8genericsINtB4_8FireworklENtNtNtCs3rFBWs28XFJ_4core3ops4drop4Drop4dropB4_ | |||
| Emitting segments for function: _RNvXs_Cs4fqI2P2rA04_8genericsINtB4_8FireworklENtNtNtCs5T9nfiiJBPh_4core3ops4drop4Drop4dropB4_ | |||
There was a problem hiding this comment.
tbh, I have no idea where these changes are coming from…
There was a problem hiding this comment.
Probably the function name changed. This hash depends on quite a lot of things, if the name changed I wouldn't worry about it.
There was a problem hiding this comment.
Actually it's not a change, per se... The ordering has been swapped.
This is a problem with llvm-cov output. It generates output for multiple instantiations of generics in a non-deterministic order.
If the diff tests are going to fail because of this, we may need to make some changes.
But I think this should have already been a problem, in which case maybe it's already handled.
I'll take another look at this later.
There was a problem hiding this comment.
This should be OK. As mentioned in another comment, the tests don't fail if counters files don't match, for multiple reasons.
jyn514
added
A-code-coverage
richkadel
suggested changes
Dec 11, 2020
There was a problem hiding this comment.
Looks really cool so far.
I'll take another look at the issues with non-deterministic order in some llvm-cov test results. (A known issue that's HOPEFULLY already addressed, but generating the expected test results on Windows will highlight those issues for sure.)
src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_crate.txt
Show resolved
Hide resolved
src/test/run-make-fulldeps/coverage-reports/expected_show_coverage_counters.async.txt
Outdated
Show resolved
Hide resolved
| @@ -32,12 +32,12 @@ Combined regions: | |||
| 10:5 -> 12:6 (count=1) | |||
| Segment at 10:5 (count = 1), RegionEntry | |||
| Segment at 12:6 (count = 0), Skipped | |||
| Emitting segments for function: _RNvXs_Cs4fqI2P2rA04_8genericsINtB4_8FireworklENtNtNtCs3rFBWs28XFJ_4core3ops4drop4Drop4dropB4_ | |||
| Emitting segments for function: _RNvXs_Cs4fqI2P2rA04_8genericsINtB4_8FireworklENtNtNtCs5T9nfiiJBPh_4core3ops4drop4Drop4dropB4_ | |||
There was a problem hiding this comment.
Actually it's not a change, per se... The ordering has been swapped.
This is a problem with llvm-cov output. It generates output for multiple instantiations of generics in a non-deterministic order.
If the diff tests are going to fail because of this, we may need to make some changes.
But I think this should have already been a problem, in which case maybe it's already handled.
I'll take another look at this later.
|
☔ The latest upstream changes (presumably #80055) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels: |
|
Looks like you just need to run |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
👍 re-blessed the coverage tests and improved the code wrapping the doctest main, also adding more comments explaining the line offsets and its effect on code coverage. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
richkadel
reviewed
Dec 16, 2020
|
Looks great, except for the one earlier FYI, I don't have authority to approve PRs in the rust-lang/rust repo. It looks like @estebank was recommended. In case @estebank is not available for review, you could request a review (with Thanks for this solution!! |
|
☔ The latest upstream changes (presumably #80105) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels: |
|
I think this should have a rustdoc reviewer, but I don't know this code well enough. r? @GuillaumeGomez maybe? |
GuillaumeGomez
approved these changes
Dec 17, 2020
|
It definitely looks like a nice improvement! I didn't see anything problematic on rustdoc side (and discovered quite a lot of new test suites while reading this PR haha). Thanks! |
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Dec 22, 2020
…Swatinem Remap instrument-coverage line numbers in doctests This uses the `SourceMap::doctest_offset_line` method to re-map line numbers from doctests. Remapping columns is not yet done, and rustdoc still does not output the correct filename when running doctests in a workspace. Part of rust-lang#79417 although I dont consider that fixed until both filenames and columns are mapped correctly. r? ```@richkadel``` I might jump on zulip the comming days. Still need to figure out how to properly write tests for this, and deal with other doctest issues in the meantime.
|
@Swatinem in the future, please use |
|
Absolutely, I thought the "delegate" meant me. anyhow ;-) |
|
Ah yes, #79762 (comment) says
So that confused me :-D |
|
Actually it should be r=tmandry I think. I don't have approval authority. |
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Dec 22, 2020
…Swatinem Remap instrument-coverage line numbers in doctests This uses the `SourceMap::doctest_offset_line` method to re-map line numbers from doctests. Remapping columns is not yet done, and rustdoc still does not output the correct filename when running doctests in a workspace. Part of rust-lang#79417 although I dont consider that fixed until both filenames and columns are mapped correctly. r? `@richkadel` I might jump on zulip the comming days. Still need to figure out how to properly write tests for this, and deal with other doctest issues in the meantime.
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Dec 23, 2020
…Swatinem Remap instrument-coverage line numbers in doctests This uses the `SourceMap::doctest_offset_line` method to re-map line numbers from doctests. Remapping columns is not yet done, and rustdoc still does not output the correct filename when running doctests in a workspace. Part of rust-lang#79417 although I dont consider that fixed until both filenames and columns are mapped correctly. r? `@richkadel` I might jump on zulip the comming days. Still need to figure out how to properly write tests for this, and deal with other doctest issues in the meantime.
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Dec 24, 2020
…Swatinem Remap instrument-coverage line numbers in doctests This uses the `SourceMap::doctest_offset_line` method to re-map line numbers from doctests. Remapping columns is not yet done, and rustdoc still does not output the correct filename when running doctests in a workspace. Part of rust-lang#79417 although I dont consider that fixed until both filenames and columns are mapped correctly. r? ``@richkadel`` I might jump on zulip the comming days. Still need to figure out how to properly write tests for this, and deal with other doctest issues in the meantime.
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Dec 24, 2020
…Swatinem Remap instrument-coverage line numbers in doctests This uses the `SourceMap::doctest_offset_line` method to re-map line numbers from doctests. Remapping columns is not yet done, and rustdoc still does not output the correct filename when running doctests in a workspace. Part of rust-lang#79417 although I dont consider that fixed until both filenames and columns are mapped correctly. r? ```@richkadel``` I might jump on zulip the comming days. Still need to figure out how to properly write tests for this, and deal with other doctest issues in the meantime.
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Dec 24, 2020
…Swatinem Remap instrument-coverage line numbers in doctests This uses the `SourceMap::doctest_offset_line` method to re-map line numbers from doctests. Remapping columns is not yet done, and rustdoc still does not output the correct filename when running doctests in a workspace. Part of rust-lang#79417 although I dont consider that fixed until both filenames and columns are mapped correctly. r? ````@richkadel```` I might jump on zulip the comming days. Still need to figure out how to properly write tests for this, and deal with other doctest issues in the meantime.
|
☀️ Test successful - checks-actions |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This uses the
SourceMap::doctest_offset_linemethod to re-map linenumbers from doctests. Remapping columns is not yet done, and rustdoc
still does not output the correct filename when running doctests in a
workspace.
Part of #79417 although I dont consider that fixed until both filenames
and columns are mapped correctly.
r? @richkadel
I might jump on zulip the comming days. Still need to figure out how to properly write tests for this, and deal with other doctest issues in the meantime.