> This branch adds a property scsi_controller to class UdevDevice in the script
> hwdbsubmissons.py.

Hi Abel. Again, thanks for the details about this branch - it's very helpful to have such a clear explanation when the topic is quite foreign (at least, to me!).

I didn't have anything other than a few questions (see below). Great work!

[snip]

> === modified file 'lib/canonical/launchpad/scripts/hwdbsubmissions.py'
> --- lib/canonical/launchpad/scripts/hwdbsubmissions.py	2009-10-13 21:03:31 +0000
> +++ lib/canonical/launchpad/scripts/hwdbsubmissions.py	2009-10-15 10:53:08 +0000
[snip]
> @@ -2617,6 +2635,29 @@
>          """See `BaseDevice`."""
>          return self.getVendorOrProductID('product')
>  
> +    @property
> +    def driver_name(self):
> +        """See `BaseDevice`."""
> +        return self.udev['E'].get('DRIVER')
> +
> +    @property
> +    def scsi_controller(self):
> +        """See `BaseDevice`."""
> +        if self.raw_bus != 'scsi_device':
> +            return None
> +
> +        current_device = self
> +        for ancestor_level in range(1, 5):
> +            if current_device.parent is None:
> +                # While SCSI devices from valid submissions should have a
> +                # parent and a grandparent, we can't be sure for bogus or
> +                # broken submissions.
> +                self.parser._logWarning(
> +                    'Found a SCSI device without a sufficient number of '
> +                    'ancestors: %s' % self.device_id)
> +                return None
> +            current_device = current_device.parent
> +        return current_device

Nice!

Actually, if you're just presenting a general log warning, I wonder whether
doing something like the following would be more readable?
    try:
        controller = self.parent.parent.parent.parent
    except AttributeError:
        return None

Up to you.

