Skip to content

Fix tests by resetting the log level #5888

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

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

amurzeau
Copy link
Contributor

While packing the version, I got tests errors, probably because the order of test execution wasn't the same.

test_main_logging tests are changing the log level by calling streamlink_cli.main.setup(parser). We need to reset it to its default value on teardown.
The log level is set by this code in setup:

# We don't want log output when we are printing JSON or a command-line.
silent_log = any(getattr(args, attr) for attr in QUIET_OPTIONS)
log_level = args.loglevel if not silent_log else "none"
log_file = args.logfile if log_level != "none" else None
setup_logger_and_console(console_out, log_file, log_level, args.json)

Here is a test failure caused by this:

________________________ TestLogging.test_default_level ________________________

self = <tests.test_logger.TestLogging object at 0x7f9d4174eb90>

    def test_default_level(self):
>       assert logging.getLogger("streamlink").level == logger.WARNING
E       AssertionError: assert 9223372036854775807 == 30
E        +  where 9223372036854775807 = <StreamlinkLogger streamlink (none)>.level
E        +    where <StreamlinkLogger streamlink (none)> = <function getLogger at 0x7f9d46eeaca0>('streamlink')
E        +      where <function getLogger at 0x7f9d46eeaca0> = logging.getLogger
E        +  and   30 = logger.WARNING

tests/test_logger.py:90: AssertionError

@bastimeyer
Copy link
Member

bastimeyer commented Mar 12, 2024

Could you please post the full pytest output?

The CLI is still very much a mess with lots of global state, and the tests which have been written in the past did a lot of monkey-patching while trying to reset state afterwards, but none of them ever did this properly.

I've recently rewritten the CLI's logging/console-print setup tests with that in mind. However, I can't reproduce the log level issue. The tests should always run in the same order, determined by this:

Also, this diff should be better, as it doesn't rely on a static value (even though it's the same value as set in streamlink.logger):

diff --git a/tests/cli/test_main_logging.py b/tests/cli/test_main_logging.py
index 3416fd69..f7024787 100644
--- a/tests/cli/test_main_logging.py
+++ b/tests/cli/test_main_logging.py
@@ -33,10 +33,13 @@ def _setup(monkeypatch: pytest.MonkeyPatch, session: Streamlink):
     monkeypatch.setattr("streamlink_cli.main.setup_signals", Mock())
     monkeypatch.setattr("streamlink_cli.argparser.find_default_player", Mock())
 
+    level = streamlink_cli.main.logger.root.level
+
     try:
         yield
     finally:
         streamlink_cli.main.logger.root.handlers.clear()
+        streamlink_cli.main.logger.root.setLevel(level)
         streamlink_cli.main.args = None  # type: ignore[assignment]
         streamlink_cli.main.console = None  # type: ignore[assignment]

@amurzeau
Copy link
Contributor Author

amurzeau commented Mar 12, 2024

Here are the results:
tests_result.txt

The folder in /workspaces/build-area/streamlink-6.7.0/.pybuild/cpython3_3.11/build is created by Debian's packaging scripts, but tests/conftest.py is there.

@bastimeyer
Copy link
Member

The uploaded file is 404... Please post a GH gist instead.

@amurzeau
Copy link
Contributor Author

Ok, uploaded here: https://gist.github.com/amurzeau/a787142847cdd962c80b96492ae2026e

I think I've found the issue, this is because of item.nodeid which in Debian setup is something like this: .pybuild/cpython3_3.11/build/tests/webbrowser/test_webbrowser.py::TestLaunch::test_terminate_on_nursery_cancellation

So nothing will match _TEST_PRIORITIES items.

@amurzeau
Copy link
Contributor Author

I get no error with python3.11 -m pytest --rootdir . tests (with rootdir)
I will check if there is something to do about that on Debian packaging side.

@bastimeyer
Copy link
Member

I think I've found the issue, this is because of item.nodeid which in Debian setup is something like this:
I will check if there is something to do about that on Debian packaging side.

https://docs.pytest.org/en/latest/reference/customize.html#initialization-determining-rootdir-and-configfile

There's nothing that can be done here to fix this. We can't set the rootdir option in pyproject.toml. It's something that must be fixed in the Debian testing scripts. You'll probably need to set the CWD to what the rootdir was previously pointing to and then run pytest from there without the rootdir option.

@amurzeau
Copy link
Contributor Author

amurzeau commented Mar 12, 2024

Yes, this is related with how Debian handle this.
It does this:

  • Build/Copy files to be included in the Debian package in a temporary directory: <streamink_root>/.pybuild/cpython3_3.11/build/
  • Run pytest from there, I guess to ensure there is no missing stuff from the package

I can fix the issue by adding this to the pytest command line: --rootdir=. -o cache_dir=$(CURDIR)/.pytest_cache
It's kind of hacky, but given:

  • tests nodeid are relative to the rootdir
  • a temporary .pytest_cache is created in the rootdir
    if I want to keep most the of Debian packaging scripts (ie. keep the .pybuild temporary build directory), I guess I need to do that.

An alternative would be to run tests directly on the root streamlink source directory instead, but I'm not sure this is really better from a Debian package perspective.

I'm closing this PR given the issue is caused by packaging scripts, let me know if you me to still reopen it.

@amurzeau amurzeau closed this Mar 12, 2024
@bastimeyer
Copy link
Member

The logger level should still be reset, so I'd appreciate if you could reopen and update the PR with the diff I have posted. Thanks.

@amurzeau amurzeau reopened this Mar 12, 2024
test_main_logging tests are changing the log level.
We need to reset it to its default value on teardown.
@amurzeau amurzeau force-pushed the fix-tests-default-logger branch from 97733f4 to 85ab2fc Compare March 12, 2024 23:38
@bastimeyer bastimeyer merged commit 251fe08 into streamlink:master Mar 12, 2024
@bastimeyer
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants