Code review comment for lp:~al-maisan/landscape-client/obsolete-results-v

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.