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.

Sign up for GitHub

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

Jump to bottom

Enable coverage for generated source files #11350

Closed
wants to merge 2 commits into from
Closed

Enable coverage for generated source files #11350

wants to merge 2 commits into from

Conversation

Rasrack
Copy link
Contributor

@Rasrack Rasrack commented May 11, 2020

Currently generated source files can't be included in the coverage report. Remove the check to enable coverage for the generated files.

@aiuto aiuto added the team-Rules-CPP Issues for C++ rules label May 13, 2020
@aiuto
Copy link
Contributor

aiuto commented May 13, 2020

Please create an issue for this first so more people can review the relative merits of the feature and if/how it should be done. I strongly suspect that this change would be contentious as is.

@keith
Copy link
Member

keith commented May 13, 2020

+1 I think this would be interesting for our use case, but I'm not sure we'd want it by default, and ideally there would be some way to disable it.

@Rasrack
Copy link
Contributor Author

Rasrack commented May 13, 2020

Created the issue #11375

@jin
Copy link
Member

jin commented May 14, 2020

Also reported on Slack: https://bazelbuild.slack.com/archives/CA31HN1T3/p1588710579121400

@meisterT meisterT requested a review from c-mita October 7, 2020 19:39
@ozio85
Copy link

ozio85 commented Nov 3, 2021

+1 This is essential for coverage to work; this should absolutely be enabled by default. Maybe it is a bug in isSourceArtifact, but I think that check is not needed.

Currently this blocks us using the Bazel release as-is

@ozio85
Copy link

ozio85 commented Nov 3, 2021 •
edited
Loading

@meteorcloudy @c-mita @meisterT is it possible to merge this? It has passed 13 months for a one-line review.

@c-mita c-mita linked an issue Nov 3, 2021 that may be closed by this pull request
Enable coverage for generated source files #11375
Closed
@c-mita
Copy link
Member

c-mita commented Nov 3, 2021

I'm wary of merging, at least right now. Internally, we have tools that expect to be able to open files referenced in coverage reports which would not be possible if they don't exist in the repo.

Either we would need to guard this feature behind a yet-another-flag or any such tool has to be able to handle files not existing in the repo.

@ozio85
Copy link

ozio85 commented Nov 3, 2021

I get it.. but this sounds like a report generator issue, not that the coverage data should avoid source files. That seems dangerous to me.

We do extra filtering on the coverage.dat file to e.g. exclude parts of debug frameworks, but that is an intermediate step, and not something we would consider on the raw output.

@ckolli5 ckolli5 added the awaiting-review PR is awaiting review from an assigned reviewer label Apr 26, 2022
@oquenchil
Copy link
Contributor

@c-mita Can this be closed?

@c-mita
Copy link
Member

c-mita commented Dec 16, 2022

@c-mita Can this be closed?

Not quite; I want to check what happens with proto_library in the new year before it's dismissed. There was interest expressed in this PR to me at Bazelcon.

@limdor
Copy link
Contributor

limdor commented Dec 16, 2022

I can also say that this would be interesting for us.

@c-mita
Copy link
Member

c-mita commented Jan 20, 2023

I can imagine supporting this feature under an experimental flag for the moment; the cost to doing so doesn't seem high to me.

c-mita
c-mita requested changes Jan 20, 2023
View reviewed changes
Copy link
Member

@c-mita c-mita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guard with an experimental flag (e.g. --experimental_collect_coverage_for_generated_files).

@oquenchil
Copy link
Contributor

@c-mita are you waiting for a user response, if so can you change the label from awaiting-review to awaiting-user-response to reflect that?

@sgowroji sgowroji added awaiting-user-response Awaiting a response from the author and removed awaiting-review PR is awaiting review from an assigned reviewer labels Feb 3, 2023
Rasrack and others added 2 commits March 11, 2023 16:09
@Rasrack
Enable coverage for generated source files
63942aa 
Currently generated source files can't be included in the coverage report. Remove the check to enable coverage for the generated files.
@Rasrack
Add experimental flag for collecting code coverage for generated files
43a1ef1
@Rasrack
Copy link
Contributor Author

Rasrack commented Mar 11, 2023

@c-mita I added the experimental flag as per review comments

@Rasrack Rasrack requested a review from c-mita March 12, 2023 08:37
c-mita
c-mita approved these changes Mar 17, 2023
View reviewed changes
Copy link
Member

