Skip to content

setuponly: remove printing out/err from capman - #6009

Merged
nicoddemus merged 2 commits into
pytest-dev:masterfrom
yoavcaspi:fix_keyboardInterrupt_on_setup_show
Oct 20, 2019
Merged

setuponly: remove printing out/err from capman#6009
nicoddemus merged 2 commits into
pytest-dev:masterfrom
yoavcaspi:fix_keyboardInterrupt_on_setup_show

Conversation

@yoavcaspi

Copy link
Copy Markdown
Contributor

resolve #5906
Thanks for @blueyed @asottile for the great help.
Really appreciate it.

Comment thread testing/python/setup_only.py Outdated
@blueyed

blueyed commented Oct 20, 2019

Copy link
Copy Markdown
Contributor

LGTM.
Consider this on top:

diff --git i/testing/python/setup_only.py w/testing/python/setup_only.py
index 8e4e3ace8..ef165c153 100644
--- i/testing/python/setup_only.py
+++ w/testing/python/setup_only.py
@@ -276,7 +276,7 @@ def test_setup_show_with_KeyboardInterrupt_in_test(testdir):
         import pytest
         @pytest.fixture
         def arg():
-            assert True
+            pass
         def test_arg(arg):
             raise KeyboardInterrupt()
     """
@@ -303,7 +303,7 @@ def test_setup_show_with_KeyboardInterrupt_in_fixture(testdir):
         def arg():
             raise KeyboardInterrupt()
         def test_arg(arg):
-            assert True
+            pass
     """
     )
     result = testdir.runpytest("--setup-show", p, no_reraise_ctrlc=True)
@@ -311,3 +311,4 @@ def test_arg(arg):
     result.stdout.fnmatch_lines(
         ["*SETUP    F arg*", "*! KeyboardInterrupt !*", "*= no tests ran in *"]
     )
+    assert "TEARDOWN F arg" not in str(result.stdout)

Also please squash and force-push it then.

@blueyed

blueyed commented Oct 20, 2019

Copy link
Copy Markdown
Contributor

(I'm not really sure we need two tests here, which adds overhead to the test suite, but I guess it is OK).

@blueyed
blueyed requested a review from asottile October 20, 2019 15:34
@blueyed

blueyed commented Oct 20, 2019

Copy link
Copy Markdown
Contributor

Note: testing/python/setup_only_new.py::test_setup_show_with_KeyboardInterrupt_in_fixture does not fail on master, i.e. we do not really need it here to test the regression/issue.

@asottile asottile left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

looks good, I think we can delete test_setup_show_with_KeyboardInterrupt_in_fixture if it doesn't fail on master 👍

@nicoddemus
nicoddemus merged commit 16efa1b into pytest-dev:master Oct 20, 2019
@blueyed

blueyed commented Oct 20, 2019

Copy link
Copy Markdown
Contributor

@nicoddemus
Have you seen the previous comments??

blueyed added a commit to blueyed/pytest that referenced this pull request Oct 20, 2019
@yoavcaspi

Copy link
Copy Markdown
Contributor Author

@blueyed I think the fixture test does not add much overhead and can left to provide some guard refactoring of the setup_only internals.
what do you think? I don't mind delete the test.

@blueyed

blueyed commented Oct 22, 2019

Copy link
Copy Markdown
Contributor

@yoavcaspi
Yeah, it's not much by itself, but all those little things add up - so that you cannot easily run the whole suite locally already. Therefore I think it is good to restrict tests to the minimum being required.
It was removed in #6013 already, thanks for asking.
Also thanks for the contribution, of course! :)

@yoavcaspi
yoavcaspi deleted the fix_keyboardInterrupt_on_setup_show branch October 22, 2019 06:32
@yoavcaspi

Copy link
Copy Markdown
Contributor Author

great :-)

phloose pushed a commit to phloose/pytest that referenced this pull request Oct 22, 2019
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.

Crash when using --setup-show and KeyboardInterrupt

4 participants