Merge into trunk : bug1210260 : Code : Autopilot

Reviewer	Review Type	Date Requested	Status
Thomi Richards (community)			Needs Fixing on 2014-03-19
PS Jenkins bot	continuous-integration	2014-02-26	Needs Fixing on 2014-03-13
Max Brustkern (community)			Needs Resubmitting on 2014-03-07
Review via email: mp+208260@code.staging.launchpad.net

Revision history for this message

Thomi Richards (thomir-deactivatedaccount) wrote on 2014-02-26:

#

Resubmitted to get a better diff, since trunk has moved on since the MP was created.

Revision history for this message

Thomi Richards (thomir-deactivatedaccount) wrote on 2014-02-26:

#

Download full text (7.4 KiB)

Hi Max,

I assume you want a review on this, since you created this MP :)

Here goes:

12 + class_type = _select_emulator(_object_registry[self._id], path, state)

let's not call this '_select_emulator'. How about '_get_proxy_object_class'. While we're changing this line, let's improve the 'class_type' variable name - it's not a class type, it's a class. (the class type would be the metaclass). We can't use 'class' since it's a reserved keyword, so how about calling it 'class_object'?

22 + p<F7>7ath, state = dbus_tuple

ummm... O.0

31 + @classmethod
32 + def validate_dbus_object(cls, path, state):
33 + name = get_classname_from_path(path)
34 + return cls.__name__ == name

This needs a docstring. The docstring needs to describe:
- What this method is for.
- How users can override it.
- What the default implementation does.

43 +def _select_emulator(objects, path, state):

Please change the parameter 'objects' to have a mode descriptive name. 'proxy_class_dictionary' or similar. 'objects' makes it sound like a list or tuple, when it's not.

44 + class_type = None

Same comment as above. I realise that this is all my fault :)

45 + for item_name in objects:
46 + item = objects[item_name]

Two things here. First, let's change thos evariable names to be a bit more informative. Second, use the dictionaries 'items' method if you want to get both keys and values (for k,v in my_dict.items():). However, in this case you only care about the values, so this should work:

for proxy_class in objects.values():

(note: I haven't changed the 'items' name from the previous point. I'm sure you can cope :))

On a further note, this entire function can be written in two lines:

possible_classes = [ c for c in objects.values() if c.validate_dbus_object(path, state) ]
if len(possible_classes) > 1:
raise ValueError(...)
return possible_classes[0] if possible_classes else None

On a final note for this function, your exception needs more information. If we have more than one proxy class in this situation, we need to be able to debug that. A nice error message should include the classes that are applicable... so perhaps something like:

raise ValueError(
  "More than one custom proxy class matches this state. Possible classes are %s. State is %s. Path is %s" % (
    ','.join([repr(c) for c in possible_classes]),
    repr(state),
    path
  )
)

Also note that the error message really shouldn't use the word 'emulator' in it :)

63 +

In general, please avoid adding whitespace to diffs unless you have to.

109 + def test_class_has_validation_method(self):
110 + self.assertThat(callable(self.Rectangle.validate_dbus_object),
111 + Equals(True))

I don't think it's a good idea to assign 'Rectangle' to the class instance. Instead, nest them in the test class. Also, 'Rectangle' and 'Circle' names don't tell us anything about the classes, so let's use better names. Something like this should work:

class MakeIntrospectionObjectTests(TestCase):

class DefaultSelector(CustomEmulatorBase):
pass

  class AlwaysSelected(CustomEmulatorBase):
    @classmethod
    def validate_dbus_object(...): return True

class NeverSelected(CustomEmulatorBase):
@classmethod
...

Hi Max,

I assume you want a review on this, since you created this MP :)

Here goes:

12	+ class_type = _select_emulator(_object_registry[self._id], path, state)

let's not call this '_select_emulator'. How about '_get_proxy_object_class'. While we're changing this line, let's improve the 'class_type' variable name - it's not a class type, it's a class. (the class type would be the metaclass). We can't use 'class' since it's a reserved keyword, so how about calling it 'class_object'?

22	+ p<F7>7ath, state = dbus_tuple

ummm... O.0

