Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Combine issues in tox examples for coverage 5 #386

Closed
joshringer opened this issue Jan 16, 2020 · 9 comments
Closed

Combine issues in tox examples for coverage 5 #386

joshringer opened this issue Jan 16, 2020 · 9 comments

Comments

@joshringer
Copy link

Re: d3daf76

I have been looking into this a bit today. It appears from the latest coveragepy source that the only reference to the [paths] config—or at least the only reference I could find—is here.
This means that the filepaths are not combined properly somehow when merely doing --cov-append (I'm guessing this difference has something to do with the new db format for the .coverage data file).

I have managed a workaround by setting a different data file per environment and then combining during the report step, i.e. for examples/src-layout/tox.ini:

...
[testenv]
commands = pytest --cov {posargs:-vv}
setenv = 
    {py27,py38}: COVERAGE_FILE=.coverage.{envname}

...
[testenv:report]
commands =
    coverage combine
    ...

I suspect the "correct" fix here is to have pytest-cov respect the [run] parallel flag and add a suffix to the data file as vanilla coverage does. This would at least remove the need for the ugly setenv above. It would kinda break old behaviour though, needing to add a coverage combine down there, so it would probably have to wait for pytest-cov 3.0...

@davidism
Copy link

davidism commented Jan 26, 2020

Possibly related to nedbat/coveragepy#713, append has been working unexpectedly for me well before coverage 5.

@abn
Copy link

abn commented Mar 25, 2020

Facing the same issue here. It would be great if, as per suggestion, pytest-cov respects the parallel config value and skips combining the data files if self.cov.config.parallel is True.

In the current implementation, coverage combine implicit. This is not necessarily a desired behaviour especially when running tests across multiple python versions, platforms or machines.

Alternatively, if the old behaviour is to be preserved, it would be great to enable a new option like --cov-skip-combine that if enabled will skip combining and replacing the default behaviour in a later release.

@ionelmc
Copy link
Member

ionelmc commented Mar 31, 2020

IMO pytest-cov shouldn't even allow users changing the parallel config as it critical for measuring subprocess coverage.

Having a --cov-skip-combine could be a solution, but ugly for both the changes it would incur in the codebase and the CLI argument polution so I'm looking to hearing your arguments for your usecases. So far they are a complete mystery to me :)

@abn
Copy link

abn commented Mar 31, 2020

If a user explicitly sets a value for parallel in their coverage configuration, I do not think that the pytest-cov should ignore it. However, if there is no way to get meaningful coverage results with this turned off under pytest then I think that is sufficient rationale. Maybe, we could also consider generating a run specific file name for the combined result if self.cov.config.parallel was True. This would allow for us to avoild polluting the CLI arguments and retain using parallel prior to combining.

Regarding the use case, the issue arose when combining results from parallel runs across python versions, which I believe is a common use case for projects supporting several python versions. We have a matrix job run using github actions, where multiple environments with different python versions are spun up in parallel, and test suite run simultaneously. Once all these runs are completed, coverage results directory each job is combined into a working directory, and coverage combine expected to be executed.

Since all this is run using tox we currently use a config very similar to the one in the description. An alternative workaround, is to add a step to each CI job to rename the file.

@ionelmc
Copy link
Member

ionelmc commented Mar 31, 2020

Ok so just to understand it correctly ... test suites are ran in parallel, in the same filesystem yes?

@abn
Copy link

abn commented Mar 31, 2020

No, they are run on independant and isolated hosts. And the only way to get all the files together is to export from the test job and import into an aggregator job once all test jobs are completed.

https://help.github.com/en/actions/configuring-and-managing-workflows/persisting-workflow-data-using-artifacts

@joshringer
Copy link
Author

I believe we can have our cake and eat it.
As I understand it, the issue arises because we have two concepts of parallelism:

  1. Running tests in different environments/on different hosts (as coverage considers parallel)
  2. Running a single suite of tests in parallel using multiple processes (pytest xdist)

Currently pytest-cov has a workflow that overrides the data_suffix parameter and then calls combine() to solve 2., which is why the parallel setting is ignored (it affects only the data_suffix value). I think it should be possible to simply recognise when config.data_suffix has been set for the main Coverage instance, and pre-emptively append it into config.data_file (which is what eventually happens) to emulate the parallel setting behaviour, thus kind of nesting 2. under 1..
Something like:

        self.cov = coverage.Coverage(source=self.cov_source,
                                     branch=self.cov_branch,
                                     config_file=self.cov_config)
+       if self.cov.config.data_suffix:
+           self.cov.config.data_file += '.' + coverage.misc.filename_suffix(self.cov.config.data_suffix)
        self.combining_cov = coverage.Coverage(source=self.cov_source,
                                               branch=self.cov_branch,
                                               data_file=os.path.abspath(self.cov.config.data_file),
                                               config_file=self.cov_config)

davvid added a commit to davvid/skeleton that referenced this issue Apr 11, 2020
pytest-cov has issues when run in parallel, as noted by the
pytest-cov authors.

Use coverage v4 until pytest-cov is ready for v5, or documents a
better solution for combining results when run in parallel.

pytest-dev/pytest-cov@d3daf76

pytest-dev/pytest-cov#386
davvid added a commit to davvid/skeleton that referenced this issue Apr 11, 2020
pytest-cov has issues when run in parallel, as noted by the
pytest-cov authors.

Use coverage v4 until pytest-cov is ready for v5, or documents a
better solution for combining results when run in parallel.

pytest-dev/pytest-cov@d3daf76

pytest-dev/pytest-cov#386
davvid added a commit to davvid/skeleton that referenced this issue Apr 11, 2020
pytest-cov has issues when run in parallel when using coverage 5,
as noted by the pytest-cov authors.

Use coverage 4 until pytest-cov is ready for coverage 5, or a
better solution is documented.

pytest-dev/pytest-cov@d3daf76

pytest-dev/pytest-cov#386
davvid added a commit to davvid/skeleton that referenced this issue Apr 13, 2020
pytest-cov has issues when run in parallel when using coverage 5,
as noted by the pytest-cov authors.

Use coverage 4 until pytest-cov is ready for coverage 5, or a
better solution is documented.

pytest-dev/pytest-cov@d3daf76

pytest-dev/pytest-cov#386
@ionelmc
Copy link
Member

ionelmc commented May 21, 2020

@joshringer or anyone: fyi there's a big change in master now to how --cov-append works. Give it a try now, there'll be a new pytest-cov release soon.

@joshringer
Copy link
Author

Nice, v2.9.0 looks good to me. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants