Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Refs #21230 -- removed direct settings manipulation from staticfile tests #2525

Closed
wants to merge 3 commits into from

2 participants

@whoshuu

No description provided.

@timgraham timgraham commented on the diff
tests/staticfiles_tests/tests.py
@@ -121,15 +121,14 @@ class BaseCollectionTestCase(BaseStaticFilesTestCase):
"""
def setUp(self):
super(BaseCollectionTestCase, self).setUp()
- self.old_root = settings.STATIC_ROOT
@timgraham Owner

I'm not sure we want to change the behavior to store the temporary files somewhere different from os.environ['DJANGO_TEST_TEMP_DIR']?

@whoshuu
whoshuu added a note

I think the behavior should change:

Originally, a new temporary directory would be created for every set up, then settings.STATIC_ROOT would be assigned to that directory until it's time to tear down the test. This is incompatible with the way the override_settings decorator should work in principle for classes. That is, the class should be able to specify settings.STATIC_ROOT in the decorator and be done with it, instead of having it change for every test run under that class, which happens if we use the tempfile module to create the directory for us.

Interestingly, changing the behavior to use a fixed directory that is deleted on tearDown by the shutil.rmtree call reveals behavior that is expected but fails TestCollectionDryRun. This is because calling collectstatic as a dry run doesn't even create the directory specified by settings.STATIC_ROOT. The test will check if the directory is empty and fail because there is no directory.

My change creates the directory in setUp to fix the test, but I suspect more testing is probably warranted for TestCollectionDryRun to ensure its behavior never creates a directory when there is none already.

@timgraham Owner

It looks like there's a problem in that staticfiles_tests/project/site_media/static/testfile.txt is left deleted after /runtests.py staticfiles_tests.

@whoshuu
whoshuu added a note

Looking through the history of that file, looks like it was originally added here: 1d32bdd3, and then migrated along with the staticfiles_tests directory here: 89f40e36.

I can't speak for why the file was added in the first place, but I suspect it was a way of keeping the site-media/static directory alive for git. When I remove that directory, the (entire) test suite passes for both the original tests and for the tests after my changes, so I don't think it's necessary.

Maybe this file is more important than I think. Perhaps have another pair of eyes take a look at this? I can commit a delete on that file and directory if everything checks out. Otherwise, I can revert the behavior back, and that would keep this file alive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/staticfiles_tests/tests.py
@@ -752,13 +751,13 @@ def assertFileNotFound(self, filepath):
self.assertEqual(self._response(filepath).status_code, 404)
+@override_settings(DEBUG=False)
class TestServeDisabled(TestServeStatic):
"""
Test serving static files disabled when DEBUG is False.
"""
def setUp(self):
@timgraham Owner

looks like the setUp() method is no longer needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/staticfiles_tests/tests.py
@@ -121,15 +121,14 @@ class BaseCollectionTestCase(BaseStaticFilesTestCase):
"""
def setUp(self):
super(BaseCollectionTestCase, self).setUp()
- self.old_root = settings.STATIC_ROOT
- settings.STATIC_ROOT = tempfile.mkdtemp(dir=os.environ['DJANGO_TEST_TEMP_DIR'])
+ if not os.path.exists(settings.STATIC_ROOT):
+ os.mkdir(settings.STATIC_ROOT)
self.run_collectstatic()
# Use our own error handler that can handle .svn dirs on Windows
self.addCleanup(shutil.rmtree, settings.STATIC_ROOT,
ignore_errors=True, onerror=rmtree_errorhandler)
def tearDown(self):
@timgraham Owner

looks like tearDown() is no longer needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham
Owner

merged in 949ee52, thanks.

@timgraham timgraham closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 7, 2014
  1. @whoshuu
Commits on Apr 8, 2014
  1. @whoshuu

    Remove unused methods

    whoshuu authored
Commits on May 16, 2014
  1. @whoshuu

    Remove unused file

    whoshuu authored
This page is out of date. Refresh to see the latest.
View
1  tests/staticfiles_tests/project/site_media/static/testfile.txt
@@ -1 +0,0 @@
-Test!
View
13 tests/staticfiles_tests/tests.py
@@ -121,17 +121,13 @@ class BaseCollectionTestCase(BaseStaticFilesTestCase):
"""
def setUp(self):
super(BaseCollectionTestCase, self).setUp()
- self.old_root = settings.STATIC_ROOT
@timgraham Owner

I'm not sure we want to change the behavior to store the temporary files somewhere different from os.environ['DJANGO_TEST_TEMP_DIR']?

@whoshuu
whoshuu added a note

I think the behavior should change:

Originally, a new temporary directory would be created for every set up, then settings.STATIC_ROOT would be assigned to that directory until it's time to tear down the test. This is incompatible with the way the override_settings decorator should work in principle for classes. That is, the class should be able to specify settings.STATIC_ROOT in the decorator and be done with it, instead of having it change for every test run under that class, which happens if we use the tempfile module to create the directory for us.

Interestingly, changing the behavior to use a fixed directory that is deleted on tearDown by the shutil.rmtree call reveals behavior that is expected but fails TestCollectionDryRun. This is because calling collectstatic as a dry run doesn't even create the directory specified by settings.STATIC_ROOT. The test will check if the directory is empty and fail because there is no directory.

My change creates the directory in setUp to fix the test, but I suspect more testing is probably warranted for TestCollectionDryRun to ensure its behavior never creates a directory when there is none already.

@timgraham Owner

It looks like there's a problem in that staticfiles_tests/project/site_media/static/testfile.txt is left deleted after /runtests.py staticfiles_tests.

@whoshuu
whoshuu added a note

Looking through the history of that file, looks like it was originally added here: 1d32bdd3, and then migrated along with the staticfiles_tests directory here: 89f40e36.

I can't speak for why the file was added in the first place, but I suspect it was a way of keeping the site-media/static directory alive for git. When I remove that directory, the (entire) test suite passes for both the original tests and for the tests after my changes, so I don't think it's necessary.

Maybe this file is more important than I think. Perhaps have another pair of eyes take a look at this? I can commit a delete on that file and directory if everything checks out. Otherwise, I can revert the behavior back, and that would keep this file alive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
- settings.STATIC_ROOT = tempfile.mkdtemp(dir=os.environ['DJANGO_TEST_TEMP_DIR'])
+ if not os.path.exists(settings.STATIC_ROOT):
+ os.mkdir(settings.STATIC_ROOT)
self.run_collectstatic()
# Use our own error handler that can handle .svn dirs on Windows
self.addCleanup(shutil.rmtree, settings.STATIC_ROOT,
ignore_errors=True, onerror=rmtree_errorhandler)
- def tearDown(self):
- settings.STATIC_ROOT = self.old_root
- super(BaseCollectionTestCase, self).tearDown()
-
def run_collectstatic(self, **kwargs):
call_command('collectstatic', interactive=False, verbosity='0',
ignore_patterns=['*.ignoreme'], **kwargs)
@@ -752,14 +748,11 @@ def assertFileNotFound(self, filepath):
self.assertEqual(self._response(filepath).status_code, 404)
+@override_settings(DEBUG=False)
class TestServeDisabled(TestServeStatic):
"""
Test serving static files disabled when DEBUG is False.
"""
- def setUp(self):
- super(TestServeDisabled, self).setUp()
- settings.DEBUG = False
-
def test_disabled_serving(self):
self.assertFileNotFound('test.txt')
Something went wrong with that request. Please try again.