Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use same cache directory #466

Merged
merged 1 commit into from
Oct 29, 2024
Merged

Use same cache directory #466

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion tests/test_cache_executor_pysqa_flux.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,9 @@ class TestCacheExecutorPysqa(unittest.TestCase):
def test_executor(self):
with Executor(
backend="pysqa_flux",
resource_dict={"cores": 2, "cwd": "exe_working_directory"},
resource_dict={"cores": 2, "cwd": "cache"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

🛠️ Refactor suggestion

Consistent test isolation improvements needed across test files

The review suggestion is valid and should be applied consistently across the test suite. The investigation reveals that:

  1. All test files use hardcoded "cache" directory
  2. All files implement tearDown for cleanup, but none use tempfile for isolation
  3. No setUp methods are currently implemented in any of these files
  4. Similar cleanup pattern if os.path.exists("cache"): shutil.rmtree("cache") is repeated

Suggested improvements for all affected files:

  • tests/test_cache_executor_pysqa_flux.py
  • tests/test_executor_backend_mpi.py
  • tests/test_cache_shared.py
  • tests/test_cache_hdf.py
  • tests/test_cache_executor_serial.py
🔗 Analysis chain

Consider improving path handling and test isolation.

While the current implementation works, consider these improvements:

  1. Use tempfile.mkdtemp() for test isolation
  2. Use absolute paths to avoid potential working directory issues

Here's a suggested implementation:

+import tempfile
+
 class TestCacheExecutorPysqa(unittest.TestCase):
+    def setUp(self):
+        self.test_dir = tempfile.mkdtemp(prefix="cache_executor_test_")
+
     def test_executor(self):
         with Executor(
             backend="pysqa_flux",
-            resource_dict={"cores": 2, "cwd": "cache"},
+            resource_dict={"cores": 2, "cwd": self.test_dir},
             block_allocation=False,
-            cache_directory="cache",
+            cache_directory=self.test_dir,
         ) as exe:
 
     def tearDown(self):
-        if os.path.exists("cache"):
-            shutil.rmtree("cache")
+        if os.path.exists(self.test_dir):
+            shutil.rmtree(self.test_dir)

Let's check if other test files use similar cache directory patterns:

Also applies to: 39-39

Let's examine the cache directory usage in the other test files to understand the pattern and verify if the suggested improvements should be applied consistently.

Let's check if these test files already have cleanup mechanisms in place and if they use any temporary directory solutions.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for cache directory patterns in test files
echo "Searching for cache directory patterns in tests:"
rg -l "cache_directory|cwd.*cache" tests/

Length of output: 312

Script:

#!/bin/bash
# Check cache directory usage in the identified test files
echo "=== test_executor_backend_mpi.py ==="
rg "cache_directory|cwd.*cache" tests/test_executor_backend_mpi.py -B 2 -A 2

echo -e "\n=== test_cache_shared.py ==="
rg "cache_directory|cwd.*cache" tests/test_cache_shared.py -B 2 -A 2

echo -e "\n=== test_cache_hdf.py ==="
rg "cache_directory|cwd.*cache" tests/test_cache_hdf.py -B 2 -A 2

echo -e "\n=== test_cache_executor_serial.py ==="
rg "cache_directory|cwd.*cache" tests/test_cache_executor_serial.py -B 2 -A 2

Length of output: 4827

Script:

#!/bin/bash
# Check for cleanup and temporary directory usage
echo "=== Checking for tearDown and temporary directory usage ==="
for file in tests/test_executor_backend_mpi.py tests/test_cache_shared.py tests/test_cache_hdf.py tests/test_cache_executor_serial.py; do
    echo -e "\n=== $file ==="
    # Look for tearDown methods and cleanup code
    rg "def tearDown|rmtree|tempfile" "$file" -B 1 -A 3
    
    # Look for setUp methods
    echo -e "\n--- setUp methods in $file ---"
    rg "def setUp" "$file" -B 1 -A 3
done

Length of output: 2585

block_allocation=False,
cache_directory="cache",
) as exe:
cloudpickle_register(ind=1)
fs1 = exe.submit(mpi_funct, 1)
Expand Down
Loading