Skip to content

Multiple cov fail under flags Fixes #323 #330

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.

Sign up for GitHub

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

Closed
graingert wants to merge 1 commit into from
Closed

Multiple cov fail under flags Fixes #323 #330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
multiple cov-failed-under flags
  • Loading branch information
@graingert
graingert committed Jan 8, 2020
commit 0a0be4db0733c2e3a270dd202f1720657a947875
22 changes: 12 additions & 10 deletions src/pytest_cov/engine.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,14 +99,18 @@ def sep(stream, s, txt):
out = '%s %s %s\n' % (s * sep_len, txt, s * (sep_len + sep_extra))
stream.write(out)

def summary(self, stream):
def summary(self, cov_fail_under, stream):
"""Produce coverage reports."""
total = None
if self.cov_report:
self.__summary(self, stream)

if not self.cov_report:
def total(cfu):
with _backup(self.cov, "config"):
return self.cov.report(show_missing=True, ignore_errors=True, file=_NullFile)
return self.cov.report(show_missing=True, ignore_errors=True, include=cfu.include, omit=cfu.omit, file=_NullFile)

return {cfu: total(cfu) for cfu in cov_fail_under}

def __summary(self, stream):
# Output coverage section header.
if len(self.node_descs) == 1:
self.sep(stream, '-', 'coverage: %s' % ''.join(self.node_descs))
Expand All @@ -133,7 +137,7 @@ def summary(self, stream):
skip_covered = isinstance(self.cov_report, dict) and 'skip-covered' in self.cov_report.values()
options.update({'skip_covered': skip_covered or None})
with _backup(self.cov, "config"):
total = self.cov.report(**options)
self.cov.report(**options)

# Produce annotated source code report if wanted.
if 'annotate' in self.cov_report:
Expand All @@ -145,7 +149,7 @@ def summary(self, stream):
# Coverage.annotate don't return any total and we need it for --cov-fail-under.

with _backup(self.cov, "config"):
total = self.cov.report(ignore_errors=True, file=_NullFile)
self.cov.report(ignore_errors=True, file=_NullFile)
if annotate_dir:
stream.write('Coverage annotated source written to dir %s\n' % annotate_dir)
else:
Expand All @@ -155,18 +159,16 @@ def summary(self, stream):
if 'html' in self.cov_report:
output = self.cov_report['html']
with _backup(self.cov, "config"):
total = self.cov.html_report(ignore_errors=True, directory=output)
self.cov.html_report(ignore_errors=True, directory=output)
stream.write('Coverage HTML written to dir %s\n' % (self.cov.config.html_dir if output is None else output))

# Produce xml report if wanted.
if 'xml' in self.cov_report:
output = self.cov_report['xml']
with _backup(self.cov, "config"):
total = self.cov.xml_report(ignore_errors=True, outfile=output)
self.cov.xml_report(ignore_errors=True, outfile=output)
stream.write('Coverage XML written to file %s\n' % (self.cov.config.xml_output if output is None else output))

return total


class Central(CovController):
"""Implementation for centralised operation."""
Expand Down
94 changes: 73 additions & 21 deletions src/pytest_cov/plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,56 @@ def validate_report(arg):
return values


def validate_fail_under(num_str):
try:
return int(num_str)
except ValueError:
return float(num_str)
class FailUnder(object):
@staticmethod
def __number(num_str):
if num_str.endswith('%'):
num_str = num_str[:-1]
try:
return int(num_str)
except ValueError:
return float(num_str)

@classmethod
def parse(cls, unparsed):
items = iter(unparsed.split(":"))
limit = cls.__number(next(items, None))

include = []
omit = []
for item in items:
if item.startswith("-"):
omit.append(item[1:])
continue
if item.startswith("+"):
include.append(item[1:])
continue
include.append(item)

return cls(
limit=limit,
include=include or None,
omit=omit or None,
)

@classmethod
def normalize(cls, unparsed):
return str(cls.parse(unparsed))

def __str__(self):
return ":".join(
['{}%'.format(self.limit)]
+ ['+{}'.format(v) for v in (self.include or [])]
+ ['-{}'.format(v) for v in (self.omit or [])]
)

def __repr__(self):
return "FailUnder.parse({})".format(self)

def __init__(self, limit, include=None, omit=None):
self.limit = limit
self.include = include
self.omit = omit


