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

#1478 Add --show-capture option #3176

Merged
merged 4 commits into from
Feb 9, 2018
Merged

#1478 Add --show-capture option #3176

merged 4 commits into from
Feb 9, 2018

Conversation

feuillemorte
Copy link
Contributor

@feuillemorte feuillemorte commented Jan 31, 2018

Task #1478

How it works:

We have a test:

def test_one():
    print('This is stdout!')
    assert 0

Run pytest:

pytest --tb=short

We got this:

tests/test_one.py F                                                                                                                                                                     [100%]
=================================================================================== short test summary info ===================================================================================
FAIL tests/test_one.py::test_one

========================================================================================== FAILURES ===========================================================================================
__________________________________________________________________________________________ test_one ___________________________________________________________________________________________
tests/test_one.py:3: in test_one
    assert 0
E   assert 0
------------------------------------------------------------------------------------ Captured stdout call -------------------------------------------------------------------------------------
This is stdout!
================================================================================== 1 failed in 0.03 seconds ===================================================================================

If we call pytest with option --no-stdout we will not see Captured stdout call section:

pytest --tb=short --no-stdout
tests/test_one.py F                                                                                                                                                                     [100%]
=================================================================================== short test summary info ===================================================================================
FAIL tests/test_one.py::test_one

========================================================================================== FAILURES ===========================================================================================
__________________________________________________________________________________________ test_one ___________________________________________________________________________________________
tests/test_one.py:3: in test_one
    assert 0
E   assert 0
================================================================================== 1 failed in 0.04 seconds ===================================================================================

P.S. stderr will be shown fine

@coveralls
Copy link

coveralls commented Jan 31, 2018

Coverage Status

Coverage increased (+0.01%) to 92.639% when pulling da5882c on feuillemorte:1478_no_stdout_option into f2fb841 on pytest-dev:features.

