diff mbox

[v2,07/16] tests: Move 'reverse' calls inside 'setUp'

Message ID 1440167540-8751-8-git-send-email-stephen.finucane@intel.com
State Accepted
Headers show

Commit Message

Stephen Finucane Aug. 21, 2015, 2:32 p.m. UTC
Django creates test databases after it loads tests. However, any
operations that exist at class level will be executed before this
database is created. Fix the instances of this issue (mostly 'reverse'
calls or similar) by moving the calls into the relevant 'setUp'
functions for each test.

Signed-off-by: Stephen Finucane <stephen.finucane@intel.com>
---
 patchwork/tests/test_mail_settings.py | 38 +++++++++++++++++------------------
 patchwork/tests/test_user.py          | 11 +++++++---
 2 files changed, 27 insertions(+), 22 deletions(-)

Comments

Damien Lespiau Sept. 17, 2015, 4:46 p.m. UTC | #1
On Fri, Aug 21, 2015 at 03:32:11PM +0100, Stephen Finucane wrote:
> Django creates test databases after it loads tests. However, any
> operations that exist at class level will be executed before this
> database is created. Fix the instances of this issue (mostly 'reverse'
> calls or similar) by moving the calls into the relevant 'setUp'
> functions for each test.
> 
> Signed-off-by: Stephen Finucane <stephen.finucane@intel.com>

I still don't get why the reverse() calls at the class level are
problematic as they don't do any DB operation, mind enlightening me?
Stephen Finucane Sept. 17, 2015, 6:28 p.m. UTC | #2
> On Fri, Aug 21, 2015 at 03:32:11PM +0100, Stephen Finucane wrote:
> > Django creates test databases after it loads tests. However, any
> > operations that exist at class level will be executed before this
> > database is created. Fix the instances of this issue (mostly 'reverse'
> > calls or similar) by moving the calls into the relevant 'setUp'
> > functions for each test.
> >
> > Signed-off-by: Stephen Finucane <stephen.finucane@intel.com>
> 
> I still don't get why the reverse() calls at the class level are
> problematic as they don't do any DB operation, mind enlightening me?
> 
> --
> Damien

I'm trying to find the article where I found this link, but I haven't been able to yet. You can test this yourself though: delete and recreate your database table but don't execute the 'migrate' step. Try running the tests without this check - you'll get failures on those 'reverse' commands.

Not that it's a reason to merge this, but it's worth noting that this change won't inherently break anything.

Stephen
diff mbox

Patch

diff --git a/patchwork/tests/test_mail_settings.py b/patchwork/tests/test_mail_settings.py
index a193c97..20ed777 100644
--- a/patchwork/tests/test_mail_settings.py
+++ b/patchwork/tests/test_mail_settings.py
@@ -28,8 +28,9 @@  from patchwork.models import EmailOptout, EmailConfirmation, Person
 from patchwork.tests.utils import create_user, error_strings
 
 class MailSettingsTest(TestCase):
-    view = 'patchwork.views.mail.settings'
-    url = reverse(view)
+
+    def setUp(self):
+        self.url = reverse('patchwork.views.mail.settings')
 
     def testMailSettingsGET(self):
         response = self.client.get(self.url)
@@ -78,8 +79,9 @@  class MailSettingsTest(TestCase):
         self.assertTrue(('action="%s"' % optin_url) in response.content)
 
 class OptoutRequestTest(TestCase):
-    view = 'patchwork.views.mail.optout'
-    url = reverse(view)
+
+    def setUp(self):
+        self.url = reverse('patchwork.views.mail.optout')
 
     def testOptOutRequestGET(self):
         response = self.client.get(self.url)
@@ -124,10 +126,9 @@  class OptoutRequestTest(TestCase):
         self.assertEquals(len(mail.outbox), 0)
 
 class OptoutTest(TestCase):
-    view = 'patchwork.views.mail.optout'
-    url = reverse(view)
 
     def setUp(self):
+        self.url = reverse('patchwork.views.mail.optout')
         self.email = u'foo@example.com'
         self.conf = EmailConfirmation(type = 'optout', email = self.email)
         self.conf.save()
@@ -157,10 +158,9 @@  class OptoutPreexistingTest(OptoutTest):
         EmailOptout(email = self.email).save()
 
 class OptinRequestTest(TestCase):
-    view = 'patchwork.views.mail.optin'
-    url = reverse(view)
 
     def setUp(self):
+        self.url = reverse('patchwork.views.mail.optin')
         self.email = u'foo@example.com'
         EmailOptout(email = self.email).save()
 
@@ -232,8 +232,9 @@  class OptinTest(TestCase):
 
 class OptinWithoutOptoutTest(TestCase):
     """Test an opt-in with no existing opt-out"""
-    view = 'patchwork.views.mail.optin'
-    url = reverse(view)
+
+    def setUp(self):
+        self.url = reverse('patchwork.views.mail.optin')
 
     def testOptInWithoutOptout(self):
         email = u'foo@example.com'
@@ -248,16 +249,15 @@  class UserProfileOptoutFormTest(TestCase):
     """Test that the correct optin/optout forms appear on the user profile
        page, for logged-in users"""
 
-    view = 'patchwork.views.user.profile'
-    url = reverse(view)
-    optout_url = reverse('patchwork.views.mail.optout')
-    optin_url = reverse('patchwork.views.mail.optin')
-    form_re_template = ('<form\s+[^>]*action="%(url)s"[^>]*>'
-                        '.*?<input\s+[^>]*value="%(email)s"[^>]*>.*?'
-                        '</form>')
-    secondary_email = 'test2@example.com'
-
     def setUp(self):
+        self.url = reverse('patchwork.views.user.profile')
+        self.optout_url = reverse('patchwork.views.mail.optout')
+        self.optin_url = reverse('patchwork.views.mail.optin')
+        self.form_re_template = ('<form\s+[^>]*action="%(url)s"[^>]*>'
+                                 '.*?<input\s+[^>]*value="%(email)s"[^>]*>.*?'
+                                 '</form>')
+        self.secondary_email = 'test2@example.com'
+
         self.user = create_user()
         self.client.login(username = self.user.username,
                 password = self.user.username)
diff --git a/patchwork/tests/test_user.py b/patchwork/tests/test_user.py
index 0faa970..ef213a7 100644
--- a/patchwork/tests/test_user.py
+++ b/patchwork/tests/test_user.py
@@ -27,6 +27,7 @@  from django.contrib.auth.models import User
 from patchwork.models import EmailConfirmation, Person, Bundle
 from patchwork.tests.utils import defaults, error_strings
 
+
 def _confirmation_url(conf):
     return reverse('patchwork.views.confirm', kwargs = {'key': conf.key})
 
@@ -121,7 +122,7 @@  class UserPersonConfirmTest(TestCase):
         self.assertEquals(conf.active, False)
 
 class UserLoginRedirectTest(TestCase):
-    
+
     def testUserLoginRedirect(self):
         url = '/user/'
         response = self.client.get(url)
@@ -157,8 +158,12 @@  class UserProfileTest(TestCase):
         self.assertContains(response, bundle.get_absolute_url())
 
 class UserPasswordChangeTest(TestCase):
-    form_url = reverse('django.contrib.auth.views.password_change')
-    done_url = reverse('django.contrib.auth.views.password_change_done')
+    user = None
+
+    def setUp(self):
+        self.form_url = reverse('django.contrib.auth.views.password_change')
+        self.done_url = reverse(
+            'django.contrib.auth.views.password_change_done')
 
     def testPasswordChangeForm(self):
         self.user = TestUser()