31	+ @classmethod
32	+ def validate_dbus_object(cls, path, state):
33	+ name = get_classname_from_path(path)
34	+ return cls.__name__ == name

This needs a docstring. The docstring needs to describe:
 - What this method is for.
 - How users can override it.
 - What the default implementation does.

43	+def _select_emulator(objects, path, state):

Please change the parameter 'objects' to have a mode descriptive name. 'proxy_class_dictionary' or similar. 'objects' makes it sound like a list or tuple, when it's not.

44	+ class_type = None

Same comment as above. I realise that this is all my fault :)

45	+ for item_name in objects:
46	+ item = objects[item_name]

Two things here. First, let's change thos evariable names to be a bit more informative. Second, use the dictionaries 'items' method if you want to get both keys and values (for k,v in my_dict.items():). However, in this case you only care about the values, so this should work:

for proxy_class in objects.values():

(note: I haven't changed the 'items' name from the previous point. I'm sure you can cope :))

On a further note, this entire function can be written in two lines:

possible_classes = [ c for c in objects.values() if c.validate_dbus_object(path, state) ]
if len(possible_classes) > 1:
    raise ValueError(...)
return possible_classes[0] if possible_classes else None

On a final note for this function, your exception needs more information. If we have more than one proxy class in this situation, we need to be able to debug that. A nice error message should include the classes that are applicable... so perhaps something like:

raise ValueError(
  "More than one custom proxy class matches this state. Possible classes are %s. State is %s. Path is %s" % (
    ','.join([repr(c) for c in possible_classes]),
    repr(state),
    path
  )
)

Also note that the error message really shouldn't use the word 'emulator' in it :)

63	+

In general, please avoid adding whitespace to diffs unless you have to.

109	+ def test_class_has_validation_method(self):
110	+ self.assertThat(callable(self.Rectangle.validate_dbus_object),
111	+ Equals(True))

I don't think it's a good idea to assign 'Rectangle' to the class instance. Instead, nest them in the test class. Also, 'Rectangle' and 'Circle' names don't tell us anything about the classes, so let's use better names. Something like this should work:

class MakeIntrospectionObjectTests(TestCase):

class DefaultSelector(CustomEmulatorBase):
    pass

class AlwaysSelected(CustomEmulatorBase):
    @classmethod
    def validate_dbus_object(...): return True

class NeverSelected(CustomEmulatorBase):
    @classmethod
    def validate_dbus_object(...): return False

def test_class_has_validation_method(self):
    self.assertTrue(callable(DefaultSelector.validate_dbus_object))

*Note: The indentation is kind of messed up in the above - that's because writing code in launchpad sucks. I'm not suggesting you copy the indentation from the above example, just the idea.

113	+ def test_select_by_name(self):
114	+ """Correct custom emulator must be returned by name."""
115	+ fake_object = self.Rectangle(
116	+ dict(id=[0, 123], path=[0, '/some/path']),
117	+ '/',
118	+ Mock()
119	+ )
120	+
121	+ rectangle = fake_object.make_introspection_object(
122	+ ('/Rectangle', {'id': [0, 0]}))
123	+ self.assertThat(isinstance(rectangle, self.Rectangle),
124	+ Equals(True))

This can be simplified:

def test_default_validate_dbus_object_matches_on_class_name(self):
  selected = DefaultSelector.validate_dbus_object(
    '/path/to/DefaultSelector',
    {}
  )
  self.assertTrue(selected)

The simplified version is a unit test, since it can really only fail in a single way.

THat's all the notes I have on the LP diff. However, I'd like you to take this refactor a bit further. Currently, in your branch, the make_introspection_object method looks like this:

def make_introspection_object(self, dbus_tuple):
        """Make an introspection object given a DBus tuple of
        (path, state_dict).

This only works for classes that derive from DBusIntrospectionObject.
        """
        path, state = dbus_tuple
        class_type = _select_emulator(_object_registry[self._id], path, state)
        if class_type is None:
            name = get_classname_from_path(path)
            get_debug_logger().warning(
                "Generating introspection instance for type '%s' based on "
                "generic class.", name)
            # we want the object to inherit from the custom emulator base, not
            # the object class that is doing the selecting
            for base in type(self).__bases__:
                if issubclass(base, CustomEmulatorBase):
                    base_class = base
                    break
            else:
                base_class = type(self)
            class_type = type(str(name), (base_class,), {})
        return class_type(state, path, self._backend)

