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 Tim Graham, 10 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Pavel Shpilev, 10 years ago

Owner: changed from nobody to Pavel Shpilev
Status: newassigned

comment:3 by Berker Peksag, 10 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, 10 years ago

Owner: Pavel Shpilev removed
Status: assignednew

Oops, sorry. I was sure I deassigned myself.
Please, feel free to take it over.

comment:5 by Carl Meyer, 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.

comment:6 by Marc Tamlyn, 9 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.

Last edited 9 years ago by Tim Graham (previous) (diff)

comment:7 by Sasha Gaevsky, 9 years ago

Owner: set to Sasha Gaevsky
Status: newassigned

in reply to:  6 comment:8 by Sasha Gaevsky, 9 years ago

Replying to mjtamlyn:

As discussed on the mailing list[1] 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.

Link is pointing to changeset, not mailing list. Could you please update it? Thanks.

comment:9 by Tim Graham, 9 years ago

I edited comment 6 to add the link.

in reply to:  9 comment:10 by Sasha Gaevsky, 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 Aymeric Augustin, 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 Sasha Gaevsky, 6 years ago

Owner: Sasha Gaevsky removed
Status: assignednew

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 Ahmad Abdallah, 5 years ago

Cc: Ahmad Abdallah added

comment:15 by Bhavya Peshavaria, 2 years ago

Cc: Bhavya Peshavaria added
Owner: set to Bhavya Peshavaria
Status: newassigned

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.

Last edited 2 years ago by Bhavya Peshavaria (previous) (diff)

comment:16 by Bhavya Peshavaria, 2 years ago

Has patch: set

comment:18 by Shai Berger, 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.

in reply to:  18 comment:19 by Mariusz Felisiak, 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 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.

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, 2 years ago

Patch needs improvement: set

comment:21 by Jarosław Wygoda, 18 months 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, 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.

Last edited 18 months ago by Bhavya Peshavaria (previous) (diff)

comment:23 by Bhavya Peshavaria, 15 months ago

Owner: Bhavya Peshavaria removed
Status: assignednew

Hi Jarosław Wygoda! I am deassigning from this ticket, you could take it over if needed.

comment:24 by bcail, 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 bcail, 9 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, 9 months ago

Owner: set to bcail
Patch needs improvement: unset
Status: newassigned

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, 9 months ago

Patch needs improvement: set

comment:28 by bcail, 3 weeks ago

Owner: bcail removed
Status: assignednew
Note: See TracTickets for help on using tickets.
Back to Top