tests/test_logging.py (1)

104-136: Consider adding explicit assertion and ensuring cleanup.

The test correctly exercises the bug scenario, but the success condition is implicit. Additionally, if an exception occurs before removeHandler, the handler remains attached to the global logger which could affect other tests.

Suggested improvements for robustness

 def test_issue4063():
     # since it's difficult to replicate the exact conditions,
     # we'll do a unit test of the relevant logging code instead of a full workflow run.
     # the issue was that if the logger is configured with printshellcmds=True,
     # shell.py calls logger.info(None, extra={"event": LogEvent.SHELLCMD, "cmd": ...}),
     # which causes a crash because the default formatter expects the first argument to be a string message.
     from snakemake.logging import ColorizingTextHandler
     from snakemake.logging import DefaultFormatter
     from snakemake.logging import DefaultFilter
 
     handler = ColorizingTextHandler(stream=sys.stdout)
     formatter = DefaultFormatter(quiet=set(), show_failed_logs=False)
     handler.setFormatter(formatter)
 
     log_filter = DefaultFilter(
         quiet=set(), debug_dag=False, dryrun=False, printshellcmds=True
     )
     handler.addFilter(log_filter)
 
     # make the logging error a hard error
     def handle_logging_error(record):
         raise sys.exc_info()[1]
 
     handler.handleError = handle_logging_error
 
     test_logger = logging.getLogger("foo")
     test_logger.setLevel(logging.INFO)
     test_logger.addHandler(handler)
 
-    test_logger.info(
-        None, extra={"event": LogEvent.SHELLCMD, "cmd": "echo 'bar'"}
-    )
-    test_logger.removeHandler(handler)
+    try:
+        # This should not raise - the fix ensures None messages are handled gracefully
+        test_logger.info(
+            None, extra={"event": LogEvent.SHELLCMD, "cmd": "echo 'bar'"}
+        )
+    finally:
+        test_logger.removeHandler(handler)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_logging.py` around lines 104 - 136, The test test_issue4063
currently relies on implicit success and may leave the ColorizingTextHandler
attached if an error occurs; wrap the logging call in a try/finally inside
test_issue4063 so you always call test_logger.removeHandler(handler) in the
finally block, and add an explicit assertion (e.g., assert handler not in
test_logger.handlers or assert no exception was raised) after the logging call
to make the success condition explicit; reference the ColorizingTextHandler,
DefaultFormatter, DefaultFilter, and test_logger variables when applying the
change.