launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #22652
[Merge] lp:~cjwatson/launchpad/explicit-proxy-roundup into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/explicit-proxy-roundup into lp:launchpad with lp:~cjwatson/launchpad/explicit-proxy-github as a prerequisite.
Commit message:
Convert the Roundup external bug tracker to urlfetch.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/explicit-proxy-roundup/+merge/348428
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.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/explicit-proxy-roundup into lp:launchpad.
=== 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('no-priv@xxxxxxxxxxxxx')
-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
Follow ups
