print captured logs before entering pdb #3210

brianmaissy · 2018-02-12T20:07:40Z

Fixes #3204. This is conceptually a continuation of #3186 but is not actually dependent on it.

coveralls · 2018-02-12T20:36:42Z

Coverage increased (+0.03%) to 92.596% when pulling 9f3ea0e on brianmaissy:feature/added_printing_of_captured_logs_before_entering_pdb into 3bc8b50 on pytest-dev:features.

twmr · 2018-02-12T21:51:49Z

_pytest/debugging.py

@@ -85,6 +85,12 @@ def _enter_pdb(node, excinfo, rep):
    # for not completely clear reasons.
    tw = node.config.pluginmanager.getplugin("terminalreporter")._tw
    tw.line()
+
+    captured_logs = rep.caplog
+    if len(captured_logs) > 0:


You should also check if live-logging is enabled here, in which case it does not make sense to print the captured logs again.

I mentioned this here #3204 (comment) as a continuation of our discussion with @nicoddemus on the other thread. I think it makes sense to print whatever we would have printed if we didn't enter pdb. If live logging is enabled, we print the logs when the happen and also in the report.

I agree with @brianmaissy that just printing them here is more consistent, but I don't have strong feeling either way to be honest.

@Thisch what do you think? If you agree we can merge this. 👍

I think we can merge this. But I definitely want to change this "before entering pdb" behavior when capturing is off (-s) and/or live logging is enabled. I'll create a new issue for this with some screenshots.

nicoddemus

Thanks @brianmaissy!

brianmaissy · 2018-02-17T18:33:47Z

fixed merge conflict

nicoddemus

As commented in #3204 (comment), we should really only output the logs if --no-print-logs is not set. 👍

nicoddemus · 2018-02-17T23:00:46Z

_pytest/debugging.py

@@ -97,6 +97,11 @@ def _enter_pdb(node, excinfo, rep):
        tw.sep(">", "captured stderr")
        tw.line(captured_stderr)

+    captured_logs = rep.caplog
+    if len(captured_logs) > 0:
+        tw.sep(">", "captured logs")


Please check the option inside the if block:

if len(captured_logs) > 0: if not config.getoption('no_print_logs'): tw.sep(">", "captured logs") tw.line(captured_logs)

Doing the alternative:

if len(captured_logs) > 0 and not config.getoption('no_print_logs'):

Is incorrect because it will raise if the logging plugin is disabled with -p no:logging.

Which reminds me, we don't have a test to ensure pytest doesn't blow up if logging is disabled (-p no:logging), could you add one as well? Make sure to emit logging messages inside the sub-test, such a test would caught the error I mention above.

Aren't they equivalent because of lazy boolean expression evaluation?
But in any case it's better to be explicit.

I encountered something strange though. If --no-print-logs is given, log_print is set to False. But if it is not given, it contains None, instead of True as I would expect. From a look at the code it seems like this is because default is not passed through to addoption. Is this intentional?

pytest/_pytest/logging.py

Lines 82 to 91 in b486e12

def add_option_ini(option, dest, default=None, type=None, **kwargs):

parser.addini(dest, default=default, type=type,

help='default value for ' + option)

group.addoption(option, dest=dest, **kwargs)

add_option_ini(

'--no-print-logs',

dest='log_print', action='store_const', const=False, default=True,

type='bool',

help='disable printing caught logs on failed tests.')

Yesterday I discovered that a --show-capture option has been added recently (see #3176). IMO this option should supersede the --no-print-logs option (see #3233 and #3234, where I removed the --no-print-logs option in favor of --show-capture). The PR #3234 is based on this PR. Please have a look at this PR and tell me what you think.

Makes sense. I'll wait until you merge #3234, and then I'll update this PR accordingly?

As I said in #3234 (comment), we should not really remove the --no-print-logs option, we should deprecate it first.

But I guess that we don't need to use print_logs in debugging.py. We can use show_capture instead.

@brianmaissy I guess that we can close this PR and merge #3234 instead, which contains your commit 069f32a.

sounds good. do you want to take the test which checks with -p no:logging or a variation of it?

Yes, we should add it there or in a new PR.

…fined

nicoddemus · 2018-02-20T01:31:19Z

Thanks @brianmaissy for the implementation! 😁

Implement the test from pytest-dev#3210, which was not merged yet, because the PR was abandoned in favor or pytest-dev#3234.

twmr reviewed Feb 12, 2018

View reviewed changes

nicoddemus approved these changes Feb 15, 2018

View reviewed changes

brianmaissy force-pushed the feature/added_printing_of_captured_logs_before_entering_pdb branch from 65dbc4d to 069f32a Compare February 17, 2018 18:33

nicoddemus requested changes Feb 17, 2018

View reviewed changes

nicoddemus reviewed Feb 17, 2018

View reviewed changes

This was referenced Feb 17, 2018

Print captured logs before entering pdb #3204

Closed

Captured-log support for --show-capture #3233

Closed

brianmaissy force-pushed the feature/added_printing_of_captured_logs_before_entering_pdb branch 2 times, most recently from 0ddb029 to b1a74cb Compare February 18, 2018 21:30

print captured logs before entering pdb, unless --no-print-logs is de…

9f3ea0e

…fined

brianmaissy force-pushed the feature/added_printing_of_captured_logs_before_entering_pdb branch from b1a74cb to 9f3ea0e Compare February 18, 2018 21:34

brianmaissy closed this Feb 19, 2018

brianmaissy deleted the feature/added_printing_of_captured_logs_before_entering_pdb branch February 19, 2018 20:44

twmr added a commit to twmr/pytest that referenced this pull request Feb 22, 2018

Add pdb test with disabled logging plugin

0f58fc8

Implement the test from pytest-dev#3210, which was not merged yet, because the PR was abandoned in favor or pytest-dev#3234.

twmr mentioned this pull request Feb 22, 2018

Add pdb test with disabled logging plugin #3253

Merged

	def add_option_ini(option, dest, default=None, type=None, **kwargs):
	parser.addini(dest, default=default, type=type,
	help='default value for ' + option)
	group.addoption(option, dest=dest, **kwargs)

	add_option_ini(
	'--no-print-logs',
	dest='log_print', action='store_const', const=False, default=True,
	type='bool',
	help='disable printing caught logs on failed tests.')

Uh oh!

print captured logs before entering pdb #3210

print captured logs before entering pdb #3210

Uh oh!

Conversation

brianmaissy commented Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicoddemus left a comment

Choose a reason for hiding this comment

Uh oh!

brianmaissy commented Feb 17, 2018

Uh oh!

nicoddemus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicoddemus Feb 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brianmaissy Feb 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicoddemus commented Feb 20, 2018

Uh oh!

Uh oh!

brianmaissy commented Feb 12, 2018 •

edited

Loading

coveralls commented Feb 12, 2018 •

edited

Loading

nicoddemus Feb 17, 2018 •

edited

Loading

brianmaissy Feb 18, 2018 •

edited

Loading