[590776] LocaleURLMiddleware's setting of a thread-local prefixer is …

…now reversed when the request ends, removing one source of apparently random test failures. A short book follows:

* Make LocaleURLMiddleware clean up after itself: clear the thread-local prefixer variable that it sets before the request. Tests can now run in any order without the thread-local setting of one influencing the next. This also uncovers a lot of buggy tests which were mistakenly depending on the work of previous ones. Much of the rest of this changeset is toward fixing these.... A good test to compare before and after this commit is `test_json_callback_validation`. Run it alone, and LocaleMiddleware tosses us a 301 because we don't have a locale prefix set. Run it in concert with other tests, and somebody else sets the thread-local for it.
* Added `LocalizingClient`, which provides the functionality of django.test.client.Client but implicitly prepends a locale code to anything you request. This saves dodging the initial 301 that LocaleURLMiddleware returns in response to an un-prepended request. It also makes "priming" the middleware by doing an initial arbitrary request unnecessary (and ineffective, since the middleware no longer leaks state). Removed all instances of such priming.
* Switched many tests to use `LocalizingClient`. Some uses of plain `Client` remain, but those are correct, since they test things like the middleware's 301s.
* Renamed `set_url_prefix` and `get_url_prefix` to better reflect their purpose: they hold Prefixers, not prefixes.
* Added a `force_locale` kwarg to `sumo.urlresolvers.reverse()`, which forces the same default locale as would normally be used by the middleware to be prepended onto the result, even if there is no prefixer set. This is useful when you need to get a reversed URL against which to compare a 301's Location in a test.
* Moved `split_path()` out of `Prefixer`, since it needs no instance state and I needed it in `LocalizingClient`.
* `Prefixer` can now be instantiated without a request, in which case it defaults to a fairly blank one and returns results largely determined by `settings.LANGUAGE_CODE`.
* Stopped expecting locale prefixes in wiki parser tests: the parser is being run outside any request and so should return unlocalized URLs.
* Ripped `get_url` off the legacy `Forum` and `ForumThread` models and deleted its tests. This was a lot faster than trying to fix them, and the entire models are dying for 2.3 anyway.
* Fixed a gabillion other tests. I like to think I understood each one's intent before causing it to pass, but extra eyes are certainly welcome.
* Fixed some pyflakes violations: semicolons, whitespace, unused imports, etc.

apps/flagit/tests/__init__.py

@@ -1,9 +1,11 @@
 from datetime import datetime
 
-from django.test import TestCase, client
+from django.test import TestCase
 from django.conf import settings
 from django.template.defaultfilters import slugify
 
+from sumo.tests import LocalizingClient
+
 
 class TestCaseBase(TestCase):
     """Base TestCase for the flagit app test cases."""
@@ -12,8 +14,7 @@ class TestCaseBase(TestCase):
 
     def setUp(self):
         """Setup"""
-        self.client = client.Client()
-        self.client.get('/')
+        self.client = LocalizingClient()
 
         # Change the CACHE_PREFIX to avoid conflicts
         self.orig_cache_prefix = getattr(settings, 'CACHE_PREFIX', None)

apps/forums/tests/__init__.py

@@ -1,11 +1,11 @@
-from django.test import TestCase, client
+from django.test import TestCase
 from django.contrib.auth.models import User
 
 from nose.tools import eq_
 
 from forums.models import Forum, Thread, Post, ThreadLockedError
 from forums.views import sort_threads
-from sumo.tests import get
+from sumo.tests import get, LocalizingClient
 
 
 def fixtures_setup():
