Skip to content

chore(tests): split tests into smaller files#1431

Merged
tcely merged 5 commits into
meeb:mainfrom
mady20:issue/1427/split-tests
Mar 22, 2026
Merged

chore(tests): split tests into smaller files#1431
tcely merged 5 commits into
meeb:mainfrom
mady20:issue/1427/split-tests

Conversation

@mady20
Copy link
Copy Markdown
Contributor

@mady20 mady20 commented Mar 19, 2026

Closes #1427

Changes:

  • Split sync/tests.py into smaller test modules under sync/tests/
  • Moved shared test metadata loading into sync/tests/fixtures.py
  • Updated the related sync/testdata/README.md reference

Happy to adjust based on your feedback.

@tcely
Copy link
Copy Markdown
Collaborator

tcely commented Mar 19, 2026

Start by adding a tubesync/sync/tests/README.md file to help people find the tests when failures happen.

There is also a conflict because another pull request added more tests. Sorry about that.

@mady20
Copy link
Copy Markdown
Contributor Author

mady20 commented Mar 19, 2026

@tcely I’ve made the changes. Please take a look and let me know if anything should be adjusted.

@tcely tcely moved this to Todo in Status Mar 19, 2026
Copy link
Copy Markdown
Collaborator

@tcely tcely left a comment

Choose a reason for hiding this comment

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

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`.

Comment thread tubesync/sync/tests/fixtures.py
Comment thread tubesync/sync/tests/test_filepath.py Outdated
Comment thread tubesync/sync/tests/test_media.py Outdated
Comment thread tubesync/sync/tests/test_media.py Outdated
Comment thread tubesync/sync/tests/test_media.py Outdated
@github-project-automation github-project-automation Bot moved this from Todo to In Progress in Status Mar 19, 2026
Comment thread tubesync/sync/tests/test_tasks.py
@tcely
Copy link
Copy Markdown
Collaborator

tcely commented Mar 19, 2026

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.

@mady20 mady20 requested a review from tcely March 21, 2026 20:15
@tcely tcely requested a review from meeb March 21, 2026 20:25
Copy link
Copy Markdown
Collaborator

@tcely tcely left a comment

Choose a reason for hiding this comment

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

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.

Comment thread tubesync/sync/tests/test_frontend.py Outdated
Comment thread tubesync/sync/tests/test_media.py Outdated
Comment thread tubesync/sync/tests/test_media.py Outdated
@mady20 mady20 requested a review from tcely March 22, 2026 11:37
Copy link
Copy Markdown
Collaborator

@tcely tcely left a comment

Choose a reason for hiding this comment

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

Thanks for contributing!

@github-project-automation github-project-automation Bot moved this from In Progress to Ready in Status Mar 22, 2026
@tcely tcely changed the title Split sync/tests.py into smaller test modules chore(tests): split tests into smaller files Mar 22, 2026
@tcely tcely merged commit 8041f54 into meeb:main Mar 22, 2026
5 checks passed
@github-project-automation github-project-automation Bot moved this from Ready to Done in Status Mar 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Split sync/tests.py into smaller files

3 participants