Hi Michael,

thanks for your review!

On 08.10.2009 13:35, Michael Nelson wrote:
> Review: Approve code
>> This branch adds some sanity checks for HWDB submission containing udev data
>> (as sent by the HWDB client in Karmic), and it defines a new class UdevDevice,
>> representing a device described by udev data. (The class is quite rudimentary
>> at present.)
> 
> Wow - I learned lots of great stuff through this branch - sorry it took me so long to review!
> 
> After we found a better way of creating test versions of some of your classes (thanks Bjorn) I've only got comments and some test suggestions left.
> 
> Great work!
> 
>> === modified file 'lib/canonical/launchpad/scripts/hwdbsubmissions.py'
>> --- lib/canonical/launchpad/scripts/hwdbsubmissions.py	2009-10-06 08:37:11 +0000
>> +++ lib/canonical/launchpad/scripts/hwdbsubmissions.py	2009-10-08 09:38:16 +0000
>> @@ -1141,6 +1141,75 @@
>>                  return False
>>          return True
>>  
>> +    PCI_PROPERTIES = set(
>> +        ('PCI_CLASS', 'PCI_ID', 'PCI_SUBSYS_ID', 'PCI_SLOT_NAME'))
>> +    pci_class_re = re.compile('^[0-9a-f]{1,6}$', re.I)
>> +    pci_id_re = re.compile('^[0-9a-f]{4}:[0-9a-f]{4}$', re.I)
>> +
>> +    def checkUdevPciProperties(self, udev_data):
>> +        """Validation of udev PCI devices.
>> +
>> +        Each PCI device must have the properties PCI_CLASS, PCI_ID,
>> +        PCI_SUBSYS_ID, PCI_SLOT_NAME. Non-PCI devices must not have
>> +        them.
>> +
>> +        The value of PCI class must be a 24 bit integer in
>> +        hexadecimal representation.
>> +
>> +        The values of PCI_ID and PCI_SUBSYS_ID must be two 16 bit
>> +        integers, separated by a ':'.
> 
> Barry would say: could you please use the :param: etc. like you have below
> for UDevDevice.

Done.

> 
>> +        """
>> +        for device in udev_data:
>> +            properties = device['E']
>> +            property_names = set(properties.keys())
> 
> I think this method could do with a few comments in general... maybe
> a lot of it is completely obvious to you, but I have to spend time
> interpreting the code, for example,...

yeah, sorry, I forgot to mention how you can see the data yourself:

	udevadm info --export-db

produces the data we are talking about. Might have made it easier for
you to get an idea what all this is about...

> 
>> +            existing_pci_properties = property_names.intersection(
>> +                self.PCI_PROPERTIES)
>> +            subsystem = device['E'].get('SUBSYSTEM')
>> +            if subsystem is None:
>> +                self._logError(
>> +                    'udev device without SUBSYSTEM property found.',
>> +                    self.submission_key)
>> +                return False
>> +            if subsystem == 'pci':
> 
> # Check whether any of the standard pci properties were missing.

Added.

>> +                if existing_pci_properties != self.PCI_PROPERTIES:
>> +                    missing_properties = self.PCI_PROPERTIES.difference(
>> +                            existing_pci_properties)
>> +
>> +                    self._logError(
>> +                        'PCI udev device without required PCI properties: '
>> +                            '%r %r'
>> +                            % (missing_properties, device['P']),
>> +                        self.submission_key)
>> +                    return False
> 
> # Ensure that the pci class and ids for this device are valid.

Added.

>> +                if self.pci_class_re.search(properties['PCI_CLASS']) is None:
>> +                    self._logError(
>> +                        'Invalid udev PCI class: %r %r'
>> +                            % (properties['PCI_CLASS'], device['P']),
>> +                        self.submission_key)
>> +                    return False
>> +                for pci_id in (properties['PCI_ID'],
>> +                               properties['PCI_SUBSYS_ID']):
>> +                    if self.pci_id_re.search(pci_id) is None:
>> +                        self._logError(
>> +                            'Invalid udev PCI device ID: %r %r'
>> +                                % (pci_id, device['P']),
>> +                            self.submission_key)
>> +                        return False
>> +            else:
>> +                if len(existing_pci_properties) > 0:
> 
> It might be a lack of background on my part, but is this check necessary?
> I mean, if checkUdevPciProperties() finds a non-pci device, shouldn't
> we just say so rather? Ah, perhaps (looking at the above logic) it's fine
> if a non PCI device is found in the udev_data, it just shouldn't have
> any pci properties. Right. 

