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

Windows: coverage through multiprocessing not catched correctly #165

Closed
Makman2 opened this Issue Jul 17, 2017 · 20 comments

Comments

Projects
None yet
2 participants
@Makman2

Makman2 commented Jul 17, 2017

Hi ;)

Recently I'm trying to get the test coverage to 100% on a windows machine, but it doesn't want to. My code under test uses an event-loop driven by a concurrent.futures.ProcessPoolExecutor. Coverage is 100% on a Linux machine, but not on Windows.

PR:
coala/coala#4475
Build on AppVeyor:
https://ci.appveyor.com/project/coala/coala/build/1.0.7774
Same build but on Linux (Travis):
https://travis-ci.org/coala/coala/builds/254572218?utm_source=github_status&utm_medium=notification
Relevant Python module/components under test:
https://github.com/coala/coala/tree/8f4113a2f471acf4c0121199c6ebb9b29dfa84ff/coalib/core, specificially

The missing lines are the ones inside def execute_task.

setup.cfg (and yes, coverage is enabled ;D):
https://github.com/coala/coala/blob/8f4113a2f471acf4c0121199c6ebb9b29dfa84ff/setup.cfg#L32

Exchanging the ProcessPoolExecutor with a ThreadPoolExecutor does the trick and produces full coverage. However it would be nice to have everything being tested with a ProcessPoolExecutor (especially due to possible pickling issues on Windows for subprocesses).

Not proven assumption:
I have the feeling that other parts of the code only run through a subprocess is measured properly, only those two modules under test don't work.

@ionelmc

This comment has been minimized.

Member

ionelmc commented Jul 17, 2017

I suspect this is related to #139. A problem with Windows it's impossible to get 100% clean process termination (no sigterm, you either tell it to exit via ipc and wait, or you kill it).

@ionelmc

This comment has been minimized.

Member

ionelmc commented Jul 17, 2017

How do you shutdown the executor in coala?

@Makman2

This comment has been minimized.

Makman2 commented Jul 17, 2017

Thanks for the quick response ;)

