feat(toolchain): Extend Python Testing Toolchain with COVERAGE_RC Support#2246
feat(toolchain): Extend Python Testing Toolchain with COVERAGE_RC Support#2246ewianda wants to merge 4 commits intobazel-contrib:mainfrom
Conversation
c7a142b to
173853e
Compare
4c0451e to
5a16df4
Compare
5a16df4 to
039012f
Compare
aignas
left a comment
There was a problem hiding this comment.
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. |
aignas
left a comment
There was a problem hiding this comment.
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 |
aignas
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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
aignas
left a comment
There was a problem hiding this comment.
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.
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.
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.
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