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

setuponly: remove printing out/err from capman #6009

Merged

Conversation

yoavcaspi
Copy link
Contributor

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

@blueyed
Copy link
Contributor

blueyed commented Oct 20, 2019

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
Copy link
Contributor

blueyed commented Oct 20, 2019

(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
Copy link
Contributor

blueyed commented Oct 20, 2019

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.

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, 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
Copy link
Contributor

blueyed commented Oct 20, 2019

@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
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
Copy link
Contributor

blueyed commented Oct 22, 2019

@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
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