zopefoundation / meta

Meta issues concerning many/all of the zopefoundation repositories.
Other
7 stars 6 forks source link

config tool: not compatible with repos which need `coverage combine` #33

Open jugmac00 opened 4 years ago

jugmac00 commented 4 years ago

eg. https://github.com/zopefoundation/zc.zodbrecipes

In order to get correct coverage, @mgedmin pointed me to https://github.com/buildout/buildout/blob/0185dc6a87f417160378547c776fb6da0c016e88/src/zc/buildout/testing.txt#L123-L152

After applying the fixes, you can't apply the config again, otherwise it deletes the fix again.

 [meta]
 template = pure-python
-commit-id = 4f6e46d28989b99f6dd6ee49829060a6720b79b9
+commit-id = 1e477452ac5420330d79b0a88bacc46abd2c858b
 fail-under = 86

diff --git a/tox.ini b/tox.ini
index ebcd810..ecacc26 100644
--- a/tox.ini
+++ b/tox.ini
@@ -38,19 +38,15 @@ deps =
     coverage
     coverage-python-version
     zope.testrunner
-setenv =
-    COVERAGE_PROCESS_START={toxinidir}/tox.ini
 commands =
     coverage run -m zope.testrunner --test-path=src []
     coverage html
-    coverage combine
     coverage report -m --fail-under=86

 [coverage:run]
 branch = True
 plugins = coverage_python_version
-source = zc.zodbrecipes
-parallel = true
+source = src

 [coverage:report]
 precision = 2
mgedmin commented 4 years ago

In theory it should be safe to use parallel coverage tracing everywhere. At least I'm not aware of any downsides, other than a slightly more complicated config file.

BTW does COVERAGE_PROCESS_START=tox.ini work? I wasn't sure coverage would look at the filename and realize it needs to check [coverage:run] rather than [run].

mgedmin commented 4 years ago

One advantage of always having parallel = true and calling coverage combine is you can drop usedevelop = true and rewrite all the .tox/lib/python*/ paths into src/..., if you add a [coverage:paths] section like this:

[coverage:paths]
source =
    src/
    .tox/*/lib/python*/site-packages/
    .tox/pypy*/site-packages/

I'm not arguing that we should drop usedevelop = true now, but this would give us an option of doing so.

jugmac00 commented 4 years ago

BTW does COVERAGE_PROCESS_START=tox.ini work? I wasn't sure coverage would look at the filename and realize it needs to check [coverage:run] rather than [run].

Yes, it works like a charm - for the moment only on my machine, but not on Travis - but maybe your suggested change will work. https://github.com/zopefoundation/zc.zodbrecipes/pull/3

mgedmin commented 4 years ago

It was using a stale .coverage file from the previous run on your machine to produce the HTML reports. ;) If you did a coverage erase before the coverage run ..., you'd've seen the same error as on Travis.

jugmac00 commented 4 years ago

Thanks for the explanation - that makes sense!

mgedmin commented 4 years ago

I just had a realization: we should be calling coverage erase before coverage run, if we set parallel = true.

The reason is this: parallel mode makes each coverage run write a new .coverage.$pid file. These get combined into a single .coverage and then erased by coverage combine.

If the tests fail, tox will stop after running coverage run, and you'll have a .coverage.$pid from the failed run. When you next run tox after fixing the problem, it'll create a new .coverage.$pid2 file, and then coverage combine will combine the results of the failed run with the results of the new successful run.

I don't think we want that, so it would be good to coverage erase explicitly to remove all .coverage.* files before each run.

jugmac00 commented 4 years ago

https://github.com/zopefoundation/zc.zodbrecipes/pull/4