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

Collect coverage from cc_binary data deps of java_test #15096

Closed
wants to merge 1 commit into from
Closed

Collect coverage from cc_binary data deps of java_test #15096

wants to merge 1 commit into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Mar 22, 2022 •
edited
Loading

Before this commit, if a java_test executed a cc_binary from its data
deps, coverage for this cc_binary would not be collected. This is fixed
by adding the implicit $collect_cc_coverage attribute to
BazelJavaTestRule, similar to how this is already done for
BazelShTestRule.

Fixes the Java part of #15098.

@fmeum fmeum force-pushed the transitive-coverage-java-test branch 2 times, most recently from 5d6d396 to f49de88 Compare March 22, 2022 08:54
@fmeum fmeum mentioned this pull request Mar 22, 2022
Closed
@fmeum
Copy link
Collaborator Author

fmeum commented Mar 22, 2022

@c-mita Did you intentionally disable the integration test in 065e2e8#diff-6dd28028af8d621974915318831c0addec4e9fd91386698481c63e65e09d9d93? It does seem to pass both with and without this PR and the commit message does not mention the new tags.

@c-mita
Copy link
Member

c-mita commented Mar 22, 2022

Yes, they were intentionally disabled, I guess I forgot to mention it in the commit.

Those tests would have failed until java_tools was updated (the Java coverage stuff is part of it). Notice that the "head" tests (which build a fresh version of the java tooling) were not disabled.

Looks like we simply forgot to re-enable the tests afterwards.

Created #15100 to fix.

@c-mita
Copy link
Member

c-mita commented Mar 22, 2022

Reviewing the rest of your PR will take me a little longer; I need to convince myself that nothing strange will happen.

@fmeum
Copy link
Collaborator Author

fmeum commented Mar 22, 2022

@c-mita Thanks! I'm currently looking into Java/C++ coverage in detail in order to get multi-language coverage reports working for rules_jni. This PR is part of fixing the issues I encountered, with #15081 being another one.

@sgowroji sgowroji added the team-Rules-Java Issues for Java rules label Mar 22, 2022
@comius comius requested a review from c-mita March 23, 2022 06:05
@fmeum
Collect coverage from cc_binary data deps of java_test
55c6023 
Before this commit, if a java_test executed a cc_binary from its data
deps, coverage for this cc_binary would not be collected. This is fixed
by adding the implicit `$collect_cc_coverage` attribute to
BazelJavaTestRule, similar to how this is already done for
BazelShTestRule.
@fmeum fmeum force-pushed the transitive-coverage-java-test branch from f49de88 to 55c6023 Compare March 23, 2022 11:25
c-mita
c-mita approved these changes Mar 31, 2022
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.

Part of me is surprised that this just "works". But I don't think there's an issue here; java_test already depends on the C++ toolchain.

I would like a "principled" way of handling coverage for multiple languages in a test rule, but this doesn't add anything in the way of complexity that would make me oppose it at this time.

@fmeum
Copy link
Collaborator Author

fmeum commented Mar 31, 2022

@c-mita Let me know if you want to discuss possible principled ways to collect multi-language coverage in tests. I am aware of a design doc on this topic, but don't know what became of it.

@bazel-io bazel-io closed this in 2770799 Apr 1, 2022
@fmeum fmeum deleted the transitive-coverage-java-test branch April 2, 2022 04:54
fmeum added a commit to fmeum/bazel that referenced this pull request Apr 12, 2022
@fmeum
Collect coverage from cc_binary data deps of java_test
31df2fc 
Before this commit, if a java_test executed a cc_binary from its data
deps, coverage for this cc_binary would not be collected. This is fixed
by adding the implicit `$collect_cc_coverage` attribute to
BazelJavaTestRule, similar to how this is already done for
BazelShTestRule.

Fixes the Java part of bazelbuild#15098.

Closes bazelbuild#15096.

PiperOrigin-RevId: 438785232
@fmeum fmeum mentioned this pull request Apr 12, 2022
Merged
@fmeum
Copy link
Collaborator Author

fmeum commented Apr 12, 2022

@bazel-io fork 5.2.0

@fmeum
Copy link
Collaborator Author

fmeum commented Apr 12, 2022

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Apr 12, 2022
fmeum added a commit to fmeum/bazel that referenced this pull request Apr 12, 2022
@fmeum
Collect coverage from cc_binary data deps of java_test
94aa451 
Before this commit, if a java_test executed a cc_binary from its data
deps, coverage for this cc_binary would not be collected. This is fixed
by adding the implicit `$collect_cc_coverage` attribute to
BazelJavaTestRule, similar to how this is already done for
BazelShTestRule.

Fixes the Java part of bazelbuild#15098.

Closes bazelbuild#15096.

PiperOrigin-RevId: 438785232
fmeum added a commit to fmeum/bazel that referenced this pull request Apr 13, 2022
@fmeum
Collect coverage from cc_binary data deps of java_test
3c56ba4 
Before this commit, if a java_test executed a cc_binary from its data
deps, coverage for this cc_binary would not be collected. This is fixed
by adding the implicit `$collect_cc_coverage` attribute to
BazelJavaTestRule, similar to how this is already done for
BazelShTestRule.

Fixes the Java part of bazelbuild#15098.

Closes bazelbuild#15096.

PiperOrigin-RevId: 438785232
@ckolli5
Copy link

ckolli5 commented Apr 13, 2022

@bazel-io fork 5.2.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Apr 13, 2022
@bazel-io bazel-io mentioned this pull request Apr 13, 2022
Closed
fmeum added a commit to fmeum/bazel that referenced this pull request Apr 19, 2022
@fmeum
Collect coverage from cc_binary data deps of java_test
9d5afdd 
Before this commit, if a java_test executed a cc_binary from its data
deps, coverage for this cc_binary would not be collected. This is fixed
by adding the implicit `$collect_cc_coverage` attribute to
BazelJavaTestRule, similar to how this is already done for
BazelShTestRule.

Fixes the Java part of bazelbuild#15098.

Closes bazelbuild#15096.

PiperOrigin-RevId: 438785232
ckolli5 pushed a commit that referenced this pull request May 9, 2022
@fmeum
Collect coverage from cc_binary data deps of java_test (#15216)
96b126e 
Before this commit, if a java_test executed a cc_binary from its data
deps, coverage for this cc_binary would not be collected. This is fixed
by adding the implicit `$collect_cc_coverage` attribute to
BazelJavaTestRule, similar to how this is already done for
BazelShTestRule.

Fixes the Java part of #15098.

Closes #15096.

PiperOrigin-RevId: 438785232
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

@c-mita c-mita

Labels
team-Rules-Java Issues for Java rules
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fmeum @c-mita @ckolli5 @bazel-io @sgowroji