Repository: pytest-dev/pytest
Pull request: setuponly: remove printing out/err from capman
resolve #5906 
Thanks for @blueyed  @asottile for the great help.
Really appreciate it.



Problem statement:
Crash when using --setup-show and KeyboardInterrupt
When interrupting a test using Ctrl-C (KeyboardInterrupt/SIGINT), the "setuponly" plugin crashes due to some interaction with the capture manager.

Using pytest 5.2.0 (fresh install on Python 3.7 in a virtual env), given a dummy test:
```
import time
import pytest


@pytest.fixture()
def setup():
    print("Setup")
    yield
    print("Teardown")


def test_something(setup):
    time.sleep(60)
```

Run it with `pytest --setup-show test_teardown.py` and abort it while the test is running.

Stacktrace:
```
Traceback (most recent call last):
  File "venv/bin/pytest", line 10, in <module>
    sys.exit(main())
  File "venv/lib/python3.7/site-packages/_pytest/config/__init__.py", line 90, in main
    return config.hook.pytest_cmdline_main(config=config)
  File "venv/lib/python3.7/site-packages/pluggy/hooks.py", line 289, in __call__
    return self._hookexec(self, self.get_hookimpls(), kwargs)
  File "venv/lib/python3.7/site-packages/pluggy/manager.py", line 87, in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
  File "venv/lib/python3.7/site-packages/pluggy/manager.py", line 81, in <lambda>
    firstresult=hook.spec.opts.get("firstresult") if hook.spec else False,
  File "venv/lib/python3.7/site-packages/pluggy/callers.py", line 208, in _multicall
    return outcome.get_result()
  File "venv/lib/python3.7/site-packages/pluggy/callers.py", line 80, in get_result
    raise ex[1].with_traceback(ex[2])
  File "venv/lib/python3.7/site-packages/pluggy/callers.py", line 187, in _multicall
    res = hook_impl.function(*args)
  File "venv/lib/python3.7/site-packages/_pytest/main.py", line 228, in pytest_cmdline_main
    return wrap_session(config, _main)
  File "venv/lib/python3.7/site-packages/_pytest/main.py", line 221, in wrap_session
    session=session, exitstatus=session.exitstatus
  File "venv/lib/python3.7/site-packages/pluggy/hooks.py", line 289, in __call__
    return self._hookexec(self, self.get_hookimpls(), kwargs)
  File "venv/lib/python3.7/site-packages/pluggy/manager.py", line 87, in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
  File "venv/lib/python3.7/site-packages/pluggy/manager.py", line 81, in <lambda>
    firstresult=hook.spec.opts.get("firstresult") if hook.spec else False,
  File "venv/lib/python3.7/site-packages/pluggy/callers.py", line 203, in _multicall
    gen.send(outcome)
  File "venv/lib/python3.7/site-packages/_pytest/terminal.py", line 650, in pytest_sessionfinish
    outcome.get_result()
  File "venv/lib/python3.7/site-packages/pluggy/callers.py", line 80, in get_result
    raise ex[1].with_traceback(ex[2])
  File "venv/lib/python3.7/site-packages/pluggy/callers.py", line 187, in _multicall
    res = hook_impl.function(*args)
  File "venv/lib/python3.7/site-packages/_pytest/runner.py", line 75, in pytest_sessionfinish
    session._setupstate.teardown_all()
  File "venv/lib/python3.7/site-packages/_pytest/runner.py", line 326, in teardown_all
    self._pop_and_teardown()
  File "venv/lib/python3.7/site-packages/_pytest/runner.py", line 299, in _pop_and_teardown
    self._teardown_with_finalization(colitem)
  File "venv/lib/python3.7/site-packages/_pytest/runner.py", line 319, in _teardown_with_finalization
    self._callfinalizers(colitem)
  File "venv/lib/python3.7/site-packages/_pytest/runner.py", line 316, in _callfinalizers
    raise val.with_traceback(tb)
  File "venv/lib/python3.7/site-packages/_pytest/runner.py", line 307, in _callfinalizers
    fin()
  File "venv/lib/python3.7/site-packages/_pytest/fixtures.py", line 867, in finish
    hook.pytest_fixture_post_finalizer(fixturedef=self, request=request)
  File "venv/lib/python3.7/site-packages/pluggy/hooks.py", line 289, in __call__
    return self._hookexec(self, self.get_hookimpls(), kwargs)
  File "venv/lib/python3.7/site-packages/pluggy/manager.py", line 87, in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
  File "venv/lib/python3.7/site-packages/pluggy/manager.py", line 81, in <lambda>
    firstresult=hook.spec.opts.get("firstresult") if hook.spec else False,
  File "venv/lib/python3.7/site-packages/pluggy/callers.py", line 208, in _multicall
    return outcome.get_result()
  File "venv/lib/python3.7/site-packages/pluggy/callers.py", line 80, in get_result
    raise ex[1].with_traceback(ex[2])
  File "venv/lib/python3.7/site-packages/pluggy/callers.py", line 187, in _multicall
    res = hook_impl.function(*args)
  File "venv/lib/python3.7/site-packages/_pytest/setuponly.py", line 44, in pytest_fixture_post_finalizer
    _show_fixture_action(fixturedef, "TEARDOWN")
  File "venv/lib/python3.7/site-packages/_pytest/setuponly.py", line 54, in _show_fixture_action
    out, err = capman.read_global_capture()
  File "venv/lib/python3.7/site-packages/_pytest/capture.py", line 141, in read_global_capture
    return self._global_capturing.readouterr()
AttributeError: 'NoneType' object has no attribute 'readouterr'
```

