refactor: Enable G logging rules and comprehensive ruff improvements (#35081)

Co-authored-by: Claude <noreply@anthropic.com>

tests/common/logger_utils.py

@@ -116,7 +116,7 @@ def _make_decorator(  # noqa: C901
             def _log_enter_to_function(*args, **kwargs) -> None:
                 if _is_log_info():
                     decorated_logger.info(
-                        f"{prefix_enter_msg}'{full_func_name}'{suffix_enter_msg}"
+                        "%s'%s'%s", prefix_enter_msg, full_func_name, suffix_enter_msg
                     )
                 elif _is_debug_enable():
                     _log_debug(*args, **kwargs)
@@ -142,19 +142,27 @@ def _make_decorator(  # noqa: C901
                 _CLS_PARAM in used_parameters and used_parameters.pop(_CLS_PARAM)
                 if used_parameters:
                     decorated_logger.debug(
-                        f"{prefix_enter_msg}'{full_func_name}'{with_arguments_msg_part}"
-                        f"{used_parameters}{suffix_enter_msg}"
+                        "%s'%s'%s%s%s",
+                        prefix_enter_msg,
+                        full_func_name,
+                        with_arguments_msg_part,
+                        used_parameters,
+                        suffix_enter_msg,
                     )
                 else:
                     decorated_logger.debug(
-                        f"{prefix_enter_msg}'{full_func_name}'{suffix_enter_msg}"
+                        "%s'%s'%s", prefix_enter_msg, full_func_name, suffix_enter_msg
                     )
 
             def _log_exit_of_function(return_value: Any) -> None:
                 if _is_debug_enable() and has_return_value:
                     decorated_logger.debug(
-                        f"{prefix_out_msg}'{full_func_name}'{return_value_msg_part}"
-                        f"'{return_value}'{suffix_out_msg}"
+                        "%s'%s'%s'%s'%s",
+                        prefix_out_msg,
+                        full_func_name,
+                        return_value_msg_part,
+                        return_value,
+                        suffix_out_msg,
                     )
 
             return _wrapper_func

tests/integration_tests/core_tests.py

@@ -242,7 +242,7 @@ class TestCore(SupersetTestCase):
                 (slc.slice_name, "explore", slc.slice_url),
             ]
         for name, method, url in urls:
-            logger.info(f"[{name}]/[{method}]: {url}")
+            logger.info("[%s]/[%s]: %s", name, method, url)
             print(f"[{name}]/[{method}]: {url}")
             resp = self.client.get(url)
             assert resp.status_code == 200

tests/integration_tests/dashboards/superset_factory_util.py

@@ -199,7 +199,7 @@ def delete_all_inserted_dashboards():
             try:
                 delete_dashboard(dashboard, False)
             except Exception:
-                logger.error(f"failed to delete {dashboard.id}", exc_info=True)
+                logger.error("failed to delete %s", dashboard.id, exc_info=True)
                 raise
         if len(inserted_dashboards_ids) > 0:
             db.session.commit()
@@ -210,7 +210,7 @@ def delete_all_inserted_dashboards():
 
 
 def delete_dashboard(dashboard: Dashboard, do_commit: bool = False) -> None:
-    logger.info(f"deleting dashboard{dashboard.id}")
+    logger.info("deleting dashboard%s", dashboard.id)
     delete_dashboard_roles_associations(dashboard)
     delete_dashboard_users_associations(dashboard)
     delete_dashboard_slices_associations(dashboard)
@@ -246,7 +246,7 @@ def delete_all_inserted_slices():
             try:
                 delete_slice(slice, False)
             except Exception:
-                logger.error(f"failed to delete {slice.id}", exc_info=True)
+                logger.error("failed to delete %s", slice.id, exc_info=True)
                 raise
         if len(inserted_slices_ids) > 0:
             db.session.commit()
@@ -257,7 +257,7 @@ def delete_all_inserted_slices():
 
 
 def delete_slice(slice_: Slice, do_commit: bool = False) -> None:
-    logger.info(f"deleting slice{slice_.id}")
+    logger.info("deleting slice%s", slice_.id)
     delete_slice_users_associations(slice_)
     db.session.delete(slice_)
     if do_commit:
@@ -279,7 +279,7 @@ def delete_all_inserted_tables():
             try:
                 delete_sqltable(table, False)
             except Exception:
-                logger.error(f"failed to delete {table.id}", exc_info=True)
+                logger.error("failed to delete %s", table.id, exc_info=True)
                 raise
         if len(inserted_sqltables_ids) > 0:
             db.session.commit()
@@ -290,7 +290,7 @@ def delete_all_inserted_tables():
 
 
 def delete_sqltable(table: SqlaTable, do_commit: bool = False) -> None:
-    logger.info(f"deleting table{table.id}")
+    logger.info("deleting table%s", table.id)
     delete_table_users_associations(table)
     db.session.delete(table)
     if do_commit:
@@ -314,7 +314,7 @@ def delete_all_inserted_dbs():
             try:
                 delete_database(database, False)
             except Exception:
-                logger.error(f"failed to delete {database.id}", exc_info=True)
+                logger.error("failed to delete %s", database.id, exc_info=True)
                 raise
         if len(inserted_databases_ids) > 0:
             db.session.commit()
@@ -325,7 +325,7 @@ def delete_all_inserted_dbs():
 
 
 def delete_database(database: Database, do_commit: bool = False) -> None:
-    logger.info(f"deleting database{database.id}")
+    logger.info("deleting database%s", database.id)
     db.session.delete(database)
     if do_commit:
         db.session.commit()

tests/unit_tests/daos/test_theme_dao.py

@@ -77,12 +77,14 @@ class TestThemeDAO:
         # Call the method
         result = ThemeDAO.find_system_default()
 
-        # Verify warning was logged
+        # Verify warning was logged with lazy logging format
         mock_logger.warning.assert_called_once()
+        call_args = mock_logger.warning.call_args
         assert (
-            "Multiple system default themes found (2)"
-            in mock_logger.warning.call_args[0][0]
+            call_args[0][0]
+            == "Multiple system default themes found (%s), falling back to config theme"
         )
+        assert call_args[0][1] == 2
 
         # Verify the result is the fallback theme
         assert result == mock_fallback
@@ -169,12 +171,14 @@ class TestThemeDAO:
         # Call the method
         result = ThemeDAO.find_system_dark()
 
-        # Verify warning was logged
+        # Verify warning was logged with lazy logging format
         mock_logger.warning.assert_called_once()
+        call_args = mock_logger.warning.call_args
         assert (
-            "Multiple system dark themes found (2)"
-            in mock_logger.warning.call_args[0][0]
+            call_args[0][0]
+            == "Multiple system dark themes found (%s), falling back to config theme"
         )
+        assert call_args[0][1] == 2
 
         # Verify the result is the fallback theme
         assert result == mock_fallback

tests/unit_tests/migrations/shared/utils_test.py

@@ -23,8 +23,13 @@ class DummyLogger:
     def __init__(self):
         self.messages = []
 
-    def info(self, message):
-        self.messages.append(message)
+    def info(self, message, *args):
+        # Handle lazy logging format with multiple arguments
+        if args:
+            formatted_message = message % args
+        else:
+            formatted_message = message
+        self.messages.append(formatted_message)
 
 
 class DummyOp:
@@ -125,7 +130,6 @@ def test_create_unique_index_creates_index(monkeypatch):
     assert call_kwargs.get("unique") is True
     assert call_kwargs.get("columns") == columns
     # And a log message mentioning "Creating index" should be generated.
-    print(dummy_logger.messages)
     assert any("Creating index" in msg for msg in dummy_logger.messages)
 
 

tests/unit_tests/utils/test_screenshot_utils.py

@@ -263,10 +263,12 @@ class TestTakeTiledScreenshot:
             with patch("superset.utils.screenshot_utils.combine_screenshot_tiles"):
                 take_tiled_screenshot(mock_page, "dashboard", viewport_height=2000)
 
-                # Should log dashboard dimensions
-                mock_logger.info.assert_any_call("Dashboard: 800x5000px at (50, 100)")
-                # Should log number of tiles
-                mock_logger.info.assert_any_call("Taking 3 screenshot tiles")
+                # Should log dashboard dimensions with lazy logging format
+                mock_logger.info.assert_any_call(
+                    "Dashboard: %sx%spx at (%s, %s)", 800, 5000, 50, 100
+                )
+                # Should log number of tiles with lazy logging format
+                mock_logger.info.assert_any_call("Taking %s screenshot tiles", 3)
 
     def test_exception_handling_returns_none(self):
         """Test that exceptions are handled and None is returned."""
@@ -277,9 +279,10 @@ class TestTakeTiledScreenshot:
             result = take_tiled_screenshot(mock_page, "dashboard", viewport_height=2000)
 
             assert result is None
-            mock_logger.exception.assert_called_once_with(
-                "Tiled screenshot failed: Unexpected error"
-            )
+            # The exception object is passed, not the string
+            call_args = mock_logger.exception.call_args
+            assert call_args[0][0] == "Tiled screenshot failed: %s"
+            assert str(call_args[0][1]) == "Unexpected error"
 
     def test_wait_timeouts_between_tiles(self, mock_page):
         """Test that there are appropriate waits between tiles."""

tests/unit_tests/utils/webdriver_test.py

@@ -152,16 +152,20 @@ class TestWebDriverSelenium:
         assert call_kwargs["connect_timeout"] is None
         assert call_kwargs["Page_Load_Timeout"] is None
 
-        # Check that warnings were logged
+        # Check that warnings were logged with lazy logging format
         assert mock_logger.warning.call_count == 3
         mock_logger.warning.assert_any_call(
-            "Invalid timeout value for timeout: invalid, setting to None"
+            "Invalid timeout value for %s: %s, setting to None", "timeout", "invalid"
         )
         mock_logger.warning.assert_any_call(
-            "Invalid timeout value for connect_timeout: not_a_number, setting to None"
+            "Invalid timeout value for %s: %s, setting to None",
+            "connect_timeout",
+            "not_a_number",
         )
         mock_logger.warning.assert_any_call(
-            "Invalid timeout value for Page_Load_Timeout: abc123, setting to None"
+            "Invalid timeout value for %s: %s, setting to None",
+            "Page_Load_Timeout",
+            "abc123",
         )
 
     @patch("superset.utils.webdriver.app")