On Fri, Mar 30, 2012 at 11:41 PM, Anthony Lenton
<email address hidden> wrote:
> Anthony Lenton has proposed merging lp:~elachuni/piston-mini-client/log-to-file into lp:piston-mini-client with lp:~elachuni/piston-mini-client/pep8-test as a prerequisite.
>
> This branch adds the ability to log all requests and responses to a file, configurable via instance setup or environment variables.

Excellent Anthony - this'll be very helpful!

Just one question about a test below, but all good!

>
> --
> https://code.launchpad.net/~elachuni/piston-mini-client/log-to-file/+merge/100246
> Your team software-store-developers is requested to review the proposed merge of lp:~elachuni/piston-mini-client/log-to-file into lp:piston-mini-client.
>
> === modified file 'doc/envvars.rst'
> === modified file 'doc/tuning.rst'
> === modified file 'piston_mini_client/__init__.py'
> === modified file 'piston_mini_client/consts.py'
> === modified file 'piston_mini_client/failhandlers.py'
> === added file 'piston_mini_client/tests/test_log_to_file.py'
> --- piston_mini_client/tests/test_log_to_file.py        1970-01-01 00:00:00 +0000
> +++ piston_mini_client/tests/test_log_to_file.py        2012-03-30 21:40:24 +0000
> @@ -0,0 +1,82 @@
> +# -*- coding: utf-8 -*-
> +# Copyright 2010-2012 Canonical Ltd.  This software is licensed under the
> +# GNU Lesser General Public License version 3 (see the file LICENSE).
> +
> +import os
> +from mock import patch
> +from tempfile import NamedTemporaryFile
> +from unittest import TestCase
> +from piston_mini_client import (
> +    PistonAPI,
> +    returns_json,
> +)
> +from piston_mini_client.consts import LOG_FILENAME_ENVVAR
> +
> +
> +class AnnoyAPI(PistonAPI):
> +    default_service_root = 'http://test.info/api/1.0/'
> +
> +    @returns_json
> +    def poke(self, data=None):
> +        return self._post('/poke/', data=data,
> +            extra_headers={'location': 'ribs'})
> +
> +
> +class LogToFileTestCase(TestCase):
> +    @patch('httplib2.Http.request')
> +    def test_requests_are_dumped_to_file(self, mock_request):
> +        mock_request.return_value = ({'status': '201', 'x-foo': 'bar'},
> +            '"Go away!"')
> +
> +        api = AnnoyAPI()
> +        with NamedTemporaryFile() as fp:
> +            api.log_filename = fp.name

Why not initialise the api the way you've recommended in the README?
That is, with:

api = AnnoyAPI(log_filename = fp.name)

? Otherwise we're not testing that passing in the initialised value
works (although it's obvious from the current code that it does).

> +
> +            api.poke([1, 2, 3])
> +
> +            fp.seek(0)
> +            data = fp.read()
> +
> +        lines = data.split('\n')
> +        self.assertEqual(10, len(lines))
> +        self.assertTrue(lines[0].endswith(
> +            'Request: POST http://test.info/api/1.0/poke/'))
> +        self.assertEqual(['Content-type: application/json',
> +            'location: ribs', '', '[1, 2, 3]'], lines[1:5])
> +        self.assertTrue(lines[5].endswith('Response: 201'))
> +        self.assertEqual(['x-foo: bar', '', '"Go away!"', ''], lines[6:])
> +
> +    @patch('httplib2.Http.request')
> +    def test_invalid_logfile_location_doesnt_fail(self, mock_request):
> +        mock_request.return_value = ({'status': '201'}, '"Go away!"')
> +        api = AnnoyAPI()
> +        unlikely_path = 'two/pangolins/walk/into/a/bar/....../so?'

lol - if you want to be really sure, you could create a temp file and
then delete it. But yours is much funnier :-D

> +        self.assertFalse(os.path.exists(unlikely_path))
> +        api.log_filename = unlikely_path
> +
> +        response = api.poke()
> +
> +        self.assertEqual('Go away!', response)
> +
> +    @patch('httplib2.Http.request')
> +    def test_perms_issue_doesnt_fail(self, mock_request):
> +        mock_request.return_value = ({'status': '201'}, '"Go away!"')
> +        api = AnnoyAPI()
> +        forbidden_path = '/usr/bin/bash'
> +        self.assertRaises(IOError, open, forbidden_path, 'a')
> +        api.log_filename = forbidden_path
> +
> +        response = api.poke()

Would it be worth having *some* output when the IOError is raised -
perhaps to stdout? ("configured logfile is not writeable").

> +
> +        self.assertEqual('Go away!', response)
> +
> +    @patch('os.environ.get')
> +    def test_log_filename_is_fetched_from_env(self, mock_get):
> +        """Check that log_filename is initialized from the environment"""
> +        sentinel = object()
> +        mock_get.return_value = sentinel
> +
> +        api = AnnoyAPI()
> +
> +        self.assertEqual(api.log_filename, sentinel)
> +        mock_get.assert_called_with(LOG_FILENAME_ENVVAR)

Great!