Refactor tests directory structure #894
Conversation
- Created subdirectories in tests/: backend, executor, standalone, task_scheduler. - Moved and renamed test files to corresponding subdirectories. - Updated relative paths in test files for executables and backends. - Added __init__.py files to new test packages. Co-authored-by: jan-janssen <3854739+jan-janssen@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR updates test file paths and invocation commands across the repository as part of a test directory structure reorganization. The changes include updating CI workflow test targets, adjusting import/path resolution in multiple test files to reflect the new test directory hierarchy, and ensuring test artifacts are created in correct locations. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/executor/test_flux_job.py (1)
113-133:⚠️ Potential issue | 🟠 MajorAvoid shared flux.out/flux.err at repo root in parallel runs.
This now writes to the same filenames and directory as
test_output_files_abs, which can race underpytest -nor similar. Use a unique temp directory to keep tests isolated.💡 Suggested fix (isolate output files per test)
+import tempfile @@ def test_output_files_cwd(self): - dirname = os.path.abspath(os.path.join(os.path.dirname(__file__), "..", "..")) - os.makedirs(dirname, exist_ok=True) - file_stdout = os.path.join(dirname, "flux.out") - file_stderr = os.path.join(dirname, "flux.err") - with FluxJobExecutor( - max_cores=1, - resource_dict={"cores": 1, "cwd": dirname}, - flux_executor=self.executor, - block_allocation=True, - flux_log_files=True, - ) as p: - output = list(p.map(calc, [1, 2, 3])) + with tempfile.TemporaryDirectory() as tmpdir: + file_stdout = os.path.join(tmpdir, "flux.out") + file_stderr = os.path.join(tmpdir, "flux.err") + with FluxJobExecutor( + max_cores=1, + resource_dict={"cores": 1, "cwd": tmpdir}, + flux_executor=self.executor, + block_allocation=True, + flux_log_files=True, + ) as p: + output = list(p.map(calc, [1, 2, 3])) self.assertEqual( output, [1, 2, 3], ) self.assertTrue(os.path.exists(file_stdout)) self.assertTrue(os.path.exists(file_stderr)) - os.remove(file_stdout) - os.remove(file_stderr)
🤖 Fix all issues with AI agents
In `@tests/task_scheduler/file/test_serial.py`:
- Around line 227-229: Tests create the cache file using an absolute path under
the test file directory (variable file_name) but tearDown only cleans up the
executorlib_cache in the current working directory, leaving artifacts; change
the file creation to use the same cache directory tearDown removes (e.g. build
file_name from os.getcwd() or use a relative "executorlib_cache/test.h5") and
ensure os.makedirs and the with open(file_name, "w") as f block use that same
path so tearDown removes the created file and directory.
🧹 Nitpick comments (1)
tests/executor/test_single_shell_interactive.py (1)
13-13: Makeexecutable_pathabsolute to avoid CWD-dependent failures.🔧 Suggested tweak
-executable_path = os.path.join(os.path.dirname(__file__), "..", "executables", "count.py") +executable_path = os.path.abspath( + os.path.join(os.path.dirname(__file__), "..", "executables", "count.py") +)
| file_name = os.path.abspath(os.path.join(os.path.dirname(__file__), "executorlib_cache", "test.h5")) | ||
| os.makedirs(os.path.dirname(file_name), exist_ok=True) | ||
| with open(file_name, "w") as f: |
There was a problem hiding this comment.
Keep cache directory consistent with tearDown cleanup.
This now writes under tests/task_scheduler/file/executorlib_cache, but tearDown() only removes executorlib_cache in the CWD, so artifacts will remain.
🧹 Suggested alignment with existing cache cleanup
- file_name = os.path.abspath(os.path.join(os.path.dirname(__file__), "executorlib_cache", "test.h5"))
- os.makedirs(os.path.dirname(file_name), exist_ok=True)
+ cache_dir = os.path.abspath("executorlib_cache")
+ os.makedirs(cache_dir, exist_ok=True)
+ file_name = os.path.join(cache_dir, "test.h5")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| file_name = os.path.abspath(os.path.join(os.path.dirname(__file__), "executorlib_cache", "test.h5")) | |
| os.makedirs(os.path.dirname(file_name), exist_ok=True) | |
| with open(file_name, "w") as f: | |
| cache_dir = os.path.abspath("executorlib_cache") | |
| os.makedirs(cache_dir, exist_ok=True) | |
| file_name = os.path.join(cache_dir, "test.h5") | |
| with open(file_name, "w") as f: |
🤖 Prompt for AI Agents
In `@tests/task_scheduler/file/test_serial.py` around lines 227 - 229, Tests
create the cache file using an absolute path under the test file directory
(variable file_name) but tearDown only cleans up the executorlib_cache in the
current working directory, leaving artifacts; change the file creation to use
the same cache directory tearDown removes (e.g. build file_name from os.getcwd()
or use a relative "executorlib_cache/test.h5") and ensure os.makedirs and the
with open(file_name, "w") as f block use that same path so tearDown removes the
created file and directory.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/pipeline.yml (1)
290-296:⚠️ Potential issue | 🔴 CriticalInconsistent path on Line 296 will cause test failure.
Line 292 changes the working directory to
tests/, so Line 295 correctly uses a relative path (executor/test_slurm_job.py). However, Line 296 usestests/executor/test_slurm_cluster.py, which will resolve totests/tests/executor/test_slurm_cluster.pyand fail.🐛 Proposed fix
python -m unittest executor/test_slurm_job.py - python -m unittest tests/executor/test_slurm_cluster.py + python -m unittest executor/test_slurm_cluster.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #894 +/- ##
=======================================
Coverage 93.42% 93.42%
=======================================
Files 38 38
Lines 1840 1840
=======================================
Hits 1719 1719
Misses 121 121 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/unit/standalone/interactive/test_communication.py (4)
47-57:⚠️ Potential issue | 🔴 CriticalIncorrect path traversal - missing one directory level.
The file is at
tests/unit/standalone/interactive/test_communication.py. To reachsrc/executorlib/backend/, you need to traverse four levels up to the repository root, but the code only traverses three levels:
tests/unit/standalone/interactive/→..→tests/unit/standalone/- →
..→tests/unit/- →
..→tests/- Missing: →
..→ repository rootThis will resolve to
tests/src/executorlib/backend/interactive_parallel.py, which doesn't exist.🐛 Proposed fix
os.path.abspath( os.path.join( os.path.dirname(__file__), "..", "..", "..", + "..", "src", "executorlib", "backend", "interactive_parallel.py", ) ),
79-89:⚠️ Potential issue | 🔴 CriticalSame path traversal issue - missing one level up.
Same issue as
test_interface_mpi: needs four".."to reach the repository root fromtests/unit/standalone/interactive/.🐛 Proposed fix
os.path.abspath( os.path.join( os.path.dirname(__file__), "..", "..", "..", + "..", "src", "executorlib", "backend", "interactive_serial.py", ) ),
111-121:⚠️ Potential issue | 🔴 CriticalSame path traversal issue - missing one level up.
Same issue: needs four
".."to reach the repository root.🐛 Proposed fix
os.path.abspath( os.path.join( os.path.dirname(__file__), "..", "..", "..", + "..", "src", "executorlib", "backend", "interactive_serial.py", ) ),
174-184:⚠️ Potential issue | 🔴 CriticalSame path traversal issue - missing one level up.
Same issue: needs four
".."to reach the repository root.🐛 Proposed fix
os.path.abspath( os.path.join( os.path.dirname(__file__), "..", "..", "..", + "..", "src", "executorlib", "backend", "interactive_serial.py", ) ),
This change restructures the tests/ folder to better reflect the internal organization of the executorlib library. Test files are now grouped by the module they test (backend, executor, standalone, task_scheduler). Relative paths within the tests have been updated to ensure that they continue to correctly reference external scripts and the library's internal backend files. Temporary files and auto-generated version files were excluded from the final submission.
PR created automatically by Jules for task 8663878069429888553 started by @jan-janssen
Summary by CodeRabbit
Release Notes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.