Skip to content
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

Proposal: pytest-cov should do less #337

Open
nedbat opened this issue Sep 10, 2019 · 33 comments

Comments

@nedbat
Copy link
Collaborator

commented Sep 10, 2019

I've been working in the pytest-cov code recently to get it to support coverage.py 5.0 (pull request: #319), and also reviewing #330. The problems that have come up have brought me to a conclusion: pytest-cov does too many things.

The current code attempts to be a complete UI for coverage. I think this is a mistake. Pytest-cov should limit itself to those functions that can only be done within a pytest plugin. In particular, the reporting options of pytest-cov add no value over just using coverage directly to generate reports.

Pytest-cov is no one's passion project. There are a few of us that would like to move it forward, but want to do it efficiently. Removing functionality from pytest-cov would make it easier to maintain.

If we keep to the current model of pytest-cov being a complete UI for coverage.py, then every time something is added to coverage.py, the pytest-cov plugin needs to be updated. As an example, we just added a new report type to coverage.py (JSON). Why do we need to add options to pytest-cov to support it?

Pytest is a test runner. Its job is to run tests. The pytest-cov plugin should limit itself to integrating the run phase of coverage into the test running process. Reporting is a separate step that is better separately.

The current design leads to tortured option syntax in an attempt to squeeze everything into the pytest command line. There's no need. Remove options that aren't related to running, and let some options be specified in configuration files. Sophisticated test suites already have configuration files. Let's simplify pytest-cov.

Thoughts?

@nedbat

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 10, 2019

@nedbat

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 10, 2019

