Reduce coverage warnings#140
Conversation
|
That certainly reduced the numer of But it still writes 13 |
|
It looks like the remaining warning is related to test_image.py. It is an elusive one though. I'll dive a bit deeper tomorrow |
tmillenaar
force-pushed
the
change_coverage_handling
branch
3 times, most recently
from
971ca96 to
d76a08d
Compare
tmillenaar
force-pushed
the
change_coverage_handling
branch
from
d76a08d to
2dca29c
Compare
tmillenaar
force-pushed
the
change_coverage_handling
branch
from
f104954 to
642026c
Compare
|
That was quite the investigation. Had I know it was this involved I would not have tried to make the test run in parallel. The warnings were all related to the use of multiprocessing in PySE which clashed the use of multiprocessing of pytest-xdist. I had to find all occurences in the test suite that use the new config and set allow_multiprocessing to False. It is tricky to find all these cases so to prevent new scenarios from popping up I added a check to the CICD build script that logs the stderr of the pytest run and then later checks it for these warnings. It will then give a little explenation on how you might fix that. My hope is than that if someone introduces a new test that does not do this, the failed CI will point them in the right direction. Otherwise it would likely just go by unnoticed. It now all seems to work nicely, but there is the downside of course that the multiprocessing logic (though about two lines of code) will not be tested. This may be a reason to consider reverting back to the single threaded test suite. @HannoSpreeuw what do you think? |
|
Yeah, I can image that this may not have been worth the hassle of running the test suite in parallel. |
|
It seems you were forced to add a lot of complexity with very little upside. I'm inclined to oppose this merge request. As a result, I'm in favor of reverting #83 Sorry for all the good effort you put into this — but I guess that’s part of the job. 🤦 |
|
Agreed, that was my conclusion too. I did at least learn a bit more about the implications of using multiprocessing ^^' |
|
PR for reverting pytest-xdist: #141 |
2 participants
PR #83 introduced coverage warnings. I thought I addressed those but that was not quite the case, see #139 (comment) .
In this PR I will make another attempt. It seems to me the following was happening:
In test_pyse.py we call the pyse cli using a subprocess. The reason to do it this way was to also include the CLI interface in the test, but it does mean that pytest-cov is not able to gather any information regarding the code coverage. Since no other parts of the package are executed, the coverage files regarding theses tests are empty. A warning is raised at the end when pytest-cov reads all .coverage files to combine the statistics. This is something pytest-cov does not handle (see: ), but we can twiddle a bit with the settings to the coverage package that pytest-cov is using under the hood by supplying a .coveragerc. This also only seems to work if it is placed in the root folder which is the mistake I made before. After this it seems that one warning remains, that I have not been able to track down yet.