fix: Fix coverage report for application with multiprocessing

[JCHacking]

When a test is performed with code coverage in an application that has multiprocessing processes, it does not come out well in the report.

This happens because each subprocess creates a .coverage, therefore it is necessary to combine them to generate the unified .coverage and thus to be able to generate the report well.

For it the solution that I have applied has been:

• That it always tries to combine files (That's why I have taken it out of the if, since it only tried to combine it when using the mp plugin).
• That if there are no files to combine it does not fail, since it means that they are already unified because it has not been given the case of having several files.

[sirosen]

Thanks for the contribution!

I need to think about this one. I'm considering merging it, since I agree that it's not harmful in a wide variety of scenarios, but don't want to do so recklessly.

The situation I'm considering, which gives me pause, is someone running a matrix style build (e.g. with tox, nox, or hatch) and then manually combining the results at the end with coverage combine.

[JCHacking]

You are right, it is important to assess these scenarios to avoid possible unwanted effects.

As far as I know, nose2 runs and does both the coverage analysis and the reporting, so I don't see how it would be possible for a client to do the combination manually in between (I think it is mandatory to specify some reporting format).

On the other hand looking at the options of the command "coverage combine" I see that there is no way to do like in code would be "strict=true" so maybe it does not fail to have previously done the combine (I have not tested it).

[sirosen]

I thought this through a little this morning and I think I would prefer for this to be an opt in config for the coverage plugin. That would do the combine even outside of mp-mode and would retain the strict flag.

The coverage plugin gives nose2 control over your coverage invocations, and if the behavior is going to change or become ambiguous, it should be exposed to the user through the plugin config. That way, if someone runs into a problem, they can always dig into the config to "undo" whatever new setting is used.
Figuring out what the default behavior should be is more nuanced, but I'd tend towards a default which matches current behavior for most things -- this is the flavor of legacy project which has more old users than new, so one of the priorities is not to break them.

---

If you agree and you'd like to work on that, I'm happy to wait for updates here (or provide guidance on what to change), or I can build on top of your PR to do the change -- you'll still be credited in the changelog, of course!

Also, thanks for injecting fresh energy into this project with PRs, ideas, and comments!

[JCHacking]

Sounds good to me, I just added a commit with the flag added to the configuration.

I have added to the coverage class the property reading, and with default value if not found of False.
Then in the method, I do an if to combine if it is with mp or if it has the flag enabled. The strict I put it equal to mp so that it does not break anything with the previous implementation.

[sirosen]

This looks good! I'm poised to merge, but I'll need to work up a test case and changelog entry. If you're able to tackle writing one or two tests for this, that would be a huge help. If not, I'll merge when I'm able to write some tests.

[JCHacking]

Sure, I will try to implement 2 tests, one with the flag and one without.

[JCHacking]

I have tried to run the tests, but I can't get it to work with the flag. I always get the same coverage...

I am still trying to make it work, but I also find it strange that in the coverage result, in missing lines I get the ones I have edited in the coverage plugin.

I have also added to the .gitignore the * at the end of the file, so that when it is something multiprocessing it does not added to git.

[sirosen]

I think coverage reporting within the testsuite might not work correctly -- perhaps due to the way that the testsuite is invoking it, and improper isolation between the nose2 testsuite itself and the subprocess used to simulate a run with coverage. I see an old test marked with a skip for similar reasons: it apparently works when run manually, but not when run under the testsuite.

You've been generous with your time as a contributor, and I want to get this change in so that you can get the benefit of it. I don't think I'll have time to fully context-switch in to this project today, but in the next few days I plan to pick up this branch and manually run your test scenarios to make sure they work when I check. After that, we'll mark them to skip and merge, with a known issue around the testsuite behavior. I can then release, and treat it as a bug that we're not able to properly test this behavior change.

Let me know if that sounds good. It's not ideal, but it's a way to make progress and avoid blocking you on what might be a legacy problem in the testsuite for this codebase.

[JCHacking]

Thank you very much!!! It seems to me a good solution to be able to use this feature.

[sirosen]

I added a fix on top of this (#605) which gets these tests working! Feel free to check it out. It seems to be working nicely and I plan to merge as soon as CI passes.

I'll make sure to get a shoutout to you in the changelog and we'll hopefully have this released in just a few hours-to-days. 😄

[JCHacking]

Great!!! Thank you very much for the help!!!