Skip to content

Clean media files between test runs #263

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

Open
leducvin wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Clean media files between test runs #263

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
41cc9c2
Update Makefile
leducvin Aug 18, 2025
87c8fca
Update pyproject.toml
leducvin Aug 18, 2025
4d0a050
Update tox.ini
leducvin Aug 18, 2025
048d784
Update Makefile
leducvin Aug 19, 2025
5ef9e48
Update tox.ini
leducvin Aug 19, 2025
a2436cc
Update settings.py
leducvin Aug 19, 2025
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
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ help: ## ⁉️ - Display help comments for each make command

clean: ## 🗑️ - Removes pycache and test media
@echo "🗑️ - Removing __pycache__ and test artifacts"
rm -rf test-media test-static .tox
rm -rf tests/testapp/test-media tests/testapp/test-static .tox
find . -type d -name "__pycache__" -exec rm -r {} +

package-setup:
Expand Down
1 change: 1 addition & 0 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ dependencies = [

[project.optional-dependencies]
testing = [
"tox>=4.22",
"coverage>=7.8.0",
]
linting = [
Expand Down
2 changes: 2 additions & 0 deletions tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,11 @@ deps =
wagtail63: wagtail>=6.3,<6.4
wagtail70: wagtail>=7.0rc1,<7.1

allowlist_externals = rm
commands_pre =
python -I {toxinidir}/tests/manage.py migrate
commands =
-rm -rf tests/testapp/test-media
Copy link
Member

@zerolab zerolab Aug 19, 2025

Choose a reason for hiding this comment

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

Sorry, but this is not an acceptable way. adding rm to allowed externals opens up the door for potential abuse.

The solution should be entirely TestCase-bound. That is, clear the test uploaded media directory on tearDown. e.g.

https://github.com/django/django/blob/f5c944b3141c58bb4a5c7bbca61180b2ad7c13aa/tests/file_storage/tests.py#L733-L735

or

https://github.com/django/django/blob/f5c944b3141c58bb4a5c7bbca61180b2ad7c13aa/tests/staticfiles_tests/test_storage.py#L438-L443

Copy link
Author

@leducvin leducvin Aug 19, 2025

Choose a reason for hiding this comment

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

Oh OK, thanks for the feedback -- I'll change it to do it in tearDown

Copy link
Author

@leducvin leducvin Aug 19, 2025

Choose a reason for hiding this comment

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

Actually, I noticed in

https://github.com/torchbox/wagtailmedia/blob/794bd7e170bb709e5e43084b106b29f34faa9209/tests/manage.py#L57C1-L63C54

that there was already an attempt to clean test media. The problem is that it didn't do anything since the test app used a different MEDIA_ROOT.

So I changed the test app settings to use the same MEDIA_ROOT. Would this be acceptable?

python -Im coverage run {toxinidir}/tests/manage.py test --deprecation all {posargs: -v 2}

[testenv:coverage-report]
Expand Down