But I'd like it to look like this instead:

def make_introspection_object(self, dbus_tuple):
        """Make an introspection object given a DBus tuple of
        (path, state_dict).

This only works for classes that derive from DBusIntrospectionObject.
        """
        path, state = dbus_tuple
        class_object = _get_proxy_object_class(_object_registry[self._id], path, state)
        return class_object(state, path, self._backend)

Doing this means that there's no flow control logic in the (hard to test) class method, and all the interesting bits are in the (easy to test) functions. Once you push this out to the _get_proxy_object_class function though, that function will start to look ugly, so you'll want to refacor that like so:

def _get_proxy_object_class(proxy_class_dict, path, state):
    custom_proxy_class = _try_find_custom_proxy_class(proxy_class_dict, path, state)
    if custom_proxy_class is None:
        custom_proxy_class = _make_generic_proxy_class(...)
    return custom_proxy_class

This function is now easy to unit test - we can patch out one, or both of the functions it calls to get total test coverage. In order to do this, you'll need to pass in 'self' from 'make_introspection_object' as well, since that's needed by the code that makes a generic class.

That seems like a reasonable point to stop. Ideally we'd go further. A lot of the pain in this code is because there's three concepts that are not very well abstracted from each other:
 * The proxy class itself.
 * The object registry.
 * The code that handles talking to the dbus backend.

Maybe in the future, if you're up for a real challenge we can talk about teasing these apart and making this code nicer to work with.

Finally, this is a rather picky review. I think it's worth making the changes though, since this is in the very core of autopilot, and we're going to need to build on top of this as well, so whatever we do now will affect is down the line.

Let me know if you want to talk about this some more.

Cheers,

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-02-26:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/autopilot-ci/527/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Thomi Richards (thomir-deactivatedaccount) wrote on 2014-02-26:

#

Please set this back to 'needs review' when you'd like another review.

Thanks

Revision history for this message

Max Brustkern (nuclearbob) wrote on 2014-02-26:

#

Should be cleaned up now per Thomi's review.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-02-26:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/autopilot-ci/532/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Thomi Richards (thomir-deactivatedaccount) wrote on 2014-03-02:

#

Download full text (6.7 KiB)

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

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,

review: Needs Fixing

Revision history for this message

Max Brustkern (nuclearbob) wrote on 2014-03-04:

#

More updates that should address the latest comments, I think.

review: Needs Resubmitting

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-04:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/autopilot-ci/544/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-05:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/autopilot-ci/545/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Thomi Richards (thomir-deactivatedaccount) wrote on 2014-03-07:

#

Hi,

This looks great - with one teensy exception - please change these:

:raises: ValueError if more than one class in the dict matches

to be:

:raises ValueError: if more than one class in the dict matches

i.e.- as we discussed in the PEP257 thread.

After that's done, I'm happy to merge it.

Cheers

Revision history for this message

Max Brustkern (nuclearbob) wrote on 2014-03-07:

#

Should be taken care of now.

review: Needs Resubmitting

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-13:

#

FAILED: Continuous integration, rev:465
http://jenkins.qa.ubuntu.com/job/autopilot-ci/574/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/autopilot-trusty-amd64-ci/300/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/autopilot-trusty-armhf-ci/300/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/autopilot-trusty-i386-ci/209/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty-autopilot/26/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/3923/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/autopilot-ci/574/rebuild

review: Needs Fixing (continuous-integration)

lp://staging/~nuclearbob/autopilot/bug1210260 updated on 2014-03-13

466. By Max Brustkern on 2014-03-13: Merged trunk

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-13:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/autopilot-ci/576/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Thomi Richards (thomir-deactivatedaccount) wrote on 2014-03-19:

#

LGTM

review: Needs Fixing

Autopilot

Merge lp://staging/~nuclearbob/autopilot/bug1210260 into lp://staging/autopilot

Commit message

Description of the change

Preview Diff

Subscribers