@@ -42,6 +42,9 @@ def pytest_addoption(parser):
action="store", dest="tbstyle", default='auto',
choices=['auto', 'long', 'short', 'no', 'line', 'native'],
help="traceback print mode (auto/long/short/line/native/no).")
group._addoption('--no-stdout',
Copy link
Member

Choose a reason for hiding this comment

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

Currently we have --no-print-logs:

  --no-print-logs       disable printing caught logs on failed tests.

So perhaps we might also have a --no-print-capture:

  --no-print-capture      disable printing captured stdout/stderr on failed tests.

Or perhaps:

--show-capture=no/stdout/stderr/both    controls how captured stdout/stderr is shown on failed tests. default is "both".

@The-Compiler @RonnyPfannschmidt @blueyed what's your opinion? I'm leaning on the second one because it seems more flexible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the second one is a good point for me :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I like --show-capture=no/stdout/stderr/both.

Copy link
Contributor Author

@feuillemorte feuillemorte Feb 1, 2018

Choose a reason for hiding this comment

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

@nicoddemus
Ok, I think I can try to fix. Is it good place for fix?

pytest/_pytest/terminal.py

Lines 623 to 629 in 89a55d8

def _outrep_summary(self, rep):
rep.toterminal(self._tw)
for secname, content in rep.sections:
self._tw.sep("-", secname)
if content[-1:] == "\n":
content = content[:-1]
self._tw.line(content)

It's very easy in this case because we have a list with stdout and stderr:

import sys


def test_one():
    print('This is stdout!')
    print('This is stdout!')
    print('This is stdout!')
    sys.stderr.write('This is stderr!')
    sys.stderr.write('This is stderr!')
    sys.stderr.write('This is stderr!')
    assert 0
[('Captured stdout call', 'This is stdout!\nThis is stdout!\nThis is stdout!\n'), ('Captured stderr call', 'This is stderr!This is stderr!This is stderr!')]

or here?

pytest/_pytest/capture.py

Lines 131 to 142 in 89a55d8

def pytest_make_collect_report(self, collector):
if isinstance(collector, pytest.File):
self.resume_global_capture()
outcome = yield
out, err = self.suspend_global_capture()
rep = outcome.get_result()
if out:
rep.sections.append(("Captured stdout", out))
if err:
rep.sections.append(("Captured stderr", err))
else:
yield

In this case I don't know how to send command option from terminal.py there

Copy link
Member

@nicoddemus nicoddemus Feb 1, 2018

Choose a reason for hiding this comment

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

I think applying the change in terminal.py is the way to go because other plugins (--junitxml for example) might be using rep.sections.

Copy link
Member

Choose a reason for hiding this comment

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

Of course we will need more tests for each variant now. 😁

@@ -0,0 +1 @@
Added `--no-stdout` feature. Stdout will not shown in terminal if you use this option. Only Stderr will shown.
Copy link
Member

Choose a reason for hiding this comment

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

This is a new feature (a new command-line option) so this file should be named 1478.feature.

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Lets change the option to --show-capture as discussed. 👍

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Thanks a lot @feuillemorte, this is looking good we seem to be almost there.

Please take a look at my comments. 👍

@@ -42,6 +42,10 @@ def pytest_addoption(parser):
action="store", dest="tbstyle", default='auto',
choices=['auto', 'long', 'short', 'no', 'line', 'native'],
help="traceback print mode (auto/long/short/line/native/no).")
group._addoption('--show-capture',
action="store", dest="showcapture",
choices=['no', 'stdout', 'stderr'],
Copy link
Member

Choose a reason for hiding this comment

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

You are missing the "both" option (which should also be the default).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I add "both" option it is not change a code, because "both" means without this option. is it really needed?

Copy link
Member

Choose a reason for hiding this comment

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

We do, the default value for options must be defined, all other options do, for example --capture, --color, any other multiple choice option really.

group._addoption('--show-capture',
action="store", dest="showcapture",
choices=['no', 'stdout', 'stderr'],
help="Print only stdout/stderr on not print both.")
Copy link
Member

Choose a reason for hiding this comment

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

This would read better IMHO: controls how captured stdout/stderr is shown on failed tests. default is "both".

@@ -0,0 +1,4 @@
Added `--show-capture` feature. You can choose 'no', 'stdout' or 'stderr' option.
Copy link
Member

Choose a reason for hiding this comment

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

We appreciate trying to write a long descriptive message here, but unfortunately towncrier (which we use to manage the changelog) does not allow for multi-paragraph entries.

I think this feature can be summarized thus:

New ``--show-capture`` command-line option that allows to specify how to display captured output when tests fail: ``no``, ``stdout``, ``stderr`` or ``both`` (the default).

Which is short and to the point.

Besides that, I think we also need to mention this on the docs, I believe capture.rst is the appropriate place for it. Just add a new paragraph to the Default stdout/stderr/stdin capturing behaviour section like this:

Default stdout/stderr/stdin capturing behaviour
---------------------------------------------------------

During test execution any output sent to ``stdout`` and ``stderr`` is
captured.  If a test fails its captured
output will be shown along with the failure traceback (this behavior 
can be configured by the ``--show-capture`` command-line option). 

Copy link
Member

Choose a reason for hiding this comment

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

@feuillemorte just realized that this bit should be added to the docs, would you mind doing it? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicoddemus I found print() =) removed...

@@ -823,6 +823,27 @@ def pytest_report_header(config, startdir):
str(testdir.tmpdir),
])

def test_show_capture(self, testdir):
Copy link
Member

Choose a reason for hiding this comment

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

We also need to test both (explicitly) and no.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Thanks @feuillemorte!

@taeunChoi
Copy link

I hope it goes faster. 👍

@nicoddemus nicoddemus merged commit 063e2da into pytest-dev:features Feb 9, 2018
@nicoddemus
Copy link
Member

@feuillemorte in the future, it helps a little to include a message like "Fixes #1478" either in the commit message or in the PR description, this way GH automatically closes the issue when it gets merged. 😁

You can read more about this here: https://help.github.com/articles/closing-issues-using-keywords/

Thanks again for the PR!

@twmr twmr changed the title #1478 Added --no-stdout option #1478 Add --show-capture option Feb 9, 2018
@feuillemorte feuillemorte deleted the 1478_no_stdout_option branch February 11, 2018 17:07
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.

5 participants