fix(coverage): add test attributes in transition module#1649
fix(coverage): add test attributes in transition module#1649rickeylev merged 5 commits intobazel-contrib:mainfrom
Conversation
rickeylev
left a comment
There was a problem hiding this comment.
Just some textual updates, otherwise LGTM.
I'd ask for a test, but I don't think we have any infrastructure setup to verify the coverage command is working.
CHANGELOG.md
Outdated
| * (coverage): `_lcov_merger` attr added in `_transition_py_test` and | ||
| coverage reports are now created when using the transition module. |
There was a problem hiding this comment.
| * (coverage): `_lcov_merger` attr added in `_transition_py_test` and | |
| coverage reports are now created when using the transition module. | |
| * (coverage): coverage reports are now created when the version-aware | |
| rules are used. | |
| ([#1600](https://github.com/bazelbuild/rules_python/issues/1600)) |
| _PY_TEST_ATTRS = { | ||
| "_collect_cc_coverage": attr.label( | ||
| default = "@bazel_tools//tools/test:collect_cc_coverage", | ||
| executable = True, | ||
| cfg = "exec", | ||
| ), | ||
| "_lcov_merger": attr.label( | ||
| default = configuration_field(fragment = "coverage", name = "output_generator"), | ||
| executable = True, | ||
| cfg = "exec", | ||
| ), | ||
| } |
There was a problem hiding this comment.
Yay magic attributes. I'm guessing you copied this from the files in python/private/common? Lets also copy the comment over so we can give what little context we have about them. Based on your analysis in the issue, I updated the text a bit.
| _PY_TEST_ATTRS = { | |
| "_collect_cc_coverage": attr.label( | |
| default = "@bazel_tools//tools/test:collect_cc_coverage", | |
| executable = True, | |
| cfg = "exec", | |
| ), | |
| "_lcov_merger": attr.label( | |
| default = configuration_field(fragment = "coverage", name = "output_generator"), | |
| executable = True, | |
| cfg = "exec", | |
| ), | |
| } | |
| _PY_TEST_ATTRS = { | |
| # Magic attribute to help C++ coverage work. There's no | |
| # docs about this; see TestActionBuilder.java | |
| "_collect_cc_coverage": attr.label( | |
| default = "@bazel_tools//tools/test:collect_cc_coverage", | |
| executable = True, | |
| cfg = "exec", | |
| ), | |
| # Magic attribute to make coverage work. There's no | |
| # docs about this; see TestActionBuilder.java | |
| "_lcov_merger": attr.label( | |
| default = configuration_field(fragment = "coverage", name = "output_generator"), | |
| executable = True, | |
| cfg = "exec", | |
| ), | |
| } |
There was a problem hiding this comment.
Thank you for your suggestion.
These attributes are also written In python/private/common/py_binary_rule_bazel.bzl, but there is no comment.
So, I'll copy the comment in it as well.
| _transition_py_binary = rule( | ||
| _transition_py_impl, | ||
| attrs = _COMMON_ATTRS, | ||
| attrs = _COMMON_ATTRS | _PY_TEST_ATTRS, |
There was a problem hiding this comment.
Why is it needed in both, py_binary and py_test as opposed just py_test?
There was a problem hiding this comment.
@aignas My understanding is that code coverage is collected from py_binary targets as well.
One of the reference is in https://bazel.build/reference/be/python#py_runtime.coverage_tool
These attributes are defined in common py_binary rule in this repo.
https://github.com/bazelbuild/rules_python/blob/main/python/private/common/py_binary_rule_bazel.bzl
There was a problem hiding this comment.
Ah, thank you, makes sense, just wanted to double check.
There was a problem hiding this comment.
Yeah, if a test runs a binary, then the binary needs to be instrumented so it can produce the coverage rules (which the outer test can collect and process)
This PR is to fix an issue that coverage report is empty when using transition module.
This is due to the absence of the
_lcov_mergerand_collect_cc_coverageattributes.Coverage reports will be created adding these attributes.
Fixes #1600