I'm not sure how the capture manager gets into this state where `_global_capturing` is None, one easy fix would be to make `read_global_capture ` check for None so it can always succeed. Fixing the underlying reason it gets into that state would of course be better.

Hints from issue discussion:
Can I implement a quick fix by checking for None in read_global_capture? Or should I figure out the reason it's None in the first place?
Better to figure out the reason.
Tried to understand the reason `global_capturing` is None.
`stop_global_capturing` called twice:
1. because this callback for KeyboardIntterupt https://github.com/pytest-dev/pytest/blob/master/src/_pytest/capture.py#L233
2. because the add_cleanup here: https://github.com/pytest-dev/pytest/blob/master/src/_pytest/capture.py#L50

My recommendation is to add a method to `CaptureManager` 
named `is_global_capturing_running()`
which will look something like this:
```
    def is_globally_capturing_running(self):
        return self._global_capturing is not None
```
and then use this method here:
https://github.com/pytest-dev/pytest/blob/master/src/_pytest/setuponly.py#L52
and here:
https://github.com/pytest-dev/pytest/blob/master/src/_pytest/setuponly.py#L75
as far as I understand the check there is in the case we disable `CaptureManager` so my recommendation is to add this check:

```
    if capman and capman.is_globally_capturing_running():
        capman.suspend_global_capture()
        out, err = capman.read_global_capture()
```
sg2m -- let's put that into a PR with a test and see how it goes!
Created https://github.com/pytest-dev/pytest/pull/5960 while looking at it.
It's not clear why it needs to be done in the first place, and none of the fast tests failed without it.

btw: for code references it is better to use the stable links (you can press "y" in GitHub's web UI to get those), since `master` changes in the future.
@yoavcaspi 
Please see https://github.com/pytest-dev/pytest/pull/5960 for a fix.
From what I've understood @asottile would like to help you with getting it ready.
Hi @blueyed,
I'll work with @asottile on it.

btw, didn't understand your explanation about copying the stable links, where I should click "y"?
if you press the `[y]` key on your keyboard it makes a permanent link to that code page :)

Commit message: setuponly: remove printing out/err from capman

Patch to review:
diff --git a/AUTHORS b/AUTHORS
index 9f6ee048d9a..e11400c1fa0 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -267,5 +267,6 @@ Wouter van Ackooy
 Xixi Zhao
 Xuan Luong
 Xuecong Liao
+Yoav Caspi
 Zac Hatfield-Dodds
 Zoltán Máté
diff --git a/changelog/5906.bugfix.rst b/changelog/5906.bugfix.rst
new file mode 100644
index 00000000000..f5ded6cd2e4
--- /dev/null
+++ b/changelog/5906.bugfix.rst
@@ -0,0 +1 @@
+Fix crash with ``KeyboardInterrupt`` during ``--setup-show``.
diff --git a/src/_pytest/setuponly.py b/src/_pytest/setuponly.py
index 70d6ed12f86..6393497481f 100644
--- a/src/_pytest/setuponly.py
+++ b/src/_pytest/setuponly.py
@@ -1,5 +1,3 @@
-import sys
-
 import pytest
 
 
@@ -51,7 +49,6 @@ def _show_fixture_action(fixturedef, msg):
     capman = config.pluginmanager.getplugin("capturemanager")
     if capman:
         capman.suspend_global_capture()
-        out, err = capman.read_global_capture()
 
     tw = config.get_terminal_writer()
     tw.line()
@@ -74,8 +71,6 @@ def _show_fixture_action(fixturedef, msg):
 
     if capman:
         capman.resume_global_capture()
-        sys.stdout.write(out)
-        sys.stderr.write(err)
 
 
 @pytest.hookimpl(tryfirst=True)
