On 04/29/2010 05:31 PM, Free Ekanayaka wrote:
> Review: Needs Fixing
Hello Free,

I addressed all of your comments below, please review the changes. An
incremental diff is attached to this message for your convenience.

> Very nice work, just a few comments but overall the branch looks great to me.
> 
> [1]
> 
> +    def __init__(self, db, id):
> 
> Please add @param entries in the class docstring for these two parameters.
> 
> [2]
> 
> +        @param filename: The file that contains the sqlite database.
> 
> Jamu recently pointed that @param docstrings for __init__ method should be actually put in the class docstring, like:
> 
> class ExchangeStore(object):
>     """Message meta data required by the L{MessageExchange}.
> 
>     The implementation uses a SQLite database as backend, with a single table
>     called "message_context", whose schema is defined in
>     L{ensure_exchange_schema}.
> 
>     @param filename: The file that contains the sqlite database.
>     """
> 
> because the callable that gets actually used is the class itself (you usually don't call __init__ directly).
> 
> [3]
> 
> For consistency with other tests and for easier reading please name test cases against the tests the exercise, plus some test specific suffix. A few examples:
> 
> test_add_operationid_is_unique -> test_add_message_context_with_duplicate_operation_id
> test_get_message_context_works -> test_get_message_context
> test_deleting_message_contexts_works -> test_deleting_message_contexts
> 
> [4]
> 
> +        self._store = ExchangeStore(
> 
> It would be better to use dependency injection (that is usually the pattern adopted in the client) and pass an instance of ExchangeStore instead of creating it in the __init__ method. This helps testing and mocking, and make it easier in general to pass objects of other types providing the same API.
> 
> For example your test case will then have a self.exchange_store attribute (that you use in to build the MessageExchange object under test) which you can use for performing assertions instead of accessing self.exchanger._store, which is a private attribute and should be considered an implementation detail.
> 
> [5]
> 
> +            return 0
> 
> Maybe None would be safer, as 0 could be used as message id number.
> 
> [6]
> 
> +        self.assertIsNot(
> +            None,
> +            self.exchanger._store.get_message_context(message['operation-id']))
> 
> I think the test should also check that the created MessageContext has the expected attributes set:
> 
> +        message_context = self.exchanger._store.get_message_context(message['operation-id'])
> +        self.assertEquals(message_context.operation_id, 123456)
> +        self.assertEquals(message_context.message_type, "type-R")
> 
> [7]
> 
> +        self.exchanger.send({"type": "resynchronize", "operation-id": 234567})
> 
> Assuming that send() returns None in this case, the test should assert it:
> 
> +        message_id = self.exchanger.send({"type": "resynchronize", "operation-id": 234567})
> +        self.assertIs(message_id, None)
> 
> [8]
> 
> +            logging.info(
> +                "Response message with operation-id %s was discarded "
> +                "because the client's secure ID has changed in the meantime"
> +                % message.get('operation-id'))
> 
> The logging is not tested, you can use self.logfile.getvalue() for that.

Best regards

-- 
Muharem Hrnjadovic <email address hidden>
Public key id   : B2BBFCFC
Key fingerprint : A5A3 CC67 2B87 D641 103F  5602 219F 6B60 B2BB FCFC