Skip to content

Makes coverage disabled message a debugging one #304

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

Closed
wants to merge 4 commits into from

Conversation

ssbarnea
Copy link
Member

Avoids poluting stdout with a warning message which is unlikely to be
caused by an accident (adding --no-cov switch)

Fixes: #284

Avoids poluting stdout with a warning message which is unlikely to be
caused by an accident (adding --no-cov switch)

Fixes: pytest-dev#284
@blueyed
Copy link
Contributor

blueyed commented Jun 24, 2019

It still adds the warning though - i.e. what is the point of logging + warning?

@ionelmc
Copy link
Member

ionelmc commented Jul 5, 2019

Maybe we should remove the terminalreporter.write( altogether. I don't see why we'd have that when we already have the warnings.warn.

@ssbarnea
Copy link
Member Author

ssbarnea commented Jul 5, 2019

@ionelmc @blueyed I am inclined to remove the entire block and not logging or printing anything related to the disabled status. How about this?

@ionelmc
Copy link
Member

ionelmc commented Jul 6, 2019

I think we should still have something. The idea is that the warning is there so people don't end up having --no-cov in pytest.ini, not notice it and then make spurious bug reports about coverage not working.

@ssbarnea
Copy link
Member Author

ssbarnea commented Jul 6, 2019

@ionelmc I guess we are in different teams here as I am trying to get rid of this spammy message. I guess this warnings was added to protect python-cov project maintainers from being spammed by false bug reports. It probably works, at the detriments of the users that are genuinely disabling coverage.

I guess we need something like https://www.amazon.co.uk/periwinkLuQ-Safety-Universal-Connector-Bayonet/dp/B07N82T6B9/ref=asc_df_B07N82T6B9/?tag=googshopuk-21&linkCode=df0&hvadid=310764720204&hvpos=1o1&hvnetw=g&hvrand=12836445195455804113&hvpone=&hvptwo=&hvqmt=&hvdev=c&hvdvcmdl=&hvlocint=&hvlocphy=1006964&hvtargid=pla-697507570464&psc=1

I may be biased here and but I really do not think we can really protect outselves from all things that could go wrong with the tool usage. I would rather change the new bug template to mentionn this issue and disable the warning.

@ionelmc
Copy link
Member

ionelmc commented Jul 7, 2019

I guess I don't understand the issue that needs to be fixed. I can understand removing the warning (it changes the test result warning count). But what's the terminal output removal for? What problem does it fix?

@ssbarnea
Copy link
Member Author

Let me try to explain, on my desktop I do have this in my user profile:

export PYTEST_ADDOPTS='--no-cov -s -x'

That counts as local developer settings applying to >100 projects I am using or contributing too, with clear resons for each: not interested in coverage as it creates lost of extra lines in console and I am forced to do extra scrolls to see stuff that mattered more (like a test failure), disabling std capture.

I added these parameters deliverately in order to be able to call pytest the way I want during development. It was was my decision to do so. Even better most projects using tox also respect that by passing PYTEST_OPTS, so I can do tox -e py27 and get the desired behavior. All of these without having to touch the code from the repository (very important aspect)

I know that you could argue that if you don't want coverage don't install the plugin, or even disable it. The bit issue is that this is impossible without altering the source code. That is because if I disable or uninstall it I will get 4/5 chances of not being able to run because: either the developer added some converage specific options to pytest.ini or to the call from inside tox.ini. This means that you still need the plugin installed in order to avoid the failure.

So now we are back to the question: How do I skip running coverage without poluting the console output?

I am not trying to say that this PR is the only way to address that issue, but I hope I explained the issue well enough so we can find a way to avoid that noise. This plugin should just respect the --no-cov option without considering it an error. Somehow I have the feeling that this would be like adding a 2nd prompt to rm -f which would ask the user I know that you asked for --force, but this is danggeresu, are your really really sure you want to do it? ;)

Example

$ pytest --cov=x --collect-only                                                                                                                                           [11:18:43]
================================================================================ test session starts ================================================================================
platform darwin -- Python 3.6.8, pytest-4.6.3, py-1.8.0, pluggy-0.12.0
rootdir: /Users/ssbarnea/rdo/selinux
plugins: forked-1.0.2, flaky-3.5.3, testinfra-1.19.0, helpers-namespace-2019.1.8, xdist-1.28.0, mock-1.10.4, allure-pytest-2.6.5, metadata-1.8.0, requests-mock-1.6.0, html-1.20.1.dev3+g67dc737, molecule-1.0.3.dev0+gbde2c20.d20190712, cov-2.7.1
collected 1 item
tests/roles/ensure-ansible/molecule/default/molecule.yml
  test

================================================================================= warnings summary ==================================================================================
/Users/ssbarnea/os/pytest-cov/src/pytest_cov/plugin.py:254
  /Users/ssbarnea/os/pytest-cov/src/pytest_cov/plugin.py:254: PytestWarning: Coverage disabled via --no-cov switch!
    warnings.warn(pytest.PytestWarning(message))

-- Docs: https://docs.pytest.org/en/latest/warnings.html
============================================================================ 1 warnings in 0.03 seconds =============================================================================

@blueyed
Copy link
Contributor

blueyed commented Jul 13, 2019

Just for information:

I know that you could argue that if you don't want coverage don't install the plugin, or even disable it. The bit issue is that this is impossible without altering the source code.

You can use -p no:cov (in PYTEST_ADDOPTS).

You say that this would then cause error when pytest-cov options are used, but on the other hand you should see errors already when pytest-cov is not installed (since --no-cov is not recognized then).

@ionelmc
Copy link
Member

ionelmc commented Jul 13, 2019

Oook so how about we remove the warning since that takes the most lines and keep the print (the terminalwriter stuff)? Fair compromise yes?

@ssbarnea
Copy link
Member Author

Of for me but with the remark that the print ling should not start with "WARNING". I do have some filters in the terminal which colorize it as a real warning. Some CI systems do parse stdout/stderr and grab all warnings based on regex, so I think it should be better be a "NOTICE:"? Once liner would a good compromise in that case.

@ionelmc
Copy link
Member

ionelmc commented Jul 14, 2019

It was red=True, bold=True before so what's the difference? I don't think anyone should ever use --no-cov on CI.

@ionelmc
Copy link
Member

ionelmc commented Sep 3, 2019

Ok so I've changed it to a notice and fixed the test. Everyone happy?

@ionelmc
Copy link
Member

ionelmc commented Sep 4, 2019

@ssbarnea well, does this fix your CI problem?

@ionelmc ionelmc mentioned this pull request Sep 4, 2019
@ionelmc
Copy link
Member

ionelmc commented Dec 6, 2019

So this was fixed differently (referenced commits), let me know if it ain't enough.

@vanschelven
Copy link

So this was fixed differently (referenced commits), let me know if it ain't enough.

I can't seem to find the commit that fixes this that actually made it into master

ionelmc added a commit that referenced this pull request Jun 1, 2020
…before --cov. Also remove some legacy pytest support and bump min version in setup.py. Closes #407. Ref #304.
ionelmc added a commit that referenced this pull request Jun 1, 2020
…before --cov. Also remove some legacy pytest support and bump min version in setup.py. Closes #407. Ref #304.
ionelmc added a commit that referenced this pull request Jun 12, 2020
…before --cov. Also remove some legacy pytest support and bump min version in setup.py. Closes #407. Ref #304.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pytest gives "Coverage disabled via --no-cov switch!" warning even --no-cov is not given!!
4 participants