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

Closed
wants to merge 3 commits into from

Conversation

BurnzZ
Copy link
Contributor

@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.

@BurnzZ
Copy link
Contributor Author

BurnzZ commented Mar 5, 2025

Noting that this proposed PR might not be needed since the PR in #2607 that adds --source to the coverage.py call would also fix the issue in #2575. As per nedbat/coveragepy#1921 (comment), using either --source or --omit would fix the issue.

We can close this (alongside #2575) once #2607 is merged.

github-merge-queue bot pushed a commit that referenced this pull request Mar 11, 2025
…ests (#2607)

This ensures that un-executed files _(i.e. files that aren't tested)_
are included in the coverage report. The current behavior is that
coverage.py excludes them by default.

This PR configures source files via the auto-generated `.coveragerc`
file.

See https://coverage.readthedocs.io/en/7.6.10/source.html#execution:

> If the source option is specified, only code in those locations will
be measured. Specifying the source option also enables coverage.py to
report on un-executed files, since it can search the source tree for
files that haven’t been measured at all.

Closes #2599
Closes #2597
Fixes #2575

---------

Co-authored-by: Ignas Anikevicius <[email protected]>
@aignas aignas closed this in #2607 Mar 11, 2025
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