diff --git a/testing/python/setup_only.py b/testing/python/setup_only.py
index 7c871a9eead..1c3a1d368f2 100644
--- a/testing/python/setup_only.py
+++ b/testing/python/setup_only.py
@@ -267,3 +267,39 @@ def test_arg(arg):
     result.stdout.fnmatch_lines(
         ["*SETUP    F arg*", "*test_arg (fixtures used: arg)F*", "*TEARDOWN F arg*"]
     )
+
+
+def test_setup_show_with_KeyboardInterrupt_in_test(testdir):
+    """ Verifies that setups are shown and tests are executed even if there was a KeyboardInterrupt in a test. """
+    p = testdir.makepyfile(
+        """
+        import pytest
+        @pytest.fixture
+        def arg():
+            assert True
+        def test_arg(arg):
+            raise KeyboardInterrupt()
+    """
+    )
+    result = testdir.runpytest("--setup-show", p, no_reraise_ctrlc=True)
+    assert result.ret == 2
+    result.stdout.fnmatch_lines(
+        ["*SETUP    F arg*", "*test_arg (fixtures used: arg)*", "*TEARDOWN F arg*"]
+    )
+
+
+def test_setup_show_with_KeyboardInterrupt_in_fixture(testdir):
+    """ Verifies that setups are shown and tests are executed even if there was a KeyboardInterrupt in a fixture. """
+    p = testdir.makepyfile(
+        """
+        import pytest
+        @pytest.fixture
+        def arg():
+            raise KeyboardInterrupt()
+        def test_arg(arg):
+            assert True
+    """
+    )
+    result = testdir.runpytest("--setup-show", p, no_reraise_ctrlc=True)
+    assert result.ret == 2
+    result.stdout.fnmatch_lines(["*SETUP    F arg*", "*KeyboardInterrupt*"])


Code review comments:
diff_hunk: @@ -267,3 +267,39 @@ def test_arg(arg):
     result.stdout.fnmatch_lines(
         ["*SETUP    F arg*", "*test_arg (fixtures used: arg)F*", "*TEARDOWN F arg*"]
     )
+
+
+def test_setup_show_with_KeyboardInterrupt_in_test(testdir):
+    """ Verifies that setups are shown and tests are executed even if there was a KeyboardInterrupt in a test. """
+    p = testdir.makepyfile(
+        """
+        import pytest
+        @pytest.fixture
+        def arg():
+            assert True
+        def test_arg(arg):
+            raise KeyboardInterrupt()
+    """
+    )
+    result = testdir.runpytest("--setup-show", p, no_reraise_ctrlc=True)
+    assert result.ret == 2
+    result.stdout.fnmatch_lines(
+        ["*SETUP    F arg*", "*test_arg (fixtures used: arg)*", "*TEARDOWN F arg*"]
+    )
+
+
+def test_setup_show_with_KeyboardInterrupt_in_fixture(testdir):
+    """ Verifies that setups are shown and tests are executed even if there was a KeyboardInterrupt in a fixture. """
+    p = testdir.makepyfile(
+        """
+        import pytest
+        @pytest.fixture
+        def arg():
+            raise KeyboardInterrupt()
+        def test_arg(arg):
+            assert True
+    """
+    )
+    result = testdir.runpytest("--setup-show", p, no_reraise_ctrlc=True)
+    assert result.ret == 2
+    result.stdout.fnmatch_lines(["*SETUP    F arg*", "*KeyboardInterrupt*"])
path: testing/python/setup_only.py
text: @user1:
Thanks!  I usually find it useful to also match the "*= 1 failed/error in *".

@author:
I don't get this result in the tests.
Here is an output example.
```
# t.py
import pytest
@user2.fixture
def arg():
    assert True
def test_arg(arg):
    raise KeyboardInterrupt()
```
pytest --setup-show t.py
========================================================== test session starts ===========================================================
platform linux -- Python 3.6.7, pytest-3.8.1.dev2489+g2e11ea610.d20191019, py-1.8.0, pluggy-0.13.0
rootdir: /home/yocaspi/workspace/pytest, inifile: tox.ini
collected 1 item                                                                                                                         

t.py 
        SETUP    F arg
        t.py::test_arg (fixtures used: arg)
        TEARDOWN F arg

!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! KeyboardInterrupt !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
/home/yocaspi/workspace/pytest/t.py:6: KeyboardInterrupt
(to show a full traceback on KeyboardInterrupt use --full-trace)
========================================================= no tests ran in 0.31s ==========================================================


@user1:
Yeah, here it would be "*= no tests ran in *".
It basically ensures that the test finished, and that there were e.g. no warnings.

@user1:
Also I'd include `"*! KeyboardInterrupt !*"`.