Merge lp:~cjwatson/launchpad/explicit-proxy-roundup into lp:launchpad

[Colin Watson]

Convert the Roundup external bug tracker to urlfetch.

I experimented with a couple of different approaches to converting the doctests, and eventually decided that the most readable option was to keep TestRoundup and add some responses-based helper code to it. This is done in such a way that it can be reused for the other test tracker subclasses.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/bugs/doc/bugwatch.txt'
--- lib/lp/bugs/doc/bugwatch.txt	2013-04-11 04:50:27 +0000
+++ lib/lp/bugs/doc/bugwatch.txt	2018-06-23 00:01:05 +0000
@@ -465,7 +465,9 @@
     >>> from lp.bugs.scripts.checkwatches import CheckwatchesMaster
     >>> bug_watch_updater = CheckwatchesMaster(transaction, FakeLogger())
     >>> external_bugtracker = TestRoundup(bug_tracker.baseurl)
-    >>> bug_watch_updater.updateBugWatches(external_bugtracker, [bug_watch])
+    >>> with external_bugtracker.responses():
+    ...     bug_watch_updater.updateBugWatches(
+    ...         external_bugtracker, [bug_watch])
     INFO Updating 1 watches for 1 bugs on http://some.where
 
     >>> bug.bugtasks[0].status.title
 
=== modified file 'lib/lp/bugs/doc/checkwatches.txt'
--- lib/lp/bugs/doc/checkwatches.txt	2016-12-22 16:32:38 +0000
+++ lib/lp/bugs/doc/checkwatches.txt	2018-06-23 00:01:05 +0000
@@ -15,8 +15,8 @@
     >>> transaction.commit()
 
 We set a default timeout on checkwatches to 30 seconds. In order to test
-this, we can monkey-patch urllib2.urlopen so that it always raises a
-timeout and call the checkwatches cronscript machinery directly.
+this, we can inject a mock timeout using `responses` and call the
+checkwatches cronscript machinery directly.
 
 First, we create some bug watches to test with:
 
@@ -51,30 +51,31 @@
 
     >>> login('[e-mail]')
 
-Next, we monkey-patch urllib2.urlopen so that it always times out.
+Next, we ensure that the request always times out.
 
 The timeout will not produce an OOPS report; they happen routinely and
 require no action from the Launchpad end.
 
+    >>> from contextlib import contextmanager
+    >>> import re
     >>> import socket
-    >>> import urllib2
-    >>> urlopen = urllib2.urlopen
-
+    >>> import responses
     >>> from lp.testing.fixture import CaptureOops
-    >>> capture = CaptureOops()
-    >>> capture.setUp()
-    >>> def do_not_urlopen(url=None, data=None, timeout=None):
-    ...     raise socket.timeout("Connection timed out.")
-    >>> try:
-    ...     urllib2.urlopen = do_not_urlopen
+
+    >>> @contextmanager
+    ... def timeout_requests():
+    ...     with responses.RequestsMock() as requests_mock:
+    ...         requests_mock.add(
+    ...             'GET', re.compile(r'.*'),
+    ...             body=socket.timeout('Connection timed out.'))
+    ...         yield
+
+    >>> with CaptureOops() as capture, timeout_requests():
     ...     updater = CheckwatchesMaster(transaction.manager)
     ...     updater.updateBugTrackers(
     ...         bug_tracker_names=[example_bug_tracker_name])
-    ... finally:
-    ...     urllib2.urlopen = urlopen
-    >>> capture.oopses
+    ...     print(capture.oopses)
     []
-    >>> capture.cleanUp()
 
 Errors that occur when updating a bug watch are recorded against that
 bug watch. The timeout will be recorded against the bug watch we just
@@ -146,8 +147,8 @@
 
 Since our example bug tracker is a Roundup bug tracker we can
 monkey-patch the Roundup ExternalBugTrackerClass in order to set its
-batch size. We will also monkey-patch urllib2.urlopen again so that no
-requests are actually made.
+batch size. We will also insert a mock response again so that no requests
+are actually made.
 
     >>> from lp.services.log.logger import FakeLogger
     >>> from lp.bugs import externalbugtracker
@@ -156,16 +157,15 @@
     >>> updater = CheckwatchesMaster(transaction.manager)
     >>> original_log = updater.logger
     >>> batch_size = externalbugtracker.Roundup.batch_size
-    >>> try:
-    ...     urllib2.urlopen = do_not_urlopen
-    ...     externalbugtracker.Roundup.batch_size = 5
-    ...     transaction.commit()
-    ...     updater.logger = FakeLogger()
-    ...     updater.updateBugTrackers([example_bug_tracker_name])
-    ... finally:
-    ...     updater.logger = original_log
-    ...     externalbugtracker.Roundup.batch_size = batch_size
-    ...     urllib2.urlopen = urlopen
+    >>> with timeout_requests():
+    ...     try:
+    ...         externalbugtracker.Roundup.batch_size = 5
+    ...         transaction.commit()
+    ...         updater.logger = FakeLogger()
+    ...         updater.updateBugTrackers([example_bug_tracker_name])
+    ...     finally:
+    ...         updater.logger = original_log
+    ...         externalbugtracker.Roundup.batch_size = batch_size
     DEBUG No global batch size specified.
     INFO Updating 5 watches for 5 bugs on http://bugs.example.com
     INFO Connection timed out when updating ...
 