>  
>  class ProcessingLoop(object):
>      """An `ITunableLoop` for processing HWDB submissions."""
> 
> === modified file 'lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py'
> --- lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py	2009-10-13 21:02:35 +0000
> +++ lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py	2009-10-15 10:53:08 +0000
> @@ -539,6 +539,214 @@
>                           'HAL property info.bus.')
>  
>  
> +    def test_HALDevice_scsi_controller_usb_storage_device(self):
> +        """test of HALDevice.scsi_controller.
> +
> +        The physical device is a USB storage device.
> +        """
> +        devices = [
> +            # The main node of the USB storage device.
> +            {
> +                'id': 1,
> +                'udi': self.UDI_USB_STORAGE,
> +                'properties': {
> +                    'info.bus': ('usb_device', 'str'),
> +                    },
> +                },

Just a style question (the answer to which I'm not certain), based on:

https://dev.launchpad.net/PythonStyleGuide#Multiline%20braces

should this be:

        devices = [{
            # The main node of the USB storage device.
            'id': 1,
            'udi': self.UDI_USB_STORAGE,
            'properties': {
                'info.bus': ('usb_device', 'str'),
                },
            }, {
            ...

hrm, probably not. Yes, afaics, the way you've done it is the only way
that fits both style rules on that page - it just looks strange with the
opening brace being indented (which doesn't happen for non-nested braces).
Hmm, maybe we should have a nested example in the styleguide.

So, nothing to change here, move along please! :)

> +            # The storage interface of the USB device.
> +            {
> +                'id': 2,
> +                'udi': self.UDI_USB_STORAGE_IF0,
> +                'properties': {
> +                    'info.bus': ('usb', 'str'),
> +                    'info.parent': (self.UDI_USB_STORAGE, 'str'),
> +                    },
> +                },
> +            # The fake SCSI host of the storage device. Note that HAL does
> +            # _not_ provide the info.bus property.
> +            {
> +                'id': 3,
> +                'udi': self.UDI_USB_STORAGE_SCSI_HOST,
> +                'properties': {
> +                    'info.parent': (self.UDI_USB_STORAGE_IF0, 'str'),
> +                    },
> +                },
> +            # The fake SCSI disk.
> +            {
> +                'id': 3,
> +                'udi': self.UDI_USB_STORAGE_SCSI_DEVICE,
> +                'properties': {
> +                    'info.bus': ('scsi', 'str'),
> +                    'info.parent': (self.UDI_USB_STORAGE_SCSI_HOST, 'str'),
> +                    },
> +                },
> +            ]
> +        parsed_data = {
> +            'hardware': {
> +                'hal': {
> +                    'devices': devices,
> +                    },
> +                },
> +            }
> +
> +        parser = SubmissionParser()
> +        parser.buildDeviceList(parsed_data)
> +
> +        usb_fake_scsi_disk = parser.hal_devices[
> +            self.UDI_USB_STORAGE_SCSI_DEVICE]
> +        usb_main_device = parser.hal_devices[self.UDI_USB_STORAGE_IF0]
> +        self.assertEqual(usb_main_device, usb_fake_scsi_disk.scsi_controller)
> +
> +    def test_HALDevice_scsi_controller_pci_controller(self):
> +        """test of HALDevice.scsi_controller.
> +
> +        Variant for a SCSI device connected to a PCI controller.
> +        """
> +        devices = [
> +            # The PCI host controller.
> +            {
> +                'id': 1,
> +                'udi': self.UDI_SATA_CONTROLLER,
> +                'properties': {
> +                    'info.bus': ('pci', 'str'),
> +                    'pci.device_class': (PCI_CLASS_STORAGE, 'int'),
> +                    'pci.device_subclass': (PCI_SUBCLASS_STORAGE_SATA,
> +                                            'int'),
> +                    },
> +                },
> +            # The (fake or real) SCSI host of the storage device.
> +            {
> +                'id': 2,
> +                'udi': self.UDI_SATA_CONTROLLER_SCSI,
> +                'properties': {
> +                    'info.parent': (self.UDI_SATA_CONTROLLER, 'str'),
> +                    },
> +                },
> +            # The (possibly fake) SCSI disk.
> +            {
> +                'id': 3,
> +                'udi': self.UDI_SATA_DISK,
> +                'properties': {
> +                    'info.bus': ('scsi', 'str'),
> +                    'info.parent': (self.UDI_SATA_CONTROLLER_SCSI, 'str'),
> +                    },
> +                },
> +            ]
> +        parsed_data = {
> +            'hardware': {
> +                'hal': {
> +                    'devices': devices,
> +                    },
> +                },
> +            }
> +
> +        parser = SubmissionParser()
> +        parser.buildDeviceList(parsed_data)
> +
> +        scsi_device = parser.hal_devices[self.UDI_SATA_DISK]
> +        controller = parser.hal_devices[self.UDI_SATA_CONTROLLER]
> +        self.assertEqual(controller, scsi_device.scsi_controller)
> +
> +    def test_HALDevice_scsi_controller_non_scsi_device(self):
> +        """test of HALDevice.scsi_controller.
> +
> +        Variant for non-SCSI devices.
> +        """
> +        devices = [
> +            {
> +                'id': 1,
> +                'udi': self.UDI_COMPUTER,
> +                'properties': {},
> +                },
> +            ]
> +        parsed_data = {
> +            'hardware': {
> +                'hal': {
> +                    'devices': devices,
> +                    },
> +                },
> +            }
> +
> +        parser = SubmissionParser()
> +        parser.buildDeviceList(parsed_data)
> +
> +        device = parser.hal_devices[self.UDI_COMPUTER]
> +        self.assertEqual(None, device.scsi_controller)
> +
> +    def test_HALDevice_scsi_controller_no_grandparent(self):
> +        """test of HALDevice.scsi_controller.
> +
> +        Variant for a SCSI device without a grandparent device.
> +        """
> +        devices = [
> +            # The (fake or real) SCSI host of the storage device.
> +            {
> +                'id': 1,
> +                'udi': self.UDI_SATA_CONTROLLER_SCSI,
> +                'properties': {},
> +                },
> +            # The (possibly fake) SCSI disk.
> +            {
> +                'id': 2,
> +                'udi': self.UDI_SATA_DISK,
> +                'properties': {
> +                    'info.bus': ('scsi', 'str'),
> +                    'info.parent': (self.UDI_SATA_CONTROLLER_SCSI, 'str'),
> +                    },
> +                },
> +            ]
> +        parsed_data = {
> +            'hardware': {
> +                'hal': {
> +                    'devices': devices,
> +                    },
> +                },
> +            }
> +
> +        parser = SubmissionParser(self.log)
> +        parser.submission_key = 'SCSI device without grandparent device'
> +        parser.buildDeviceList(parsed_data)
> +
> +        scsi_device = parser.hal_devices[self.UDI_SATA_DISK]
> +        self.assertEqual(None, scsi_device.scsi_controller)
> +        self.assertWarningMessage(
> +            parser.submission_key,
> +            "Found SCSI device without a grandparent: %s."
> +            % self.UDI_SATA_DISK)
> +
> +    def test_HALDevice_scsi_controller_no_parent(self):
> +        """test of HALDevice.scsi_controller.
> +
> +        Variant for a SCSI device without a parent device.
> +        """
> +        devices = [
> +            # The (possibly fake) SCSI disk.
> +            {
> +                'id': 1,
> +                'udi': self.UDI_SATA_DISK,
> +                'properties': {
> +                    'info.bus': ('scsi', 'str'),
> +                    },
> +                },
> +            ]
> +        parsed_data = {
> +            'hardware': {
> +                'hal': {
> +                    'devices': devices,
> +                    },
> +                },
> +            }
> +
> +        parser = SubmissionParser(self.log)
> +        parser.submission_key = 'SCSI device without parent device'
> +        parser.buildDeviceList(parsed_data)
> +
> +        scsi_device = parser.hal_devices[self.UDI_SATA_DISK]
> +        self.assertEqual(None, scsi_device.scsi_controller)
> +        self.assertWarningMessage(
> +            parser.submission_key,
> +            "Found SCSI device without a parent: %s." % self.UDI_SATA_DISK)
> +
>      def testHALDeviceGetRealBus(self):
>          """Test of HALDevice.real_bus, generic case.
>  
> @@ -2585,11 +2793,9 @@
>              'property not treated as a real device.')
>  
>  
> -class TestUdevDevice(TestCase):
> +class TestUdevDevice(TestCaseHWDB):
>      """Tests of class UdevDevice."""
>  
> -    layer = BaseLayer
> -
>      root_device = {
>          'P': '/devices/LNXSYSTM:00',
>          'E': {
> @@ -2613,6 +2819,7 @@
>              'PCI_SUBSYS_ID': '10CF:1387',
>              'PCI_SLOT_NAME': '0000:00:1f.2',
>              'SUBSYSTEM': 'pci',
> +            'DRIVER': 'ahci',
>              }
>          }
>  
> @@ -2623,22 +2830,194 @@
>              'DEVTYPE': 'usb_device',
>              'PRODUCT': '46d/a01/1013',
>              'TYPE': '0/0/0',
> -            },
> -        }
> -
> -    scsi_device_data = {
> -        'P': '/devices/pci0000:00/0000:00:1f.1/host4/target4:0:0/4:0:0:0',
> -        'E': {
> -            'SUBSYSTEM': 'scsi',
> -            'DEVTYPE': 'scsi_device',
> -            },
> -        }
> -
> -    scsi_device_sysfs_data = {
> -        'vendor': 'MATSHITA',
> -        'model': 'DVD-RAM UJ-841S',
> -        'type': '5',
> -        }
> +            'DRIVER': 'usb',
> +            },
> +        }
> +
> +    pci_scsi_controller_data = {
> +        'P': '/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0',
> +        'E': {
> +            'DRIVER': 'aic7xxx',
> +            'PCI_CLASS': '10000',
> +            'PCI_ID': '9004:6075',
> +            'PCI_SUBSYS_ID': '9004:7560',
> +            'SUBSYSTEM': 'pci',
> +            },
> +        }
> +
> +    pci_scsi_controller_scsi_side_1 = {
> +        'P': ('/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0/'
> +              'host6'),
> +        'E': {
> +            'DEVTYPE': 'scsi_host',
> +            'SUBSYSTEM': 'scsi',
> +            },
> +        }
> +
> +    pci_scsi_controller_scsi_side_2 = {
> +        'P': ('/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0/'
> +              'host6/scsi_host/host6'),
> +        'E': {
> +            'SUBSYSTEM': 'scsi_host',
> +            },
> +        }
> +
> +    scsi_scanner_target_data = {
> +        'P': ('/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0/'
> +              'host6/target6:0:1'),
> +        'E': {
> +            'DEVTYPE': 'scsi_target',
> +            'SUBSYSTEM': 'scsi'
> +            },
> +        }
> +
> +    scsi_scanner_device_data = {
> +        'P': ('/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0/'
> +              'host6/target6:0:1/6:0:1:0'),
> +        'E': {
> +            'DEVTYPE': 'scsi_device',
> +            'SUBSYSTEM': 'scsi',
> +            },
> +        }
> +
> +    scsi_scanner_device_sysfs_data = {
> +        'vendor': 'FUJITSU',
> +        'model': 'fi-5120Cdj',
> +        'type': '6',
> +        }
> +
> +    scsi_device_hierarchy_data = [
> +        {'udev_data': pci_scsi_controller_data},
> +        {'udev_data': pci_scsi_controller_scsi_side_1},
> +        {'udev_data': pci_scsi_controller_scsi_side_2},
> +        {'udev_data': scsi_scanner_target_data},
> +        {
> +            'udev_data': scsi_scanner_device_data,
> +            'sysfs_data': scsi_scanner_device_sysfs_data,
> +            },
> +        ]
> +
> +    pci_ide_controller = {
> +        'P': '/devices/pci0000:00/0000:00:1f.1',
> +        'E': {
> +            'DRIVER': 'ata_piix',
> +            'PCI_CLASS': '1018A',
> +            'PCI_ID': '8086:27DF',
> +            'PCI_SUBSYS_ID': '10CF:1385',
> +            'SUBSYSTEM': 'pci',
> +            },
> +        }
> +
> +    pci_ide_controller_scsi_side_1 = {
> +        'P': '/devices/pci0000:00/0000:00:1f.1/host4',
> +        'E': {
> +            'DEVTYPE': 'scsi_host',
> +            'SUBSYSTEM': 'scsi',
> +            },
> +        }
> +
> +    pci_ide_controller_scsi_side_2 = {
> +        'P': '/devices/pci0000:00/0000:00:1f.1/host4/scsi_host/host4',
> +        'E': {
> +            'SUBSYSTEM': 'scsi_host',
> +            },
> +        }
> +
> +    ide_device_target_data = {
> +        'P': '/devices/pci0000:00/0000:00:1f.1/host4/target4:0:0',
> +        'E': {
> +            'DEVTYPE': 'scsi_target',
> +            'SUBSYSTEM': 'scsi',
> +            },
> +        }
> +
> +    ide_cdrom_device_data = {
> +         'P': '/devices/pci0000:00/0000:00:1f.1/host4/target4:0:0/4:0:0:0',
> +         'E': {
> +             'SUBSYSTEM': 'scsi',
> +             'DEVTYPE': 'scsi_device',
> +             'DRIVER': 'sr',
> +             },
> +         }
> +
> +    ide_cdrom_device_sysfs_data = {
> +         'vendor': 'MATSHITA',
> +         'model': 'DVD-RAM UJ-841S',
> +         'type': '5',
> +         }
> +
> +    ide_device_hierarchy_data = [
> +        {'udev_data': pci_ide_controller},
> +        {'udev_data': pci_ide_controller_scsi_side_1},
> +        {'udev_data': pci_ide_controller_scsi_side_2},
> +        {'udev_data': ide_device_target_data},
> +        {
> +            'udev_data': ide_cdrom_device_data,
> +            'sysfs_data': ide_cdrom_device_sysfs_data,
> +            },
> +        ]
> +
> +    usb_storage_usb_interface = {
> +        'P': '/devices/pci0000:00/0000:00:1d.7/usb1/1-1/1-1:1.0',
> +        'E': {
> +            'DRIVER': 'usb-storage',
> +            'PRODUCT': '1307/163/100',
> +            'TYPE': '0/0/0',
> +            'INTERFACE': '8/6/80',
> +            'SUBSYSTEM': 'usb',
> +            },
> +        }
> +    usb_storage_scsi_host_1 = {
> +        'P': '/devices/pci0000:00/0000:00:1d.7/usb1/1-1/1-1:1.0/host7',
> +        'E': {
> +            'DEVTYPE': 'scsi_host',
> +            'SUBSYSTEM': 'scsi',
> +            },
> +        }
> +
> +    usb_storage_scsi_host_2 = {
> +        'P': ('/devices/pci0000:00/0000:00:1d.7/usb1/1-1/1-1:1.0/host7/'
> +              'scsi_host/host7'),
> +        'E': {
> +            'SUBSYSTEM': 'scsi_host',
> +            },
> +        }
> +
> +    usb_storage_scsi_target = {
> +        'P': ('/devices/pci0000:00/0000:00:1d.7/usb1/1-1/1-1:1.0/host7/'
> +              'target7:0:0'),
> +        'E': {
> +            'DEVTYPE': 'scsi_target',
> +            'SUBSYSTEM': 'scsi',
> +            },
> +        }
> +
> +    usb_storage_scsi_device = {
> +        'P': ('/devices/pci0000:00/0000:00:1d.7/usb1/1-1/1-1:1.0/host7/'
> +              'target7:0:0/7:0:0:0'),
> +        'E': {
> +            'DEVTYPE': 'scsi_device',
> +            'DRIVER': 'sd',
> +            'SUBSYSTEM': 'scsi',
> +            },
> +        }
> +
> +    usb_storage_scsi_device_sysfs = {
> +        'vendor': 'Ut163',
> +        'model': 'USB2FlashStorage',
> +        'type': '0',
> +        }
> +
> +    usb_storage_hierarchy_data = [
> +        {'udev_data': usb_storage_usb_interface},
> +        {'udev_data': usb_storage_scsi_host_1},
> +        {'udev_data': usb_storage_scsi_host_2},
> +        {'udev_data': usb_storage_scsi_target},
> +        {
> +            'udev_data': usb_storage_scsi_device,
> +            'sysfs_data': usb_storage_scsi_device_sysfs,
> +            },
> +        ]

I'll have to trust you on the values of all that test data, but shoudn't
it all be created as part of setUp() - so that if an individual test
modifies the data in some way, it won't affect other tests? As it is I think
it could lead to some hard-to-debug failures in the future? The added benefit
is that you may be able to get away with less test data, as individual
tests can update the values of the test data as they need.

Oh, I see below that the way you've done it makes your tests very clear
(just creating UdevDevices based on different combinations of the above).
OK, so if it wouldn't help at all, forget my point in the previous
paragraph (although it would be good to ensure the data is somehow immutable
if it stays there as class attributes?)

>  
>      no_subsystem_device_data = {
>          'P': '/devices/pnp0/00:00',
> @@ -2790,7 +3169,8 @@
>      def test_is_scsi_device(self):
>          """Test of UdevDevice.is_scsi_device."""
>          device = UdevDevice(
> -            None, self.scsi_device_data, self.scsi_device_sysfs_data)
> +            None, self.scsi_scanner_device_data,
> +            self.scsi_scanner_device_sysfs_data)
>          self.assertTrue(device.is_scsi_device)
>  
>          device = UdevDevice(None, self.root_device)
> @@ -2799,16 +3179,18 @@
>      def test_scsi_vendor(self):
>          """Test of UdevDevice.scsi_vendor."""
>          device = UdevDevice(
> -            None, self.scsi_device_data, self.scsi_device_sysfs_data, None)
> -        self.assertEqual('MATSHITA', device.scsi_vendor)
> +            None, self.scsi_scanner_device_data,
> +            self.scsi_scanner_device_sysfs_data)
> +        self.assertEqual('FUJITSU', device.scsi_vendor)
>          device = UdevDevice(None, self.root_device)
>          self.assertEqual(None, device.scsi_vendor)
>  
>      def test_scsi_model(self):
>          """Test of UdevDevice.scsi_model."""
>          device = UdevDevice(
> -            None, self.scsi_device_data, self.scsi_device_sysfs_data)
> -        self.assertEqual('DVD-RAM UJ-841S', device.scsi_model)
> +            None, self.scsi_scanner_device_data,
> +            self.scsi_scanner_device_sysfs_data)
> +        self.assertEqual('fi-5120Cdj', device.scsi_model)
>  
>          device = UdevDevice(None, self.root_device)
>          self.assertEqual(None, device.scsi_model)
> @@ -2847,10 +3229,10 @@
>          self.assertEqual('Unknown', device.getVendorOrProduct('product'))
>  
>          device = UdevDevice(
> -            None, self.scsi_device_data, self.scsi_device_sysfs_data)
> -        self.assertEqual('MATSHITA', device.getVendorOrProduct('vendor'))
> -        self.assertEqual(
> -            'DVD-RAM UJ-841S', device.getVendorOrProduct('product'))
> +            None, self.scsi_scanner_device_data,
> +            self.scsi_scanner_device_sysfs_data)
> +        self.assertEqual('FUJITSU', device.getVendorOrProduct('vendor'))
> +        self.assertEqual('fi-5120Cdj', device.getVendorOrProduct('product'))
>  
>          device = UdevDevice(None, self.no_subsystem_device_data)
>          self.assertEqual(None, device.getVendorOrProduct('vendor'))
> @@ -2888,10 +3270,10 @@
>          self.assertEqual(0xa01, device.getVendorOrProductID('product'))
>  
>          device = UdevDevice(
> -            None, self.scsi_device_data, self.scsi_device_sysfs_data)
> -        self.assertEqual('MATSHITA', device.getVendorOrProductID('vendor'))
> -        self.assertEqual(
> -            'DVD-RAM UJ-841S', device.getVendorOrProductID('product'))
> +            None, self.scsi_scanner_device_data,
> +            self.scsi_scanner_device_sysfs_data)
> +        self.assertEqual('FUJITSU', device.getVendorOrProductID('vendor'))
> +        self.assertEqual('fi-5120Cdj', device.getVendorOrProductID('product'))
>  
>          device = UdevDevice(
>              None, self.no_subsystem_device_data)
> @@ -2910,6 +3292,132 @@
>              None, self.root_device, None, self.root_device_dmi_data)
>          self.assertEqual('LIFEBOOK E8210', device.product_id)
>  
> +    def test_vendor_id_for_db(self):
> +        """Test of UdevDevice.vendor_id_for_db."""
> +        device = UdevDevice(
> +            None, self.root_device, None, self.root_device_dmi_data)
> +        self.assertEqual('FUJITSU SIEMENS', device.vendor_id_for_db)
> +
> +        device = UdevDevice(None, self.pci_device_data)
> +        self.assertEqual('0x8086', device.vendor_id_for_db)
> +
> +        device = UdevDevice(None, self.usb_device_data)
> +        self.assertEqual('0x046d', device.vendor_id_for_db)
> +
> +        device = UdevDevice(
> +            None, self.scsi_scanner_device_data,
> +            self.scsi_scanner_device_sysfs_data)
> +        self.assertEqual('FUJITSU ', device.vendor_id_for_db)
> +
> +    def test_product_id_for_db(self):
> +        """Test of UdevDevice.product_id_for_db."""
> +        device = UdevDevice(
> +            None, self.root_device, None, self.root_device_dmi_data)
> +        self.assertEqual('LIFEBOOK E8210', device.product_id_for_db)
> +
> +        device = UdevDevice(None, self.pci_device_data)
> +        self.assertEqual('0x27c5', device.product_id_for_db)
> +
> +        device = UdevDevice(None, self.usb_device_data)
> +        self.assertEqual('0x0a01', device.product_id_for_db)
> +
> +        device = UdevDevice(
> +            None, self.scsi_scanner_device_data,
> +            self.scsi_scanner_device_sysfs_data)
> +        self.assertEqual('fi-5120Cdj      ', device.product_id_for_db)
> +
> +    def test_driver_name(self):
> +        """Test of UdevDevice.driver_name."""
> +        device = UdevDevice(None, self.pci_device_data)
> +        self.assertEqual('ahci', device.driver_name)
> +
> +        device = UdevDevice(
> +            None, self.root_device, None, self.root_device_dmi_data)
> +        self.assertEqual(None, device.driver_name)
> +
> +    def buildUdevDeviceHierarchy(self, device_data, parser=None):
> +        """Build a UdevDevice hierarchy from device_data.
> +
> +        :param device_data: A sequence of arguments that are passed
> +            to the UdevDevice constructor. Each element must be a
> +            dictionary that can be used as a **kwargs argument.
> +
> +            Element N if the sequence is the parent of element N+1.
> +            is the ancestor of the fo
> +        :param parser: A SubmissionParser instance to be passed to
> +            the constructor of UdevDevice.
> +        """
> +        parent = None
> +        devices = []
> +        for kwargs in device_data:
> +            device = UdevDevice(parser, **kwargs)
> +            devices.append(device)
> +            if parent is not None:
> +                parent.addChild(device)
> +            parent = device
> +        return devices
> +
> +    def test_scsi_controller(self):
> +        """Test of UdevDevice.scsi_controller for a PCI controller."""
> +        devices = self.buildUdevDeviceHierarchy(
> +            self.scsi_device_hierarchy_data)
> +        controller = devices[0]
> +        scsi_device = devices[-1]
> +        self.assertEqual(controller, scsi_device.scsi_controller)
> +
> +    def test_scsi_controller_insufficient_anchestors(self):
> +        """Test of UdevDevice.scsi_controller for a PCI controller.
> +
> +        If a SCSI device does not have a sufficient number of ancestors,
> +        UdevDevice.scsi_controller returns None.
> +        """
> +        parser = SubmissionParser(self.log)
> +        parser.submission_key = 'UdevDevice.scsi_controller ancestor missing'
> +        devices = self.buildUdevDeviceHierarchy(
> +            self.scsi_device_hierarchy_data[1:], parser)
> +        scsi_device = devices[-1]
> +        self.assertEqual(None, scsi_device.scsi_controller)
> +        self.assertWarningMessage(
> +            parser.submission_key,
> +            'Found a SCSI device without a sufficient number of ancestors: '
> +            '/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0/'
> +            'host6/target6:0:1/6:0:1:0')
> +
> +    def test_scsi_controller_no_scsi_device(self):
> +        """Test of UdevDevice.scsi_controller for a PCI controller.
> +
> +        For non-SCSI devices, this property is None.
> +        """
> +        device = UdevDevice(None, self.pci_device_data)
> +        self.assertEqual(None, device.scsi_controller)
> +
> +    def test_translateScsiBus_real_scsi_device(self):
> +        """Test of UdevDevice.translateScsiBus() with a real SCSI device."""
> +        devices = self.buildUdevDeviceHierarchy(
> +            self.scsi_device_hierarchy_data)
> +        scsi_device = devices[-1]
> +        self.assertEqual(
> +            HWBus.SCSI, scsi_device.translateScsiBus())
> +
> +    def test_translateScsiBus_ide_device(self):
> +        """Test of UdevDevice.translateScsiBus() with an IDE device."""
> +        devices = self.buildUdevDeviceHierarchy(
> +            self.ide_device_hierarchy_data)
> +        ide_device = devices[-1]
> +        self.assertEqual(HWBus.IDE, ide_device.translateScsiBus())
> +
> +    def test_translateScsiBus_usb_device(self):
> +        """Test of UdevDevice.translateScsiBus() with a USB device."""
> +        devices = self.buildUdevDeviceHierarchy(
> +            self.usb_storage_hierarchy_data)
> +        usb_scsi_device = devices[-1]
> +        self.assertEqual(None, usb_scsi_device.translateScsiBus())
> +
> +    def test_translateScsiBus_non_scsi_device(self):
> +        """Test of UdevDevice.translateScsiBus() for a non-SCSI device."""
> +        device = UdevDevice(None, self.root_device)
> +        self.assertEqual(None, device.translateScsiBus())
> +

Wow - great work on the test front there! It's great to see a branch where
3/4 of the diff is tests ensuring the code continues to work! One thing
that might help the readability for people like me who are not familiar
with HWDB stuff, would be to make it obvious why you are always choosing
the last item in the heirarchy (devices[-1]) for the scsi/usb device? A
comment would do, but then you'd need the comment in each test. Perhaps
defining:

getDeviceFromHierarchy(devices) - where you could have a comment explaining
why you're simply returning the last device in the heirarchy?

Great work!

>  
>  class TestHWDBSubmissionTablePopulation(TestCaseHWDB):
>      """Tests of the HWDB popoluation with submitted data."""

-- 
Michael