Skip to content

fix(coverage): prevent issues when packages execute files in tmp dir.#2599

Closed
BurnzZ wants to merge 3 commits intobazel-contrib:mainfrom
BurnzZ:fix-omit-tmpdir-coveragepy
Closed

fix(coverage): prevent issues when packages execute files in tmp dir.#2599
BurnzZ wants to merge 3 commits intobazel-contrib:mainfrom
BurnzZ:fix-omit-tmpdir-coveragepy

Conversation

@BurnzZ
Copy link
Contributor

@BurnzZ BurnzZ commented Feb 3, 2025 •
edited
Loading

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"

BurnzZ
BurnzZ commented Feb 3, 2025
View reviewed changes
@BurnzZ BurnzZ mentioned this pull request Feb 3, 2025
Closed
rickeylev
rickeylev reviewed Feb 3, 2025
View reviewed changes
@BurnzZ @rickeylev
Use tempfile module to instead of $TMPDIR
91d6cbe 
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.

@aignas aignas mentioned this pull request Feb 4, 2025
Closed
@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 coveragepy/coveragepy#1921 (comment), using either --source or --omit would fix the issue.

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

@aignas aignas mentioned this pull request Mar 7, 2025
Merged
github-merge-queue bot pushed a commit that referenced this pull request Mar 11, 2025
@BurnzZ @aignas
fix(coverage): missing files in the coverage report if they have no t…
8f51731 
…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
jklukas added a commit to bazeltools/rules_python that referenced this pull request Sep 23, 2025
@jklukas
Prevent coverage reporting issues in tmp dir
1c62fcd 
Pytorch generates some python files in the tmp dir and executes them; coverage
then fails due to not being able to find the source files. We pass a flag to
omit the tmp dir from coverage reporting.

Commit: 6ff10ae
Branch: https://github.com/bazeltools/rules_python/tree/patch-tmp-coverage-1.6.1

This approach is from the discussion in
bazel-contrib#2599 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rickeylev rickeylev rickeylev left review comments

@aignas aignas Awaiting requested review from aignas aignas is a code owner

+1 more reviewer

@timothy-tlj-jones-canva timothy-tlj-jones-canva timothy-tlj-jones-canva left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@BurnzZ @aignas @groodt @rickeylev @timothy-tlj-jones-canva