=== modified file 'lib/lp/bugs/doc/externalbugtracker-roundup.txt'
--- lib/lp/bugs/doc/externalbugtracker-roundup.txt	2012-12-26 01:32:19 +0000
+++ lib/lp/bugs/doc/externalbugtracker-roundup.txt	2018-06-23 00:01:05 +0000
@@ -75,13 +75,13 @@
 local variable for later use.
 
 We use a test-oriented implementation for the purposes of these tests, which
-overrides ExternalBugTracker.urlopen() so that we don't have to rely on a
-working network connection.
+avoids relying on a working network connection.
 
     >>> from lp.bugs.tests.externalbugtracker import (
     ...     TestRoundup, print_bugwatches)
     >>> roundup = TestRoundup(u'http://test.roundup/')
-    >>> roundup.initializeRemoteBugDB([1])
+    >>> with roundup.responses():
+    ...     roundup.initializeRemoteBugDB([1])
     >>> sorted(roundup.bugs.keys())
     [1]
 
@@ -90,26 +90,27 @@
 
 There are two means by which we can export Roundup bug statuses: on a
 bug-by-bug basis and as a batch. When the number of bugs that need updating is
-less than a given bug roundupker's batch_query_threshold the bugs will be
+less than a given bug tracker's batch_query_threshold the bugs will be
 fetched one-at-a-time:
 
     >>> roundup.batch_query_threshold
     10
 
-    >>> roundup.trace_calls = True
-    >>> roundup.initializeRemoteBugDB([6, 7, 8, 9, 10])
-    CALLED urlopen(u'http://test.roundup/issue?...&id=6')
-    CALLED urlopen(u'http://test.roundup/issue?...&id=7')
-    CALLED urlopen(u'http://test.roundup/issue?...&id=8')
-    CALLED urlopen(u'http://test.roundup/issue?...&id=9')
-    CALLED urlopen(u'http://test.roundup/issue?...&id=10')
+    >>> with roundup.responses(trace_calls=True):
+    ...     roundup.initializeRemoteBugDB([6, 7, 8, 9, 10])
+    GET http://test.roundup/issue?...&id=6
+    GET http://test.roundup/issue?...&id=7
+    GET http://test.roundup/issue?...&id=8
+    GET http://test.roundup/issue?...&id=9
+    GET http://test.roundup/issue?...&id=10
 
 If there are more than batch_query_threshold bugs to update then they are
 fetched as a batch:
 
     >>> roundup.batch_query_threshold = 4
-    >>> roundup.initializeRemoteBugDB([6, 7, 8, 9, 10])
-    CALLED urlopen(u'http://test.roundup/issue?...@startwith=0')
+    >>> with roundup.responses(trace_calls=True):
+    ...     roundup.initializeRemoteBugDB([6, 7, 8, 9, 10])
+    GET http://test.roundup/issue?...@startwith=0
 
 
 == Updating Bug Watches ==
@@ -145,8 +146,9 @@
     >>> bug_watch_updater = CheckwatchesMaster(
     ...     txn, logger=FakeLogger())
     >>> roundup = TestRoundup(example_bug_tracker.baseurl)
-    >>> bug_watch_updater.updateBugWatches(
-    ...     roundup, example_bug_tracker.watches)
+    >>> with roundup.responses():
+    ...     bug_watch_updater.updateBugWatches(
+    ...         roundup, example_bug_tracker.watches)
     INFO Updating 1 watches for 1 bugs on http://bugs.some.where
     >>> print_bugwatches(example_bug_tracker.watches)
     Remote bug 1: 1
@@ -178,11 +180,11 @@
     ...         bugtracker=example_bug_tracker,
     ...         remotebug=str(remote_bug_id))
 
-    >>> roundup.trace_calls = True
-    >>> bug_watch_updater.updateBugWatches(
-    ...     roundup, example_bug_tracker.watches)
+    >>> with roundup.responses(trace_calls=True):
+    ...     bug_watch_updater.updateBugWatches(
+    ...         roundup, example_bug_tracker.watches)
     INFO Updating 11 watches for 11 bugs on http://bugs.some.where
-    CALLED urlopen(u'http://.../issue?...@startwith=0')
+    GET http://.../issue?...@startwith=0
 
     >>> print_bugwatches(example_bug_tracker.watches,
     ...     roundup.convertRemoteStatus)
 
