Review: Approve
Diff comments: > > === modified file 'lib/lp/services/webhooks/tests/test_webhookjob.py' > --- lib/lp/services/webhooks/tests/test_webhookjob.py 2015-07-29 08:31:06 > +0000 > +++ lib/lp/services/webhooks/tests/test_webhookjob.py 2015-08-03 10:51:28 > +0000 > @@ -140,56 +150,88 @@ > result = WebhookClient().deliver( > 'http://hookep.com/foo', > {'http': 'http://squid.example.com:3128'}, > - {'foo': 'bar'}) > + 'TestWebhookClient', 30, 'sekrit', {'foo': 'bar'}) > > return reqs, result > > + @property > + def request_matcher(self): > + return MatchesDict({ > + 'url': Equals('http://hookep.com/foo'), I realise there are already other tests using the same hostname, but I'd be more comfortable if we were using a RFC2606 reserved name here. > + 'method': Equals('POST'), > + 'headers': Equals( > + {'Content-Type': 'application/json', > + 'Content-Length': '14', > + 'User-Agent': 'TestWebhookClient', > + 'X-Hub-Signature': > + 'sha1=de75f136c37d89f5eb24834468c1ecd602fa95dd', > + }), > + 'body': Equals('{"foo": "bar"}'), > + }) > + > def test_sends_request(self): > [request], result = self.sendToWebhook() > - self.assertEqual( > - {'Content-Type': 'application/json', 'Content-Length': '14'}, > - result['request']['headers']) > - self.assertEqual('{"foo": "bar"}', result['request']['body']) > - self.assertEqual(200, result['response']['status_code']) > - self.assertEqual({}, result['response']['headers']) > - self.assertEqual('Content', result['response']['body']) > + self.assertThat( > + result, > + MatchesDict({ > + 'request': self.request_matcher, > + 'response': MatchesDict({ > + 'status_code': Equals(200), > + 'headers': Equals({}), > + 'body': Equals('Content'), > + }), > + })) > > def test_accepts_404(self): > [request], result = self.sendToWebhook(response_status=404) > - self.assertEqual( > - {'Content-Type': 'application/json', 'Content-Length': '14'}, > - result['request']['headers']) > - self.assertEqual('{"foo": "bar"}', result['request']['body']) > - self.assertEqual(404, result['response']['status_code']) > - self.assertEqual({}, result['response']['headers']) > - self.assertEqual('Content', result['response']['body']) > + self.assertThat( > + result, > + MatchesDict({ > + 'request': self.request_matcher, > + 'response': MatchesDict({ > + 'status_code': Equals(404), > + 'headers': Equals({}), > + 'body': Equals('Content'), > + }), > + })) > > def test_connection_error(self): > # Attempts that fail to connect have a connection_error rather > # than a response. > reqs, result = self.sendToWebhook( > raises=requests.ConnectionError('Connection refused')) > - self.assertNotIn('response', result) > - self.assertEqual( > - 'Connection refused', result['connection_error']) > + self.assertThat( > + result, > + MatchesDict({ > + 'request': self.request_matcher, > + 'connection_error': Equals('Connection refused'), > + })) > self.assertEqual([], reqs) > > > -class MockWebhookClient: > +class MockWebhookClient(WebhookClient): > > def __init__(self, response_status=200, raises=None): > self.response_status = response_status > self.raises = raises > self.requests = [] > > - def deliver(self, url, proxy, payload): > - result = {'request': {}} > + def deliver(self, url, proxy, user_agent, timeout, secret, payload): > + body, headers = create_request(user_agent, secret, payload) > + result = { > + 'request': { > + 'url': url, > + 'method': 'POST', > + 'headers': headers, > + 'body': body, > + }, > + } > if isinstance(self.raises, requests.ConnectionError): > result['connection_error'] = str(self.raises) > elif self.raises is not None: > raise self.raises > else: > - self.requests.append(('POST', url)) > + self.requests.append(('POST', url, result['request']['headers'])) > result['response'] = {'status_code': self.response_status} > return result > > @@ -321,7 +399,7 @@ > # Deliveries are retried every 5 minutes for the first hour, and > # every hour thereafter. This comment needs updating, and it may be worth making the test more detailed about the ten-minute boundary as well. > job, reqs = self.makeAndRunJob(response_status=404) > - self.assertEqual(timedelta(minutes=5), job.retry_delay) > + self.assertEqual(timedelta(minutes=1), job.retry_delay) > job.json_data['date_first_sent'] = ( > job.date_first_sent - timedelta(minutes=30)).isoformat() > self.assertEqual(timedelta(minutes=5), job.retry_delay) -- https://code.launchpad.net/~wgrant/launchpad/webhook-delivery-tweaks/+merge/266703 Your team Launchpad code reviewers is subscribed to branch lp:launchpad. _______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : [email protected] Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
