Skip to content

Remove unneeded 2nd _setup_cli_logging call #4719

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Remove unneeded 2nd _setup_cli_logging call #4719

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions src/_pytest/logging.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,10 +430,6 @@ def _log_cli_enabled(self):

@pytest.hookimpl(hookwrapper=True, tryfirst=True)
def pytest_collection(self):
# This has to be called before the first log message is logged,
# so we can access the terminal reporter plugin.
self._setup_cli_logging()

with self.live_logs_context():
if self.log_cli_handler:
self.log_cli_handler.set_when("collection")
Expand Down Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

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

im wondering - shouldn't we do this as early as pytest_configure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, maybe. We can call it once the terminalwrapper and capturemanager plugins are configured (see code of _setup_cli_logging).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 None

So I guess that both capturemanager and terminalreporter are not available in pytest_configure

Copy link
Member

Choose a reason for hiding this comment

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

@Thisch i believe a @hookimpl(trylast=True) can sort that

Copy link
Member

Choose a reason for hiding this comment

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

it may be more tricky :/ i didnt account for setuptools entrypoints

# so we can access the terminal reporter plugin.
self._setup_cli_logging()
with self.live_logs_context():
if self.log_cli_handler:
Expand Down