bazelbuild / rules_python

Bazel Python Rules
https://rules-python.readthedocs.io
Apache License 2.0
517 stars 531 forks source link

Allow customized coveragerc for Python coverage #1434

Open tingilee opened 11 months ago

tingilee commented 11 months ago

Description of the feature request:

For python coverage collection, .coveragerc is hardcoded in https://github.com/bazelbuild/bazel/blame/d435c6dd6e977a5c3ea1bc726557a9321948a317/tools/python/python_bootstrap_template.txt#L393-L397. To provide configurability, we'd like to introduce custom .coveragerc to set configurations including https://coverage.readthedocs.io/en/stable/excluding.html#advanced-exclusion.

Which category does this issue belong to?

Configurability, Python Rules

What underlying problem are you trying to solve with this feature?

Allow coverage configurations. In particular, we're interested in excluding import statements being collected as executable lines in coverage reports.

Which operating system are you running Bazel on?

macOS, Linux

What is the output of bazel info release?

release 6.3.0

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

rickeylev commented 11 months ago

It seems like a reasonable feature request. Several questions, though:

All this said, this will probably work out better if we add this in rules_python instead of Bazel itself. I'll transfer this issue over to the rules_python repo.

tingilee commented 11 months ago

How does the binary get the path to the config file?

In general, I'd like to propose that we set environment variable in rules_python to allow overrides:

coverage_env["PYTHON_COVERAGERC"] = ":coveragerc_file"

this matches how the coverage binary is defined, for example:

coverage_env["PYTHON_COVERAGE"] = "bazel_python_internal_pytest_pip_deps_coverage/pypi__coverage/coverage"

How do other languages plumb through coverage settings?

I have not seen coverage setting in other languages, ie. cc or Go. This seems to be pretty specific to Python or implementation of coveragepy.

Do coverage settings need to be per-test, or does a single build-wide config suffice?

.coveragerc configuration should be customized for every py_test target. This would allow us to choose/customize coverage collection per test invocation

github-actions[bot] commented 5 months ago

This issue has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days. Collaborators can add an assignee to keep this open indefinitely. Thanks for your contributions to rules_python!

raylu commented 5 months ago

Do coverage settings need to be per-test, or does a single build-wide config suffice?

I would definitely settle for a single, build-wide config

ewianda commented 4 months ago

This will be very useful. I am trying to migrate from pytest-cov and the coverage runs are almost 4X slower. From the look of it, it seems the time is spent on generating coverage report for third party code. Currently I am using the old PYTHON_COVERAGE tool to configure coverage e.g

"""Coverage entry point."""
import argparse
import os
import re
import sys

from coverage.cmdline import main

if __name__ == "__main__":
    sys.argv[0] = re.sub(r"(-script\.pyw|\.exe)?$", "", sys.argv[0])
    sys.path.append(sys.path.pop(0))
    parser = argparse.ArgumentParser()
    for idx, arg in enumerate(sys.argv.copy()):
        if arg.endswith("pytest_helper.py"):
            parser.add_argument("--package", help="Package containing test script")
            args, unknown = parser.parse_known_args()
            sys.argv.insert(
                idx, "--omit='*/external/*,*/py_deps*/*''"
            )
            sys.argv.insert(idx, f"--source={args.package.split(os.path.sep)[0]}")
    sys.exit(main())
alexeagle commented 3 months ago

I need this for a client who runs into an error with some third-party library, which creates a file in /tmp that ends up in the coverage database file, but the lcov reporter then can't locate it and errors.

https://stackoverflow.com/questions/76807257/python-coverage-report-error-no-source-for-code-path-remote-module-non the fix is to add a line to the coveragerc file: https://coverage.readthedocs.io/en/7.5.3/config.html#report-ignore-errors so this feature would unblock that.

rickeylev commented 3 months ago

error from /tmp file

Yeah, I ran into similar, too, when rewriting the bootstrap code (coverage doesn't like non-seekable files like /dev/fd/N).

I think having it ignores errors makes sense. That way you get some coverage results, even if it's incomplete. I don't think there's much a user can do about a transient source file that got instrumented (writing temp files of code-under-test and running them in-process seems pretty reasonable).

The alternative is to add paths to the omit setting. That worked for /dev/fd/* paths. We could add /tmp, too, I suppose, but that feels problematic.

ewianda commented 1 week ago

Even after https://github.com/bazelbuild/rules_python/pull/2136 , there is a need to better configure coverage-py, for example to collect coverage for a multithreaded program uses something other than the build in threading, one needs to configure

concurrency =
    greenlet
    thread

I think the idea of having config per test target doesn't match the expectation of coverage-py, as such I think there should be an option to override the rc file using the standard coverage-py COVERAGE_RCFILE environment variable e.g

    py_test(
        name = name,
         ...
        env =  {
            "COVERAGE_RCFILE": "$(location //.coveragerc)",
        },
  )      

Another option is to specify rc file as part of the toolchain


python.toolchain( 
    configure_coverage_tool = True,
    coverage_rc = "//.coveragerc", 
    python_version = "3.11.6",
)