More tolerant in certain circumstances where report generation was failed

[huyphan]

Back in version 2.2.1 we were more tolerant when something went wrong with coverage report generation:

        if not (self.failed and self.options.no_cov_on_fail):
            try:
                total = self.cov_controller.summary(terminalreporter.writer)
            except CoverageException as exc:
                terminalreporter.writer.write('Failed to generate report: %s\n' % exc)
                total = 0

With 2.5.1, we raise exception instead:

        if not self._is_slave(session) and self._should_report():
            try:
                self.cov_total = self.cov_controller.summary(self.cov_report)
            except CoverageException as exc:
                raise pytest.UsageError(
                    'Failed to generate report: %s\n' % exc
                )
            assert self.cov_total is not None, 'Test coverage should never be `None`'

Do you think it would make sense if we make it to be tolerant like the old days? I'm having some projects that are built on multiple Python implementations with the same code base. One of the implementation (Jython) doesn't support code coverage and will cause CoverageException. If we re-raise the exception here, the whole test will fail.

I can send a PR for this, but I would like to get some opinions first.

[nicoddemus]

Optionally a new ini option:

[pytest]
cov_fail_no_coverage = false

[ionelmc]

Not saying no but I'm not a huge fan of yet another option.

If coverage is broken on jython why are you running it there? what's the point?

And the old behavior could be brought back, sure, but then coverage failure suddenly become a silent problem. How will people notice and act upon it if it's just one line in a ocean of lines of the output?

[huyphan]

In my use case, I use the same code base (including setup.py and pytest configurations) to build the package under multiple Python implementations. I still want to have code coverage report in Python2, Python3 etc. I wish there were an easy way to change pytest parameters under different Python implementations.

I agree that bringing back the old behavior is not a good idea for the reason you mentioned. However making it a new option is an appropriate practice I guess. We can default that option to true to maintain the current behavior and still allow people to have a choice to ignore coverage issue.

[huyphan]

@ionelmc: Do you think that's fine to move forward with cov_fail_no_coverage ini option then?

[ionelmc]

Lemme dig a bit in the history. Maybe we can have the old behavior back again (without option).

[ionelmc]

Looks like this new behavior was introduced in 3656515 (#116).

I think it's safe-ish to revert. @davidszotten, you have anything against? It's about making this sort of change:

diff --git a/src/pytest_cov/plugin.py b/src/pytest_cov/plugin.py
index 05ac039..e02f6cd 100644
--- a/src/pytest_cov/plugin.py
+++ b/src/pytest_cov/plugin.py
@@ -238,9 +238,8 @@ class CovPlugin(object):
             try:
                 self.cov_total = self.cov_controller.summary(self.cov_report)
             except CoverageException as exc:
-                raise pytest.UsageError(
-                    'Failed to generate report: %s\n' % exc
-                )
+                terminalreporter.writer.write('Failed to generate report: %s\n' % exc)
+                self.cov_total = 0
             assert self.cov_total is not None, 'Test coverage should never be `None`'
             if self._failed_cov_total():
                 # make sure we get the EXIT_TESTSFAILED exit code

[davidszotten]

hi. thanks for pinging me. i'd like to reply in more depth than i have time for right now, but wanted to at least respond.

to me "failed to generate a report" feels like a failure condition, not something we should just warn about and silently continue

having said that i don't recall the exact reason i cared about this, which might have been mainly around not hiding the coverage output when fail-under triggered. presumably even with the patch above, given setting total to 0, fail-under would still result in an error if report generation failed

alternatively, maybe with more context we could help op pass arguments conditionally based on environment to not request options they expect to fail

[ionelmc]

Well there is PYTEST_ADDOPTS, which @huyphan could set for non-jython envs. Eg: PYTEST_ADDOPTS=--cov

[ionelmc]

Or alternatively use PYTEST_ADDOPTS=--no-cov just in the jython envs.

[huyphan]

Thanks @ionelmc. Yea I'm aware of those options, unfortunately the CI system that we are using is not flexible enough to allow setting different environment variables based on Python implementation.

I understand that this is something the build system should fix rather than pytest-cov. I went to pytest-cov first because it's more lightweight to fix it here, and because it used to work fine with our edge case.

I think that's totally fair if we don't want to follow this path, it wouldn't make much sense to change a community library just because of a single/rare use case.

[ionelmc]

It's not that I don't care about your usecase, but I want to know for sure you got no other way to deal with this. Take this example: #164

Most of the problems can be solved with some wrappers - lets talk about those first. I don't suppose your CI has hardcoded the pytest binary name as well? :)

[ionelmc]

@davidszotten another way, maybe too clever for its own good, could be enabling strict summary handling only if --cov-fail-under is used. Then again, one could argue that incorrect coverage configuration (resulting in no coverage) is a failure condition as well but we don't do or can do anything about that. That's why I'm suggesting dropping the strict .summary(...) handling

[huyphan]

@ionelmc: I totally got you :) It's just that I do see my use case a bit strange and narrow after thinking about it for a while.

My CI doesn't hardcode the pytest binary name, I will have to dig into it a bit to see how hard it really is to have some wrappers.

[ionelmc]

@huyphan @davidszotten so this change is going into a release soon, last chance to voice your concerns/disagreements.