@jezdez Is there something i can do to clear up the confusion (if that's what the emoji means!)? I may have written too fast and left out some context.

@jezdez

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

I absolutely understand the "simple is better than complex" idea in this proposal and I support you in making maintenance easier. 👍🏻

I definitely worry about having to know the coverage CLI tool as well as setting up pytest-cov to generate terminal output though. At the moment it's one step in a very pytest-typical config setup (updating addopts in pytest.ini) and you're proposing two steps to get the same result, addopts and another way to run coverage report. Given how difficult some CI environments are (e.g. path problems, incompatible dependencies etc) I think this change would basically trade lower maintenance friction on your side with a higher barrier of ease of use. Not saying that's a bad trade, just that it would make sense to maybe find a way to alleviate that higher barrier for users.

Alternatively to your suggestion would there be a way to instead implement a simplified version of the coverage API in pytest-cov? Only cover terminal reporting in pytest-cov for example and document how to do more complex reporting via the coverage CLI?

@graingert

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

I'd be in favour of the coverage API providing a method like .summary() such that this code would pass the tests: 5493d82

@graingert

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

would be some wiggle room in how cov_fail_under would be passed to summary of course, or it could handle the cov_fail_under entirely and raise an exception after having finished all the writing

@nedbat

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 10, 2019

  1. A few people have mentioned the convenience of having one command (pytest --cov-etc) that will run tests and produce coverage reports. I understand that convenience, but a simple shell script would do the same job. I don't think it makes sense for pytest-cov to implement a new but worse UI for coverage.py to get that convenience.

I could compromise on @jezdez's idea of keeping text reporting, and leaving other reporting to the coverage.py command line. How would other people feel about that?

  1. I don't understand how a new API method in coverage.py will solve the problem? The UI would still be a set of baroque options in pytest-cov, that would need to be maintained, extended, and tested.

BTW: I'm very glad for the discussion! :)

@somacdivad

This comment has been minimized.

Copy link

commented Sep 10, 2019

As a frequent user of Pytest-cov, I'm strongly in favor of reducing its features.

And I agree with @jezdez that keeping the console reports is worth thinking about. It's literally the only reporting feature I use with Pytest-cov, if I use it for reporting at all, and I can see how it's handy enough to keep around.

The benefits of the Pytest-cov reporting UI always felt marginal to me, at best. At worst they just add bloat to my Pytest commands.

@okken

This comment has been minimized.

Copy link

commented Sep 10, 2019

simplifying the plugin would be great. Allowing text reporting would be preferable to not.

@gaborbernat

This comment has been minimized.

Copy link

commented Sep 10, 2019

You get my vote to go down this path with pytest cov 3 👍

@maurobaraldi

This comment has been minimized.

Copy link

commented Sep 11, 2019

What about two "versions" of plugin: pytest-cov and pytest-cov (enhanced).Ok, it does not much sense for me too, an enhanced version from a plugin, but the version could be like a branch from vcs project.

Maybe, this would be like a experiment, to analyse how many people miss this "enhanced features".

@nedbat

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 11, 2019

@maurobaraldi I'm not sure who would maintain the enhanced one. If I only contribute to the plain version (to add support for new coverage.py features), then the enhanced won't be a superset of the plain version. That sounds extra-confusing. :(

@loechel

This comment has been minimized.

Copy link

commented Sep 11, 2019

It may be good for the discussion to talk about the affected API and options and common use cases and configurations.

I think it should be differentiated between three major useages:

  • direct call of pytest
  • usage of tox
  • usage of CI/CD with coveralls

The direct useage of pytest is for simplicity and speedup of concurrent testing / subprocesses

Usage of tox may follow two mayor config styles:

as described in coverage docs:

[testenv]
...
commands =
    pytest --cov=src --cov=tests --cov-report=xml

setenv =
  COVERAGE_FILE=.coverage.{envname}

deps =
    pytest
    pytest-cov

[testenv:coverage]
skip_install = true

deps =
    coverage

setenv =
  COVERAGE_FILE=.coverage

commands =
    coverage erase
    coverage combine
    coverage html
    coverage xml
    coverage report --fail-under=100.0

or as documentet in pytest-cov docs:

[testenv]
commands = pytest --cov --cov-append --cov-report=term-missing
deps =
    pytest
    pytest-cov
depends =
    {py27,py36}: clean
    report: py27,py36

[testenv:report]
deps = coverage
skip_install = true
commands =
    coverage report
    coverage html

[testenv:clean]
deps = coverage
skip_install = true
commands = coverage erase

My question is, if cov-report options are removed, would both approches still work. Are other options as terminal and xml actually used?
May it be possible to delegate the whole reporting to the coverage framework itself?

Could it be an option make pytest-cov just a commad wrapper around coverage and do noting itself anymore?

@nedbat

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 11, 2019

Could it be an option make pytest-cov just a commad wrapper around coverage and do noting itself anymore?

@loechel, there are important things pytest-cov does that only it can do: managing the distribution and re-collection of coverage data under xdist; signaling when tests begin and end for who-tests-what; probably a few other things. It should continue to do those things. I don't see the value in pytest-cov being a wrapper around coverage.py's other functionality.

@nedbat

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 15, 2019

There were 26 +1's on the top description. I don't know how to see the names in the GitHub UI. I had to use the API to get them: blueyed, davidism, sethmlarson, gvoysey, styvane, boxed, CodeMouse92, yeraydiazdiaz, ionelmc, RazerM, timofurrer, ivoflipse, somacdivad, obestwalter, okken, dcoles, otrenav, Stranger6667, gaborbernat, mblayman, eddieantonio, sivy, merwok, fschulze, jab, gkapfham.

@ionelmc

This comment has been minimized.

Copy link
Member

commented Sep 20, 2019

Mkay so I'll add my feedback:

  • If we do a pytest-cov 3.0 that removes shitton of features then we'd have more things to do: spending time on arguing what to remove, explaining why was it removed and ensuring that removal doesn't make the plugin completely unusable. So the questions is who offers time to fight the bulk of the user complaints.
  • It's a good idea architecturally to avoid implementing features that should be, could be or are in coveragepy.
  • I think we should start with something less radical: drop compatibility with weird or old combos or dependencies: only latest pytest, only latest coveragepy, only latest python and so on.

I do have the same "things are not optimal" feeling, that's why I +1.

@nedbat

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 21, 2019

I hear what you are saying about dealing with the fallout.

I think probably everyone is on-board with dropping support for older everything (though I would like to keep Python 2.7 in the mix for now). Let's assume that is happening no matter what.

Then we have to decide what else to do. Here are some possibilities, a gradient of severity:
A. Nothing more than that. (This is your third bullet.)
B. Keep everything working as-is, but add warnings to features that will be dropped. So if you use --cov-report=html, you will get a warning, and an HTML report.
C. Drop features we don't want any more, but print helpful messages about what happened. If you use --cov-report=html, you get a message saying, "This feature has been removed, use 'coverage html' instead."
D. Drop features completely. If you use --cov-report=html, you get an error message, "--cov-report: unrecognized option"

(Note: in the above, I used --cov-report=html as a proxy for any feature that has been dropped.)

While D would mean the leanest resultant code, I would lean toward option C, to help people transition.

@fschulze

This comment has been minimized.

Copy link

commented Sep 23, 2019

+1 on C

@offbyone

This comment has been minimized.

Copy link

commented Sep 24, 2019

I'd personally want the current reporting to be available as an API then; I use pytest to drive coverage extensively for a variety of reasons, and if pytest-cov loses those features I'll need to reimplement them. Wrapping a command line when I'm already in Python is not ideal; I'd like to be able to invoke them as code.

If so, that'd be fine; I'd be happy to have consistent coverage reporting control between test frameworks.

@nedbat

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 24, 2019

@offbyone I'm not sure what you mean by an API. Coverage.py has an API. Would that work? Keeping all the code in pytest-cov but exposing it in an API from the plugin only adds to the complexity, it doesn't reduce it.

@meejah

This comment has been minimized.

Copy link

commented Sep 24, 2019

coverage should inherit pytest-cov's ability to get coverage out of sub-proceses easily. Every time I (attempt) to do subprocess coverage, I re-discover a bunch of gotcha's with atexit, signals, environment and .pth files ;) that mean it doesn't "just work".

@blueyed

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

@nedbat

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 25, 2019

@meejah I am in favor of exploring the parts of pytest-cov that could be moved into coverage.py itself. To keep the process a little simpler now, I'm only talking here about dropping code from pytest-cov that is already in coverage.py.

The .pth/subprocess/etc stuff is gross and scary however it is done, but I understand that people find it frustrating.

@rixx

This comment has been minimized.

Copy link

commented Sep 25, 2019

I'm a frequent user of pytest and pytest-cov. I use the various reporting options (both terminal in various versions, and HTML) through pytest-cov, currently, as a matter of my regular development cycle. Thank you for soliciting opinions and making the discussion so open for everybody.

I, as a user, would be happy enough to deal with option C. I frequently work offline or on very bad internet, so being confronted with an error after an already tedious update woud be unpleasant, whereas having explicit instructions available would be an unexpectedly positive experience. (I assume I'm not speaking for a majority here, but maybe for a subset of devs that tend to be forgotten, occasionally.)

I'd understand if you choose option D instead, but I'd feel less happy about it. If adding instructions for the different flags is hard, then please make that fact very clear in the changelog – it would definitely reduce my annoyance if I ran into it by accident.

@offbyone

This comment has been minimized.

Copy link

commented Sep 25, 2019

@nedbat yes, probably. I wasn't saying it didn't, only that I need one to replace pytest-cov's reporting for in-process work :)

@ionelmc ionelmc referenced this issue Sep 26, 2019
@ionelmc

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

@meejah re subprocesses see: https://pypi.org/project/coverage_enable_subprocess/ - nedbat/coveragepy#367

pytest-cov does much more than that, and much better. It supports all packaging and installation forms (coverage_enable_subprocess does not).

Someone has suggested that there's no need for pytest-cov (ignoring subprocess stuff) since you can just run coverage run -mpytest, however that has some problems:

@merwok

This comment has been minimized.

Copy link

commented Sep 27, 2019

FWIW: I always use coverage run form, because I knew it before I started using pytest, I have the (incorrect?) notion that coverage needs to run first to setup its tracing hooks, and I like memorizing a generic way that works with any test runner or other command. I also visit the coverage doc regularly to see settings and news, so I guess in my mind coverage is its own tool, not just a test runner plugin.

I thought pytest-cov was sugar (only a shortcut) until I read #337 (comment), and tox already gives me a shortcut for coverage run + coverage report. Now I see that it’s useful for test suites with subprocesses or pytest-xdist.

Also, I also really enjoy the possibility of running bare pytest without virtualenv or coverage for maximum feedback speed; on client projects configured with pytest-cov it’s a little cumbersome to type pytest --no-cov=project to disable coverage report when you’re in a «many tests broken, fix one at a time» phase.

@rixx If you have pytest-cov installed, you also have coverage, and coverage help gives offline help about commands and options. Does that address your needs?

@rixx

This comment has been minimized.

Copy link

commented Sep 27, 2019

@rixx If you have pytest-cov installed, you also have coverage, and coverage help gives offline help about commands and options. Does that address your needs?

Not exactly. My use case is that I'm used to running pytest with --cov-report flags of various kinds. If those suddenly just throw errors at me, I won't know where to look. (Please note that I'd understand if that option is the one that wins out, I just think the other one will make a bunch of people pretty happy when upgrading.)

@merwok

This comment has been minimized.

Copy link

commented Sep 30, 2019

Throwing an idea out there: what if pytest-cov was not changed, and the new ideas implemented in a new plugin with another name? That way no automated upgrade can break command lines, maintainers will have to see warnings or announcements and decide to switch (the docs could help with translation of pytest-cov options to coverage.py commands).

@nedbat

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 9, 2019

@merwok I think I like the idea of changing the name. We could make one more release of pytest-cov, where it prints a warning that there will be no more updates to it, and that you should switch to pytest-coverage. Then pytest-coverage can simply remove all the obsolete options.

This has advantages: people have to explicitly switch over, and we don't need an awkward middle phase where the code is still complex, but doesn't do what people want.

@gaborbernat

This comment has been minimized.

Copy link

commented Oct 9, 2019

I think I like the proposal minus the warning part 👍

@gpfreitas

This comment has been minimized.

Copy link

commented Oct 10, 2019

I am a user, and the name change @merwok suggested is my favorite option if the UI or functionality changes significantly, for the reasons he and @nedbat explained.

I support any effort to notify the user of the changes and give clear directions about what to do. Seeing a warning about future changes would be a a nice touch and wouldn't bother me.

@hugovk

This comment has been minimized.

Copy link
Member

commented Oct 11, 2019

For comparison, pep8 was renamed to pycodestyle in June 2016, with a runtime warning added to pep8 in October 2017.

In September 2019, pep8 had nearly a million downloads, pycodestyle had 5.5m.

category percent downloads
2.7 46.19% 451,264
3.7 21.71% 212,100
3.6 20.77% 202,906
3.5 8.97% 87,607
3.4 1.07% 10,445
null 0.77% 7,486
3.8 0.25% 2,438
3.3 0.17% 1,695
2.6 0.09% 882
3.2 0.00% 48
3.9 0.00% 2
Total 976,873

Source: pip install -U pypistats && pypistats python_minor pep8 --last-month

category percent downloads
3.6 33.61% 1,849,079
3.7 31.72% 1,745,178
2.7 26.43% 1,454,039
3.5 5.94% 326,832
3.4 1.07% 58,727
null 0.81% 44,587
3.8 0.38% 20,789
2.6 0.04% 2,147
3.3 0.01% 288
3.9 0.00% 67
Total 5,501,733

Source: pip install -U pypistats && pypistats python_minor pycodestyle --last-month

Not sure what to conclude exactly! People still use the old one a lot. But renaming would be a clear signal and let you concentrate only on the new project and retire the old.

@ssbarnea

This comment has been minimized.

Copy link

commented Oct 15, 2019

I don't think that rename is necessary here but I cant help to notice that there is not even one downvote on the original issue. Rarely I see so much agreement. Go and and start removing stuff from it, I am myself busy with similar tasks on other projects. Less is more! ...especially for older projects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.