Add #[no_coverage] tests for nested functions
#92695
Conversation
|
r? rust-lang/compiler |
camelid
added
S-waiting-on-review
There was a problem hiding this comment.
Sorry for the delayed reply. I just moved to a new city and house so I was out of office for a while.
I added a couple of comments.
Worth noting, I don't think #[no_coverage] is widely used, at the current time. It has at least one specific use case (in the standard library), which isn't affected by this bug.
Nevertheless, it's something we should look into.
If you haven't already, can you file an "issue" with this example, so we can track a future fix to the known issue?
cc: @tmandry @wesleywiser
src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.no_cov_crate.txt
Show resolved
Hide resolved
| 50| 1| fn inner_not_covered(is_true: bool) { | ||
| 51| 1| if is_true { | ||
| 52| 1| println!("called but not covered"); | ||
| 53| 1| } else { |
There was a problem hiding this comment.
Agreed, this looks like a bug, at least in how #[no_coverage] works.
I was experimenting with adding that attr to all Another interesting this that this test does not cover: How do nested functions look with normal covered/uncovered ranges, independently of |
|
I don't really know code coverage. @richkadel, who do you think would be a good reviewer for this? |
|
@tmandry or @wesleywiser could review this.
@Swatinem - Yes, I think you should add that to the test, for comparison. And add a FIXME comment to the test referencing your issue number, if you haven't done that yet. Thanks! |
|
r? @wesleywiser |
|
Thanks for adding these tests! I agree with @richkadel, it would be good to have that additional test case mentioned added as well. If you have time to do that, great! If not, let me know and I'll try to add it myself. |
Swatinem
force-pushed
the
cover-nested
branch
from
a1040bd
to
fe9271a
Compare
|
Rebased and added a comment linking to the issue, and another testcase for nested fns without any |
|
📌 Commit fe9271a has been approved by |
bors
added
S-waiting-on-bors
| 67| 1| println!("called and covered"); | ||
| 68| 1| } else { | ||
| 69| 0| println!("absolutely not covered"); | ||
| 70| 0| } |
There was a problem hiding this comment.
Thanks for adding this @Swatinem !!
Interesting. I'm kind of surprised to see zeros here on lines 69-70 (correctly), given the reported issue with no_coverage on inner functions. LLVM tools must recognize the overlapping inner function range and, in that case, only reports coverage for the inner function's code range.
(I'm fairly certain there were tests for inner functions, so I'm happy to see at least that part is doing what is expected. It wasn't a total oversight.)
IMO, there's not a strong use case for no_coverage of inner functions, so while it is still a bug, it isn't an urgent one. And the no_coverage attribute is not stabilized (at this point).
…askrgr Rollup of 9 pull requests Successful merges: - rust-lang#86497 (Add {floor,ceil}_char_boundary methods to str) - rust-lang#92695 (Add `#[no_coverage]` tests for nested functions) - rust-lang#93521 (Fix hover effects in sidebar) - rust-lang#93568 (Include all contents of first line of scraped item in Rustdoc) - rust-lang#93569 (rustdoc: correct unclosed HTML tags as generics) - rust-lang#93672 (update comment wrt const param defaults) - rust-lang#93715 (Fix horizontal trim for block doc comments) - rust-lang#93721 (rustdoc: Special-case macro lookups less) - rust-lang#93728 (Add in ValuePair::Term) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
I was playing around a bit trying to figure out how
#[no_coverage]behaves for nested functions and thought I might as well add this as a testcase.The "nesting covered fn inside not covered fn" case looks pretty much as expected.
The "nesting not covered fn inside a covered fn" case however seems a bit counterintuitive.
Essentially the region of the outer function "covers" its whole lexical range. And the inner function does not generate any region at all. 🤷🏻♂️
r? @richkadel