Opened 11 years ago
Last modified 12 months 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 Tim Graham, 11 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by Pavel Shpilev, 11 years ago
Owner: | changed from nobody to Pavel Shpilev |
---|---|
Status: | new → assigned |
comment:3 by Berker Peksag, 11 years ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
Pavel, do you working on this? If not, I can take a look at this.
comment:4 by Pavel Shpilev, 11 years ago
Owner: | Pavel Shpilev removed |
---|---|
Status: | assigned → new |
Oops, sorry. I was sure I deassigned myself.
Please, feel free to take it over.
comment:5 by Carl Meyer, 10 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 Marc Tamlyn, 10 years ago
Cc: | Marc Tamlyn 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 Sasha Gaevsky, 10 years ago
Owner: | set to Sasha Gaevsky |
---|---|
Status: | new → assigned |
in reply to: 6 comment:8 by Sasha Gaevsky, 10 years ago
in reply to: 9 comment:10 by Sasha Gaevsky, 10 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 Aymeric Augustin, 10 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 Sasha Gaevsky, 7 years ago
Owner: | Sasha Gaevsky removed |
---|---|
Status: | assigned → new |
comment:13 by אורי, 6 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 Ahmad Abdallah, 6 years ago
Cc: | Ahmad Abdallah added |
---|
comment:15 by Bhavya Peshavaria, 3 years ago
Cc: | Bhavya Peshavaria added |
---|---|
Owner: | set to Bhavya Peshavaria |
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 Bhavya Peshavaria, 3 years ago
Has patch: | set |
---|
follow-up: 19 comment:18 by Shai Berger, 3 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.
in reply to: 18 comment:19 by Mariusz Felisiak, 3 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 Mariusz Felisiak, 3 years ago
Patch needs improvement: | set |
---|
follow-up: 22 comment:21 by Jarosław Wygoda, 2 years ago
Cc: | Jarosław Wygoda added |
---|
Hi Bhavya! Are you working on this ticket? Can I take it over?
in reply to: 21 comment:22 by Bhavya Peshavaria, 2 years 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 Bhavya Peshavaria, 2 years ago
Owner: | Bhavya Peshavaria removed |
---|---|
Status: | assigned → new |
Hi Jarosław Wygoda! I am deassigning from this ticket, you could take it over if needed.
comment:24 by bcail, 20 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 bcail, 20 months ago
Cc: | bcail 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 bcail, 20 months ago
Owner: | set to bcail |
---|---|
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 Mariusz Felisiak, 20 months ago
Patch needs improvement: | set |
---|
comment:28 by bcail, 12 months ago
Owner: | bcail removed |
---|---|
Status: | assigned → new |