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

Bugfix/capsys with cli logging #3822

Merged
merged 12 commits into from
Aug 18, 2018

Conversation

Sup3rGeo
Copy link
Member

@Sup3rGeo Sup3rGeo commented Aug 16, 2018

Fixes #3819

  • Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.
  • Target the master branch for bug fixes, documentation updates and trivial changes.
  • Target the features branch for new features and removals/deprecations.
  • Include documentation when adding new features.
  • Include new tests or update existing tests when applicable.

Unless your change is trivial or a small documentation fix (e.g., a typo or reword of a small section) please:

  • Add yourself to AUTHORS in alphabetical order;

@coveralls
Copy link

coveralls commented Aug 16, 2018

Coverage Status

Coverage increased (+0.06%) to 92.597% when pulling 5cf7d1d on Sup3rGeo:bugfix/capsys-with-cli-logging into 7d4c4c6 on pytest-dev:master.

@Sup3rGeo Sup3rGeo requested a review from asottile August 17, 2018 15:34
Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

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

looks good, one minor thing I think :D

@contextlib.contextmanager
def disabled(self):
"""Temporarily disables capture while inside the 'with' block."""
if self._current_item is None:
Copy link
Member

Choose a reason for hiding this comment

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

can probably combine with the below and do:

if getattr(self._current_item, '_capture_fixture', None) is None:
    yield
else:
    # use self._current_item._capture_fixture directly 
    ...

else:
ctx_manager = _dummy_context_manager()

with ctx_manager:
Copy link
Member

Choose a reason for hiding this comment

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

nice, I love me some context managers!

@Sup3rGeo
Copy link
Member Author

Sup3rGeo commented Aug 18, 2018

Used your nice tip, also decided to keep saving to a fixture variable for personal style reasons.

Refactored also considering the previous capsys.disabled function.

@Sup3rGeo
Copy link
Member Author

It seems like I could merge but just waiting for a last OK.

@nicoddemus
Copy link
Member

Hi @Sup3rGeo, hold on please, I'm working on this branch with a few minor improvements which I decided to just go ahead with, if you are OK with that. 👍

Also change the context-manager to global_and_fixture_disabled to
better convey its meaning
@nicoddemus
Copy link
Member

nicoddemus commented Aug 18, 2018

Made some tweaks, let me know what you think:

  • Set self._current_item = None at the end of the pytest_runtest_* calls to avoid leaking the last item in the run.
  • Moved the dummy_context_manager to _pytest.compat so we can reuse it in capture and logging.
  • Renamed suspend to _suspend in CaptureFixture as that's an internal API.
  • Renamed CaptureManager.disabled to CaptureManager.global_and_fixture_disabled to better convey its meaning (this is internal so I think it is OK to break the abstraction at what constitutes "disabled" to improve code clarity).

@Sup3rGeo
Copy link
Member Author

That's great @nicoddemus, thanks!

Just one little question about the capture - why do we have both a global and a fixture capture?

This is related to your last item - will we ever want to disable global capture without disabling also the fixture captures? As at least, as I understand, the fixture capture is also part of the whole capture plugin (and CaptureManager.disable would just mean to disable the capture plugin altogether)?

But this is really not very important, I agree with merging the way it is now :)

@nicoddemus
Copy link
Member

Just one little question about the capture - why do we have both a global and a fixture capture?

The global capture is controlled by the --capture option and is independent of a cap* fixture being active. When a cap* fixture gets activated, it needs to "steal" the global capturing, even when --capture=no is used. Sorry, not sure if I've answered your question TBH.

And I agree this code is quite complex. I hope I can find some time in the future to study it and perhaps improve it, as it has grown organically over the years.

This is related to your last item - will we ever want to disable global capture without disabling also the fixture captures? As at least, as I understand, the fixture capture is also part of the whole capture plugin (and CaptureManager.disable would just mean to disable the capture plugin altogether)?

That's a fair question, that's why I wrote that disclaimer about this breaking the abstraction of the "disabled" method. Because this is an internal API I think it is OK to break the abstraction and be very explicit at what it does, so the caller is well aware that we are disabling global and fixture capturing with it, and takes that in account correctly. This is somewhat subjective though.

@Sup3rGeo
Copy link
Member Author

I think it is OK to break the abstraction and be very explicit at what it does, so the caller is well aware that we are disabling global and fixture capturing with it

Thanks for the explanation! It makes sense.

So if I understood correctly the purpose of the capsys/etc fixtures is really to steal the capture even from pytest itself. Otherwise we could just have provided tests access to the global capture from a fixture I guess.

@nicoddemus
Copy link
Member

So if I understood correctly the purpose of the capsys/etc fixtures is really to steal the capture even from pytest itself. Otherwise we could just have provided tests access to the global capture from a fixture I guess.

One of the reasons is that you can change capture methods: execute with --capture=sys and still use the capfd fixture.

@nicoddemus nicoddemus merged commit 28aff05 into pytest-dev:master Aug 18, 2018
@nicoddemus
Copy link
Member

Awesome, thanks @Sup3rGeo!

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.

stdout capture (capsys) does not seem to work with real-time cli logging
5 participants