[Django] #23251: Use a temporary folder to store uploaded files during tests
Django
-------------------------------------+-------------------------------------
Reporter: shai | Owner: nobody
Type: Bug | Status: new
Component: Testing | Version: master
framework | Keywords: file storage upload
Severity: Normal | Has patch: 0
Triage Stage: | Needs tests: 1
Unreviewed | Easy pickings: 0
Needs documentation: 1 |
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
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:
{{{#!python
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23251>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+------------------------------------
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: file storage upload | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
Changes (by timgraham):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/23251#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: shai | Owner:
Component: Testing framework | Status: assigned
Severity: Normal | Version: master
Keywords: file storage upload | Resolution:
Has patch: 0 | Triage Stage: Accepted
Needs tests: 1 | Needs documentation: 1
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by pavel_shpilev):
* owner: nobody => pavel_shpilev
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/23251#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: shai | Owner:
Component: Testing framework | Status: assigned
Severity: Normal | Version: master
Keywords: file storage upload | Resolution:
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by berkerpeksag):
* needs_docs: 1 => 0
* needs_tests: 1 => 0
Comment:
Pavel, do you working on this? If not, I can take a look at this.
--
Ticket URL: <https://code.djangoproject.com/ticket/23251#comment:3>
Django
-------------------------------------+------------------------------------
Type: Bug | Status: new
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: file storage upload | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
* owner: pavel_shpilev =>
* status: assigned => new
Comment:
Oops, sorry. I was sure I deassigned myself.
Please, feel free to take it over.
--
Ticket URL: <https://code.djangoproject.com/ticket/23251#comment:4>
Django
-------------------------------------+------------------------------------
Type: Bug | Status: new
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: file storage upload | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
Comment (by carljm):
FWIW, there's also [https://pypi.python.org/pypi/django-inmemorystorage
django-inmemorystorage], which is even simpler/faster. But it wouldn't
allow leaving the files around after a failed test run for inspection.
--
Ticket URL: <https://code.djangoproject.com/ticket/23251#comment:5>
Django
-------------------------------------+------------------------------------
Type: Bug | Status: new
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: file storage upload | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
* cc: mjtamlyn (added)
Comment:
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.
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23251#comment:6>
Django
-------------------------------------+------------------------------------
Type: Bug | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: file storage upload | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
* owner: => sasha0
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/23251#comment:7>
Django
Reporter: shai | Owner: sasha0
Type: Bug | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: file storage upload | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
Comment (by sasha0):
Replying to [comment:6 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23251#comment:8>
Django
Reporter: shai | Owner: sasha0
Type: Bug | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: file storage upload | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
Comment (by timgraham):
I edited comment 6 to add the link.
--
Ticket URL: <https://code.djangoproject.com/ticket/23251#comment:9>
Django
Reporter: shai | Owner: sasha0
Type: Bug | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: file storage upload | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
Comment (by sasha0):
Replying to [comment:9 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/23251#comment:10>
Django
Reporter: shai | Owner: sasha0
Type: Bug | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: file storage upload | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
Comment (by aaugustin):
I created #26029 to track the concept of multiple file storage backends,
which may indeed make this easier to implement.
--
Ticket URL: <https://code.djangoproject.com/ticket/23251#comment:11>
Django
-------------------------------------+------------------------------------
Type: Bug | Status: new
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: file storage upload | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
* status: assigned => new
* owner: Sasha Gaevsky => (none)
--
Ticket URL: <https://code.djangoproject.com/ticket/23251#comment:12>
Django
Reporter: Shai Berger | Owner: (none)
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: file storage upload | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
Comment (by אורי):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/23251#comment:13>
Django
Reporter: Shai Berger | Owner: (none)
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: file storage upload | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
* cc: Ahmad Abdallah (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/23251#comment:14>
Django
-------------------------------------+-------------------------------------
| Peshavaria
Type: Bug | Status: assigned
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: file storage upload | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Bhavya Peshavaria):
* cc: Bhavya Peshavaria (added)
* owner: (none) => Bhavya Peshavaria
* status: new => assigned
Comment:
I like the idea of using `TESTS_MEDIA_ROOT` as suggested by Ahmad
Abdallah. 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23251#comment:15>
Django
-------------------------------------+-------------------------------------
| Peshavaria
Type: Bug | Status: assigned
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: file storage upload | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Bhavya Peshavaria):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/23251#comment:16>
Django
-------------------------------------+-------------------------------------
| Peshavaria
Type: Bug | Status: assigned
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: file storage upload | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Bhavya Peshavaria):
PR:[ https://github.com/django/django/pull/15848]
--
Ticket URL: <https://code.djangoproject.com/ticket/23251#comment:17>
Django
-------------------------------------+-------------------------------------
| Peshavaria
Type: Bug | Status: assigned
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: file storage upload | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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.
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23251#comment:18>
Django
-------------------------------------+-------------------------------------
| Peshavaria
Type: Bug | Status: assigned
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: file storage upload | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:18 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
([https://github.com/django/django/pull/15610 PR]) to add `STORAGES`, then
we can add support for the new `test` key.
--
Ticket URL: <https://code.djangoproject.com/ticket/23251#comment:19>
Django
-------------------------------------+-------------------------------------
| Peshavaria
Type: Bug | Status: assigned
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: file storage upload | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/23251#comment:20>
Django
-------------------------------------+-------------------------------------
| Peshavaria
Type: Bug | Status: assigned
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: file storage upload | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Jarosław Wygoda (added)
Comment:
Hi Bhavya! Are you working on this ticket? Can I take it over?
--
Ticket URL: <https://code.djangoproject.com/ticket/23251#comment:21>
Django
-------------------------------------+-------------------------------------
| Peshavaria
Type: Bug | Status: assigned
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: file storage upload | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Bhavya Peshavaria):
Replying to [comment:21 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23251#comment:22>
Django
Reporter: Shai Berger | Owner: (none)
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: file storage upload | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
* owner: Bhavya Peshavaria => (none)
* status: assigned => new
Comment:
Hi Jarosław Wygoda! I am deassigning from this ticket, you could take it
over if needed.
--
Ticket URL: <https://code.djangoproject.com/ticket/23251#comment:23>
Django
Reporter: Shai Berger | Owner: (none)
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: file storage upload | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/23251#comment:24>
Django
Reporter: Shai Berger | Owner: (none)
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: file storage upload | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
* cc: bcail (added)
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23251#comment:25>
Django
-------------------------------------+------------------------------------
Type: Bug | Status: assigned
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: file storage upload | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
Changes (by bcail):
* owner: (none) => bcail
* needs_better_patch: 1 => 0
* status: new => assigned
Comment:
I opened a [https://github.com/django/django/pull/17853 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/23251#comment:26>
Django
-------------------------------------+------------------------------------
Reporter: Shai Berger | Owner: bcail
Type: Bug | Status: assigned
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: file storage upload | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/23251#comment:27>