← Back to team overview

launchpad-reviewers team mailing list archive

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