@@ -42,9 +42,7 @@ def setUp(self):
         installed. This will set them to the correct values."""
 
         fixtures_setup()
-
-        self.client = client.Client()
-        self.client.get('/')
+        self.client = LocalizingClient()
 
 
 class PostTestCase(ForumTestCase):

apps/forums/tests/test_feeds.py

@@ -1,8 +1,6 @@
 from nose.tools import eq_
 from pyquery import PyQuery as pq
 
-from django.test import client
-
 from forums.feeds import ThreadsFeed, PostsFeed
 from forums.models import Forum, Thread
 from forums.tests import ForumTestCase, get
@@ -13,9 +11,6 @@ class ForumTestFeedSorting(ForumTestCase):
     def setUp(self):
         super(ForumTestFeedSorting, self).setUp()
 
-        # Warm up the prefixer for reverse()
-        client.Client().get('/')
-
     def test_threads_sort(self):
         """Ensure that threads are being sorted properly by date/time."""
         f = Forum.objects.get(pk=1)

apps/forums/tests/test_models.py

@@ -1,6 +1,5 @@
 import datetime
 
-from django.test import client
 from django.contrib.auth.models import User
 
 from nose.tools import eq_
@@ -18,9 +17,6 @@ class ForumModelTestCase(ForumTestCase):
     def setUp(self):
         super(ForumModelTestCase, self).setUp()
 
-        # Warm up the prefixer for reverse()
-        client.Client().get('/')
-
     def test_forum_absolute_url(self):
         f = Forum.objects.get(pk=1)
         exp_ = reverse('forums.threads', kwargs={'forum_slug': f.slug})
@@ -120,12 +116,8 @@ class ThreadModelTestCase(ForumTestCase):
 
     def setUp(self):
         super(ThreadModelTestCase, self).setUp()
-
         self.fixtures = self.fixtures + ['notifications.json']
 
-        # Warm up the prefixer for reverse()
-        client.Client().get('/')
-
     def test_delete_thread_with_last_forum_post(self):
         """Deleting the thread with a forum's last post should
         update the last_post field on the forum

apps/forums/tests/test_notifications.py

@@ -11,6 +11,9 @@
 from sumo.tests import post
 
 
+# Some of these contain a locale prefix on included links, while others don't.
+# This depends on whether the tests use them inside or outside the scope of a
+# request. See the long explanation in questions.tests.test_notifications.
 EMAIL_CONTENT = (
     u"""
 
@@ -28,7 +31,7 @@
 To view this post on the site, click the following link, or
 paste it into your browser's location bar:
 
-https://testserver/en-US/forums/test-forum/2#post-4
+https://testserver/forums/test-forum/2#post-4
 """,
     u"""
 
@@ -64,7 +67,7 @@
 To view this post on the site, click the following link, or
 paste it into your browser's location bar:
 
-https://testserver/en-US/forums/test-forum/2
+https://testserver/forums/test-forum/2
 """,
     u"""
 

apps/notifications/views.py

@@ -1,13 +1,13 @@
 from django.http import Http404, HttpResponseRedirect
 from django.shortcuts import get_object_or_404
-from django.contrib.contenttypes.models import ContentType
 from django.core.exceptions import ObjectDoesNotExist
 
 import jingo
 
 from sumo.urlresolvers import reverse
 from .models import EventWatch
 
+
 def remove(request, key):
     """Verify and remove an EventWatch."""
     email = request.GET.get('email')

apps/questions/tasks.py

@@ -61,6 +61,7 @@ def build_answer_notification(answer):
 
 @task
 def build_solution_notification(question):
+    """Send emails to everyone watching the question--except the asker."""
     ct = ContentType.objects.get_for_model(question)
     # Cache solution.question as a workaround for replication lag (bug 585029)
     question.solution.question = question

apps/questions/tests/__init__.py

@@ -1,4 +1,4 @@
-from django.test import TestCase, client
+from django.test import TestCase
 from datetime import datetime
 
 from django.conf import settings
@@ -7,6 +7,7 @@
 from nose.tools import eq_
 
 from questions.models import Question
+from sumo.tests import LocalizingClient
 
 
 class TestCaseBase(TestCase):
@@ -15,16 +16,14 @@ class TestCaseBase(TestCase):
     fixtures = ['users.json', 'questions.json']
 
     def setUp(self):
-        """Setup"""
-
         q = Question.objects.get(pk=1)
         q.last_answer_id = 1
         q.save()
 
-        self.client = client.Client()
-        self.client.get('/')
+        self.client = LocalizingClient()
 
         # create a new cache key for top contributors to avoid conflict
+        # TODO: May be able to go away once we flush memcache between tests.
         self.orig_tc_cache_key = settings.TOP_CONTRIBUTORS_CACHE_KEY
         settings.TOP_CONTRIBUTORS_CACHE_KEY += slugify(datetime.now())
 

apps/questions/tests/test_notifications.py

@@ -12,6 +12,23 @@
 from sumo.tests import post
 
 
+# These mails are generated using reverse() calls, which return different
+# results depending on whether a request is being processed at the time. This
+# is because reverse() depends on a thread-local var which is set/unset at
+# request boundaries by LocaleURLMiddleware. While a request is in progress,
+# reverse() prepends a locale code; otherwise, it doesn't. Thus, when making a
+# mock request that fires off a celery task that generates one of these emails,
+# expect a locale in reverse()d URLs. When firing off a celery task outside the
+# scope of a request, expect none. For example, when firing an email-building
+# task from within a POST request, compare the result with
+# SOLUTION_EMAIL_INSIDE_REQUEST. When calling the task directly from the test,
+# compare with SOLUTION_EMAIL_OUTSIDE_REQUEST.
+#
+# In production, with CELERY_ALWAYS_EAGER=False, celery tasks run in a
+# different interpreter (with no access to the thread-local), so reverse() will
+# never prepend a locale code unless passed force_locale=True. Thus, these
+# test-emails with locale prefixes are not identical to the ones sent in
+# production.
 EMAIL_CONTENT = (
     """
 
@@ -29,7 +46,7 @@
 To view this answer on the site, click the following link, or
 paste it into your browser's location bar:
 
-https://testserver/en-US/questions/1#answer-1
+https://testserver/questions/1#answer-1
 """,
     """
 
@@ -47,8 +64,9 @@
 To view this answer on the site, click the following link, or
 paste it into your browser's location bar:
 
-https://testserver/en-US/questions/1#answer-%s
-""",
+https://testserver/questions/1#answer-%s
+""")
+SOLUTION_EMAIL_INSIDE_REQUEST, SOLUTION_EMAIL_OUTSIDE_REQUEST = [
     """
 
 Solution to question: Lorem ipsum dolor sit amet?
@@ -65,9 +83,8 @@
 To view the solution on the site, click the following link, or
 paste it into your browser's location bar:
 
-https://testserver/en-US/questions/1#answer-1
-""",
-)
+https://testserver%s/questions/1#answer-1
+""" % s for s in ['/en-US', '']]
 
 
 class NotificationTestCase(TestCaseBase):
@@ -107,7 +124,7 @@ def test_solution_notification(self, get_current, delay):
         delay.assert_called_with(
             self.ct, question.id,
             u'Solution to: %s' % question.title,
-            EMAIL_CONTENT[2],
+            SOLUTION_EMAIL_OUTSIDE_REQUEST,
             (u'user118533@nowhere',),
             'solution')
 
@@ -136,7 +153,7 @@ def test_solution_notification_deleted(self, get_current, delay):
         delay.assert_called_with(
             self.ct, question.id,
             u'Solution to: %s' % question.title,
-            EMAIL_CONTENT[2],
+            SOLUTION_EMAIL_OUTSIDE_REQUEST,
             (u'user118533@nowhere',),
             'solution')
 
@@ -170,6 +187,6 @@ def test_notification_on_solution(self, get_current, delay):
         delay.assert_called_with(
             self.ct, question.id,
             u'Solution to: %s' % question.title,
-            EMAIL_CONTENT[2],
+            SOLUTION_EMAIL_INSIDE_REQUEST,
             (u'user118533@nowhere',),
             'solution')

apps/questions/tests/test_templates.py

@@ -577,7 +577,8 @@ def test_add_tag_get_method(self):
         """Assert GETting the add_tag view redirects to the answers page."""
         response = self.client.get(_add_tag_url())
         url = 'http://testserver%s' % reverse('questions.answers',
-                                              kwargs={'question_id': 1})
+                                              kwargs={'question_id': 1},
+                                              force_locale=True)
         self.assertRedirects(response, url)
 
     def test_add_nonexistent_tag(self):
@@ -648,7 +649,8 @@ def test_remove_no_tag(self):
 
     def _assert_redirects_to_question_2(self, response):
         url = 'http://testserver%s' % reverse('questions.answers',
-                                              kwargs={'question_id': 2})
+                                              kwargs={'question_id': 2},
+                                              force_locale=True)
         self.assertRedirects(response, url)
 
     # remove_tag_async view:
@@ -931,7 +933,8 @@ def test_post(self):
 
         # Make sure the form redirects and thus appears to succeed:
         url = 'http://testserver%s' % reverse('questions.answers',
-                                           kwargs={'question_id': question_id})
+                                           kwargs={'question_id': question_id},
+                                           force_locale=True)
         self.assertRedirects(response, url)
 
         # Make sure the static fields, the metadata, and the updated_by field

apps/search/models.py

@@ -1,3 +0,0 @@
-from django.db import models
-
-# Create your models here.

apps/search/tests/test_json.py

@@ -1,18 +1,15 @@
 from nose.tools import eq_
 
-from django.test.client import Client
-
-import test_utils
-
 from sumo.urlresolvers import reverse
+from sumo.tests import LocalizingClient
 
 from .test_search import SphinxTestCase
 
 
 class JSONTest(SphinxTestCase):
     def test_json_format(self):
         """JSON without callback should return application/json"""
-        c = Client()
+        c = LocalizingClient()
         response = c.get(reverse('search'), {
             'q': 'bookmarks',
             'format': 'json',
@@ -21,7 +18,7 @@ def test_json_format(self):
 
     def test_json_callback_validation(self):
         """Various json callbacks -- validation"""
-        c = Client()
+        c = LocalizingClient()
         q = 'bookmarks'
         format = 'json'
 
@@ -56,7 +53,7 @@ def test_json_callback_validation(self):
 
     def test_json_empty_query(self):
         """Empty query returns JSON format"""
-        c = Client()
+        c = LocalizingClient()
 
         # Test with flags for advanced search or not
         a_types = (0, 1, 2)
@@ -69,7 +66,7 @@ def test_json_empty_query(self):
 
 def test_json_down():
     """When the Sphinx is down, return JSON and 503 status"""
-    c = Client()
+    c = LocalizingClient()
 
     # Test with flags for advanced search or not
     callbacks = (
@@ -83,5 +80,5 @@ def test_json_down():
             'q': 'json down', 'format': 'json',
             'callback': callback,
         })
-        eq_(response['Content-Type'], mimetype);
+        eq_(response['Content-Type'], mimetype)
         eq_(response.status_code, status)

apps/search/tests/test_search.py

@@ -7,7 +7,6 @@
 import json
 import socket
 
-from django.test import client
 from django.db import connection
 
 import mock
@@ -24,6 +23,7 @@
 from search.clients import (WikiClient, QuestionsClient,
                             DiscussionClient, SearchError)
 from sumo.models import WikiPage
+from sumo.tests import LocalizingClient
 from forums.models import Post
 
 
@@ -147,7 +147,7 @@ class SphinxTestCase(test_utils.TransactionTestCase):
     """
 
     fixtures = ['pages.json', 'categories.json', 'users.json',
-                'posts.json', 'questions.json', ]
+                'posts.json', 'questions.json']
     sphinx = True
     sphinx_is_running = False
 
@@ -200,10 +200,7 @@ class SearchTest(SphinxTestCase):
 
     def setUp(self):
         super(SearchTest, self).setUp()
-        self.client = client.Client()
-
-        # Warm up the prefixer
-        self.client.get('/')
+        self.client = LocalizingClient()
 
     def test_indexer(self):
         wc = WikiClient()