=== modified file 'lib/lp/bugs/externalbugtracker/roundup.py'
--- lib/lp/bugs/externalbugtracker/roundup.py	2015-10-15 14:09:50 +0000
+++ lib/lp/bugs/externalbugtracker/roundup.py	2018-06-23 00:01:05 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Round ExternalBugTracker utility."""
@@ -13,7 +13,7 @@
 
 from lp.bugs.externalbugtracker import (
     BugNotFound,
-    ExternalBugTracker,
+    ExternalBugTrackerRequests,
     InvalidBugId,
     LookupTree,
     UnknownRemoteStatusError,
@@ -42,7 +42,7 @@
         for (key, value) in items)
 
 
-class Roundup(ExternalBugTracker):
+class Roundup(ExternalBugTrackerRequests):
     """An ExternalBugTracker descendant for handling Roundup bug trackers."""
 
     _status_fields_map = {
@@ -218,7 +218,7 @@
         """See `ExternalBugTracker`."""
         bug_id = int(bug_id)
         query_url = self.getSingleBugExportURL(bug_id)
-        reader = csv.DictReader(self._fetchPage(query_url))
+        reader = csv.DictReader(self._getPage(query_url).iter_lines())
         return (bug_id, reader.next())
 
     def getRemoteBugBatch(self, bug_ids):
@@ -230,7 +230,7 @@
         #      export the bug ids needed rather than hitting the remote
         #      tracker for a potentially massive number of bugs.
         query_url = self.getBatchBugExportURL()
-        remote_bugs = csv.DictReader(self._fetchPage(query_url))
+        remote_bugs = csv.DictReader(self._getPage(query_url).iter_lines())
         bugs = {}
         for remote_bug in remote_bugs:
             # We're only interested in the bug if it's one of the ones in
 
=== modified file 'lib/lp/bugs/tests/externalbugtracker.py'
--- lib/lp/bugs/tests/externalbugtracker.py	2018-06-05 12:18:58 +0000
+++ lib/lp/bugs/tests/externalbugtracker.py	2018-06-23 00:01:05 +0000
@@ -5,6 +5,7 @@
 
 __metaclass__ = type
 
+from contextlib import contextmanager
 from copy import deepcopy
 from datetime import (
     datetime,
@@ -20,9 +21,15 @@
     BaseHandler,
     Request,
     )
-import urlparse
 import xmlrpclib
 
+import responses
+from six.moves.urllib_parse import (
+    parse_qs,
+    urljoin,
+    urlparse,
+    urlsplit,
+    )
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
@@ -158,6 +165,30 @@
         naked.updateStatus(UNKNOWN_REMOTE_STATUS, BugTaskStatus.UNKNOWN)
 
 
+class BugTrackerResponsesMixin:
+    """A responses-based test mixin for bug trackers.
+
+    The simplest way to use this is as a context manager:
+
+        with bug_tracker.responses():
+            ...
+
+    Classes using this mixin should implement `addResponses`.
+    """
+
+    def addResponses(self, requests_mock, **kwargs):
+        """Add test responses."""
+
+    @contextmanager
+    def responses(self, trace_calls=False, **kwargs):
+        with responses.RequestsMock() as requests_mock:
+            self.addResponses(requests_mock, **kwargs)
+            yield requests_mock
+            if trace_calls:
+                for call in requests_mock.calls:
+                    print call.request.method, call.request.url
+
+
 class TestExternalBugTracker(ExternalBugTracker):
     """A test version of `ExternalBugTracker`.
 
@@ -1522,30 +1553,26 @@
         return [self.utc_time]
 
 
-class TestRoundup(Roundup):
-    """Roundup ExternalBugTracker for testing purposes.
-
-    It overrides urlopen, so that access to a real Roundup instance isn't
-    needed.
-    """
+class TestRoundup(BugTrackerResponsesMixin, Roundup):
+    """Roundup ExternalBugTracker for testing purposes."""
 
     # We remove the batch_size limit for the purposes of the tests so
     # that we can test batching and not batching correctly.
     batch_size = None
-    trace_calls = False
-
-    def urlopen(self, url, data=None):
-        if self.trace_calls:
-            print "CALLED urlopen(%r)" % (url.get_full_url())
-
-        file_path = os.path.join(os.path.dirname(__file__), 'testfiles')
-
-        if self.host == 'bugs.python.org':
-            return open(
-                file_path + '/' + 'python_example_ticket_export.csv', 'r')
+
+    @staticmethod
+    def _getCallback(request):
+        url = urlsplit(request.url)
+        if url.netloc == 'bugs.python.org':
+            body = read_test_file('python_example_ticket_export.csv')
         else:
-            return open(
-                file_path + '/' + 'roundup_example_ticket_export.csv', 'r')
+            body = read_test_file('roundup_example_ticket_export.csv')
+        return 200, {}, body
+
+    def addResponses(self, requests_mock):
+        """Add test responses."""
+        requests_mock.add_callback(
+            'GET', re.compile(re.escape(self.baseurl)), self._getCallback)
 
 
 class TestRequestTracker(RequestTracker):
@@ -1559,7 +1586,7 @@
 
     def urlopen(self, page, data=None):
         file_path = os.path.join(os.path.dirname(__file__), 'testfiles')
-        path = urlparse.urlparse(page)[2].lstrip('/')
+        path = urlparse(page)[2].lstrip('/')
         if self.trace_calls:
             print "CALLED urlopen(%r)" % path