launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #19140
Re: [Merge] lp:~wgrant/launchpad/webhook-delivery-tweaks into lp:launchpad
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.
References