Only call _setup_cli_logging in __init__ by twmr · Pull Request #4720 · pytest-dev/pytest

twmr · 2019-02-04T22:34:08Z

Supersedes #4719

Thanks for submitting a PR, your contribution is really appreciated!

Here's a quick checklist that should be present in PRs (you can delete this text from the final description, this is
just a guideline):

Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.
Target the master branch for bug fixes, documentation updates and trivial changes.
Target the features branch for new features and removals/deprecations.
Include documentation when adding new features.
Include new tests or update existing tests when applicable.

Unless your change is trivial or a small documentation fix (e.g., a typo or reword of a small section) please:

Add yourself to AUTHORS in alphabetical order;

twmr · 2019-02-04T22:34:40Z

@RonnyPfannschmidt if you agree, let's close this in favor of #4719

twmr · 2019-02-04T23:19:06Z

It seems as if it is not so easy. Passing trylast=True to the hookimpl decorator used for pytest_configure leads to a couple of test failures.

RonnyPfannschmidt

looks good to me and i agree to the superseeding

twmr · 2019-02-05T08:22:06Z

looks good to me and i agree to the superseding

The tests are failing because the handling of the verbosity level has changed.

without --verbose on the cmd line a simple test outputs:

test_case.py 
---------------------------------------------------------------------- live log call ----------------------------------------------------------------------
test_case.py                 4 WARNING  log message from test_log_1
.
---------------------------------------------------------------------- live log call ----------------------------------------------------------------------
test_case.py                 7 WARNING  log message from test_log_2

but with --verbose (= same output as without --verbose before my change) it outputs

test_case.py::test_log_1 
---------------------------------------------------------------------- live log call ----------------------------------------------------------------------
test_case.py                 4 WARNING  log message from test_log_1
PASSED                                                                                                                                              [ 50%]
test_case.py::test_log_2 
---------------------------------------------------------------------- live log call ----------------------------------------------------------------------
test_case.py                 7 WARNING  log message from test_log_2
PASSED

Any ideas?

twmr · 2019-02-05T08:27:50Z

looks good to me and i agree to the superseding

The tests are failing because the handling of the verbosity level has changed.

without --verbose on the cmd line a simple test outputs:

test_case.py 
---------------------------------------------------------------------- live log call ----------------------------------------------------------------------
test_case.py                 4 WARNING  log message from test_log_1
.
---------------------------------------------------------------------- live log call ----------------------------------------------------------------------
test_case.py                 7 WARNING  log message from test_log_2

but with --verbose (= same output as without --verbose before my change) it outputs

test_case.py::test_log_1 
---------------------------------------------------------------------- live log call ----------------------------------------------------------------------
test_case.py                 4 WARNING  log message from test_log_1
PASSED                                                                                                                                              [ 50%]
test_case.py::test_log_2 
---------------------------------------------------------------------- live log call ----------------------------------------------------------------------
test_case.py                 7 WARNING  log message from test_log_2
PASSED

Any ideas?

I know what the problem is. Before this PR config.option.verbose=1 was called in pytest_configure of the logging plugin before the terminalreporter plugin was configured. Now it is the other way around. I guess that the terminalreporter doesn't notice with the code from this PR that the logging plugin manually sets the verbose flag to 1.

Any ideas @nicoddemus ?

RonnyPfannschmidt · 2019-02-05T08:49:42Z

@Thisch i did a quick check of the terminal plugin, and im under the impression its inconsistent in handling verbosity (parts use config.option.verbose, parts use a stored copy)

a quick workaround would be to fetch the terminalwriter instance as well setting its verbosity attribute accordingly, but we will have to do a followup to sanitize things

blueyed · 2019-02-05T14:22:20Z

src/_pytest/logging.py


        self.log_cli_handler = None

+        terminal_reporter = self._config.pluginmanager.get_plugin("terminalreporter")


Should this code kept being wrapped in a function/method (_setup_cli_logging)?

I also prefer keeping the _setup_cli_method. Thx for pointing that out!

codecov · 2019-02-05T18:57:50Z

Codecov Report

Merging #4720 into master will decrease coverage by 0.01%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           master    #4720      +/-   ##
==========================================
- Coverage   95.68%   95.67%   -0.02%     
==========================================
  Files         113      113              
  Lines       24963    24965       +2     
  Branches     2478     2478              
==========================================
- Hits        23886    23885       -1     
  Misses        762      762              
- Partials      315      318       +3

Flag	Coverage Δ
#docs	`29.69% <25%> (+0.09%)`	⬆️
#doctesting	`29.69% <25%> (+0.09%)`	⬆️
#linting	`29.69% <25%> (+0.09%)`	⬆️
#linux	`95.5% <90%> (ø)`	⬆️
#nobyte	`92.28% <90%> (-0.02%)`	⬇️
#numpy	`93.09% <90%> (-0.02%)`	⬇️
#pexpect	`42.09% <25%> (-0.01%)`	⬇️
#py27	`93.68% <90%> (ø)`	⬆️
#py34	`91.77% <90%> (+0.06%)`	⬆️
#py35	`91.77% <90%> (+0.06%)`	⬆️
#py36	`91.79% <90%> (+0.06%)`	⬆️
#py37	`93.8% <90%> (ø)`	⬆️
#trial	`93.09% <90%> (-0.02%)`	⬇️
#windows	`93.86% <90%> (-0.02%)`	⬇️
#xdist	`93.71% <90%> (ø)`	⬆️

Impacted Files	Coverage Δ
src/_pytest/logging.py	`96.08% <90%> (+0.02%)`	⬆️
src/_pytest/cacheprovider.py	`95.75% <0%> (-1.42%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c780d1f...0ce8b91. Read the comment docs.

nicoddemus · 2019-02-05T20:55:12Z

src/_pytest/logging.py

+        terminal_reporter = self._config.pluginmanager.get_plugin("terminalreporter")
+        # FIXME don't set verbosity level and derived attributes of
+        # terminalwriter directly
+        terminal_reporter.verbosity = self._config.option.verbose


What if we change verbosity, showheader, etc, to read-only properties? As we have seen, setting them manually is error prone and leads to surprising results.

thats a long term goal, a initial gloss over review of terminalwriter indicated, that it is inconsistent in terms of option/configuration usage

Why you mean a long term goal @RonnyPfannschmidt ? Changing them to read-only properties should be really simple and would avoid the hack that @Thisch had to put in place. 😁

Note that this hack does not fully solve all issues with the unit tests. I can't really explain why the terminalwrapper plugin seems to be not ready when LogginPlugin.init is called in the travis-ci jobs (Nonetype has not attribute ...; see https://travis-ci.org/pytest-dev/pytest/jobs/489189341)

Any ideas?

Oh xdist disables the terminal writer, so it never gets loaded.

@nicoddemus since there seems tobe inconsistent usage, that issue should be resolved in a own pr instead of shoehorning it into this one which is different

Problem is that the tests are breaking because of this inconsistency... I'm not against making the change here (in separate commits of course), but if you guys prefer then we should open a new PR changing the terminal attributes to read-only properties, and when that gets merged this PR should be rebased on that one. @Thisch you up for it?

Supersedes pytest-dev#4719

twmr · 2019-02-06T18:12:28Z

Can this be merged now?

RonnyPfannschmidt · 2019-02-06T19:05:50Z

@Thisch thanks for the reminder

RonnyPfannschmidt approved these changes Feb 5, 2019

View reviewed changes

blueyed mentioned this pull request Feb 5, 2019

Remove unneeded 2nd _setup_cli_logging call #4719

Closed

6 tasks

blueyed reviewed Feb 5, 2019

View reviewed changes

twmr force-pushed the removesetupclilogging branch from 2bb2593 to ac17af8 Compare February 5, 2019 18:56

twmr changed the title ~~Move _setup_cli_logging into __init__~~ Only call _setup_cli_logging in __init__ Feb 5, 2019

nicoddemus reviewed Feb 5, 2019

View reviewed changes

Only call _setup_cli_logging in __init__

0ce8b91

Supersedes pytest-dev#4719

twmr force-pushed the removesetupclilogging branch from ac17af8 to 0ce8b91 Compare February 6, 2019 06:03

RonnyPfannschmidt merged commit 429485e into pytest-dev:master Feb 6, 2019

RonnyPfannschmidt mentioned this pull request Feb 6, 2019

refactor terminalwriter configuration #4733

Closed


		self.log_cli_handler = None

		terminal_reporter = self._config.pluginmanager.get_plugin("terminalreporter")

Uh oh!

Conversation

twmr commented Feb 4, 2019

Uh oh!

twmr commented Feb 4, 2019

Uh oh!

twmr commented Feb 4, 2019

Uh oh!

RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

Uh oh!

twmr commented Feb 5, 2019

Uh oh!

twmr commented Feb 5, 2019

Uh oh!

RonnyPfannschmidt commented Feb 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Feb 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

twmr commented Feb 6, 2019

Uh oh!

RonnyPfannschmidt commented Feb 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Feb 5, 2019 •

edited

Loading