I'm waiting until all remaining tasks are processed. I wait with event_loop.run_forever() until event_loop.stop() is called. After that event_loop.close() is called for cleanup.
(See https://github.com/coala/coala/blob/8f4113a2f471acf4c0121199c6ebb9b29dfa84ff/coalib/core/Core.py#L360)

@Makman2

This comment has been minimized.

Makman2 commented Jul 21, 2017

Is this considered a "good" shutdown?

@ionelmc

This comment has been minimized.

Member

ionelmc commented Jul 21, 2017

I doubt your ioloop magically figures out that executor.shutdown() needs to be run at some point ... try fixing that first.

@Makman2

This comment has been minimized.

Makman2 commented Jul 21, 2017

Damn yeah, that makes totally sense... I've actually forgotten that I always pass the executor explicitly to event_loop.run_in_executor, so how shall it know about my executor's lifetime...

I'm currently trying that out on the CI, I hope that does the trick :D

@Makman2

This comment has been minimized.

Makman2 commented Jul 21, 2017

No effect, tried to shutdown the executor manually at different places (inside the done-callback of my futures before and after event_loop.stop(), immediately before event_loop.close()), and also tried to register the executor as default (with event_loop.set_default_executor) with the hope that the event-loop takes the shutdown over completely.

@Makman2

This comment has been minimized.

Makman2 commented Jul 29, 2017

Any other ideas? :/

(I did workaround the problem now by executing my tests also with a ThreadPoolExecutor, where coverage data is correctly merged)

@ionelmc

This comment has been minimized.

Member

ionelmc commented Jul 29, 2017

@Makman2 seems the branch is gone? can you republish it somewhere so I can try it?

@Makman2

This comment has been minimized.

Makman2 commented Jul 29, 2017

Oh I accidentally removed it... my bad sorry :/
Recreated problem on coala/coala#4589
(The PR shuts down the executor, but without effect like mentioned above)

@Makman2

This comment has been minimized.

@ionelmc

This comment has been minimized.

Member

ionelmc commented Jul 29, 2017

Well I've tried to run it locally but it has failed tests and the whole suite timeout (no coverage report). Please provide a more minimal reproducer.

@Makman2

This comment has been minimized.

Makman2 commented Jul 29, 2017

You can only run the relevant core tests by doing py.test -k core. Then you don't need to run the complete test suite ;) (note that you will get many files not covered 100%, but the relevant files mentioned above - FileBear, ProjectBear and DependencyBear - have the same missing lines as running the whole test suite)

@Makman2

This comment has been minimized.

Makman2 commented Jul 29, 2017

PS: To run the tests you need to install the requirements:

pip3 install -r requirements.txt -r test-requirements.txt
@ionelmc

This comment has been minimized.

Member

ionelmc commented Jul 30, 2017

What I had in mind is something that don't involve your codebase or your test suite. At least nail it down for me to a single test. "core" selects a ton of borken tests.

@Makman2

This comment has been minimized.

Makman2 commented Jul 30, 2017

This takes a while, I hope I can reproduce it^^

@ionelmc

This comment has been minimized.

Member

ionelmc commented Jul 30, 2017

Are you saying the problem doesn't reproduce if you make a small test with just the executor?

@ionelmc

This comment has been minimized.

Member

ionelmc commented Jul 30, 2017

Anyway, so I've managed to find a problem with a test like this:

import os
from concurrent.futures import ProcessPoolExecutor


def my_func(_):
    print('FUNC:', [i for i in os.environ if i.startswith('COV')])
    a = 1
    b = 2
    return a + b


def test_mp():
    print('PARENT:', [i for i in os.environ if i.startswith('COV')])

    with ProcessPoolExecutor() as executor:
        assert list(executor.map(my_func, [1])) == [3]

It turns out if you use bare --cov the mp subprocess will not have the COV_CORE_SOURCE set cause of some weird windows problem: if env var is empty then it's gonna be unset in the child. A fix will follow shortly.

PS. Don't make bug reports like this no more. I'll try to look over other peoples projects but this is taking it too far.

ionelmc added a commit that referenced this issue Jul 30, 2017

@ionelmc

This comment has been minimized.

Member

ionelmc commented Jul 30, 2017

Try the latest master.

@Makman2

This comment has been minimized.

Makman2 commented Jul 30, 2017

Yes the commit was the fix. Thanks 👍

@Makman2 Makman2 closed this Jul 30, 2017

bors bot added a commit to rehandalal/therapist that referenced this issue Sep 6, 2018

Merge #31
31: Update pytest-cov to 2.6.0 r=rehandalal a=pyup-bot


This PR updates [pytest-cov](https://pypi.org/project/pytest-cov) from **2.5.1** to **2.6.0**.



<details>
  <summary>Changelog</summary>
  
  
   ### 2.6.0
   ```
   ------------------

* Dropped support for Python &lt; 3.4, Pytest &lt; 3.5 and Coverage &lt; 4.4.
* Fixed some documentation formatting. Contributed by Jean Jordaan and Julian.
* Added an example with ``addopts`` in documentation. Contributed by Samuel Giffard in
  `195 &lt;https://github.com/pytest-dev/pytest-cov/pull/195&gt;`_.
* Fixed ``TypeError: &#39;NoneType&#39; object is not iterable`` in certain xdist configurations. Contributed by Jeremy Bowman in
  `213 &lt;https://github.com/pytest-dev/pytest-cov/pull/213&gt;`_.
* Added a ``no_cover`` marker and fixture. Fixes
  `78 &lt;https://github.com/pytest-dev/pytest-cov/issues/78&gt;`_.
* Fixed broken ``no_cover`` check when running doctests. Contributed by Terence Honles in
  `200 &lt;https://github.com/pytest-dev/pytest-cov/pull/200&gt;`_.
* Fixed various issues with path normalization in reports (when combining coverage data from parallel mode). Fixes
  `130 &lt;https://github.com/pytest-dev/pytest-cov/issues/161&gt;`_.
  Contributed by Ryan Hiebert &amp; Ionel Cristian Mărieș in
  `178 &lt;https://github.com/pytest-dev/pytest-cov/pull/178&gt;`_.
* Report generation failures don&#39;t raise exceptions anymore. A warning will be logged instead. Fixes
  `161 &lt;https://github.com/pytest-dev/pytest-cov/issues/161&gt;`_.
* Fixed multiprocessing issue on Windows (empty env vars are not passed). Fixes
  `165 &lt;https://github.com/pytest-dev/pytest-cov/issues/165&gt;`_.
   ```
   
  
</details>


 

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/pytest-cov
  - Changelog: https://pyup.io/changelogs/pytest-cov/
  - Repo: https://github.com/pytest-dev/pytest-cov
</details>



Co-authored-by: pyup-bot <github-bot@pyup.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment