Bug#754143: Fix the tests to not pollute the data directory
Hi,
Raphael Hertzog écrivait:
> I merged an enhanced version of your patch:
Great!
> I'm waiting your patch for the cleanup left to be done.
Here it follows. (Note I tried and failed in getting rid of methods
import_key_from_test_file and get_test_file_path in tests_utils,
tests_models and tests_management)
Cheers,
Christophe
---
distro_tracker/accounts/tests.py | 2 +-
distro_tracker/auto_news/tests.py| 2 -
distro_tracker/core/tests/common.py | 123 ---
distro_tracker/core/tests/tests_management.py| 4 +-
distro_tracker/core/tests/tests_models.py| 15 +--
distro_tracker/core/tests/tests_retrieve_data.py | 4 +-
distro_tracker/core/tests/tests_utils.py | 4 +-
distro_tracker/extract_source_files/tests.py | 8 +-
distro_tracker/mail/tests/tests_mail_news.py | 5 -
distro_tracker/test/utils.py | 109
distro_tracker/vendor/debian/tests.py| 14 +--
functional_tests/tests.py| 2 +-
12 files changed, 121 insertions(+), 171 deletions(-)
delete mode 100644 distro_tracker/core/tests/common.py
create mode 100644 distro_tracker/test/utils.py
diff --git a/distro_tracker/accounts/tests.py b/distro_tracker/accounts/tests.py
index 417e5d6..50b560d 100644
--- a/distro_tracker/accounts/tests.py
+++ b/distro_tracker/accounts/tests.py
@@ -11,7 +11,7 @@
Tests for the :mod:`distro_tracker.accounts` app.
"""
from __future__ import unicode_literals
-from django.test import TestCase
+from distro_tracker.test import TestCase
from distro_tracker.accounts.models import User
from distro_tracker.accounts.models import UserEmail
from distro_tracker.core.models import EmailSettings
diff --git a/distro_tracker/auto_news/tests.py
b/distro_tracker/auto_news/tests.py
index 46f38d4..a68d2f3 100644
--- a/distro_tracker/auto_news/tests.py
+++ b/distro_tracker/auto_news/tests.py
@@ -23,7 +23,6 @@ from distro_tracker.core.tasks import Job
from distro_tracker.core.tasks import JobState
from distro_tracker.core.tasks import Event
from distro_tracker.auto_news.tracker_tasks import
GenerateNewsFromRepositoryUpdates
-from distro_tracker.core.tests.common import temporary_media_dir
class GenerateNewsFromRepositoryUpdatesTest(TestCase):
@@ -543,7 +542,6 @@ class GenerateNewsFromRepositoryUpdatesTest(TestCase):
news = News.objects.all()[0]
self.assertEqual(news.content, expected_content)
-@temporary_media_dir
def test_changelog_entry_in_news_content(self):
"""
Tests that the news item created for new source package versions
diff --git a/distro_tracker/core/tests/common.py
b/distro_tracker/core/tests/common.py
deleted file mode 100644
index 86febb1..000
--- a/distro_tracker/core/tests/common.py
+++ /dev/null
@@ -1,123 +0,0 @@
-# -*- coding: utf-8 -*-
-
-# Copyright 2013 The Distro Tracker Developers
-# See the COPYRIGHT file at the top-level directory of this distribution and
-# at http://deb.li/DTAuthors
-#
-# This file is part of Distro Tracker. It is subject to the license terms
-# in the LICENSE file found in the top-level directory of this
-# distribution and at http://deb.li/DTLicense. No part of Distro Tracker,
-# including this file, may be copied, modified, propagated, or distributed
-# except according to the terms contained in the LICENSE file.
-from distro_tracker.core.models import Architecture
-from distro_tracker.accounts.models import UserEmail
-from distro_tracker.core.models import ContributorName
-from distro_tracker.core.models import SourcePackage
-from distro_tracker.core.models import SourcePackageName
-from distro_tracker.core.models import BinaryPackageName
-
-import shutil
-import tempfile
-import contextlib
-
-
-@contextlib.contextmanager
-def make_temp_directory(suffix=''):
-"""
-Helper context manager which creates a temporary directory on enter and
-cleans it up on exit.
-"""
-temp_dir_name = tempfile.mkdtemp(suffix=suffix)
-try:
-yield temp_dir_name
-finally:
-shutil.rmtree(temp_dir_name)
-
-
-def temporary_media_dir(meth):
-"""
-Method decorator which creates a temporary media directory which is
-automatically cleaned up once the method exits.
-"""
-def wrap(self, *args, **kwargs):
-with make_temp_directory('-dtracker-media') as temp_media_dir:
-with self.settings(MEDIA_ROOT=temp_media_dir):
-meth(self, *args, **kwargs)
-
-return wrap
-
-
-def create_source_package(arguments):
-"""
-Creates and returns a new :class:`SourcePackage
`
-instance based on the parameters given in the arguments.
-
-It takes care to automatically create any missing maintainers, package
-names, etc.
-"""
-kwargs = {}
-if 'maintainer' in arguments:
-maintainer = arguments['maintainer']
-maintainer_email = UserEmail.objects.get_or_create
Bug#754143: Fix the tests to not pollute the data directory
Hi Christophe, On Fri, 11 Jul 2014, Christophe Siraut wrote: > The following patch should solve the issues you mention. Only the > cleanup part remains, I am waiting for your review before doing that. > Note we override the __call__ method instead of setUp, because the > latter gets overriden by the tests instances. I merged an enhanced version of your patch: commit 400455106e3a434feecabb9766e5d06f33c1dfe2 Author: Christophe Siraut Date: Sat Jul 12 23:28:52 2014 +0200 test: add TestCase objects that setup temporary directories [hert...@debian.org: - Use a single temporary directory with sub-directories. - Add more unit tests cases both for SimpleTestCase and TestCase. ] I'm waiting your patch for the cleanup left to be done. Cheers, -- Raphaël Hertzog ◈ Debian Developer Discover the Debian Administrator's Handbook: → http://debian-handbook.info/get/ -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Bug#754143: Fix the tests to not pollute the data directory
Hi Raphaël,
Raphael Hertzog wrote:
> On Wed, 09 Jul 2014, Christophe Siraut wrote:
> > > I'm not sure what's the best approach but I see two clean solutions:
> > >
> > > - something global implemented as a derivative class of TestCase that
> > > does the required directory creation and settings change in self.setUp
> > > and drops the directory with a function recorded with self.addCleanup
> >
> > See attached patch. Feel free to adapt, I am unsure where to put the
> > subclass bits.
>
> Thanks, it's a good base but here are more things that I'd like to see:
>
> 1/ we move this to distro_tracker.test and and we provide replacements for
>all of SimpleTestCase, TestCase and LiveServerTestCase
>(we should use a mixin class to factorize the code between
>all those)
>ALTERNATIVE: we don't do all the classe but just the mixin
>and we modify only the test cases that need it to insert
>the mixin there...
>
> 2/ we override all the settings which are derived from
>DISTRO_TRACKER_DATA_PATH (even directories which are only
>used to read data should point to empty directories and not
>to directories with possible production data)
>
> 3/ ideally we should also restore the settings on tearDown so that we
>don't interfer with tests that are in standard unittest.TestCase...
>
> 4/ we add unit tests for the new classes (except maybe for
>LiveServerTestCase which might be a bit heavy...) to make
>sure that the settings point to empty directories which
>are outside of DISTRO_TRACKER_BASE_PATH
>
> 5/ some docstring documentation of the new classes would be welcome
>
> Once this is done we obviously have some cleanups to do:
>
> * change all our tests to use those classes
> * get rid of core.tests.common.temporary_media_dir() since we handle that
> at the class level now
> * merge rest of distro_tracker.core.tests.common in distro_tracker.test
The following patch should solve the issues you mention. Only the
cleanup part remains, I am waiting for your review before doing that.
Note we override the __call__ method instead of setUp, because the
latter gets overriden by the tests instances.
Best regards,
Christophe
---
distro_tracker/test/__init__.py | 58 +
distro_tracker/test/tests.py | 34 +++
distro_tracker/vendor/debian/tests.py |3 +-
3 files changed, 94 insertions(+), 1 deletion(-)
create mode 100644 distro_tracker/test/__init__.py
create mode 100644 distro_tracker/test/tests.py
diff --git a/distro_tracker/test/__init__.py b/distro_tracker/test/__init__.py
new file mode 100644
index 000..61c5923
--- /dev/null
+++ b/distro_tracker/test/__init__.py
@@ -0,0 +1,58 @@
+# -*- coding: utf-8 -*-
+
+# Copyright 2013 The Distro Tracker Developers
+# See the COPYRIGHT file at the top-level directory of this distribution and
+# at http://deb.li/DTAuthors
+#
+# This file is part of Distro Tracker. It is subject to the license terms
+# in the LICENSE file found in the top-level directory of this
+# distribution and at http://deb.li/DTLicense. No part of Distro Tracker,
+# including this file, may be copied, modified, propagated, or distributed
+# except according to the terms contained in the LICENSE file.
+
+"""
+Distro Tracker test module.
+"""
+
+from django.test import TestCase as DjangoTestCase
+from django.test import SimpleTestCase as DjangoSimpleTestCase
+from django.test import LiveServerTestCase as DjangoLiveServerTestCase
+from django.conf import settings
+import shutil
+import tempfile
+import copy
+
+class CleanMixin(object):
+"""
+Use temporary folders while testing
+"""
+folders = ('STATIC_ROOT',
+ 'MEDIA_ROOT',
+ 'DISTRO_TRACKER_CACHE_DIRECTORY',
+ 'DISTRO_TRACKER_KEYRING_DIRECTORY',
+ 'DISTRO_TRACKER_TEMPLATE_DIRECTORY',
+ 'DISTRO_TRACKER_LOG_DIRECTORY')
+settings_copy = copy.deepcopy(settings)
+
+def __call__(self, result=None):
+for f in self.folders:
+d = tempfile.mkdtemp()
+setattr(settings, f, d)
+self.addCleanup(shutil.rmtree, d)
+super(CleanMixin, self).__call__(result=result)
+
+def tearDown(self):
+for f in self.folders:
+settings = self.settings_copy
+
+
+class TestCase(CleanMixin, DjangoTestCase):
+pass
+
+
+class SimpleTestCase(CleanMixin, DjangoSimpleTestCase):
+pass
+
+
+class LiveServerTestCase(CleanMixin, DjangoLiveServerTestCase):
+pass
diff --git a/distro_tracker/test/tests.py b/distro_tracker/test/tests.py
new file mode 100644
index 000..977ae7c
--- /dev/null
+++ b/distro_tracker/test/tests.py
@@ -0,0 +1,34 @@
+# -*- coding: utf-8 -*-
+
+# Copyright 2013 The Distro Tracker Developers
+# See the COPYRIGHT file at the top-level directory of this distribution and
+# at http://deb.li/DTAuthors
+#
+# This file is part of Distro Tracker. It is subject to the license terms
+
Bug#754143: Fix the tests to not pollute the data directory
On Wed, 09 Jul 2014, Christophe Siraut wrote: > > I'm not sure what's the best approach but I see two clean solutions: > > > > - something global implemented as a derivative class of TestCase that > > does the required directory creation and settings change in self.setUp > > and drops the directory with a function recorded with self.addCleanup > > See attached patch. Feel free to adapt, I am unsure where to put the > subclass bits. Thanks, it's a good base but here are more things that I'd like to see: 1/ we move this to distro_tracker.test and and we provide replacements for all of SimpleTestCase, TestCase and LiveServerTestCase (we should use a mixin class to factorize the code between all those) ALTERNATIVE: we don't do all the classe but just the mixin and we modify only the test cases that need it to insert the mixin there... 2/ we override all the settings which are derived from DISTRO_TRACKER_DATA_PATH (even directories which are only used to read data should point to empty directories and not to directories with possible production data) 3/ ideally we should also restore the settings on tearDown so that we don't interfer with tests that are in standard unittest.TestCase... 4/ we add unit tests for the new classes (except maybe for LiveServerTestCase which might be a bit heavy...) to make sure that the settings point to empty directories which are outside of DISTRO_TRACKER_BASE_PATH 5/ some docstring documentation of the new classes would be welcome Once this is done we obviously have some cleanups to do: * change all our tests to use those classes * get rid of core.tests.common.temporary_media_dir() since we handle that at the class level now * merge rest of distro_tracker.core.tests.common in distro_tracker.test Cheers, -- Raphaël Hertzog ◈ Debian Developer Discover the Debian Administrator's Handbook: → http://debian-handbook.info/get/ -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Bug#754143: Fix the tests to not pollute the data directory
--- distro_tracker/core/tests/cleantestcase.py | 13 + distro_tracker/vendor/debian/tests.py |3 ++- 2 files changed, 15 insertions(+), 1 deletion(-) create mode 100644 distro_tracker/core/tests/cleantestcase.py diff --git a/distro_tracker/core/tests/cleantestcase.py b/distro_tracker/core/tests/cleantestcase.py new file mode 100644 index 000..bef8906 --- /dev/null +++ b/distro_tracker/core/tests/cleantestcase.py @@ -0,0 +1,13 @@ +import shutil +import tempfile +from django.test import TestCase as DjangoTestCase +from django.conf import settings + +class TestCase(DjangoTestCase): +'''Use temporary data folders while testing''' +def setUp(self): +settings.DISTRO_TRACKER_CACHE_DIRECTORY = tempfile.mkdtemp() +settings.DISTRO_TRACKER_KEYRING_DIRECTORY = tempfile.mkdtemp() +self.addCleanup(shutil.rmtree, settings.DISTRO_TRACKER_CACHE_DIRECTORY) +self.addCleanup(shutil.rmtree, settings.DISTRO_TRACKER_KEYRING_DIRECTORY) +super(TestCase, self).setUp() diff --git a/distro_tracker/vendor/debian/tests.py b/distro_tracker/vendor/debian/tests.py index 0848044..3ed2fbf 100644 --- a/distro_tracker/vendor/debian/tests.py +++ b/distro_tracker/vendor/debian/tests.py @@ -15,7 +15,8 @@ Tests for Debian-specific modules/functionality of Distro Tracker. """ from __future__ import unicode_literals -from django.test import TestCase, SimpleTestCase +from django.test import SimpleTestCase +from distro_tracker.core.tests.cleantestcase import TestCase from django.test.utils import override_settings from django.core import mail from django.core.urlresolvers import reverse -- 1.7.10.4 -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Bug#754143: Fix the tests to not pollute the data directory
> I'm not sure what's the best approach but I see two clean solutions: > > - something global implemented as a derivative class of TestCase that > does the required directory creation and settings change in self.setUp > and drops the directory with a function recorded with self.addCleanup See attached patch. Feel free to adapt, I am unsure where to put the subclass bits. Cheers, Christophe >From 59dfcc53ab2e3c294b09dbddb9334b63132092e0 Mon Sep 17 00:00:00 2001 From: Christophe Siraut Date: Wed, 9 Jul 2014 17:35:23 +0200 Subject: [PATCH] Subclass TestCase for using temporary folders --- distro_tracker/core/tests/cleantestcase.py | 12 distro_tracker/vendor/debian/tests.py |3 ++- 2 files changed, 14 insertions(+), 1 deletion(-) create mode 100644 distro_tracker/core/tests/cleantestcase.py diff --git a/distro_tracker/core/tests/cleantestcase.py b/distro_tracker/core/tests/cleantestcase.py new file mode 100644 index 000..cff53a7 --- /dev/null +++ b/distro_tracker/core/tests/cleantestcase.py @@ -0,0 +1,12 @@ +import shutil +import tempfile +from django.test import TestCase as DjangoTestCase +from django.conf import settings + +class TestCase(DjangoTestCase): +def setUp(self): +settings.DISTRO_TRACKER_CACHE_DIRECTORY = tempfile.mkdtemp() +settings.DISTRO_TRACKER_KEYRING_DIRECTORY = tempfile.mkdtemp() +self.addCleanup(shutil.rmtree, settings.DISTRO_TRACKER_CACHE_DIRECTORY) +self.addCleanup(shutil.rmtree, settings.DISTRO_TRACKER_KEYRING_DIRECTORY) +super(TestCase, self).setUp() diff --git a/distro_tracker/vendor/debian/tests.py b/distro_tracker/vendor/debian/tests.py index 0848044..3ed2fbf 100644 --- a/distro_tracker/vendor/debian/tests.py +++ b/distro_tracker/vendor/debian/tests.py @@ -15,7 +15,8 @@ Tests for Debian-specific modules/functionality of Distro Tracker. """ from __future__ import unicode_literals -from django.test import TestCase, SimpleTestCase +from django.test import SimpleTestCase +from distro_tracker.core.tests.cleantestcase import TestCase from django.test.utils import override_settings from django.core import mail from django.core.urlresolvers import reverse -- 1.7.10.4
Bug#754143: Fix the tests to not pollute the data directory
Hi Christophe, On Wed, 09 Jul 2014, Christophe Siraut wrote: > Please review the attached patch. Thanks for the patch but I don't believe that you're following the correct approach here. First, tests should work and behave correctly even without the settings in project/settings/test.py. This file is just a convenience to improve some aspects of the test runs, it should not be mandatory. Then using a fixed name in /tmp/ is not acceptable either, it's a security issue. And obviously not cleaning up is also a problem, subsquent test could be polluted by the data of former tries... I'm not sure what's the best approach but I see two clean solutions: - something global implemented as a derivative class of TestCase that does the required directory creation and settings change in self.setUp and drops the directory with a function recorded with self.addCleanup - something more local with a class/method decorator that does the same (in the spirit of distro_tracker.core.tests.common.temporary_media_dir) > +# Note TestCase does not reset the cache yet, > +# see https://code.djangoproject.com/ticket/11505 Good point, but I don't think we're using a cache yet. Cheers, -- Raphaël Hertzog ◈ Debian Developer Discover the Debian Administrator's Handbook: → http://debian-handbook.info/get/ -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Bug#754143: Fix the tests to not pollute the data directory
Hi,
Please review the attached patch.
Cheers,
Christophe
>From 2fa4bbb478f6c2164caae90b898e5efef5a9a9e3 Mon Sep 17 00:00:00 2001
From: Christophe Siraut
Date: Wed, 9 Jul 2014 13:56:31 +0200
Subject: [PATCH] Fix the tests to not pollute the data directory
---
distro_tracker/project/settings/test.py | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/distro_tracker/project/settings/test.py b/distro_tracker/project/settings/test.py
index 586b9f7..168bbbc 100644
--- a/distro_tracker/project/settings/test.py
+++ b/distro_tracker/project/settings/test.py
@@ -1,5 +1,6 @@
"""Appropriate settings to run the test suite."""
-
+import os
+import shutil
from .development import *
# Don't use bcrypt to run tests (speed gain)
@@ -18,3 +19,12 @@ INSTALLED_APPS += (
'distro_tracker.vendor',
'distro_tracker.vendor.debian',
)
+
+# Use an temporary data path
+# Note TestCase does not reset the cache yet,
+# see https://code.djangoproject.com/ticket/11505
+DISTRO_TRACKER_DATA_PATH = '/tmp/distro-tracker'
+if os.path.exists(DISTRO_TRACKER_DATA_PATH):
+shutil.rmtree(DISTRO_TRACKER_DATA_PATH)
+for directory in ('keyring', 'cache'):
+ os.makedirs(os.path.join(DISTRO_TRACKER_DATA_PATH, directory))
--
1.7.10.4
Bug#754143: Fix the tests to not pollute the data directory
Package: tracker.debian.org Severity: minor After a run of "./manage.py test" you get a few files in the data directory: data/cache: 4a1eaab869bd498dd2218b580f58a891 data/cache: 4a1eaab869bd498dd2218b580f58a891.headers data/cache: 78016d49ce86f47b39d5712970ca07cf data/cache: 78016d49ce86f47b39d5712970ca07cf.headers data/cache: 99ce8528c1922aba6057158485a10285 data/cache: 99ce8528c1922aba6057158485a10285.headers data/cache: apt-cache data/cache: fabdbad731826600bd98665400c59da6 data/cache: fabdbad731826600bd98665400c59da6.headers data/keyring: pubring.gpg data/keyring: secring.gpg The tests should always work on temporary directories and not on directories meant for use in production. We want to be able to run tests on our production server without risking the integrity of our data. -- System Information: Debian Release: jessie/sid APT prefers unstable APT policy: (500, 'unstable'), (500, 'testing'), (500, 'stable'), (1, 'experimental') Architecture: amd64 (x86_64) Foreign Architectures: i386 Kernel: Linux 3.14-1-amd64 (SMP w/4 CPU cores) Locale: LANG=fr_FR.utf8, LC_CTYPE=fr_FR.utf8 (charmap=UTF-8) Shell: /bin/sh linked to /bin/dash -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
