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.

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): prevent issues when packages execute files in tmp dir. #2599

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

BurnzZ
Copy link

@BurnzZ BurnzZ commented Feb 3, 2025

Background

(A good addition to #2597)

This is a proposal to have a workaround for #2575 where an inherent issue from coverage.py when creating the lcov output file.

Proposed Changes

Add an --omit='...' flag pointing to tempdir.

Reproducible steps

We can see how this proposal addresses the issue described from https://github.com/BurnzZ/rules_python.

  1. Clone the repo: https://github.com/BurnzZ/rules_python
git clone https://github.com/BurnzZ/rules_python.git rules_python_tmp
cd rules_python_tmp/examples
git checkout fix-omit-tmpdir-coveragepy
  1. Clone the reproducible example
git clone https://github.com/BurnzZ/bazel-python-coverage-issue.git
cd bazel-python-coverage-issue
git checkout enable-torchvision
  1. Add these lines in line 2 of MODULE.bazel:
local_path_override(
    module_name = "rules_python",
    path = "../..",
)
  1. Generate the coverage report:
bazel coverage --combined_report=lcov :test --nocache_test_results --test_output=all
lcov --list "$(bazel info output_path)/_coverage/_coverage_report.dat"
genhtml --branch-coverage --output genhtml "$(bazel info output_path)/_coverage/_coverage_report.dat"

since subprocess.call won't exapnd environment variables.

Co-authored-by: Richard Levasseur <[email protected]>
@BurnzZ BurnzZ marked this pull request as ready for review February 3, 2025 00:46
@BurnzZ BurnzZ requested a review from aignas as a code owner February 3, 2025 00:46
@BurnzZ BurnzZ changed the title [WIP] fix(coverage): prevent issues when packages execute files in tmp dir. fix(coverage): prevent issues when packages execute files in tmp dir. Feb 3, 2025
@aignas
Copy link
Collaborator

aignas commented Feb 3, 2025

My thinking is that the COVERAGE_RC issue was a similar topic - we need to be able to customize coverage in some way but currently we have no way to do that. The coverage tool right now is coupled to the python toolchain, but maybe we should have a python_test toolchain that can touch/configure the following parts:

  • Test runner deps
  • Test runner itself
  • Coverage deps
  • Coverage configuration
  • Report generation?

We have a proposal here for this: #2246.

Some thoughts about this change:

  • We may want to be able to change coverage configuration per target, similar to the precompile flag.
  • rules_python should do the right thing out of the box.

I think there are many other things I am forgetting, but wanted to mention greater design concerns here. I am not too much against this as a temporary fix for the actual problem at hand, but I would like to have a better story for testing and configuration in rules_python in general, so if other people have requirements as to what is needed and/or ideas about design, please share them.

@groodt
Copy link
Collaborator

groodt commented Feb 3, 2025

we need to be able to customize coverage in some way but currently we have no way to do that

Yes, indeed. There are also solutions to this which might be solved in userspace instead. Similar to https://pypi.org/project/pytest-bazel/

I do like a userspace solution, but the "batteries included" coverage supporting configuration files makes sense while we are bundling coverage with the rules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants