Lee-W commented on code in PR #54555:
URL: https://github.com/apache/airflow/pull/54555#discussion_r2284724643
##########
airflow-core/tests/unit/core/test_logging_config.py:
##########
@@ -96,6 +96,18 @@
# Other settings here
"""
+SETTINGS_FILE_WITH_REMOTE_VARS = (
+ SETTINGS_FILE_VALID
+ + """
+REMOTE_TASK_LOG = None
+DEFAULT_REMOTE_CONN_ID = "test_conn_id"
+"""
+)
+
+SETTINGS_FILE_NO_REMOTE_VARS =
SETTINGS_FILE_WITH_REMOTE_VARS.replace("REMOTE_TASK_LOG = None", "").replace(
Review Comment:
Are `SETTINGS_FILE_NO_REMOTE_VARS` and `SETTINGS_FILE_VALID` the same? Do we
need another variable for it?
##########
airflow-core/tests/unit/core/test_logging_config.py:
##########
@@ -191,6 +203,13 @@ def teardown_method(self):
importlib.reload(airflow_local_settings)
configure_logging()
+ def _verify_basic_logging_config(self, logging_config: dict,
logging_class_path: str, expected_path: str):
Review Comment:
```suggestion
def _verify_basic_logging_config(self, logging_config: dict,
logging_class_path: str, expected_path: str) -> None:
```
##########
airflow-core/tests/unit/core/test_logging_config.py:
##########
@@ -96,6 +96,18 @@
# Other settings here
"""
+SETTINGS_FILE_WITH_REMOTE_VARS = (
+ SETTINGS_FILE_VALID
+ + """
+REMOTE_TASK_LOG = None
+DEFAULT_REMOTE_CONN_ID = "test_conn_id"
+"""
+)
Review Comment:
```suggestion
SETTINGS_FILE_WITH_REMOTE_VARS = (
f"""{SETTINGS_FILE_VALID}
REMOTE_TASK_LOG = None
DEFAULT_REMOTE_CONN_ID = "test_conn_id"
"""
)
```
##########
airflow-core/tests/unit/core/test_logging_config.py:
##########
@@ -365,3 +384,43 @@ def test_loading_remote_logging_with_hdfs_handler(self):
airflow.logging_config.configure_logging()
assert isinstance(airflow.logging_config.REMOTE_TASK_LOG,
HdfsRemoteLogIO)
+
+ @pytest.mark.parametrize(
+ "settings_content,module_structure,expected_path",
+ [
+ (SETTINGS_FILE_WITH_REMOTE_VARS, None,
f"{SETTINGS_DEFAULT_NAME}.LOGGING_CONFIG"),
+ (
+ SETTINGS_FILE_WITH_REMOTE_VARS,
+ "nested.config.module",
+ f"nested.config.module.{SETTINGS_DEFAULT_NAME}.LOGGING_CONFIG",
+ ),
+ ],
+ )
+ def test_load_logging_config_module_paths(
+ self, settings_content: str, module_structure: str, expected_path: str
+ ):
+ """Test that load_logging_config works with different module path
structures"""
+ dir_structure = module_structure.replace(".", "/") if module_structure
else None
+
+ with settings_context(settings_content, dir_structure):
+ from airflow.logging_config import load_logging_config
+
+ logging_config, logging_class_path = load_logging_config()
+ self._verify_basic_logging_config(logging_config,
logging_class_path, expected_path)
+
+ def test_load_logging_config_fallback_behavior(self):
+ """Test that load_logging_config falls back gracefully when remote
logging vars are missing"""
+ with settings_context(SETTINGS_FILE_NO_REMOTE_VARS):
+ from airflow.logging_config import load_logging_config
+
+ with patch("airflow.logging_config.log") as mock_log:
+ logging_config, logging_class_path = load_logging_config()
+
+ self._verify_basic_logging_config(
+ logging_config, logging_class_path,
f"{SETTINGS_DEFAULT_NAME}.LOGGING_CONFIG"
+ )
+
+ mock_log.info.assert_called_with(
+ "Remote task logs will not be available due to an error:
%s",
+ mock_log.info.call_args[0][1],
+ )
Review Comment:
```suggestion
mock_log.info.mock_calls = [
call(
"Remote task logs will not be available due to an
error: %s",
)
]
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
