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

Coverage instrumentation of unit tests also instruments a closure added by #[test] #120046

Closed
Zalathar opened this issue Jan 17, 2024 · 1 comment · Fixed by #120183
Closed

Coverage instrumentation of unit tests also instruments a closure added by #[test] #120046

Zalathar opened this issue Jan 17, 2024 · 1 comment · Fixed by #120183
Labels
A-closures Area: closures (`|args| { .. }`) A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Zalathar
Copy link
Contributor

Zalathar commented Jan 17, 2024

Consider this source file from the coverage test suite:

#[test]
fn my_test() {}

When we instrument this file for coverage, we generate coverage mappings for my_test as expected. However, we also generate coverage mappings for a mysterious closure inside my_test:

Function name: test_harness::my_test::{closure#0}
Raw bytes (9): 0x[01, 01, 00, 01, 01, 09, 01, 00, 08]
Number of files: 1
- file 0 => global file 1
Number of expressions: 0
Number of file 0 mappings: 1
- Code(Counter(0)) at (prev + 9, 1) to (start + 0, 8)

This closure turns out to have been added by the builtin macro that expands #[test] attributes.

Adding coverage instrumentation to this closure is not useful, and we should not be doing it.


Right now this only causes minor problems in the final coverage reports, because coverage for the closure shows up on its own line with the span of the attribute. But while working on some other changes to how we handle macro-expanded closure bodies, I found that some of my changes had the side-effect of causing these test closures to show up and start interfering with the reports.

We should have some way for the coverage instrumentor to know that it should ignore closures introduced by #[test] attributes, and not instrument them at all.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jan 17, 2024
@Zalathar
Copy link
Contributor Author

@rustbot label +A-code-coverage

In theory the correct way to handle this is to have the builtin #[test] macro add the #[coverage(off)] attribute to the closure it introduces. I tried implementing that, but it turned out to be a bit tricky, which is why I ended up filing this issue instead of a PR to fix it.

Another less-pleasing but more immediately practical option would be to have better heuristics that would detect and discard this sort of closure. The trick is finding something that does the job without also negatively impacting real user code that uses macros.

@rustbot rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Jan 17, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 22, 2024
…errors

Add `#[coverage(off)]` to closures introduced by `#[test]` and `#[bench]`

These closures are an internal implementation detail of the `#[test]` and `#[bench]` attribute macros, so from a user perspective there is no reason to instrument them for coverage.

Skipping them makes coverage reports slightly cleaner, and will also allow other changes to span processing during coverage instrumentation, without having to worry about how they affect the `#[test]` macro.

The `#[coverage(off)]` attribute has no effect when `-Cinstrument-coverage` is not used.

Fixes rust-lang#120046.

---

Note that this PR has no effect on the user-written function that has the `#[test]` attribute attached to it. That function will still be instrumented as normal.
fmease added a commit to fmease/rust that referenced this issue Jan 23, 2024
…errors

Add `#[coverage(off)]` to closures introduced by `#[test]` and `#[bench]`

These closures are an internal implementation detail of the `#[test]` and `#[bench]` attribute macros, so from a user perspective there is no reason to instrument them for coverage.

Skipping them makes coverage reports slightly cleaner, and will also allow other changes to span processing during coverage instrumentation, without having to worry about how they affect the `#[test]` macro.

The `#[coverage(off)]` attribute has no effect when `-Cinstrument-coverage` is not used.

Fixes rust-lang#120046.

---

Note that this PR has no effect on the user-written function that has the `#[test]` attribute attached to it. That function will still be instrumented as normal.
@bors bors closed this as completed in ecb8702 Jan 24, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jan 24, 2024
Rollup merge of rust-lang#120183 - Zalathar:test-closure, r=compiler-errors

Add `#[coverage(off)]` to closures introduced by `#[test]` and `#[bench]`

These closures are an internal implementation detail of the `#[test]` and `#[bench]` attribute macros, so from a user perspective there is no reason to instrument them for coverage.

Skipping them makes coverage reports slightly cleaner, and will also allow other changes to span processing during coverage instrumentation, without having to worry about how they affect the `#[test]` macro.

The `#[coverage(off)]` attribute has no effect when `-Cinstrument-coverage` is not used.

Fixes rust-lang#120046.

---

Note that this PR has no effect on the user-written function that has the `#[test]` attribute attached to it. That function will still be instrumented as normal.
@fmease fmease added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-closures Area: closures (`|args| { .. }`) C-enhancement Category: An issue proposing an enhancement or a PR with one. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-closures Area: closures (`|args| { .. }`) A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants