Merge lp:~adeuring/launchpad/bug-458160-fix-submissionparser-checkconsistency into lp:launchpad

A two-line change or "real" code together with 100 lines of a new test helper (including tests of the helper) and ~450 lines of changed/improved/fixed tests...

The method SubmissionParser.checkConsistency() in l/c/l/scripts/hwdbsubmissions.py did not specify enough parameters when it called SubmissionParser.checkConsistentUdevDeviceData(). This bug was introduced in a previous branch when I modified the latter method to do more checks which required one more parameter (dmi_data).

The bug in checkConsistency() was not detected because of the way this method was tested: All tests of it created a regular instance of SubmissionParser and then monkey-patched the methods called by checkConsistency(), thus avoiding to set up lots of quite convoluted looking test data required for the non-monkey-patched versions of these methods. The problem is that the surrogate of checkConsistentUdevDeviceData() did not get the new parameter...

I assume that such an error can occur in other tests too, so my idea to avoid a similar problem in the future was this: Don't monkey-patch class SubmissionParser, but use instead a mock class, derived from the real class, and ensure that the methods of mock class have the same signatures as the methods of the base class.

Such a test can be made by calling the new function validate_mock_class() in lp.testing. The test is quite simple: It compares the signature of methods defined in the mock class with the signature of the first method with same name found in the base classes. Even the names of method parameters must not be changed. I think this not a serious problem: having identical parameters names isn't that bad, and allowing differernt names would require a somewhat more complicated comparison, which isn't worth the effort, I think.

the "doc string tests" in lp.testing.__init__ were not tested at all, so I added a test suite for them.

sigh, forgot half of the cover letter:

tests:

./bin/test --test=test_hwdb_submission_parser
./bin/test -m lp.testing

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/launchpad/scripts/hwdbsubmissions.py
  lib/canonical/launchpad/scripts/tests/test_hwdb_submission_parser.py
  lib/lp/testing/__init__.py
  lib/lp/testing/tests/test_inlinetests.py

== Pyflakes notices ==

lib/canonical/launchpad/scripts/hwdbsubmissions.py
    22: redefinition of unused 'etree' from line 20

lib/canonical/launchpad/scripts/tests/test_hwdb_submission_parser.py
    10: redefinition of unused 'etree' from line 8

lib/lp/testing/__init__.py
    20: 'InvalidURLJoin' imported but unused
    43: 'is_logged_in' imported but unused
    45: 'test_tales' imported but unused

== Pylint notices ==

lib/canonical/launchpad/scripts/hwdbsubmissions.py
    20: [F0401] Unable to import 'xml.etree.cElementTree' (No module named etree)

lib/canonical/launchpad/scripts/tests/test_hwdb_submission_parser.py
    8: [F0401] Unable to import 'xml.etree.cElementTree' (No module named etree)

The imported but unused "thingies" in lib/lp/testing/__init__.py are elsewhere imported from lp.testing.

hwdbsubmisisons,py tries to import cElementTree from two different locations. One of them is needed for Python 2.4, the other one for 2.5.

This branch is based on lp:~adeuring/launchpad/bug-458029-kernel-package-name-for-udev-submissions, which is reveiwed but has not yet landed. (ec2testing for that branch got stuck somehow; I killed the ec2 instance yesterday evening after 7 or 8 hours.)

The diff against the base branch:

=== modified file 'lib/canonical/launchpad/scripts/hwdbsubmissions.py'
--- lib/canonical/launchpad/scripts/hwdbsubmissions.py 2009-10-22 11:08:18 +0000
+++ lib/canonical/launchpad/scripts/hwdbsubmissions.py 2009-10-22 19:40:00 +0000
@@ -1365,7 +1365,8 @@
         if ('udev' in parsed_data['hardware']
             and not self.checkConsistentUdevDeviceData(
                 parsed_data['hardware']['udev'],
- parsed_data['hardware']['sysfs-attributes'])):
+ parsed_data['hardware']['sysfs-attributes'],
+ parsed_data['hardware']['dmi'],)):
             return False
         duplicate_ids = self.findDuplicateIDs(parsed_data)
         if duplicate_ids:

=== modified file 'lib/canonical/launchpad/scripts/tests/test_hwdb_submission_parser.py'
--- lib/canonical/launchpad/scripts/tests/test_hwdb_submission_parser.py 2009-10-21 16:49:03 +0000
+++ lib/canonical/launchpad/scripts/tests/test_hwdb_submission_parser.py 2009-10-22 19:59:45 +0000
@@ -23,6 +23,7 @@
     ROOT_UDI)
 from canonical.testing import BaseLayer

+from lp.testing import validate_mock_class

 class SubmissionParserTestParseSoftware(SubmissionParser):
     """A Variant used to test SubmissionParser._parseSoftware.
@@ -2102,7 +2103,7 @@

         All shortcut methods return True.
         """
- def checkUdevDictsHavePathKey(self, udev_data):
+ def checkUdevDictsHavePathKey(self, udev_nodes):
             """See `Submissio...

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?

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?

> +
> + >>> 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.

> + mock_args = getargspec(obj)
> + real_args = getargspec(base_class.__dict__[name])
> + if mock_args != real_args:
> + raise AssertionError(
> + ...

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 isclas...