@c-mita c-mita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I was going to ask for a test, but in investigating how best to do it, I ended up writing it so I'll just add it in myself when I import.

@fmeum
Copy link
Collaborator

fmeum commented Mar 17, 2023

@c-mita Not entirely sure whether it's relevant for this particular flag, but this may lack a line in CoreOptions#getExec to carry it over to the exec configuration.

@c-mita
Copy link
Member

c-mita commented Mar 17, 2023

@c-mita Not entirely sure whether it's relevant for this particular flag, but this may lack a line in CoreOptions#getExec to carry it over to the exec configuration.

I don't that matters since, AFAIU, this shouldn't be carried over to the exec configuration. The other coverage flags aren't carried over either.

@c-mita c-mita removed the awaiting-user-response Awaiting a response from the author label Mar 17, 2023
@c-mita
Copy link
Member

c-mita commented Mar 17, 2023

LGTM.

I was going to ask for a test, but in investigating how best to do it, I ended up writing it so I'll just add it in myself when I import.

Actually, i couldn't even ask you to write tests; the tests for InstrumentedFilesCollector haven't been open-sourced for some reason...

@sgowroji
Copy link
Member

Hi @c-mita, Since I can see that this PR has been approved, please let me know whether I should proceed with importing it. Thanks!

@c-mita
Copy link
Member

c-mita commented Mar 24, 2023

I've done the import myself and it's currently undergoing internal review.

@Rasrack Rasrack mentioned this pull request Apr 12, 2023
Closed
@limdor
Copy link
Contributor

limdor commented May 15, 2023

@c-mita could you please provide an update on this? Is it still under internal review? Thanks

@UebelAndre
Copy link
Contributor

UebelAndre commented Jun 11, 2023 •
edited
Loading

@c-mita @lberki friendly ping 😄

@c-mita
Copy link
Member

c-mita commented Jun 12, 2023

There were some discussions internally and a little bit of push-back regarding the addition of a new flag. Then it sort of just fell through the cracks; sorry about that.

If there are no issues, then we may want to push for enabling this all the time and removing the flag.

@UebelAndre
Copy link
Contributor

Thank you! Is there any work needed to get this in the upcoming Bazel 6.3 release?

@c-mita
Copy link
Member

c-mita commented Jun 12, 2023

@bazel-io fork 6.3.0

@bazel-io bazel-io mentioned this pull request Jun 12, 2023
Closed
iancha1992 pushed a commit to iancha1992/bazel that referenced this pull request Jun 13, 2023
@Rasrack @iancha1992
Adds a flag to enable coverage for generated source files
ccad108 
Closes bazelbuild#11350.

RELNOTES: Add flag --experimental_collect_code_coverage_for_generated_files.
PiperOrigin-RevId: 539648731
Change-Id: I352de7a74c522db6fbe5e10a21268914d1e39d58
@iancha1992 iancha1992 mentioned this pull request Jun 13, 2023
Merged
@iancha1992
Copy link
Member

Thank you! Is there any work needed to get this in the upcoming Bazel 6.3 release?

@UebelAndre Here is the PR to push this change to the 6.3.0 just for your reference: #18664

Also, just for your reference, here is the list of the issues/PR's that shall be resolved for the 6.3.0 release: https://github.com/bazelbuild/bazel/milestone/53

Thanks

iancha1992 added a commit that referenced this pull request Jun 20, 2023
@iancha1992 @Rasrack
Adds a flag to enable coverage for generated source files (#18664)
092ef1c 
Closes #11350.

RELNOTES: Add flag --experimental_collect_code_coverage_for_generated_files.
PiperOrigin-RevId: 539648731
Change-Id: I352de7a74c522db6fbe5e10a21268914d1e39d58

Co-authored-by: Rasrack <[email protected]>
traversaro pushed a commit to traversaro/bazel that referenced this pull request Jun 24, 2023
@Rasrack @traversaro
Adds a flag to enable coverage for generated source files
753b7ae 
Closes bazelbuild#11350.

RELNOTES: Add flag --experimental_collect_code_coverage_for_generated_files.
PiperOrigin-RevId: 539648731
Change-Id: I352de7a74c522db6fbe5e10a21268914d1e39d58
@c-mita c-mita mentioned this pull request Aug 29, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@c-mita c-mita c-mita approved these changes

Assignees

@lberki lberki

@c-mita c-mita

Labels
cla: yes coverage team-Rules-CPP Issues for C++ rules
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable coverage for generated source files