Skip to content

feat: new COVERAGEPY_IGNORE_ERRORS env var#2597

Closed
BurnzZ wants to merge 2 commits intobazel-contrib:mainfrom
BurnzZ:feat-new-env-var-COVERAGEPY_IGNORE_ERRORS
Closed

feat: new COVERAGEPY_IGNORE_ERRORS env var#2597
BurnzZ wants to merge 2 commits intobazel-contrib:mainfrom
BurnzZ:feat-new-env-var-COVERAGEPY_IGNORE_ERRORS

Conversation

@BurnzZ
Copy link
Contributor

@BurnzZ BurnzZ commented Feb 1, 2025

Background

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

An env var flag that a user can turn on via --test_env=COVERAGEPY_IGNORE_ERRORS=true when running bazel coverage.

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 feat-new-env-var-COVERAGEPY_IGNORE_ERRORS
  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 --test_env=COVERAGEPY_IGNORE_ERRORS=true
lcov --list "$(bazel info output_path)/_coverage/_coverage_report.dat"
genhtml --branch-coverage --output genhtml "$(bazel info output_path)/_coverage/_coverage_report.dat"

Issue from #2575 now resolved. 🎉

@BurnzZ BurnzZ mentioned this pull request Feb 1, 2025
Closed
@groodt
Copy link
Collaborator

groodt commented Feb 2, 2025 •
edited
Loading

I think it makes sense to provide a mechanism to let coveragepy ignore errors.

I also wonder if it makes sense to provide a way for all of the coveragepy CLI arguments to be supplied?

I think the torch errors that motivate this PR can probably also be avoided by excluding that source from coverage. Ignoring all errors would avoid the torch error, but possibly also mask others.

To be clear, I still think there are valid scenarios to ignore all, so this PR still LGTM.

WDYT @rickeylev

groodt
groodt approved these changes Feb 2, 2025
View reviewed changes
Copy link
Collaborator

@groodt groodt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@BurnzZ
Copy link
Contributor Author

BurnzZ commented Feb 2, 2025

I also wonder if it makes sense to provide a way for all of the coveragepy CLI arguments to be supplied?

This was initially my approach but since there are 2 runs: coverage run and coverage lcov, we need to consider that the args could be exclusive to each. On the other hand, there might be cases that they should be the same to ensure proper processing of the coverage artifacts. I haven't thought deeply about the other use cases for this yet.

I think the torch errors that motivate this PR can probably also be avoided by excluding that source from coverage.

From coveragepy/coveragepy#1921 (comment), I think an alternative would be coverage run --omit="$TMPDIR/*" test_foo.py. Will need to properly test this out.

Ignoring all errors would avoid the torch error, but possibly also mask others.

Yeah, that's indeed another risk.

Although, I think we should be fine since --ignore-errors "tells coverage.py to ignore problems encountered trying to find source files to report on. This can be useful if some files are missing, or if your Python execution is tricky enough that file names are synthesized without real source files" (reference). We still get useful warning messages (unless --disable_warning is added).

@rickeylev
Copy link
Collaborator

rickeylev commented Feb 2, 2025

How about enabling ignore errors by default instead?

@BurnzZ
Copy link
Contributor Author

BurnzZ commented Feb 2, 2025

That's one option but it could hide other potential issues by default.

I think it'd be best if the user can first see the error, then decide if errors are to be ignored. This opt-in approach is sufficient for the time being.

@BurnzZ BurnzZ mentioned this pull request Feb 3, 2025
Closed
timothy-tlj-jones-canva
timothy-tlj-jones-canva approved these changes Feb 3, 2025
View reviewed changes
Copy link

@timothy-tlj-jones-canva timothy-tlj-jones-canva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I heartily endorse this

@aignas aignas mentioned this pull request Feb 4, 2025
Closed
@aignas
Copy link
Collaborator

aignas commented Mar 7, 2025

Is this needed if we have #2607?

@BurnzZ
Copy link
Contributor Author

BurnzZ commented Mar 7, 2025

We can close this since #2607 is on the way.

Although there are some benefits in having an escape hatch like --ignore-errors could provide outside of #2575. We can revisit if needed.

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@groodt groodt groodt approved these changes

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

@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 approved these changes

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 @groodt @rickeylev @aignas @timothy-tlj-jones-canva