DM-11725: Initial attempt at pytest coverage testing #35
Conversation
timj
force-pushed
the
tickets/DM-11725
branch
from
2f5d6ae
to
d798d3d
Compare
There was a problem hiding this comment.
See comments.
I ran this on my current jointcal branch, and it does work: the default summary table is nice, though it appears above any pytest errors/warnings (which is maybe a good thing?). It also tells the user where the html and xml files go, which is good.
It would be very nice to include C++ coverage with this, but that's work for a future ticket, since it appears to be nontrivial to accomplish.
python/lsst/sconsUtils/tests.py
Outdated
| @@ -293,3 +305,38 @@ def junitPrefix(self): | |||
| prefix += ".{0}".format(os.environ[controlVar]) | |||
|
|
|||
| return prefix | |||
|
|
|||
| def _pytestCoverage(self, prefix="pytest"): | |||
There was a problem hiding this comment.
I'd rename this method _getPytestCoverageCommand or similar, to clarify what it does.
It should also have a Returns block.
python/lsst/sconsUtils/tests.py
Outdated
| # Use a prefix for the HTML if the prefix is not the default | ||
| htmlfile = "" | ||
| if prefix != "pytest": | ||
| htmlfile = ":'htmlcov-{}'".format(prefix) |
There was a problem hiding this comment.
This puts the output in the package root directory: ./htmlcov: can we get it put into the tests/ directory, or maybe in doc/? The xml report is written to tests/.tests/pytest-cov-*.xml, so tests/.tests/htmlcov seems like a good place.
We also need to add it to the .gitignore and as a scons --clean target.
python/lsst/sconsUtils/tests.py
Outdated
|
|
||
| def _pytestCoverage(self, prefix="pytest"): | ||
| """Form the additional arguments required to enable coverage testing. | ||
| Prefix is used to form the output files. |
There was a problem hiding this comment.
I think this docstring needs to breakdown what the various possibilities for prefix do. I'm having trouble parsing it from the code itself.
If the goal is to just turn on or off html and xml output, why not just have explicit options for those (terminal=True, html=True, xml=False seems like a sensible default), and then Jenkins or whatever can configure that as part of run().
There was a problem hiding this comment.
The prefix really is the prefix to use for the output files, since each pytest invocation needs to have different output files. I think I can remove prefix if I use Scons ${TARGET} instead. If I do that I solve your problem of writing the files in .tests but they get directory names like tests/.tests/pytest-utils.xml-htmlcov (for utils). If people don't mind the extra .xml in there then the code is much much simpler.
python/lsst/sconsUtils/tests.py
Outdated
|
|
||
| # if we have a standard package layout, restrict coverage to the | ||
| # python code and do not include the tests. | ||
| if os.path.exists("python") or os.path.exists(os.path.join(os.path.pardir, "python")): |
There was a problem hiding this comment.
Why these two? Could you instead just use eups.packagedir() here to take care of both cases?
There was a problem hiding this comment.
I've changed it to always use . and include coverage of tests.
python/lsst/sconsUtils/tests.py
Outdated
| options += " --cov=python" | ||
| else: | ||
| # Default to coverage for everything in this directory and below | ||
| options += " --cov=." |
There was a problem hiding this comment.
What packages do we have that would end up in this branch?
There was a problem hiding this comment.
I can't remember why I needed that. I'm using --cov=. for everything now.
* Always use ${TARGET}
* Always do coverage relative to root directory.
They are the same each time now and printing multiple identical copies is not helpful.
|
I've made all the requested changes. The coverage files are now always written into the |
No description provided.