Exactly. The udev data describes other stuff as well, like USB devices,
disks, even things like the power button.

> Perhaps a comment above the previous 'if' would
> have saved me wondering, something along the lines of:
> 
> # It's fine if we find a non-pci device while checking the udev_data,
> # but we should not find a non-pci device with pci properties.

I think it will become obvious that the udev data describes a variety of
devices, once we have more sanity checks.

> 
>> +                    self._logError(
>> +                        'Non-PCI udev device with PCI properties: %r %r'
>> +                            % (existing_pci_properties, device['P']),
>> +                        self.submission_key)
>> +                    return False
>> +        return True
>> +
>> +    def checkConsistentUdevDeviceData(self, udev_data):
>> +        """Consistency checks for udev data."""
>> +        if not self.checkUdevDictsHavePathKey(udev_data):
>> +            return False
>> +        return self.checkUdevPciProperties(udev_data)
>> +
>>      def checkConsistency(self, parsed_data):
>>          """Run consistency checks on the submitted data.
>>  
>> @@ -1148,10 +1217,10 @@
>>          :param: parsed_data: parsed submission data, as returned by
>>                               parseSubmission
>>          """
>> -        if 'udev' in parsed_data['hardware']:
>> -            if not self.checkUdevDictsHavePathKey(
>> -                parsed_data['hardware']['udev']):
>> -                return False
>> +        if ('udev' in parsed_data['hardware']
>> +            and not self.checkConsistentUdevDeviceData(
>> +                parsed_data['hardware']['udev'])):
>> +            return False
>>          duplicate_ids = self.findDuplicateIDs(parsed_data)
>>          if duplicate_ids:
>>              self._logError('Duplicate IDs found: %s' % duplicate_ids,
>> @@ -1174,7 +1243,8 @@
>>                  self._logError(value, self.submission_key)
>>                  return False
>>  
>> -            circular = self.checkHALDevicesParentChildConsistency(udi_children)
>> +            circular = self.checkHALDevicesParentChildConsistency(
>> +                udi_children)
>>              if circular:
>>                  self._logError('Found HAL devices with circular parent/child '
>>                                 'relationship: %s' % circular,
>> @@ -2146,6 +2216,54 @@
>>          return self.getVendorOrProductID('product')
>>  
>>  
>> +class UdevDevice(BaseDevice):
>> +    """The representation of a udev device node."""
>> +
>> +    def __init__(self, udev_data, sysfs_data, parser):
>> +        """HALDevice constructor.
>> +
>> +        :param udevdata: The udev data for this device
>> +        :param sysfs_data: sysfs data for this device.
>> +        :param parser: The parser processing a submission.
>> +        :type parser: SubmissionParser
>> +        """
>> +        super(UdevDevice, self).__init__(parser)
>> +        self.udev = udev_data
>> +        self.sysfs = sysfs_data
>> +
>> +    @property
>> +    def device_id(self):
>> +        """See `BaseDevice`."""
>> +        return self.udev['P']
>> +
>> +    @property
>> +    def pci_class_info(self):
>> +        """Parse the udev property PCI_SUBSYS_ID.
>> +
>> +        :return: (PCI class, PCI sub-class, version) for a PCI device
>> +            or (None, None, None) for other devices.
>> +        """
>> +        if self.udev['E'].get('SUBSYSTEM') == 'pci':
> 
> Would it be worth adding a self.is_pci property?

Great idea, added!

> 
>> +            # SubmissionParser.checkConsistentUdevDeviceData() ensures
>> +            # that PCI_CLASS is a 24 bit integer in hexadecimal
>> +            # representation.
> 
> Great - but what's going on below, I mean I can read the right shifts and
> masks, and I assume this is the format required for pci_class_info?

I added a comment.

> 
>> +            class_info = int(self.udev['E']['PCI_CLASS'], 16)
>> +            return (class_info >> 16, (class_info >> 8) & 0xFF,
>> +                    class_info & 0xFF)
>> +        else:
>> +            return (None, None, None)
>> +
>> +    @property
>> +    def pci_class(self):
>> +        """See `BaseDevice`."""
>> +        return self.pci_class_info[0]
>> +
>> +    @property
>> +    def pci_subclass(self):
>> +        """See `BaseDevice`."""
>> +        return self.pci_class_info[1]
>> +
>> +
>>  class ProcessingLoop(object):
>>      """An `ITunableLoop` for processing HWDB submissions."""
>>  
>>
>> === modified file 'lib/canonical/launchpad/scripts/tests/test_hwdb_submission_parser.py'
>> --- lib/canonical/launchpad/scripts/tests/test_hwdb_submission_parser.py	2009-10-06 09:31:32 +0000
>> +++ lib/canonical/launchpad/scripts/tests/test_hwdb_submission_parser.py	2009-10-08 09:38:16 +0000
>> @@ -3,6 +3,7 @@
>>  
>>  """Tests of the HWDB submissions parser."""
>>  
>> +from copy import deepcopy
>>  from cStringIO import StringIO
>>  try:
>>      import xml.etree.cElementTree as etree
>> @@ -1773,6 +1774,145 @@
>>          self.assertErrorMessage(
>>              parser.submission_key, 'udev node found without a "P" key')
>>  
>> +    udev_root_device = {
>> +        'P': '/devices/LNXSYSTM:00',
>> +        'E': {'SUBSYSTEM': 'acpi'},
>> +        }
>> +    udev_pci_device = {
>> +        'P': '/devices/pci0000:00/0000:00:1f.2',
>> +        'E': {
>> +            'SUBSYSTEM': 'pci',
>> +            'PCI_CLASS': '10601',
>> +            'PCI_ID': '8086:27C5',
>> +            'PCI_SUBSYS_ID': '10CF:1387',
>> +            'PCI_SLOT_NAME': '0000:00:1f.2',
>> +            }
>> +        }
>> +
>> +    def testCheckUdevPciProperties(self):
> 
> I'm assuming camelCase is for consistency with the rest of the file.

Right.

> 
>> +        """Test of SubmmissionParser.checkUdevPciProperties()."""
>> +        # udev PCI devices must have the properties PCI_CLASS, PCI_ID,
>> +        # PCI_SUBSYS_ID, PCI_SLOT_NAME; other devices must not have
>> +        # these properties.
>> +        parser = SubmissionParser()
>> +        self.assertTrue(parser.checkUdevPciProperties(
>> +            [self.udev_root_device, self.udev_pci_device]))
>> +
> 
> Sheesh - that's one long single test there! Could it be split up into
> a bunch of unit-tests? Like, testCheckUdevPciProperties_valid_device,
> testCheckUdevPciProperties_bad_device, etc etc. as outlined at:

Done.

> 
> https://dev.launchpad.net/TestsStyleGuide#Python%20Test%20Cases
> 
>> +        bad_root_device = deepcopy(self.udev_root_device)
> 
> If you defined the above class properties (udev_root_device and
> udev_pci_device instead as instance properties within setUp, you could
> then re-use and modify them in each test without having to deep_copy etc.

Ah, right. done.

> 
>> +        bad_root_device['E']['PCI_SLOT_NAME'] = '0000:00:1f.2'
>> +        parser = SubmissionParser(self.log)
>> +        parser.submission_key = 'invalid non-PCI device'
>> +        self.assertFalse(parser.checkUdevPciProperties(
>> +            [bad_root_device, self.udev_pci_device]))
>> +        self.assertErrorMessage(
>> +            parser.submission_key,
>> +            "Non-PCI udev device with PCI properties: set(['PCI_SLOT_NAME']) "
>> +            "'/devices/LNXSYSTM:00'")
> 
> next test.
> 
>> +
>> +        bad_pci_device = deepcopy(self.udev_pci_device)
>> +        del bad_pci_device['E']['PCI_CLASS']
>> +        parser = SubmissionParser(self.log)
>> +        parser.submission_key = 'invalid PCI device'
>> +        self.assertFalse(parser.checkUdevPciProperties(
>> +            [self.udev_root_device, bad_pci_device]))
>> +        self.assertErrorMessage(
>> +            parser.submission_key,
>> +            "PCI udev device without required PCI properties: "
>> +            "set(['PCI_CLASS']) '/devices/pci0000:00/0000:00:1f.2'")
> 
> next test.
> 
>> +
>> +        bad_pci_device = deepcopy(self.udev_pci_device)
>> +        bad_pci_device['E']['PCI_CLASS'] = 'not-an-integer'
>> +        parser = SubmissionParser(self.log)
>> +        parser.submission_key = 'invalid PCI class value'
>> +        self.assertFalse(parser.checkUdevPciProperties(
>> +            [self.udev_root_device, bad_pci_device]))
>> +        self.assertErrorMessage(
>> +            parser.submission_key,
>> +            "Invalid udev PCI class: 'not-an-integer' "
>> +            "'/devices/pci0000:00/0000:00:1f.2'")
> 
> next test.
> 
>> +
>> +        bad_pci_device = deepcopy(self.udev_pci_device)
>> +        bad_pci_device['E']['PCI_CLASS'] = '1234567'
>> +        parser = SubmissionParser(self.log)
>> +        parser.submission_key = 'too large PCI class value'
>> +        self.assertFalse(parser.checkUdevPciProperties(
>> +            [self.udev_root_device, bad_pci_device]))
>> +        self.assertErrorMessage(
>> +            parser.submission_key,
>> +            "Invalid udev PCI class: '1234567' "
>> +            "'/devices/pci0000:00/0000:00:1f.2'")
> 
> next test.
> 
>> +
>> +        bad_pci_device = deepcopy(self.udev_pci_device)
>> +        bad_pci_device['E']['PCI_ID'] = 'not-an-id'
>> +        parser = SubmissionParser(self.log)
>> +        parser.submission_key = 'invalid PCI ID'
>> +        self.assertFalse(parser.checkUdevPciProperties(
>> +            [self.udev_root_device, bad_pci_device]))
>> +        self.assertErrorMessage(
>> +            parser.submission_key,
>> +            "Invalid udev PCI device ID: 'not-an-id' "
>> +            "'/devices/pci0000:00/0000:00:1f.2'")
>> +
> 
> next test.
> 
>> +        bad_pci_device = deepcopy(self.udev_pci_device)
>> +        bad_pci_device['E']['PCI_SUBSYS_ID'] = 'not-a-subsystem-id'
>> +        parser = SubmissionParser(self.log)
>> +        parser.submission_key = 'invalid PCI subsystem ID'
>> +        self.assertFalse(parser.checkUdevPciProperties(
>> +            [self.udev_root_device, bad_pci_device]))
>> +        self.assertErrorMessage(
>> +            parser.submission_key,
>> +            "Invalid udev PCI device ID: 'not-a-subsystem-id' "
>> +            "'/devices/pci0000:00/0000:00:1f.2'")
>> +
>> +    def _setupUdevConsistencyCheckTests(self):
>> +        """Prepare and return a SubmissionParser instance.
> 
> OK, a few things here.
> 
> First, this looks like the kind of thing that a setUp() method
> should be doing, so perhaps the following tests could be in a separate
> class with a setUp() that sets self.parser?
> 
> Second, as we chatted about on irc, I'm all for using mocks/doubles etc.,
> but not by manually patching things like this. So I'd be keen to see this
> changed to either (1) simply setup the required data - but you said
> this will get tedious as this class grows, so (2) using mocker to
> do what you've done below - you could potentially just patch the class
> in your setup method - but I'm not sure - I haven't used mocker itself.
> 
> From: http://labix.org/mocker
> "Mocking via temporary patching of existent classes and instances."
> 
> Ah, then BjornT chipped in with:
> 
> <BjornT> noodles775, adeuring: i don't see a reason for using mocker to do that. can't you simply subclass SubmissionParser and override the methods in the new class?
> * barry has quit (Read error: 104 (Connection reset by peer))
> <adeuring> BjornT: nice idea, thanks!
> <noodles775> BjornT: yep - that sounds like a good option.

I added the class  UdevTestSubmissionParser and a few similar classes
which are now used in these tests.

> 
>> +
>> +        Replace udev related methods with dummies that just return True.
>> +        """
>> +        def checkUdevDictsHavePathKey(self, udev_data):
>> +            return True
>> +
>> +        def checkUdevPciProperties(self, udev_data):
>> +            return True
>> +
>> +        parser = SubmissionParser()
>> +        parser.checkUdevDictsHavePathKey = (
>> +            lambda data: checkUdevDictsHavePathKey(parser, data))
>> +        parser.checkUdevPciProperties = (
>> +            lambda data: checkUdevPciProperties(parser, data))
>> +        return parser
>> +
>> +    def testCheckConsistentUdevDeviceData(self):
>> +        """Test of SubmissionParser.checkConsistentUdevDeviceData(),"""
>> +        parser = self._setupUdevConsistencyCheckTests()
>> +        self.assertTrue(parser.checkConsistentUdevDeviceData(None))
>> +
>> +    def testCheckConsistentUdevDeviceDataInvalidPathData(self):
> 
> This should be either:
> 
> testCheckConsistentUdevDeviceData_invalid_path_data() (consistent),

Changed.

> 
> or
> 
> test_checkConsistentUdevDeviceData_invalid_path_data()
> 
> as per
> 
> https://dev.launchpad.net/TestsStyleGuide#Python%20Test%20Cases
> 
>> +        """Test of SubmissionParser.checkConsistentUdevDeviceData(),
>> +
>> +        Detection of invalid path data lets the check fail.
>> +        """
>> +        def checkUdevDictsHavePathKey(self, udev_data):
>> +            return False
>> +
>> +        parser = self._setupUdevConsistencyCheckTests()
>> +        parser.checkUdevDictsHavePathKey = (
>> +            lambda data: checkUdevDictsHavePathKey(parser, data))
> 
> Same thing as above - using BjornT's method, you could have a subclass with
> a property that you can just set in your test, and the method will return
> that property? That way you could use it for above and below.

Changed.

> 
>> +        self.assertFalse(parser.checkConsistentUdevDeviceData(None))
>> +
>> +    def testCheckConsistentUdevDeviceDataInvalidPciData(self):
> 
> Again - style-guide for tests ^^^

Changed.

> 
>> +        """Test of SubmissionParser.checkConsistentUdevDeviceData(),
>> +
>> +        Detection of invalid PCI data lets the check fail.
>> +        """
>> +        def checkUdevPciProperties(self, udev_data):
>> +            return False
>> +
>> +        parser = self._setupUdevConsistencyCheckTests()
>> +        parser.checkUdevPciProperties = (
>> +            lambda data: checkUdevPciProperties(parser, data))
>> +        self.assertFalse(parser.checkConsistentUdevDeviceData(None))
>> +
>>      def _setupConsistencyCheckParser(self):
>>          """Prepare and return a SubmissionParser instance.
>>  
>> @@ -1799,6 +1939,9 @@
>>              test.assertTrue(isinstance(self, SubmissionParser))
>>              return []
>>  
>> +        def checkConsistentUdevDeviceData(self, udev_data):
>> +            return True
>> +
>>          parser = SubmissionParser(self.log)
>>          parser.findDuplicateIDs = (
>>              lambda parsed_data: findDuplicateIDs(parser, parsed_data))
>> @@ -1811,6 +1954,9 @@
>>          parser.checkHALDevicesParentChildConsistency = (
>>              lambda udi_children: checkHALDevicesParentChildConsistency(
>>                  parser, udi_children))
>> +        parser.checkConsistentUdevDeviceData = (
>> +            lambda udev_data: checkConsistentUdevDeviceData(
>> +                parser, udev_data))
>>          return parser
> 
> Another one? OK, you've already got some test classes in place for this,
> so I'll stop mentioning it :)

That's an extenstion of an extsting test -- I won't change that now ;)

> 
>>  
>>      def assertErrorMessage(self, submission_key, log_message):
>> @@ -1884,8 +2030,13 @@
>>  
>>      def testConsistencyCheckInvalidUdevData(self):
> 
> Previously mentioned test styles ^^^

Changed.

> 
>>          """Test of SubmissionParser.checkConsistency."""
>> +        def checkConsistentUdevDeviceData(self, udev_data):
>> +            return False
>> +
>>          parser = self._setupConsistencyCheckParser()
>> -        parser.submission_key = 'Consistency check with invalid udev data'
>> +        parser.checkConsistentUdevDeviceData = (
>> +            lambda udev_data: checkConsistentUdevDeviceData(
>> +                parser, udev_data))
>>          self.assertFalse(parser.checkConsistency(
>>              {
>>                  'hardware': {
>> @@ -1894,8 +2045,6 @@
>>                      }
>>                  }
>>              ))
>> -        self.assertErrorMessage(
>> -            parser.submission_key, 'udev node found without a "P" key')
>>  
>>      def testConsistencyCheckWithDuplicateIDs(self):
>>          """SubmissionParser.checkConsistency detects duplicate IDs."""
>>
>> === modified file 'lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py'
>> --- lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py	2009-10-01 14:48:33 +0000
>> +++ lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py	2009-10-08 09:38:16 +0000
>> @@ -27,8 +27,8 @@
>>  from canonical.launchpad.scripts.hwdbsubmissions import (
>>      HALDevice, PCI_CLASS_BRIDGE, PCI_CLASS_SERIALBUS_CONTROLLER,
>>      PCI_CLASS_STORAGE, PCI_SUBCLASS_BRIDGE_CARDBUS, PCI_SUBCLASS_BRIDGE_PCI,
>> -    PCI_SUBCLASS_SERIALBUS_USB, PCI_SUBCLASS_STORAGE_SATA, SubmissionParser,
>> -    process_pending_submissions)
>> +    PCI_SUBCLASS_SERIALBUS_USB, PCI_SUBCLASS_STORAGE_SATA,
>> +    process_pending_submissions, SubmissionParser, UdevDevice)
>>  from canonical.launchpad.webapp.errorlog import ErrorReportingUtility
>>  from canonical.testing import BaseLayer, LaunchpadZopelessLayer
>>  
>> @@ -2591,6 +2591,76 @@
>>              'property not treated as a real device.')
>>  
>>  
>> +class TestUdevDevice(TestCase):
>> +    """Tests of class UdevDevice."""
>> +
>> +    layer = BaseLayer
>> +
>> +    root_device = {
>> +        'P': '/devices/LNXSYSTM:00',
>> +        'E': {
>> +            'UDEV_LOG': '3',
>> +            'DEVPATH': '/devices/LNXSYSTM:00',
>> +            'MODALIAS': 'acpi:LNXSYSTM:',
>> +            'SUBSYSTEM': 'acpi',
>> +            }
>> +        }
>> +
>> +    pci_device_data = {
>> +        'P': '/devices/pci0000:00/0000:00:1f.2',
>> +        'E': {
>> +            'PCI_CLASS': '10602',
>> +            'PCI_ID': '8086:27C5',
>> +            'PCI_SUBSYS_ID': '10CF:1387',
>> +            'PCI_SLOT_NAME': '0000:00:1f.2',
>> +            'SUBSYSTEM': 'pci',
>> +            }
>> +        }
>> +
>> +    def test_device_id(self):
>> +        """Test of UdevDevice.device_id."""
>> +        device = UdevDevice(self.pci_device_data, None, None)
>> +        self.assertEqual(
>> +            '/devices/pci0000:00/0000:00:1f.2', device.device_id,
>> +            'Unexpected value of UdevDevice.device_id.')
>> +
>> +    def test_pci_class_info(self):
>> +        """Test of UdevDevice.pci_class_info"""
>> +        device = UdevDevice(self.pci_device_data, None, None)
>> +        self.assertEqual(
>> +            (1, 6, 2), device.pci_class_info,
>> +            'Invalid value of UdevDevice.pci_class_info for PCI device.')
>> +
>> +        device = UdevDevice(self.root_device, None, None)
>> +        self.assertEqual(
>> +            (None, None, None), device.pci_class_info,
>> +            'Invalid value of UdevDevice.pci_class_info for Non-PCI device.')
>> +
>> +    def test_pci_class(self):
>> +        """Test of UdevDevice.pci_class"""
>> +        device = UdevDevice(self.pci_device_data, None, None)
>> +        self.assertEqual(
>> +            1, device.pci_class,
>> +            'Invalid value of UdevDevice.pci_class for PCI device.')
>> +
>> +        device = UdevDevice(self.root_device, None, None)
>> +        self.assertEqual(
>> +            None, device.pci_class,
>> +            'Invalid value of UdevDevice.pci_class for Non-PCI device.')
>> +
>> +    def test_pci_subclass(self):
>> +        """Test of UdevDevice.pci_subclass"""
>> +        device = UdevDevice(self.pci_device_data, None, None)
>> +        self.assertEqual(
>> +            6, device.pci_subclass,
>> +            'Invalid value of UdevDevice.pci_class for PCI device.')
>> +
>> +        device = UdevDevice(self.root_device, None, None)
>> +        self.assertEqual(
>> +            None, device.pci_class,
>> +            'Invalid value of UdevDevice.pci_class for Non-PCI device.')
>> +
>> +
>>  class TestHWDBSubmissionTablePopulation(TestCaseHWDB):
>>      """Tests of the HWDB popoluation with submitted data."""
>>  
> 
> *phew* Great! Thanks Abel!
>