python: Add pytest and coverage #15888
Conversation
repokitteh-read-only
bot
added
the
deps
phlax
force-pushed
the
py-add-pytest
branch
2 times, most recently
from
4aaa260 to
0ba669e
Compare
phlax
changed the title
phlax
changed the title
There was a problem hiding this comment.
@phlax generally looks good, but need to understand a bit why there is so much custom stuff we need to do for coverage here and how the average end user of the tooling interacts. Is there some documentation of how I start to add coverage in my Python tools?
tools/base/tests/test_runner.py
Outdated
| from tools.base import pytest_runner, runner | ||
|
|
||
|
|
||
| # this is necessary to fix coverage |
There was a problem hiding this comment.
this module is imported before pytest - when pytest runs it doesnt see this file has having coverage (altho it does see the content of modules and functions in the file - just not the module level stuff)
effectively this gives coverage to the module level vars - the stuff that would be covered my just importing the file and not doing anything with it
There was a problem hiding this comment.
Do we have to do this in every pytest consuming file?
There was a problem hiding this comment.
no - only the files that have been imported before pytest is invoked - i expect only the files in this PR will need it - but deps slightly on how we implement in the future
i have only ever run into this problem with fixture/pytest plugin files before
tools/base/utils.py
Outdated
|
|
||
| # this is testing specific - consider moving to tools.testing.utils | ||
| @contextmanager | ||
| def custom_coverage_data(data_file: str) -> Iterator[str]: |
There was a problem hiding this comment.
I don't understand why we need to have this customization? Why isn't coverage push button?
There was a problem hiding this comment.
afaict coverage doesnt take the data_file arg as an arg - only from the config - so i had to do this config mangling to make it work
There was a problem hiding this comment.
I don't quite get how the data file figures into using coverage..
There was a problem hiding this comment.
when you run coverage it creates a data file - its the same format for all langs - so you can create coverage reports from multiple langs
when you run coverage (or invoke it with some test runner like pytest) it can either create or append to an existing file
running lots of pytests with bazel means it has to be passed between the test runs
at the end this implementation invokes coverage directly to generate html from the data file
There was a problem hiding this comment.
ive renamed this to coverage_with_data_file which hopefully is a bit clearer
hopefully that is answered inline
in this pr each in the subsequent PR another runner -
add tests. i can add a README explaining how to setup tests for a module and ensure it is included/covered |
+1 |
not sure, i tried another implementation but did not get it to work - the python modules were not importable i came to the conclusion that if you want to use python in this way with bazel you need to have a file the big benefit of using this pattern tho is that you know its going to be exactly the same as when another python target invokes it
that is an unfixable issue i think - but not one anyone else will need to concern themselves with |
|
/retest |
|
Retrying Azure Pipelines: |
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
|
@htuch this is hopefully ready for final review |
tools/base/envoy_python.bzl
Outdated
| visibility = visibility, | ||
| ) | ||
|
|
||
| native.genrule( |
There was a problem hiding this comment.
Why not have envoy_py_binary defined in terms of envoy_py_library or some common macro for genrule stuff?
There was a problem hiding this comment.
i can certainly factor out the common code
There was a problem hiding this comment.
i didnt do this because py_lib and py_bin take slightly different args
There was a problem hiding this comment.
ive factored out the common code - i think it works ok
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
|
/lgtm deps |
repokitteh-read-only
bot
removed
the
deps
- add separate tooling test job in ci - add pytest and a pytest runner - add coverage and cov runner - add pytests for all runners - upload coverage to https://storage.googleapis.com/envoy-pr/15888/tooling/index.html Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey ryan@synca.io
Commit Message: python: Add pytest and coverage
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]