Split sync/tests.py into smaller test modules#1431
Conversation
|
Start by adding a There is also a conflict because another pull request added more tests. Sorry about that. |
|
@tcely I’ve made the changes. Please take a look and let me know if anything should be adjusted. |
There was a problem hiding this comment.
Here are some items to work on while I keep reading. 🙂
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
### Issue description
A test writes a real file to disk under `self.media.filepath.parent` and does not remove it. This leaves artifacts under the configured download directory and can make tests environment-dependent.
### Issue Context
`Media.filepath` is derived from `Source.directory_path`, and `Source.directory_path` is rooted at `media_file_storage.location`, which is configured to `settings.DOWNLOAD_ROOT`. `DOWNLOAD_ROOT` defaults to a directory under the project base directory.
### Fix Focus Areas
- tubesync/sync/tests/test_media.py[259-262]
- tubesync/sync/models/media.py[845-848]
- tubesync/sync/models/source.py[467-479]
- tubesync/sync/models/_migrations.py[6-10]
- tubesync/tubesync/settings.py[136-142]
### Suggested fix
In the test, either:
1) Use `tempfile.TemporaryDirectory()` and `@override_settings(DOWNLOAD_ROOT=Path(tmpdir))` (and recreate `media_file_storage` if needed), or
2) Register cleanup with `self.addCleanup(lambda: filepath.unlink(missing_ok=True))` and remove created directories if appropriate.
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
### Issue description
`test_directory_prefix` assigns to `settings.SOURCE_DOWNLOAD_DIRECTORY_PREFIX` and does not restore the original value. This leaks global state across the test suite and can make other tests order-dependent.
### Issue Context
`Source.type_directory_path` uses `settings.SOURCE_DOWNLOAD_DIRECTORY_PREFIX` to decide whether to prepend `audio/` or `video/` directories, so changing the setting changes `Source.directory_path` calculations globally.
### Fix Focus Areas
- tubesync/sync/tests/test_filepath.py[158-175]
- tubesync/sync/models/source.py[467-479]
### Suggested fix
Wrap the test (or each sub-assertion) with Django’s `@override_settings(SOURCE_DOWNLOAD_DIRECTORY_PREFIX=True/False)` or store the original value and restore it via `self.addCleanup(...)`/`try...finally`.
|
I've read this over. It mostly looks great! The tests run and the AI review bot only flagged a couple of issues that need to be fixed up because of the files being split up. |
tcely
left a comment
tcely
left a comment
There was a problem hiding this comment.
Do not use shutil, and be careful to only remove individual files created by the test. Optionally, try removing empty directories created by the test.
Removal of anything that might be there is too prone to damaging things that should be preserved.
| fallback=Val(Fallback.FAIL) | ||
| ) | ||
| # Add some media | ||
| from .fixtures import all_test_metadata |
There was a problem hiding this comment.
This import should be moved to the top of the file.
| minute=1, second=1) | ||
|
|
||
| def test_nfo(self): | ||
| from .fixtures import all_test_metadata |
There was a problem hiding this comment.
This import is a duplicate, isn't it?
Let's keep this import at the top of the file.
| def test_download_finished_clears_stale_video_fields_for_audio(self): | ||
| filepath = self.media.filepath.parent / 'downloaded-audio.ogg' | ||
| filepath.parent.mkdir(parents=True, exist_ok=True) | ||
| self.addCleanup(lambda: shutil.rmtree(self.media.source.directory_path, ignore_errors=True)) |
There was a problem hiding this comment.
This is a very unsafe way to cleanup.
Consider someone who tries running the tests on their actual container.
2 participants
Closes #1427
Changes:
sync/tests.pyinto smaller test modules undersync/tests/sync/tests/fixtures.pysync/testdata/README.mdreferenceHappy to adjust based on your feedback.