Code review comment for lp:~adeuring/launchpad/bug-458160-fix-submissionparser-checkconsistency

Hi Guilherme,

thanks for your reveiw!

On 23.10.2009 15:12, Guilherme Salgado wrote:
> Review: Approve
> Hi Abel,
>
> Your tests are certainly more readable *and* robust now, but I was left
> wondering if there isn't a mock library [1] that already does what we
> want. Did you check that?

No, I was not aware of this library. After a quick check I think that it
is a very useful tool, but it would not solve the problem I was having.
(Though I may of course have missed something.)

>
> Other than that, I have only a couple suggestions below, but I think
> this is a nice improvement as it is, so I'm approving it.
>
> status approved
>
> [1] http://www.voidspace.org.uk/python/mock/
> http://labix.org/mocker
> etc
>
> On Fri, 2009-10-23 at 08:54 +0000, Abel Deuring wrote:
> [...]
>> === modified file 'lib/lp/testing/__init__.py'
>> --- lib/lp/testing/__init__.py 2009-10-20 01:55:17 +0000
>> +++ lib/lp/testing/__init__.py 2009-10-23 08:01:11 +0000
>> @@ -8,6 +8,7 @@
>> from datetime import datetime, timedelta
>> from pprint import pformat
>> import copy
>> +from inspect import getargspec, getmembers, getmro, isclass, ismethod
>> import os
>> import shutil
>> import subprocess
>> @@ -744,3 +745,69 @@
>> tree.unlock()
>>
>> return contents
>> +
>> +def validate_mock_class(mock_class):
>> + """Validate method signatures in mock classes derived from real classes.
>> +
>> + We often use mock classes in tests which are derived from real
>> + classes.
>> +
>> + This decorator ensures that methods redefined in the mock
>> + class have the same signature as the corresponding methods of
>> + the base class.
>
> This is not really a decorator, right? It'd be nice if it was a class
> decorator, though. Did you consider that?

Argh, I forgot to change the doc string... I _wanted_ to use it as a
decorator, but then discovered that we cannot use class decorators in
Python 2.4 and 2.5.

>
>> +
>> + >>> class A:
>> + ...
>> + ... def method_one(self, a):
>> + ... pass
>> +
>> + >>>
>> + >>> class B(A):
>> + ... def method_one(self, a):
>> + ... pass
>> + >>> validate_mock_class(B)
>> +
>> + If a class derived from A defines method_one with a different
>> + signature, we get an AssertionError.
>> +
>> + >>> class C(A):
>> + ... def method_one(self, a, b):
>> + ... pass
>> + >>> validate_mock_class(C)
>> + Traceback (most recent call last):
>> + ...
>> + AssertionError: Different method signature for method_one:...
>> +
>> + Even a parameter name must not be modified.
>> +
>> + >>> class D(A):
>> + ... def method_one(self, b):
>> + ... pass
>> + >>> validate_mock_class(D)
>> + Traceback (most recent call last):
>> + ...
>> + AssertionError: Different method signature for method_one:...
>> +
>> + If validate_mock_class() for anything but a class, we get an
>> + AssertionError.
>> +
>> + >>> validate_mock_class('a string')
>> + Traceback (most recent call last):
>> + ...
>> + AssertionError: validate_mock_class() must be called for a class
>> + """
>> + assert isclass(mock_class), (
>> + "validate_mock_class() must be called for a class")
>> + base_classes = getmro(mock_class)
>> + for name, obj in getmembers(mock_class):
>> + if ismethod(obj):
>> + for base_class in base_classes[1:]:
>> + if (name in base_class.__dict__):
>
> No need for the parenthesis here.

sure, removed.

Abel