Code review comment for lp://staging/~nuclearbob/autopilot/bug1210260

Hi,

This is looking much better, thanks for your work. There are still a few issues though.

10 + :rtype: instance of appropriate CustomProxyObject class

You seem to like using the :rtype: field. In general, we haven't used this before in autopilot, so unless there's a good reason to, I'd prefer to encode this information in the :returns: field. For example, this:

9 + :returns: introspection object
10 + :rtype: instance of appropriate CustomProxyObject class

...could more simply be written as:

:returns: A proxy object that derives from DBusIntrospectionObject

...or something similar. If there's a good reason to use :rtype: that I've missed, please let me know.

51 + :type path: string

Same here. This:

50 + :param path: The dbus path of the object to check
51 + :type path: string

should just be:

:param path: A string containing the path of the object to check.

Finally, in general we don't insist on docstrings for private methods / functions, unless their intent is hard to divine from the function / class name (however, if tht's the case then the name probably needs to be changed). I'm not saying *don't* document them, just htat you don't need to go to so much effort to document them. Often a single line docstring will suffice.

Now, on to the tests:

    @patch('autopilot.introspection.dbus._try_custom_proxy_classes')
    @patch('autopilot.introspection.dbus._get_default_proxy_class')
    def test_get_proxy_object_class_from_list(self, gdpc, tcpc):
        """_get_proxy_object_class should return the value of
        _try_custom_proxy_classes if there is one."""
        gdpc.side_effect = Exception(
            'Called _get_default_proxy_class when _try_custom_proxy_classes '
            'returned a class.')
        retval = 'return'
        tcpc.return_value = retval
        class_dict = {'DefaultSelector': self.DefaultSelector}
        default = self.DefaultSelector
        path = '/path/to/DefaultSelector'
        state = {}
        gpoc_return = _get_proxy_object_class(class_dict,
                                              default,
                                              path,
                                              state)
        self.assertThat(gpoc_return, Equals(retval))
        tcpc.assert_called_once_with(class_dict, path, state)

A few things:

First, don't use the side_effects attribute on the mock to indicate a failure. First, it will produce an error, not a failure, and second, it's not particularly expressive.

Second, when you just want some placeholder text, it's better to use 'self.getUniqueString()', since it doesn't add a literal to the test that distracts from the test intent.

Third, since this is a unit test, this should fail in exactly one way. We often ignore that rule, but in this case, obeying it helps make this test a lot clearer. The intent of this test is that the '_get_proxy_object_class' should call '_try_custom_proxy_classes' first, not not call anything else if it succeeds. We can pair down the test until it's just testing that:

Here's how I'd do it:

    @patch('autopilot.introspection.dbus._try_custom_proxy_classes')
    @patch('autopilot.introspection.dbus._get_default_proxy_class')
    def test_get_proxy_object_class_from_list(self, gdpc, tcpc):
        """_get_proxy_object_class should return the value of
        _try_custom_proxy_classes if there is one."""
        token = self.getUniqueString()
        tcpc.return_value = token
        gpoc_return = _get_proxy_object_class(None, None, None, None)

        self.assertThat(gpoc_return, Equals(token))
        self.assertThat(gdpc.call_count, Equals(0))

Note that we don't care about the actual arguments, and we don't check that those arguments are passed through to the correct class - that's for a second test:

    @patch('autopilot.introspection.dbus._try_custom_proxy_classes')
    @patch('autopilot.introspection.dbus._get_default_proxy_class')
    def test_get_proxy_object_class_passes_correct_args(self, gdpc, tcpc):
        class_dict = {'DefaultSelector': self.DefaultSelector}
        default = self.DefaultSelector
        path = '/path/to/DefaultSelector'
        state = {}
        _get_proxy_object_class(class_dict, default, path, state)
        tcpc.assert_called_once_with(class_dict, path, state)

Note how, by making each test really only test one thing, the tests are simpler. In both these tests, we can throw away a bunch of lines of code. So, when thinking about the tests required for _get_proxy_object_class, I think you need to cover the following scenarios:

 * Test that when _try_custom_proxy_classes returns something, it is returned.
 * Test that the correct arguments are passed to _try_custom_proxy_classes
 * Test that when _try_custom_proxy_classes retuns None, _get_default_proxy_class is called.
 * That that when _try_custom_proxy_classes retuns None, the correct arguments are passed to _get_default_proxy_class.
 * That that when _try_custom_proxy_classes retuns None, the return value of _get_default_proxy_class is returned.
 * Test that when _get_default_proxy_class raises a ValueError, the error is not handled.

That last one is important - you need to make sure that the fact that make_introspection_object (and all methods that call it) can now raise a ValueError is documented. I suspect this means you'll need to update a bunch of docstrings in the DBusIntrospectionObject class methods.

So, for _get_default_proxy_class, there's at least the following tests required:

 * Test that _get_default_proxy_class logs a sensible error at the correct level.
 * Test that _get_default_proxy_class uses a class from within the for loop.
 * Test that _get_default_proxy_class uses default_class when the for loop is exhausted.
 * Test that the returned class object has the name from the 'name' parameter.
 * Test that the returned class object has the correct base class.

The _try_custom_proxy_classes function has a bunch of tests required as well. You need to cover the case where no classes are found, where one class is found, and where many classes are found.

A final note on formatting: When you need to wrap a function over many lines, like this:

gpoc_return = _get_proxy_object_class(class_dict,
                                      default,
                                      path,
                                      state)

Please use this style instead:

gpoc_return = _get_proxy_object_class(
    class_dict,
    default,
    path,
    state
)

(opening and closing parenthisis by themselves, each arg tabbed in one level, closing paren level with function call line).

Both styles are PEP8 compliant, but the style adopted within autopilot is the latter, and I find it more readable - it also allows you more space for long arguments :)

Thanks again for the work on this - Let me know once the tests are cleaned up and we can merge it!

Cheers,