gemini-code-assist[bot] commented on code in PR #38498:
URL: https://github.com/apache/beam/pull/38498#discussion_r3239277523


##########
sdks/python/apache_beam/utils/multi_process_shared_test.py:
##########
@@ -23,11 +23,31 @@
 import threading
 import unittest
 from typing import Any
+
+import pytest
 from unittest.mock import patch
 
 from apache_beam.utils import multi_process_shared
 
 
[email protected](autouse=True)
+def isolate_multi_process_shared_tests(tmp_path, monkeypatch):
+  """Isolates MultiProcessShared tests by using a unique temp directory per 
test.
+
+  This prevents tests running in parallel (e.g. with pytest-xdist) from
+  interfering with each other by writing to the same shared default temp 
directory.
+  """
+  orig_init = multi_process_shared.MultiProcessShared.__init__
+
+  def mock_init(self, constructor, tag, *args, **kwargs):
+    if 'path' not in kwargs:
+      kwargs['path'] = str(tmp_path)
+    return orig_init(self, constructor, tag, *args, **kwargs)

Review Comment:
   ![high](https://www.gstatic.com/codereviewagent/high-priority.svg)
   
   The current implementation of `mock_init` is fragile and will fail in 
several scenarios:
   
   1. **Positional Arguments**: If `path` is passed as a positional argument 
(e.g., `MultiProcessShared(cons, tag, "/path")`), it will be captured in 
`*args`. Since `'path'` is not in `kwargs`, the code will add it to `kwargs`, 
causing `orig_init` to receive multiple values for the `path` argument and 
raising a `TypeError`.
   2. **Explicit `None`**: If `path=None` is passed explicitly as a keyword 
argument, the isolation will fail because `'path' in kwargs` will be true, and 
the mock won't replace it with the unique `tmp_path`. The original `__init__` 
will then fall back to the global temp directory.
   
   A more robust approach is to explicitly capture `path` in the signature and 
check if it is `None`.
   
   ```suggestion
     def mock_init(self, constructor, tag, path=None, *args, **kwargs):
       if path is None:
         path = str(tmp_path)
       return orig_init(self, constructor, tag, path, *args, **kwargs)
   ```



-- 
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]

Reply via email to