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

To run coverage with python, setting PYTHON_COVERAGE in every test env is required #14436

Closed
bradb423 opened this issue Dec 15, 2021 · 2 comments
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-Python Native rules for Python

Comments

@bradb423
Copy link
Contributor

bradb423 commented Dec 15, 2021

Description of the problem / feature request:

Currently, to obtain coverage for projects in python, the location macro is used to point to the entry point of a python coverage tool. This has to be repeated for each test in a given project. Perhaps an environment variable could be created e.g: PYTHON_COVERAGE_TARGET, to make the process simpler.

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

For an example project with the following python files:

import sys


def main() -> int:
    return 0


if __name__ == "__main__":
    sys.exit(main())
import unittest

from main import main


class TestMain(unittest.TestCase):
    def test_main(self):
        self.assertEqual(main(), 0)


if __name__ == "__main__":
    unittest.main()

To achieve coverage for the files above, one can use the location macro to point to a specific coverage tool. This is shown below in the BUILD file:

alias(
    name = "python_coverage_tools",
    actual = entry_point("coverage"),
    visibility = ["//src:__subpackages__"],
)

py_test(
    name = "test",
    srcs = ["test.py"],
    env = {
        "PYTHON_COVERAGE": "$(location :python_coverage_tools)",
    },
    deps = [
        ":main",
        ":python_coverage_tools",
    ],
)

The problem for this is that for additional python test files, the PYTHON_COVERAGE must be set each time. While this process could be automated, it may be easier to use an environment variable that would allow us to use --test_env flag instead of the location macro each time.

What operating system are you running Bazel on?

Debian 10(Buster)

Bazel Version

bazel version 4.2.1 and above

@c-mita

@TLATER
Copy link

TLATER commented Dec 16, 2021

@lfpino will be interested too :)

@aiuto aiuto added team-Rules-Python Native rules for Python untriaged labels Dec 18, 2021
@comius comius added P3 We're not considering working on this, but happy to review a PR. (No assignee) and removed untriaged labels Dec 20, 2021
TLATER added a commit to EngFlow/bazel that referenced this issue Feb 1, 2022
This resolves bazelbuild#14436 by adding a new environment variable that will
perform the coverage label resolution inside the python_stub_template
directly, which resolves the python coverage tool by label rather than
path.

Currently this resolves the path in the runfiles directory by guessing
the path the label should resolve to - this of course does not work in
general, even just defining an alias breaks it. Since labels appear to
only be resolved in the analysis phase, there does not seem to be an
easy way around this, however.

This is a draft, showing the behavior I would like - suggestions on
how to correctly implement this would be appreciated.

Co-authored-by: Bradley Burns <bradley.burns@codethink.co.uk>
TLATER added a commit to EngFlow/bazel that referenced this issue Feb 2, 2022
This resolves bazelbuild#14436 by adding a new environment variable that will
perform the coverage label resolution inside the python_stub_template
directly, which resolves the python coverage tool by label rather than
path.

Currently this resolves the path in the runfiles directory by guessing
the path the label should resolve to - this of course does not work in
general, even just defining an alias breaks it. Since labels appear to
only be resolved in the analysis phase, there does not seem to be an
easy way around this, however.

This is a draft, showing the behavior I would like - suggestions on
how to correctly implement this would be appreciated.

Co-authored-by: Bradley Burns <bradley.burns@codethink.co.uk>
@adam-azarchs
Copy link
Contributor

I agree it's important to be able to set this up, though I think it would make more sense for this to be configured in the python toolchain configuration rather than an environment variable, e.g. add it to py_runtime.

adam-azarchs added a commit to adam-azarchs/bazel that referenced this issue May 27, 2022
If set, and coverage is enabled, the path to this target is used
instead of the PYTHON_COVERAGE environment variable.  This permits
use of in-build versions of the coverage tool, closing bazelbuild#14436.
adam-azarchs added a commit to adam-azarchs/bazel that referenced this issue May 28, 2022
If set, and coverage is enabled, the path to this target is used
instead of the PYTHON_COVERAGE environment variable.  This permits
use of in-build versions of the coverage tool, closing bazelbuild#14436.
adam-azarchs added a commit to adam-azarchs/bazel that referenced this issue May 31, 2022
If set, and coverage is enabled, the path to this target is used
instead of the PYTHON_COVERAGE environment variable.  This permits
use of in-build versions of the coverage tool, closing bazelbuild#14436.
adam-azarchs added a commit to adam-azarchs/bazel that referenced this issue Jun 1, 2022
If set, and coverage is enabled, the path to this target is used
instead of the PYTHON_COVERAGE environment variable.  This permits
use of in-build versions of the coverage tool, closing bazelbuild#14436.
adam-azarchs added a commit to adam-azarchs/bazel that referenced this issue Aug 11, 2022
If set, and coverage is enabled, the path to this target is used
instead of the PYTHON_COVERAGE environment variable.  This permits
use of in-build versions of the coverage tool, closing bazelbuild#14436.
aiuto pushed a commit to aiuto/bazel that referenced this issue Oct 12, 2022
This allows users to specify a target providing the coveragepy tool (and its dependencies).  This is essential for hermetic python builds, where an absolute path will not really work.  It's also superior to other potential methods using environment variables because the runfiles dependency on the coverage tool and its files is only incurred when building with coverage enabled.

This also builds on the work @TLATER began with bazelbuild#14677 to integrate with `coveragepy`'s `lcov` support, with an additional step of at least attempting to convert the absolute paths which `coveragepy` uses in the lcov output into the relative paths which the rest of bazel can actually consume.

This is my first time touching Java code professionally, so I'll admit to mostly cargo-culting those parts, and would welcome any feedback on how to improve things there.  I also would have no objections to someone else taking over this PR to get it over the finish line.  I've tested this out with our own team's internal monorepo, and have successfully generated a full combined coverage report for most of our python and go code.  There's still a bunch of things which don't quite work, in particular when it comes to compiled extension modules or executables run from within python tests, but those will need to be addressed separately, and this is already a giant step forward for our team.

Closes bazelbuild#14436.

Closes bazelbuild#15590.

PiperOrigin-RevId: 476314433
Change-Id: I4be4d10e0af741f4ba1a7b5367c6f7a338a3c43d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-Python Native rules for Python
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants