Remove unneeded 2nd _setup_cli_logging call #4719
Conversation
During the review of pytest-dev#4204 it was not noticed that _setup_cli_logging was already called in the collection phase (pytest_collection). Now _setup_cli_Logging is called in two hooks: pytest_collection and pytest_sessionstart. It only makes sense to call _setup_cli_logging once (in the first called hook of the logging plugin).
| @@ -513,6 +509,8 @@ def pytest_sessionfinish(self): | |||
|
|
|||
| @pytest.hookimpl(hookwrapper=True, tryfirst=True) | |||
| def pytest_sessionstart(self): | |||
| # This has to be called before the first log message is logged, | |||
There was a problem hiding this comment.
im wondering - shouldn't we do this as early as pytest_configure
There was a problem hiding this comment.
Yes, maybe. We can call it once the terminalwrapper and capturemanager plugins are configured (see code of _setup_cli_logging).
There was a problem hiding this comment.
I've just stumbled upon this code in LoggingPlugin.init (which is called in pytest_configure):
# sanity check: terminal reporter should not have been loaded at this point
assert self._config.pluginmanager.get_plugin("terminalreporter") is NoneSo I guess that both capturemanager and terminalreporter are not available in pytest_configure
There was a problem hiding this comment.
@Thisch i believe a @hookimpl(trylast=True) can sort that
There was a problem hiding this comment.
it may be more tricky :/ i didnt account for setuptools entrypoints
Codecov Report
@@ Coverage Diff @@
## master #4719 +/- ##
==========================================
- Coverage 95.68% 95.67% -0.02%
==========================================
Files 113 113
Lines 24970 24969 -1
Branches 2479 2479
==========================================
- Hits 23892 23888 -4
Misses 762 762
- Partials 316 319 +3
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #4719 +/- ##
==========================================
- Coverage 95.68% 95.67% -0.02%
==========================================
Files 113 113
Lines 24970 24969 -1
Branches 2479 2479
==========================================
- Hits 23892 23888 -4
Misses 762 762
- Partials 316 319 +3
Continue to review full report at Codecov.
|
|
Closing in favor of #4720. |
During the review of #4204 it was not noticed that _setup_cli_logging
was already called in the collection phase (pytest_collection). After #4204 was merged
_setup_cli_logging is called in two hooks: pytest_collection and
pytest_sessionstart. However, it only makes sense to call _setup_cli_logging once
(in the first called hook of the logging plugin).
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):
changelogfolder, with a name like<ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.masterbranch for bug fixes, documentation updates and trivial changes.featuresbranch for new features and removals/deprecations.Unless your change is trivial or a small documentation fix (e.g., a typo or reword of a small section) please:
AUTHORSin alphabetical order;