Use new coverage instance to combine

[ryanhiebert]

This is a likely fix for #130.

I use tox, parallel mode for coverage, and coverage combine in order to produce report with tox that is a) readable, and b) ready for upload to codecov.

Re-appropriated from the repositories of folks smarter than myself, I have the following commands in my tox.ini, that I'd like to replace with a single pytest --cov command.

coverage run --parallel-mode -m pytest {posargs}
coverage combine
coverage report

I eventually figured out that I can add parallel mode to the coverage configuration files, so combined with my previous configuration, I ended up with this at the end of my tox.ini (where I like my coverage configuration):

[coverage:run]
source = mypackage
parallel = True

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

But this didn't work. I went looking at how pytest-cov uses coverage.py, and even though it looks like it is combining, for some reason it doesn't work.

I did a bunch of experimenting, and the only thing that I've found works is the contents of this pull request. I've found that the self.cov.combine() call before the save didn't seem to do anything, at least for my use-case, so I removed that. If I'm missing a use-case, I doesn't hurt my case to add it back.

I couldn't find any way to make this work without replacing the Coverage instance at self.cov. Trying to erase the file after saving didn't do what I wanted. I don't think that load would either, though I didn't actually try it, because it seemed like it conflicted with the parallel builds.

One interesting result was that performing a save(), then a combine() on the same instance of coverage gave a report with both the paths as I expected, as well as the raw paths that should have been eliminated during combining. That's the closest I came to making it work without replacing the coverage instance.

I'd really appreciate a review on this by somebody who knows coverage well. Perhaps @nedbat would be willing to take a look, and see if there's something obvious I've missed?

[ionelmc]

Note that this change doesn't really work. Not sure what's going on but it looks like .combine() doesn't load the .coverage file (non-parallel mode cov data file basically)?

[ryanhiebert]

Tested locally with the latest versions of tox, coverage, and pytest, and it did work. In fact, it wouldn't work without the change. In my local install, it's leaving it with just the .coverage file, so the other files are getting combined and deleted from parallel mode, which is perfect, I only need that file for uploading to codecov.

What is it that you're saying doesn't work? I seriously tried every combination I could think of, because making a new instance really seemed hacky to me. Nevertheless, it did work for me. Can you tell me steps to reproduce the failure you're seeing? I'm happy to figure out what I did wrong, if I know how to reproduce the problem.

[ionelmc]

I mean the tests are failing all over the place, see https://travis-ci.org/pytest-dev/pytest-cov/builds/188173071

[ryanhiebert]

Gotcha. It's difficult to tell, because the Travis CI build matrix is failing anyway. I'll find a passing one on my local environment, and use that to figure out the tests. Thanks.

[ionelmc]

Hmmm it seems that are indeed some failures, but they are all around test_multiprocessing_subprocess_with_terminate - you can ignore that.

[ionelmc]

Looks like adding a self.cov.load() before self.cov.combine() makes more stuff pass, but anyway, would need to have a testcase for the situation in #130 ...

[ryanhiebert]

Thanks. I'll try to look at that tonight.

[ionelmc]

I added one test and fixed some more stuff in #143

Give it a try. If you need to add more fixes just push the stuff from there in your branch.

[ryanhiebert]

Thanks!

[ryanhiebert]

I pulled in the stuff from #143 so that I could add the self.cov override I mentioned there. Also, when I created the PR I noticed that GitHub asked if I wanted to give you access to make commits to my branch. It's pretty cool, you should be able to update at least this branch on my fork.

[ionelmc]

I've tried to figure out how to fix this but it seems there's no way to handle this without breaking the xdist stuff.

There are two issues this would solve regarding paths like .tox/py35/lib/python3.5/site-packages/foobar:

• Verbose report from pytest-cov. This is a minor esthetic issue - we can live with this.

• Incorrect paths in datafiles. This is a problem for publishing directly to codecov/coveralls.

  However, this can easily be fixed with post-processing steps like coverage combine --append.

I'm inclined to just give up on this and let users fix it up before publishing to coveralls/codecov (by running coverage combine --append or whatever). For reference, it seems to work, see build and codecov (note paths are alright in files tab) or coveralls.

[ryanhiebert]

Are the xdist problems related to the Central CovController, or only to the other ones? The problem I'm trying to fix, specifically, perhaps only makes sense in the central controller.

I'd be disappointed if we just gave up on this, and I'd go back to using coverage directly. There'd be no benefit to pytest-cov for me, under tox. But my use-case seems like it would be a really common one, so I'd really like to see it handled well in pytest-cov.

I don't know exactly when, but I will take a closer look when I get a chance, to see if I can understand the test failures better. Thank you for all the work that you've already put into it.

I'm debating to some extend if this might be to a logical mismatch of the features of coverage. I'm not sure how to answer that yet, but it seems like normalizing paths should be possible to do without running in parallel mode, which has a different semantic purpose.

[ionelmc]

The same aliasing issue is in xdist mode too. It'd be pretty inconsistent to only fix it in central mode.

In my mind this is not a problem purely in coverage, but rather a compound issue: pytest-cov wants to not rely on CWD (because sloppy tests can change CWD) but coverage is very sensitive to CWD changes and everything blows up if it changes. Seem intractable but if someone provides a complete solution I'm not against it.