Opened 10 years ago
Last modified 3 weeks ago
#23251 new Bug
Use a temporary folder to store uploaded files during tests
Reported by: | Shai Berger | Owned by: | |
---|---|---|---|
Component: | Testing framework | Version: | dev |
Severity: | Normal | Keywords: | file storage upload |
Cc: | Marc Tamlyn, Ahmad Abdallah, Bhavya Peshavaria, Jarosław Wygoda, bcail | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
Today, when running tests, Django uses the production storage for uploaded files -- meaning any tests which upload files will save copies of them, under different names, for every test run.
We need to treat this essentially like we treat mail -- during tests, a special file-storage should be set up to receive the uploads. Like with mail, it is probably better for this folder to be kept after the test run ends, and be cleared only when the tests are run again; but this is of lower priority.
This should be the default, or enabled easily in settings.
As @apollo13 noted, Django's own tests define an environment variable:
TEMP_DIR = tempfile.mkdtemp(prefix='django_') os.environ['DJANGO_TEST_TEMP_DIR'] = TEMP_DIR
and all storages used in the suite use it or a subfolder. This alone, however, is not enough for user tests, as there is currently no way to define separate storages for test and production.
Change History (28)
comment:1 by , 10 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 10 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 10 years ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
comment:4 by , 10 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
Oops, sorry. I was sure I deassigned myself.
Please, feel free to take it over.
comment:5 by , 9 years ago
FWIW, there's also django-inmemorystorage, which is even simpler/faster. But it wouldn't allow leaving the files around after a failed test run for inspection.
follow-up: 8 comment:6 by , 9 years ago
Cc: | added |
---|
As discussed on the mailing list this links nicely to the idea of a STORAGES
setting, and a new temp storage backend for testing purposes like some of our other dummy back ends.
I think it also relates to ideas in #24721 about test extensions, so that the temo storage could (optionally?) be cleared in teardown/teardowntestdata or similar.
comment:7 by , 9 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:8 by , 9 years ago
comment:10 by , 9 years ago
Replying to timgraham:
I edited comment 6 to add the link.
Thanks, I've reviewed.
So basically the idea is to introduce STORAGES
setting, move there DEFAULT_FILE_STORAGE
and STATICFILES_STORAGE
settings and finally introduce separate storage backend for testing?
comment:11 by , 9 years ago
I created #26029 to track the concept of multiple file storage backends, which may indeed make this easier to implement.
comment:12 by , 6 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:13 by , 5 years ago
In Speedy Net I defined TESTS_MEDIA_ROOT
in settings, which is different than the production MEDIA_ROOT
(it is defined only when running tests). All the files in the tests are saved there and it is deleted after the tests end. We might want to do something similar in Django for all users, and maybe define a default value to TESTS_MEDIA_ROOT
.
I also defined a variable TESTS
in settings, which is True only when running tests, and the tests asserts this value to True before running tests. This is for not to run tests accidentally with the production or other settings, but only with the tests settings. We might want to add a similar variable to the settings in Django.
You can see my commits (from today) in this branch:
https://github.com/speedy-net/speedy-net/commits/uri_main_branch_2019-06-30_a
comment:14 by , 5 years ago
Cc: | added |
---|
comment:15 by , 2 years ago
Cc: | added |
---|---|
Owner: | set to |
Status: | new → assigned |
I like the idea of using TESTS_MEDIA_ROOT
as suggested by אורי. However, how would we handle it when the user passes a custom Storage
object rather than using DEFAULT_FILE_STORAGE
?
My current plan is to keep the TESTS_MEDIA_ROOT
optional in Django settings. If it is defined, then the files will be uploaded to that path. If not, then will follow default behaviour to maintain reverse compatibility.
comment:16 by , 2 years ago
Has patch: | set |
---|
follow-up: 19 comment:18 by , 2 years ago
In the years since this ticket was first filed, cloud services have become even more pervasive and important than they were. IMO, this means that the TEST_MEDIA_ROOT
approach is inferior to a TEST_FILE_STORAGE
, where the whole default storage is replaced for tests. That would allow users to choose if the media in their tests are saved to memory (as suggested in comment:5), to a different bucket in their cloud object storage, or to a local file. Of course, if a user explicitly creates their own storage in code, there's little Django can do as a framework.
Also, I don't think backwards compatibility in the sense of "keep putting test files into production folders" is something we want here. I'd much prefer things working properly out of the box -- meaning, the default should be that media files go into a temporary folder on the local system.
comment:19 by , 2 years ago
Replying to Shai Berger:
In the years since this ticket was first filed, cloud services have become even more pervasive and important than they were. IMO, this means that the
TEST_MEDIA_ROOT
approach is inferior to aTEST_FILE_STORAGE
, where the whole default storage is replaced for tests. That would allow users to choose if the media in their tests are saved to memory (as suggested in comment:5), to a different bucket in their cloud object storage, or to a local file. Of course, if a user explicitly creates their own storage in code, there's little Django can do as a framework.
Agreed, we should first resolved #26029 (PR) to add STORAGES
, then we can add support for the new test
key.
comment:20 by , 2 years ago
Patch needs improvement: | set |
---|
follow-up: 22 comment:21 by , 18 months ago
Cc: | added |
---|
Hi Bhavya! Are you working on this ticket? Can I take it over?
comment:22 by , 18 months ago
Replying to Jarosław Wygoda:
Hi Bhavya! Are you working on this ticket? Can I take it over?
I apologize for the delay, but I am currently working on this ticket. It's taking a bit longer than expected.
comment:23 by , 15 months ago
Owner: | removed |
---|---|
Status: | assigned → new |
Hi Jarosław Wygoda! I am deassigning from this ticket, you could take it over if needed.
comment:24 by , 9 months ago
Hi Jarosław, are you planning to work on this ticket? If not, I can work on it.
For everyone, is the plan here to check for a "test" key in STORAGES
, and use that if it's present, or else use a custom temporary directory that gets automatically created and destroyed?
comment:25 by , 9 months ago
Cc: | added |
---|
Here's a small patch that sets the default storage to a temporary directory:
diff --git a/django/test/utils.py b/django/test/utils.py index ddb85127dc..2564e745ca 100644 --- a/django/test/utils.py +++ b/django/test/utils.py @@ -1,9 +1,11 @@ import collections +import copy import gc import logging import os import re import sys +import tempfile import time import warnings from contextlib import contextmanager @@ -149,6 +151,10 @@ def setup_test_environment(debug=None): saved_data.template_render = Template._render Template._render = instrumented_test_render + saved_data.storages = copy.deepcopy(settings.STORAGES) + settings.STORAGES["default"]["BACKEND"] = "django.core.files.storage.FileSystemStorage" + settings.STORAGES["default"]["OPTIONS"] = {"location": tempfile.mkdtemp()} + mail.outbox = [] deactivate() @@ -165,6 +171,7 @@ def teardown_test_environment(): settings.DEBUG = saved_data.debug settings.EMAIL_BACKEND = saved_data.email_backend Template._render = saved_data.template_render + settings.STORAGES = saved_data.storages del _TestState.saved_data del mail.outbox
Note: it doesn't currently allow for users to opt out of that behavior. Also, it makes some of the tests fail when they expect the settings to be different.
comment:26 by , 9 months ago
Owner: | set to |
---|---|
Patch needs improvement: | unset |
Status: | new → assigned |
I opened a PR.
I fixed some of the tests by overriding the settings to be more like the defaults. There's one test that tests the defaults, and it's still failing.
Right now there's no override for the temp directory... do we want to add a setting? Or check for something in the STORAGES
setting?
comment:27 by , 9 months ago
Patch needs improvement: | set |
---|
comment:28 by , 3 weeks ago
Owner: | removed |
---|---|
Status: | assigned → new |
Pavel, do you working on this? If not, I can take a look at this.