def validate_context(arg):
Expand Down Expand Up @@ -91,8 +136,7 @@ def pytest_addoption(parser):
group.addoption('--no-cov', action='store_true', default=False,
help='Disable coverage report completely (useful for debuggers). '
'Default: False')
group.addoption('--cov-fail-under', action='store', metavar='MIN',
type=validate_fail_under,
group.addoption('--cov-fail-under', nargs="?", action='append', default=[], metavar='MIN', type=FailUnder.normalize,
help='Fail if the total coverage is less than MIN.')
group.addoption('--cov-append', action='store_true', default=False,
help='Do not delete coverage but append to current. '
Expand Down Expand Up @@ -141,12 +185,14 @@ def __init__(self, options, pluginmanager, start=True):
self.pid = None
self.cov_controller = None
self.cov_report = compat.StringIO()
self.cov_total = None
self.cov_totals = None
self.failed = False
self._started = False
self._disabled = False
self.options = options

self._fail_under = [FailUnder.parse(v) for v in getattr(self.options, 'cov_fail_under', [])]

is_dist = (getattr(options, 'numprocesses', False) or
getattr(options, 'distload', False) or
getattr(options, 'dist', 'no') != 'no')
Expand Down Expand Up @@ -188,8 +234,8 @@ class Config(object):
self.cov_controller.start()
self._started = True
cov_config = self.cov_controller.cov.config
if self.options.cov_fail_under is None and hasattr(cov_config, 'fail_under'):
self.options.cov_fail_under = cov_config.fail_under
if not self._fail_under and hasattr(cov_config, 'fail_under'):
self._fail_under = [FailUnder(limit=cov_config.fail_under)]

def _is_worker(self, session):
return compat.workerinput(session.config, None) is not None
Expand Down Expand Up @@ -237,8 +283,8 @@ def _should_report(self):
return not (self.failed and self.options.no_cov_on_fail)

def _failed_cov_total(self):
cov_fail_under = self.options.cov_fail_under
return cov_fail_under is not None and self.cov_total < cov_fail_under
totals = self.cov_totals
return any(totals.get(cfu, 0) < cfu.limit for cfu in self._fail_under)

# we need to wrap pytest_runtestloop. by the time pytest_sessionfinish
# runs, it's too late to set testsfailed
Expand All @@ -257,7 +303,10 @@ def pytest_runtestloop(self, session):

if not self._is_worker(session) and self._should_report():
try:
self.cov_total = self.cov_controller.summary(self.cov_report)
self.cov_totals = self.cov_controller.summary(
self._fail_under,
self.cov_report,
)
except CoverageException as exc:
message = 'Failed to generate report: %s\n' % exc
session.config.pluginmanager.getplugin("terminalreporter").write(
Expand All @@ -266,8 +315,8 @@ def pytest_runtestloop(self, session):
warnings.warn(pytest.PytestWarning(message))
else:
session.config.warn(code='COV-2', message=message)
self.cov_total = 0
assert self.cov_total is not None, 'Test coverage should never be `None`'
self.cov_totals = {}
assert self.cov_totals is not None, 'Test coverage should never be `None`'
if self._failed_cov_total():
# make sure we get the EXIT_TESTSFAILED exit code
compat_session.testsfailed += 1
Expand All @@ -284,21 +333,24 @@ def pytest_terminal_summary(self, terminalreporter):
if self.cov_controller is None:
return

if self.cov_total is None:
if self.cov_totals is None:
# we shouldn't report, or report generation failed (error raised above)
return

terminalreporter.write('\n' + self.cov_report.getvalue() + '\n')
for cfu in self._fail_under:
if cfu.limit <= 0:
continue

if self.options.cov_fail_under is not None and self.options.cov_fail_under > 0:
failed = self.cov_total < self.options.cov_fail_under
total = self.cov_totals.get(cfu, 0)
failed = total < cfu.limit
markup = {'red': True, 'bold': True} if failed else {'green': True}
message = (
'{fail}Required test coverage of {required}% {reached}. '
'{fail}Required test coverage of {required} {reached}. '
'Total coverage: {actual:.2f}%\n'
.format(
required=self.options.cov_fail_under,
actual=self.cov_total,
required=cfu,
actual=total,
fail="FAIL " if failed else "",
reached="not reached" if failed else "reached"
)
Expand Down
52 changes: 52 additions & 0 deletions tests/test_pytest_cov.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import re
import subprocess
import sys
import textwrap
from itertools import chain

import coverage
Expand Down Expand Up @@ -421,6 +422,57 @@ def test_cov_min_float_value(testdir):
])


@xdist_params
def test_cov_min_multi(testdir, opts):
module = testdir.mkpydir("legacy")
module.join("uncovered.py").write(
textwrap.dedent(
"""\
import sut

def some_legacy_code():
strategy = sut.AbstractFactoryBean()
assert strategy.business_critical()
"""
)
)
module.join("covered.py").write(
textwrap.dedent(
"""\
def two_returner_strategy():
return 2
"""
)
)
testsubdir = testdir.mkdir("test")

testsubdir.join("test_legacy.py").write(
textwrap.dedent(
"""\
from legacy import covered

def test_two_returner_strategy():
assert covered.two_returner_strategy() == 2
"""
)
)
result = testdir.runpytest('-v',
'--cov=%s' % str(testdir.tmpdir),
'--cov-report=term-missing',
'--cov-fail-under=55.55',
'--cov-fail-under=100.0:test/*',
'--cov-fail-under=100.0:+test/*',
'--cov-fail-under=33.33:-test/*',
testsubdir, *opts.split())
assert result.ret == 0
result.stdout.fnmatch_lines([
"Required test coverage of 55.55% reached. Total coverage: 55.56%",
"Required test coverage of 100.0%:+test/* reached. Total coverage: 100.00%",
"Required test coverage of 100.0%:+test/* reached. Total coverage: 100.00%",
"Required test coverage of 33.33%:-test/* reached. Total coverage: 33.33%",
])
Copy link
Member

@ionelmc ionelmc Sep 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So to understand what the argument do - in this test it's like this right?

  • overall coverage must be at least 55.55%
  • test coverage must be 100%
  • test coverage must be 100% (alternative syntax with "+")
  • overall coverage but without tests must be at least 33.33%

Also if I understand it correctly "%" is optional, and "+" as well yes?

And --cov-fail-under=100:foo:bar means coverage must be 100% for foo and bar right? It should be part of the test suite.

There are few things that I think users will find confusing or produce undesired results:

  • the multiple syntax to obtain the same thing
  • the reporting discrepancy - you can set fail-under for something that you cannot see in the reporting - suddenly you can't know what to fix to make the suite pass

Do we really need the omit mode (the "-" syntax)?

Copy link
Member Author

@graingert graingert Sep 2, 2019 •
edited by ionelmc
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So to understand what the argument do - in this test it's like this right?

* overall coverage must be at least 55.55%

* test coverage must be 100%

* test coverage must be 100% (alternative syntax with "+")

* overall coverage but without tests must be at least 33.33%

Also if I understand it correctly "%" is optional, and "+" as well yes?

And --cov-fail-under=100:foo:bar means coverage must be 100% for foo and bar right? It should be part of the test suite.

There are few things that I think users will find confusing or produce undesired results:

* the multiple syntax to obtain the same thing

I don't think it's confusing. You get the same with +1 and 1

* the reporting discrepancy - you can set fail-under for something that you cannot see in the reporting - suddenly you can't know what to fix to make the suite pass

I don't believe you can

Do we really need the omit mode (the "-" syntax)?

Yes so you can get an idea of coverage for code without tests. This is the coverage result people ignore the test suite for.

Copy link
Member Author

@graingert graingert Sep 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most people upgrading to this syntax will have ignored tests globally and will still want to assert on that number

Copy link
Member

@ionelmc ionelmc Sep 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes so you can get an idea of coverage for code without tests. This is the coverage result people ignore the test suite for.

Most people have just one top level package thus omit is unnecessary.

I don't believe you can

The reporting show percentages for individual files, doesn't show any totals for packages, subpackages and so on. This is why I don't think this is a great idea.

@nedbat do you have any input on this syntax? I don't want to have something in pytest-cov that will never ever be part of coveragepy.

Copy link
Member Author

@graingert graingert Sep 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes so you can get an idea of coverage for code without tests. This is the coverage result people ignore the test suite for.

Most people have just one top level package thus omit is unnecessary

That doesn't follow:
Most people have just one top level package
thus some people do not have just one top level package
thus omit is necessary
Also omit is needed for the http://doc.pytest.org/en/latest/goodpractices.html#tests-as-part-of-application-code layout

I don't believe you can

The reporting show percentages for individual files, doesn't show any totals for packages, subpackages and so on. This is why I don't think this is a great idea.

No the reporting lists a percentage for each --cov-fail-under

@nedbat do you have any input on this syntax? I don't want to have something in pytest-cov that will never ever be part of coveragepy.

This interface is already available in coveragepy: you just run "coverage report" with different include/omit overrides



def test_cov_min_float_value_not_reached(testdir):
script = testdir.makepyfile(SCRIPT)

Expand Down