-
-
Couldn't load subscription status.
- Fork 635
feat(toolchain): Extend Python Testing Toolchain with COVERAGE_RC Support #2246
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
base: main
Are you sure you want to change the base?
feat(toolchain): Extend Python Testing Toolchain with COVERAGE_RC Support #2246
Conversation
c7a142b to
173853e
Compare
4c0451e to
5a16df4
Compare
5a16df4 to
039012f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is certainly interesting. It seems that there are //tests/python/python_tests.bzl failing due to the mock module_ctx not being updated.
Shall we mark this as Ready for review and ask others to also have a look? Before doing that could you please update the PR description?
I am going to work on the description and get the tests to pass, then we can open it up for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some extra thoughts about the API design
039012f to
0d3a405
Compare
f256992 to
9ebd564
Compare
|
The test under Bazel 6.4.0 test is failing. My guess is because this native java |
02d40e0 to
9e4665c
Compare
|
Ah, this is because bazel 6.4 is not using the starlark implementation of the rules, so it is expected to not work for now. |
0371be3 to
3060b83
Compare
|
Idea for why the Windows build is failing - it is using a very different bootstrap mechanism and it could be that the coveragerc is not wired properly. I am not 100% sure how we should do this. @rickeylev, any thoughts here? |
|
@rickeylev @aignas Figured out the issue |
de0204d to
d7181bc
Compare
d7181bc to
2270aa0
Compare
|
ping @aignas @rickeylev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the general direction, but my main question is about how we could implement the additions of the toolchain so that we could have different flavours of py_test.
| use_repo(python_test, "py_test_toolchain") | ||
| register_toolchains("@py_test_toolchain//:all") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The python_test extension in theory could register to toolchain itself, just like python does it. What do you think?
python/private/py_executable.bzl
Outdated
| if effective_runtime.coverage_files: | ||
| transitive.append(effective_runtime.coverage_files) | ||
| if is_test: | ||
| py_test_toolchain = ctx.exec_groups["test"].toolchains[PY_TEST_TOOLCHAIN_TYPE] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be something that is a parameter to this function. I am thinking that we could somehow allow users creating executable rules with their own toolchain types. I would love to be able to move toward the direction of #1647.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use the ability to use a callback macro at the level of the toolchain. I didn't use that approach here since I was not adding an executable. Will change the implementation.
python/private/py_executable.bzl
Outdated
| py_test_toolchain = ctx.exec_groups["test"].toolchains[PY_TEST_TOOLCHAIN_TYPE] | ||
| if py_test_toolchain: | ||
| coverage_rc = py_test_toolchain.py_test_info.coverage_rc | ||
| extra_test_env = {"COVERAGE_RC": coverage_rc.files.to_list()[0].short_path} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in general we should check the ctx.configuration.coverage_enabled and add this in only then.
Regarding my comment above, if we would like to generalize it, do you have ideas how we could do it?
| exec_groups = { | ||
| "test": exec_group(toolchains = [config_common.toolchain_type(PY_TEST_TOOLCHAIN_TYPE, mandatory = False)]), | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally I would love to be able to express this as
| exec_groups = { | |
| "test": exec_group(toolchains = [config_common.toolchain_type(PY_TEST_TOOLCHAIN_TYPE, mandatory = False)]), | |
| }, | |
| extra_toolchains = [PY_TEST_TOOLCHAIN_TYPE], | |
| extend_foo = _my_function_to_add_env_and_set_coverage, | |
| exec_groups = { | |
| "test": exec_group(toolchains = [config_common.toolchain_type(PY_TEST_TOOLCHAIN_TYPE, mandatory = False)]), | |
| }, |
My goal is for us to extend py_test with toolchain such that it would be possible to define a different flavour of py_test with its own toolchain and for that we would be reusing the create_executable_rule snippet like above.
Do you have any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the C++ Testing Toolchain approach, it is better to have this configured at the toolchain level rather than the rule level. If I understand this correctly, users will be required to specify extend_foo on every py_test rule
I think I don't understand how _my_function_to_add_env_and_set_coverage will access the information provided by the toolchain.
2270aa0 to
acbd8c7
Compare
acbd8c7 to
e670fd3
Compare
Inspired by https://github.com/trybka/scraps/blob/master/cc_test.md This PR extends Test Runner enviroment to provide a coveragerc enviroment variable COVERAGE_RC, allowing user to provide coverage resource in what ever format
e670fd3 to
23011c4
Compare
Create partially-bound function for configuring py_test
23011c4 to
214972f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I think I like the approach here and would like to hear what @rickeylev thinks about such a way to do dependency injection.
I can see how I could add a @pypi//pytest_bazel target in the python_test.configure call and in this way tell bazel that we have to use that package for running all of the tests. I would probably then need to somehow add the extra invocation of python -m pytest_bazel somewhere in the py_executable.bzl or related in order to benefit from the extra dependency in there.
Remaining bits before merging:
- Have more than just me believing in fruitfulness of this approach.
- Add this to the
//docs:BUILD.bazeland add an EXPERIMENTAL note in the symbols. - Profit?
| func = _get_runner, | ||
| args = { | ||
| "coverage_rc": ctx.attr.coverage_rc, | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think we could use a lambda here so that the call site can just do get_runner(my_extra_args).
| register_py_test_toolchain( | ||
| name = "py_test_toolchain", | ||
| coverage_rc = tag.coveragerc, | ||
| register_toolchains = False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that the register_toolchains arg here has been borrowed from the uv toolchain. I am personally not a fan for the func name to say register_py_test_toolchain and then there being an arg register_toolchains = False. So what are we doing here? Maybe just not registering the toolchains and calling the function py_test_toolchain_repositories would be more acurate?
| """ | ||
| for mod in module_ctx.modules: | ||
| for tag in mod.tags.configure: | ||
| register_py_test_toolchain( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we allow non-root modules to do the registrations? I think we could just ignore any non-root modules.
@rickeylev any chance to take a look, I work on your suggestions @aignas |
|
@aignas is this still something to pursue? I want to upgrade rules_python, and find myself needing to carry forward a patch for this. |
|
I'm torn about this. On one hand I don't think we have the coverage story sorted out properly, but I think the proposed PR may be a little bit hard to maintain. There was no consensus among the maintainers about maintaing this sort of API long term. There was some desire to make this a user space pnoblem - like what pytest-bazel is doing, but there is a need for more integration. Maybe there can be a way in between. |
Fair enough, I had proposed a low maintenance approach (open for debate) of just providing the coveragerc using an environemnt variable. This is how I am currently solving this as illustrated here . This should not be far out, given that we already use the This approach was discused in #1434 and implemented in bazelbuild/bazel#19656 |
This PR enhances the Test Runner environment to support custom coverage configuration via the COVERAGE_RC environment variable. Inspired by this approach, this feature allows users to specify a .coveragerc file in any compatible format to customize coverage reporting.
Key Changes
Adds support for the COVERAGE_RC environment variable in the Test Runner environment.
Enables users to define and pass their own .coveragerc configurations, enhancing flexibility for various testing and reporting requirements.
close #1434