test: run pytest in parallel with pytest xdist #1319
Conversation
There was a problem hiding this comment.
Looks reasonable, and this takes the tox -e unit on my laptop from 1m26s to 21s, roughly 4x as fast, which is great. I've had a quick look at pytest-xdist project, and it seems reasonable -- definitely not a thorough look by any means.
One thing this breaks is the coverage report. Is that something we can fix? Or maybe we can talk about whether we care, and turn off coverage? I actually find it gets in the way most of the time anyway. :-)
/home/ben/w/operator/.tox/unit/lib/python3.12/site-packages/coverage/control.py:888: CoverageWarning: No data was collected. (no-data-collected)
self._warn("No data was collected.", slug="no-data-collected")
unit: commands[1]> coverage report
Name Stmts Miss Branch BrPart Cover Missing
----------------------------------------------------------------------
ops/__init__.py 9 9 0 0 0% 44-323
ops/_private/__init__.py 0 0 0 0 100%
ops/_private/timeconv.py 60 60 28 0 0% 17-129
ops/_private/yaml.py 8 8 0 0 0% 17-33
ops/charm.py 660 660 159 0 0% 17-1929
ops/framework.py 684 684 266 0 0% 17-1426
ops/jujuversion.py 79 79 48 0 0% 17-146
ops/lib/__init__.py 175 175 77 0 0% 23-289
ops/log.py 26 26 4 0 0% 17-73
ops/main.py 264 264 94 0 0% 22-566
ops/model.py 1546 1546 606 0 0% 17-3909
ops/pebble.py 1357 1357 427 0 0% 20-3361
ops/storage.py 158 158 30 0 0% 17-425
ops/testing.py 1632 1632 784 0 0% 17-3797
ops/version.py 1 1 0 0 0% 20
----------------------------------------------------------------------
TOTAL 6659 6659 2523 0 0%
|
Indeed, coverage needs a special fix to combine the data from N processes into one file. Maybe we can try https://pytest-cov.readthedocs.io/en/latest/xdist.html ? I've done similar with a hack in the past, the hard way; there's another, better way with pre- or post-fork hook now, but those are still kinda ugly. |
|
Fwiw, coverage is run as part of the TIOBE reports and in the work-in-progress PR I have it running separately because it seemed cleaner/simpler than having it stored by one of the other runs and then loaded by the TIOBE one. So if we can get that running properly then we'd at least have a monthly coverage report (in the action results and in the TIOBE viewer). |
|
Per discussion at today's standup, we decided to remove coverage and do it in a separate PR/workflow. |
There was a problem hiding this comment.
I'm fine with this overall, however consider Tony's #1301 where TIOBE run relies on tox -e unit.
Maybe we can offer a "covering" alternative, like tox -e unit-coverage or tox -e coverage which includes coverage dep, and runs tests "undistributed" under coverage?
WDYT?
Poking @tonyandrewmeyer in case he'd rather include the new tox environment in his PR instead.
This sounds good to me. It could maybe also generate the XML coverage file that TIOBE wants (maybe even in the weird place they want it) which would simplify the workflow for TIOBE>
Either's fine with me. |
|
That's a good idea. I'd go with just |
|
Per the above discussion, I added |
Distributing tests across multiple CPUs to speed up test execution.
On my 10-core mac m1, time is reduced by a little more than a third (~4m39s to ~3m3s). According to GitHub Actions it also speeds up proportionally to that (see this run without pytest xdist and this run with it).