chore: clean up files created during testing#11844
Conversation
modified test_save_file_component.py to clean up generated test files once tests are done
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughA class-scoped pytest fixture was added to clean up test output files generated by Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (41.50%) is below the target coverage (60.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #11844 +/- ##
=======================================
Coverage 37.05% 37.05%
=======================================
Files 1588 1588
Lines 77959 77959
Branches 11800 11800
=======================================
+ Hits 28885 28886 +1
Misses 47455 47455
+ Partials 1619 1618 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/backend/tests/unit/components/processing/test_save_file_component.py (2)
14-24: Preferred fix: redirect file creation to a temp dir rather than cleaning up CWD artifactsThe cleanup fixture treats the symptom (leftover CWD files) rather than the root cause. The coding guidelines explicitly require using
tmp_path/tmp_path_factoryso test files never land in the working tree. A class-scopedtmp_path_factoryfixture combined with amonkeypatchofPath(or the component's output directory) eliminates the need for any explicit cleanup and works regardless of which directory pytest is invoked from.♻️ Suggested approach
+import os import contextlib from pathlib import Path ... class TestSaveToFileComponent(ComponentTestBaseWithoutClient): - `@pytest.fixture`(scope="class", autouse=True) - def cleanup_test_files(self): - """Clean up test files after all tests in the class complete.""" - yield - # Clean up test files created during tests - test_files = ["test_data.json", "test_message.txt", "test_output.csv"] - for filename in test_files: - filepath = Path(filename) - if filepath.exists(): - with contextlib.suppress(Exception): - filepath.unlink() + `@pytest.fixture`(scope="class") + def test_output_dir(self, tmp_path_factory): + """Provide a dedicated temp directory for all file-output tests.""" + return tmp_path_factory.mktemp("save_file_tests")Then pass
test_output_dirinto the individual tests that exercise local-file output and setfile_nametostr(test_output_dir / "test_output"), etc., so files land in the temp directory that pytest cleans up automatically.Based on learnings: "Use
aiofilesandanyio.Pathfor async file operations in tests; create temporary test files usingtmp_pathfixture and verify file existence and content"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/tests/unit/components/processing/test_save_file_component.py` around lines 14 - 24, The cleanup_test_files fixture is masking the real issue by deleting CWD artifacts; replace it with a class-scoped tmp_path_factory-based temp directory and update tests to write into that temp directory instead of the CWD. Create a class-scoped fixture (using tmp_path_factory) that yields test_output_dir and monkeypatch the component's output directory or Path usage so functions under test (e.g., save file code paths that accept file_name/output dir) write to str(test_output_dir / "test_output..."); remove cleanup_test_files and update tests to use aiofiles/anyio.Path or pytest's tmp_path for async file operations and assert file existence/content in the temp dir so pytest cleans up automatically.
14-24: Minor fixture implementation nitsThree small clean-ups for the current approach (if kept):
try/finally— per project conventions, fixture teardown should usetry/finallyinstead of relying on implicit post-yieldexecution.- Redundant existence check —
filepath.exists()is unnecessary becausecontextlib.suppressalready swallowsFileNotFoundError.- Overly broad exception suppression —
contextlib.suppress(Exception)silences all errors;OSError(which coversFileNotFoundErrorandPermissionError) is the appropriate type for filesystem operations.♻️ Proposed fix
`@pytest.fixture`(scope="class", autouse=True) def cleanup_test_files(self): """Clean up test files after all tests in the class complete.""" - yield - # Clean up test files created during tests - test_files = ["test_data.json", "test_message.txt", "test_output.csv"] - for filename in test_files: - filepath = Path(filename) - if filepath.exists(): - with contextlib.suppress(Exception): - filepath.unlink() + try: + yield + finally: + test_files = ["test_data.json", "test_message.txt", "test_output.csv"] + for filename in test_files: + with contextlib.suppress(OSError): + Path(filename).unlink()Based on learnings: "Use async fixtures with proper cleanup using try/finally blocks to ensure resources are properly released after tests complete"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/tests/unit/components/processing/test_save_file_component.py` around lines 14 - 24, The fixture cleanup_test_files should perform teardown in an explicit try/finally block rather than relying on post-yield execution: wrap the test body in try and move the file-removal loop into the finally block; remove the redundant filepath.exists() check inside that loop; and narrow exception suppression to filesystem errors by using contextlib.suppress(OSError) when calling filepath.unlink() (refer to the cleanup_test_files fixture and the test_files list).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/backend/tests/unit/components/processing/test_save_file_component.py`:
- Around line 14-24: The cleanup_test_files fixture is masking the real issue by
deleting CWD artifacts; replace it with a class-scoped tmp_path_factory-based
temp directory and update tests to write into that temp directory instead of the
CWD. Create a class-scoped fixture (using tmp_path_factory) that yields
test_output_dir and monkeypatch the component's output directory or Path usage
so functions under test (e.g., save file code paths that accept file_name/output
dir) write to str(test_output_dir / "test_output..."); remove cleanup_test_files
and update tests to use aiofiles/anyio.Path or pytest's tmp_path for async file
operations and assert file existence/content in the temp dir so pytest cleans up
automatically.
- Around line 14-24: The fixture cleanup_test_files should perform teardown in
an explicit try/finally block rather than relying on post-yield execution: wrap
the test body in try and move the file-removal loop into the finally block;
remove the redundant filepath.exists() check inside that loop; and narrow
exception suppression to filesystem errors by using contextlib.suppress(OSError)
when calling filepath.unlink() (refer to the cleanup_test_files fixture and the
test_files list).
|
Build successful! ✅ |
d3a5ab3 to
b5f2fcd
Compare
modified test_save_file_component.py to clean up generated test files once tests are done
modified test_save_file_component.py to clean up generated test files once tests are done
Added files after runing

make unit_testsBefore
After

Summary by CodeRabbit
Release Notes
Note: This release contains internal testing infrastructure improvements with no direct impact on end-user functionality.