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

fix(coverage): add test attributes in transition module #1649

Merged
merged 5 commits into from
Dec 21, 2023
Merged

fix(coverage): add test attributes in transition module #1649

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8fa4a21
fix(coverage): add test attr in transition module
tanan Dec 21, 2023
12a6072
fix(docs): change attributes
tanan Dec 21, 2023
6b46710
fix(docs): add an issue reference
tanan Dec 21, 2023
4e93b7b
fix(docs): add comments for coverage attributes
tanan Dec 21, 2023
e7a9f6d
fix conflict
tanan Dec 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ A brief description of the categories of changes:
specifying a local system interpreter.
* (bzlmod pip.parse) Requirements files with duplicate entries for the same
package (e.g. one for the package, one for an extra) now work.
* (coverage): `_lcov_merger` attr added in `_transition_py_test` and
coverage reports are now created when using the transition module.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* (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))


### Added

Expand Down
17 changes: 15 additions & 2 deletions python/config_settings/transition.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,16 +142,29 @@ _COMMON_ATTRS = {
),
}

_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",
),
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Suggested change
_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",
),
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is it needed in both, py_binary and py_test as opposed just py_test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, thank you, makes sense, just wanted to double check.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

cfg = _transition_python_version,
executable = True,
)

_transition_py_test = rule(
_transition_py_impl,
attrs = _COMMON_ATTRS,
attrs = _COMMON_ATTRS | _PY_TEST_ATTRS,
cfg = _transition_python_version,
test = True,
)
Expand Down