jason810496 commented on code in PR #54555:
URL: https://github.com/apache/airflow/pull/54555#discussion_r2280271375
##########
airflow-core/tests/unit/core/test_logging_config.py:
##########
@@ -96,6 +96,108 @@
# Other settings here
"""
+SETTINGS_FILE_SIMPLE_MODULE = """
+LOGGING_CONFIG = {
+ 'version': 1,
+ 'disable_existing_loggers': False,
+ 'formatters': {
+ 'airflow.task': {
+ 'format': '[%(asctime)s] {%(filename)s:%(lineno)d} %(levelname)s -
%(message)s'
+ },
+ },
+ 'handlers': {
+ 'console': {
+ 'class': 'logging.StreamHandler',
+ 'formatter': 'airflow.task',
+ 'stream': 'ext://sys.stdout'
+ },
+ 'task': {
+ 'class': 'logging.StreamHandler',
+ 'formatter': 'airflow.task',
+ 'stream': 'ext://sys.stdout'
+ },
+ },
+ 'loggers': {
+ 'airflow.task': {
+ 'handlers': ['task'],
+ 'level': 'INFO',
+ 'propagate': False,
+ },
+ }
+}
+
+# Test remote logging variables
+REMOTE_TASK_LOG = None
+DEFAULT_REMOTE_CONN_ID = "test_conn_id"
+"""
+
+SETTINGS_FILE_NESTED_MODULE = """
Review Comment:
It seems that the content of `SETTINGS_FILE_NESTED_MODULE ` is exactly same
as `SETTINGS_FILE_SIMPLE_MODULE` expect the last line ( `DEFAULT_REMOTE_CONN_ID
= "nested_conn_id"` ).
How about we copy from `SETTINGS_FILE_SIMPLE_MODULE` and then replace the
last line?
##########
airflow-core/src/airflow/logging_config.py:
##########
@@ -73,7 +73,10 @@ def load_logging_config() -> tuple[dict[str, Any], str]:
else:
modpath = logging_class_path.rsplit(".", 1)[0]
try:
- mod = import_string(modpath)
+ # Import here to avoid circular imports
+ from importlib import import_module
Review Comment:
IMO, it's fine to move the import of `importlib` to top level, as
`airflow.utils.module_loading` will also import `importlib` at top level.
```suggestion
```
##########
airflow-core/tests/unit/core/test_logging_config.py:
##########
@@ -365,3 +474,40 @@ 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_SIMPLE_MODULE, None,
f"{SETTINGS_DEFAULT_NAME}.LOGGING_CONFIG"),
+ (
+ SETTINGS_FILE_NESTED_MODULE,
+ "nested.config.module",
+ f"nested.config.module.{SETTINGS_DEFAULT_NAME}.LOGGING_CONFIG",
+ ),
+ ],
+ )
+ def test_load_logging_config_module_paths(self, settings_content,
module_structure, expected_path):
+ """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, _ = load_logging_config()
+
+ assert isinstance(logging_config, dict)
+ assert logging_config["version"] == 1
Review Comment:
Should we leverage `self._verify_basic_logging_config` utility as well?
##########
airflow-core/tests/unit/core/test_logging_config.py:
##########
@@ -365,3 +474,40 @@ 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_SIMPLE_MODULE, None,
f"{SETTINGS_DEFAULT_NAME}.LOGGING_CONFIG"),
+ (
+ SETTINGS_FILE_NESTED_MODULE,
+ "nested.config.module",
+ f"nested.config.module.{SETTINGS_DEFAULT_NAME}.LOGGING_CONFIG",
+ ),
+ ],
+ )
+ def test_load_logging_config_module_paths(self, settings_content,
module_structure, expected_path):
Review Comment:
```suggestion
def test_load_logging_config_module_paths(self, settings_content: str,
module_structure: str, expected_path: str):
```
##########
airflow-core/tests/unit/core/test_logging_config.py:
##########
@@ -191,6 +293,13 @@ def teardown_method(self):
importlib.reload(airflow_local_settings)
configure_logging()
+ def _verify_basic_logging_config(self, logging_config, logging_class_path,
expected_path):
Review Comment:
```suggestion
def _verify_basic_logging_config(self, logging_config: dict,
logging_class_path: str, expected_path: str):
```
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
