This is an automated email from the ASF dual-hosted git repository.
potiuk pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/airflow.git
The following commit(s) were added to refs/heads/main by this push:
new c497c5dea6 Fix side-effect of setting log handlers in tests (#43175)
c497c5dea6 is described below
commit c497c5dea686f2337d94094ef3aa25b271ac0881
Author: Jarek Potiuk <[email protected]>
AuthorDate: Fri Oct 18 23:00:42 2024 +0200
Fix side-effect of setting log handlers in tests (#43175)
In some of our tests, handlers for loggers were set to the
"logging.Handler()" and that created a side effect where
when they were not cleaned by another test, they produced
`NotImplementedError: emit must be implemented by Handler subclasses`
message - because indeed `logging.Handler()` emit message
raised NotImplementedError.
The problem was that those tests (and some others) cleaned-up
the handlers only BEFORE the tests but not AFTER - so if it
happened that the same "test worker" run another test afterwards,
that cleaned the handlers but did not set the handlers back, the
side effect was gone. But when xdist arranged the tests to
different pytest workers, the Handlers could remain registered
in loggers so other tests could attempt to emit some logs
in case they logged something to a parent logger.
The fix applied is to create a fixture that cleans all loggers
BEFORE and AFTER a test - and add the fixture to tests that
modify handlers.
---
tests/cli/commands/test_task_command.py | 2 +-
tests/conftest.py | 23 +++++++++++++++++++++++
tests/jobs/test_triggerer_job.py | 4 +---
tests/jobs/test_triggerer_job_logging.py | 20 ++++----------------
tests_common/test_utils/log_handlers.py | 21 +++++++++++++++++++++
5 files changed, 50 insertions(+), 20 deletions(-)
diff --git a/tests/cli/commands/test_task_command.py
b/tests/cli/commands/test_task_command.py
index 67204a1ad7..1de3f7b68d 100644
--- a/tests/cli/commands/test_task_command.py
+++ b/tests/cli/commands/test_task_command.py
@@ -1070,7 +1070,7 @@ class TestLoggerMutationHelper:
assert tgt.propagate is False if target_name else True # root
propagate unchanged
assert tgt.level == -1
- def test_apply_no_replace(self):
+ def test_apply_no_replace(self, clear_all_logger_handlers):
"""
Handlers, level and propagate should be applied on target.
"""
diff --git a/tests/conftest.py b/tests/conftest.py
index c5c6441f0b..de13fe99c4 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -16,12 +16,15 @@
# under the License.
from __future__ import annotations
+import logging
import os
import sys
from typing import TYPE_CHECKING
import pytest
+from tests_common.test_utils.log_handlers import non_pytest_handlers
+
# We should set these before loading _any_ of the rest of airflow so that the
# unit test mode config is set as early as possible.
assert "airflow" not in sys.modules, "No airflow module can be imported before
these lines"
@@ -58,6 +61,26 @@ def reset_environment():
os.environ[key] = init_env[key]
+def remove_non_pytest_log_handlers(logger, *classes):
+ for handler in non_pytest_handlers(logger.handlers):
+ logger.removeHandler(handler)
+
+
+def remove_all_non_pytest_log_handlers():
+ # Remove handlers from the root logger
+ remove_non_pytest_log_handlers(logging.getLogger())
+ # Remove handlers from all other loggers
+ for logger_name in logging.root.manager.loggerDict:
+ remove_non_pytest_log_handlers(logging.getLogger(logger_name))
+
+
[email protected]
+def clear_all_logger_handlers():
+ remove_all_non_pytest_log_handlers()
+ yield
+ remove_all_non_pytest_log_handlers()
+
+
if TYPE_CHECKING:
# Static checkers do not know about pytest fixtures' types and return,
# In case if them distributed through third party packages.
diff --git a/tests/jobs/test_triggerer_job.py b/tests/jobs/test_triggerer_job.py
index cc457687b0..3ad03b0c35 100644
--- a/tests/jobs/test_triggerer_job.py
+++ b/tests/jobs/test_triggerer_job.py
@@ -49,6 +49,7 @@ from airflow.utils.types import DagRunType
from tests.core.test_logging_config import reset_logging
from tests_common.test_utils.db import clear_db_dags, clear_db_runs
+from tests_common.test_utils.log_handlers import non_pytest_handlers
pytestmark = pytest.mark.db_test
@@ -772,9 +773,6 @@ def test_queue_listener():
importlib.reload(airflow_local_settings)
configure_logging()
- def non_pytest_handlers(val):
- return [h for h in val if "pytest" not in h.__module__]
-
import logging
log = logging.getLogger()
diff --git a/tests/jobs/test_triggerer_job_logging.py
b/tests/jobs/test_triggerer_job_logging.py
index 335f9e6b81..5e6a8e3ac8 100644
--- a/tests/jobs/test_triggerer_job_logging.py
+++ b/tests/jobs/test_triggerer_job_logging.py
@@ -32,10 +32,7 @@ from airflow.utils.log.logging_mixin import
RedirectStdHandler
from airflow.utils.log.trigger_handler import DropTriggerLogsFilter,
TriggererHandlerWrapper
from tests_common.test_utils.config import conf_vars
-
-
-def non_pytest_handlers(val):
- return [h for h in val if "pytest" not in h.__module__]
+from tests_common.test_utils.log_handlers import non_pytest_handlers
def assert_handlers(logger, *classes):
@@ -44,12 +41,6 @@ def assert_handlers(logger, *classes):
return handlers
-def clear_logger_handlers(log):
- for h in log.handlers[:]:
- if "pytest" not in h.__module__:
- log.removeHandler(h)
-
-
@pytest.fixture(autouse=True)
def reload_triggerer_job():
importlib.reload(triggerer_job_runner)
@@ -64,7 +55,6 @@ def test_configure_trigger_log_handler_file():
"""
# reset logging
root_logger = logging.getLogger()
- clear_logger_handlers(root_logger)
configure_logging()
# before config
@@ -168,15 +158,12 @@ not_found_message = ["Could not find log handler suitable
for individual trigger
("non_file_task_handler", logging.Handler, not_found_message),
],
)
-def test_configure_trigger_log_handler_not_file_task_handler(cfg, cls, msg):
+def test_configure_trigger_log_handler_not_file_task_handler(cfg, cls, msg,
clear_all_logger_handlers):
"""
No root handler configured.
When non FileTaskHandler is configured, don't modify.
When an incompatible subclass of FileTaskHandler is configured, don't
modify.
"""
- # reset handlers
- root_logger = logging.getLogger()
- clear_logger_handlers(root_logger)
with conf_vars(
{
@@ -190,6 +177,7 @@ def
test_configure_trigger_log_handler_not_file_task_handler(cfg, cls, msg):
configure_logging()
# no root handlers
+ root_logger = logging.getLogger()
assert_handlers(root_logger)
# default task logger
@@ -328,7 +316,7 @@ root_not_file_task = {
}
-def test_configure_trigger_log_handler_root_not_file_task():
+def
test_configure_trigger_log_handler_root_not_file_task(clear_all_logger_handlers):
"""
root: A handler that doesn't support trigger or inherit FileTaskHandler
task: Supports triggerer
diff --git a/tests_common/test_utils/log_handlers.py
b/tests_common/test_utils/log_handlers.py
new file mode 100644
index 0000000000..b5335c4e1b
--- /dev/null
+++ b/tests_common/test_utils/log_handlers.py
@@ -0,0 +1,21 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+
+def non_pytest_handlers(handlers):
+ return [h for h in handlers if "pytest" not in h.__module__]
