Autopilot

Merge lp:~canonical-platform-qa/autopilot/cpo-base-classes-fix into lp:autopilot

cpo-base-classes-fix
Merge into trunk

Proposed by Federico Gimenez on 2015-04-28

Status:	Merged
Approved by:	Christopher Lee on 2015-05-08
Approved revision:	578
Merged at revision:	554
Proposed branch:	lp:~canonical-platform-qa/autopilot/cpo-base-classes-fix
Merge into:	lp:autopilot
Diff against target:	590 lines (+399/-24) 9 files modified apparmor/click.rules (+0/-12) autopilot/introspection/_object_registry.py (+68/-6) autopilot/introspection/_search.py (+35/-0) autopilot/tests/functional/test_introspection_features.py (+47/-2) autopilot/tests/unit/test_introspection_object_registry.py (+147/-1) autopilot/tests/unit/test_introspection_search.py (+94/-0) debian/autopilot-touch.dirs (+0/-1) debian/autopilot-touch.install (+0/-1) debian/changelog (+8/-1)
To merge this branch:	bzr merge lp:~canonical-platform-qa/autopilot/cpo-base-classes-fix
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	continuous-integration		Approve on 2015-05-07
Christopher Lee (community)		2015-04-28	Needs Information on 2015-05-06
Review via email: mp+257623@code.launchpad.net

Commit Message

CPO base classes fix

Description of the Change

CPO base classes fix

Christopher Lee (veebers) wrote on 2015-04-29:

Hi Federico, you'll see that I added some tests but haven't filled them in.

While back-filling the tests but stopped short of actually implementing them; mainly because the way you've made the fix is different to what I originally had and my initial thoughts wouldn't work :-)

Could you take a look at writing the tests? My current concern with the current implementation is that _combine_base_and_extensions has to know about class stuff (__bases__ and mro) where as the previous method only dealt with tuples of information.
Not only does this mean that it didn't need to know more than 'merge these tuples' it was easy to write tests for (again, just dealing with tuples if data, could be strings etc.).

Also, without a test in place I'm not sure that the sorting of the mro is the correct thing to do.
While I didn't have a test in place to prove it, the intention of the purely tuple approach is that it kept the expected order of merging 2 bases.

Federico Gimenez (fgimenez) wrote on 2015-04-29:

Hi Chris, yes, I totally agree that adding the MRO stuff to the combine method is changing its initial behaviour. I added it after trying to make the current tests pass, I've commented in the code trying to make it clear.

Using the combine method to put together arbitrary tuples was giving at first an error about a cyclic dependency, one of the elements of the tuple was the class itself.

TypeError: a __bases__ item causes an inheritance cycle

This can be solved by passing the original class itself to the method, and checking that it is not a member of the collections being merged. At this point, we could continue treating the combine method as an abstract information merger. But after this, a second error arose:

TypeError: Cannot create a consistent method resolution order (MRO)

So that, to combine them appropriately, the method has to know which is the right order, and this information can be obtained from the classes MROs.

Unless I'm missing something it seems that the combine method has to know that it is combining classes, moreover it needs to know that the return value is going to be the bases tuple of one of the parameters. IMO the responsibility is well delimited, maybe we could split it.

Anyway, I'll try to implement the tests that you mention, thanks a lot Chris!

Federico Gimenez (fgimenez) wrote on 2015-04-29:

I've already added the tests for the current implementation of _combine_base_and_extensions.

While writing them I've noticed that when two classes had the same mro the order wasn't deterministic, I've added another function for having a more fine-grained control. Currently the classes in the original __bases__ tuple have lower index than the extensions in the resulting tuple, this can be changed.

Thanks,

Christopher Lee (veebers) wrote on 2015-04-30:

Looking good guys, just a couple of comments inline.

As per the changes I made to day; I was thinking about how to handle detecting and choosing the correct base class when there was multiple inheritance involved (i.e. multiple CPO inheritance).

I came to the conclusion that it is bad form to change things under the cover in this fashion as it can be misleading and cause unexpected changes.
Instead I've opted for issuing a warning when this is detected with the option to upgrade this to raising an error in the new future (i.e. when we can be certain we don't break everyone with this new enforcement rule.)

I was of 2 minds making it raise an exception straight off, but I feel that it was to heavy handed at this stage.
What do you guys think?

I've done some small scale testing locally and the address book and unity8 tests pass as expected. I would love to see this go through the gatekeeper to see the results there.

Federico Gimenez (fgimenez) wrote on 2015-04-30:

Thanks Chris, I've addressed the issues you mentioned, putting this in review

PS Jenkins bot (ps-jenkins) wrote on 2015-04-30:

FAILED: Continuous integration, rev:569
http://jenkins.qa.ubuntu.com/job/autopilot-ci/1048/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/autopilot-vivid-amd64-ci/111/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/autopilot-vivid-armhf-ci/111/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/autopilot-vivid-i386-ci/115/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-vivid-autopilot/163/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-amd64/1011/console

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

review: Needs Fixing (continuous-integration)

lp:~canonical-platform-qa/autopilot/cpo-base-classes-fix updated on 2015-04-30

570. By Federico Gimenez on 2015-04-30: Fixed flake8

Vincent Ladeuil (vila) wrote on 2015-04-30:

> Looking good guys, just a couple of comments inline.
>
> As per the changes I made to day; I was thinking about how to handle detecting
> and choosing the correct base class when there was multiple inheritance
> involved (i.e. multiple CPO inheritance).
>
> I came to the conclusion that it is bad form to change things under the cover
> in this fashion as it can be misleading and cause unexpected changes.

/me nods

> Instead I've opted for issuing a warning when this is detected with the option
> to upgrade this to raising an error in the new future (i.e. when we can be
> certain we don't break everyone with this new enforcement rule.)

I would prefer raising an error than fixing the base class too.

Misuse of the CPO is a bug, clients should be fixed.

Especially when looking at Leo's branches fixing this issue in several clients, the fix seems light enough that it's worth breaking the clients so they are fixed asap.

A warning may be missed and the client test suites will not be fixed until it's noticed.

>
> I was of 2 minds making it raise an exception straight off, but I feel that it
> was to heavy handed at this stage.
> What do you guys think?

Let's collect some test results to get a better understanding ;)

>
> I've done some small scale testing locally and the address book and unity8
> tests pass as expected. I would love to see this go through the gatekeeper to
> see the results there.

Looking at http://q-jenkins.ubuntu-ci:8080/job/autopilot-release-gatekeeper it seems we can pass a PPA parameter (it doesn't have to be associated with a silo) so it should be possible to test the two approaches (warning/error) by building in 2 different PPAs and get an idea of the fallouts for both.

PS Jenkins bot (ps-jenkins) wrote on 2015-04-30:

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

review: Needs Fixing (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2015-04-30:

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

review: Needs Fixing (continuous-integration)

lp:~canonical-platform-qa/autopilot/cpo-base-classes-fix updated on 2015-04-30

571. By Federico Gimenez on 2015-04-30: More on comment about the classes ordering

PS Jenkins bot (ps-jenkins) wrote on 2015-04-30:

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

review: Needs Fixing (continuous-integration)

lp:~canonical-platform-qa/autopilot/cpo-base-classes-fix updated on 2015-05-01

572. By Federico Gimenez on 2015-05-01: merged vila's branch with packaging fix

PS Jenkins bot (ps-jenkins) wrote on 2015-05-01:

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

review: Needs Fixing (continuous-integration)

lp:~canonical-platform-qa/autopilot/cpo-base-classes-fix updated on 2015-05-04

573. By Federico Gimenez on 2015-05-04: Removing validate_dbus_object method in the extesnsions before adding to the bases to preserve the original method resolution
574. By Federico Gimenez on 2015-05-04: Fixed changelog
575. By Federico Gimenez on 2015-05-04: changelog signature
576. By Federico Gimenez on 2015-05-04: Fixed unit test

Federico Gimenez (fgimenez) wrote on 2015-05-04:

I've uploaded some fixes for errors spotted in the gatekeeper jobs. These were related to the breadth-first nature of method resolution: given one class that doesn't have a validate_dbus_object method of its own and that method is inherited from a non-direct ancestor, when you insert in its bases an extension class with that method defined then that inserted method have preference over the inherited one.

The solution adopted is removing the validate_dbus_object method from the extension classes being added to the bases of the CPOs, so that the correct method resolution is preserved. This has given good results in gatekeeper runs with partial suites, currently there's a complete run being executed (job #271).

PS Jenkins bot (ps-jenkins) wrote on 2015-05-04:

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

review: Needs Fixing (continuous-integration)

Christopher Lee (veebers) wrote on 2015-05-06:

I'm really not a fan of modifying the classes in such a way, perhaps it's indicating that we're going down the wrong path and we need to find a better solution.

That being said the 2 tests that failed due to _try_custom_proxy_classes raising an exception [1] (the 3rd not being related) shows that the warning re: base CPO classes not being passed in correctly.

I'm just trying to get my local devices etc. setup so I can test this, but I think that if we instead change the warning to an error (yes, I was wrong there it should have always been an error :-) ). and use the updated addresss book (i.e. the one that is passing the correct base_emulator in aka Leos branch) we shouldn't need to modify the class attributes like we are here.

I'll try to post the results when I get them, but at this rate it's going to take ages (my device is acting up.)

[1] http://q-jenkins.ubuntu-ci:8080/job/autopilot-release-gatekeeper/263/label=daily-mako/testReport/

review: Needs Information

Christopher Lee (veebers) wrote on 2015-05-06:

Follow up:
I ran the fixed (i.e. Leos base_emulator update branch) and non fixed address book tests, the fixed worked well, the non-fixed exhibited the error we see.

We should remove the additional code that removes the method attribute and change the warning into an error (probably RuntimeError? or ValueError?).

lp:~canonical-platform-qa/autopilot/cpo-base-classes-fix updated on 2015-05-06

577. By Federico Gimenez on 2015-05-06: The validate_dbus_object method is not removed from extensions classes before adding them to cpos; raised exception instead of logging warning when the emulator_base does not match the base extension
578. By Federico Gimenez on 2015-05-06: Updated error message

Federico Gimenez (fgimenez) wrote on 2015-05-06:

>
> We should remove the additional code that removes the method attribute and
> change the warning into an error (probably RuntimeError? or ValueError?).

Done, regarding the exception after talking with vila we have gone for ValueError, it can be changed of course. I've also updated the unit tests accordingly.

Cheers!

PS Jenkins bot (ps-jenkins) wrote on 2015-05-06:

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

review: Needs Fixing (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2015-05-07:

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

review: Approve (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2015-05-07:

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

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Autopilot Hackers

Canonical Platform QA Team

Chris Gagnon

 === removed directory 'apparmor'
 === removed file 'apparmor/click.rules'
 --- apparmor/click.rules	2014-05-23 09:35:07 +0000
 +++ apparmor/click.rules	1970-01-01 00:00:00 +0000
@@ -1,12 +0,0 @@
--dbus (receive, send)
--     bus=session
--     path=/com/canonical/Autopilot/**,
--# Allow writes to various (application-specific) XDG directories
--  owner @{HOME}/autopilot/fakeenv/*/.cache/@{APP_PKGNAME}/ rw, # subdir of XDG_CACHE_HOME
--  owner @{HOME}/autopilot/fakeenv/*/.cache/@{APP_PKGNAME}/** mrwkl,
--  owner @{HOME}/autopilot/fakeenv/*/.config/@{APP_PKGNAME}/ rw, # subdir of XDG_CONFIG_HOME
--  owner @{HOME}/autopilot/fakeenv/*/.config/@{APP_PKGNAME}/** mrwkl,
--  owner @{HOME}/autopilot/fakeenv/*/.local/share/@{APP_PKGNAME}/ rw, # subdir of XDG_DATA_HOME
--  owner @{HOMEDIRS}/*/autopilot/fakeenv/*/.local/share/@{APP_PKGNAME}/** mrwklix,
--  owner @{HOME}/autopilot/fakeenv/*/confined/@{APP_PKGNAME}/ rw, # subdir of XDG_RUNTIME_DIR
--  owner @{HOME}/autopilot/fakeenv/*/confined/@{APP_PKGNAME}/** mrwkl,
 === modified file 'autopilot/introspection/_object_registry.py'
 --- autopilot/introspection/_object_registry.py	2015-02-25 21:51:40 +0000
 +++ autopilot/introspection/_object_registry.py	2015-05-06 10:03:54 +0000
@@ -1,7 +1,7 @@
  # -*- Mode: Python; coding: utf-8; indent-tabs-mode: nil; tab-width: 4 -*-
+ #
  # Autopilot Functional Test Tool
--# Copyright (C) 2014 Canonical
++# Copyright (C) 2014, 2015 Canonical
+ #
  # This program is free software: you can redistribute it and/or modify
  # it under the terms of the GNU General Public License as published by
@@ -134,7 +134,6 @@
      :param object_id: The _id attribute of the class doing the lookup. This is
          used to index into the object registry to retrieve the dict of proxy
          classes to try.
--    :param default_class: default class to use if nothing in dict matches
      :param path: dbus path
      :param state: dbus state
      :returns: appropriate custom proxy class
@@ -153,7 +152,8 @@
      """Identify which custom proxy class matches the dbus path and state.
      If more than one class in proxy_class_dict matches, raise an exception.
--    :param proxy_class_dict: dict of proxy classes to try
++
++    :param object_id: id to use to get the dict of proxy classes to  try
      :param path: dbus path
      :param state: dbus state dict
      :returns: matching custom proxy class
@@ -173,18 +173,80 @@
+             )
+         )
      if len(possible_classes) == 1:
++        extended_proxy_bases = _get_proxy_bases_for_id(object_id)
++        mixed = _combine_base_and_extensions(
++            possible_classes[0],
++            extended_proxy_bases
++        )
++        possible_classes[0].__bases__ = mixed
          return possible_classes[0]
      return None
++def _combine_base_and_extensions(kls, extensions):
++    """Returns the bases of the given class augmented with extensions
++
++    In order to get the right bases tuple, the given class is removed
++    from the result (to prevent cyclic dependencies), there's only one
++    occurrence of each final base class in the result and the result
++    is ordered following the inheritance order (classes lower in the
++    inheritance tree are listed before in the resulting tuple)
++
++    :param kls: class for which we are combining bases and extensions
++    :param extensions: tuple of extensions to be added to kls' bases
++    :returns: bases tuple for kls, including its former bases and the
++             extensions
++
++    """
++    # set of bases + extensions removing the original class to prevent
++    # TypeError: a __bases__ item causes an inheritance cycle
++    unique_bases = {x for x in kls.__bases__ + extensions if x != kls}
++
++    # sort them taking into account inheritance to prevent
++    # TypeError: Cannot create a consistent method resolution order (MRO)
++    return tuple(
++        sorted(
++            unique_bases,
++            key=lambda cls: _get_mro_sort_order(cls, extensions),
++            reverse=True
++        )
++    )
++
++
++def _get_mro_sort_order(cls, promoted_collection=()):
++    """Returns the comparable numerical order for the given class honouring
++    its MRO
++
++    It accepts an optional parameter for promoting classes in a certain
++    group, this can give more control over the sorting when two classes
++    have the a MRO of the same length
++
++    :param cls: the subject class
++    :param promoted_collection: tuple of classes which must be promoted
++    :returns: comparable numerical order, higher for classes with MROs of
++        greater length
++
++    """
++    # Multiplying by 2 the lenght of the MRO list gives the chance to promote
++    # items in the promoted_collection by adding them 1 later: non promoted
++    # classes will have even scores and promoted classes with MRO of the same
++    # length will have odd scores one point higher
++    order = 2 * len(cls.mro())
++
++    if cls in promoted_collection:
++        order += 1
++
++    return order
++
++
  def _get_default_proxy_class(id, name):
      """Return a custom proxy object class of the default or a base class.
      We want the object to inherit from the class that is set as the emulator
--    base class, not the class that is doing the selecting (which will be the
--    'default_class' parameter).
++    base class, not the class that is doing the selecting. Using the passed id
++    we retrieve the relevant bases from the object registry.
--    :param default_class: default class to use if no bases match
++    :param id: The object id (_id attribute) of the class doing the lookup.
      :param name: name of new class
      :returns: custom proxy object class
 === modified file 'autopilot/introspection/_search.py'
 --- autopilot/introspection/_search.py	2015-04-02 08:27:42 +0000
 +++ autopilot/introspection/_search.py	2015-05-06 10:03:54 +0000
@@ -387,12 +387,15 @@
      # make sure we always have an emulator base. Either use the one the user
      # gave us, or make one:
      emulator_base = emulator_base or _make_default_emulator_base()
++    _raise_if_base_class_not_actually_base(emulator_base)
++
      # Get the dbus introspection Xml for the backend.
      intro_xml = _get_introspection_xml_from_backend(dbus_address)
      try:
          # Figure out if the backend has any extension methods, and return
          # classes that understand how to use each of those extensions:
          extension_classes = _get_proxy_bases_from_introspection_xml(intro_xml)
++
          # Register those base classes for everything that will derive from this
          # emulator base class.
          _object_registry.register_extension_classes_for_proxy_base(
@@ -427,6 +430,38 @@
      return type("DefaultEmulatorBase", (ap_dbus.DBusIntrospectionObject,), {})
++WRONG_CPO_CLASS_MSG = '''\
++base_class: {passed} does not appear to be the actual base CPO class.
++Perhaps you meant to use: {actual}.'''
++
++
++def _raise_if_base_class_not_actually_base(base_class):
++    """Raises ValueError if the provided base_class is not actually the
++       base_class
++
++    To ensure that the expected base classes are used when creating proxy
++    objects.
++
++    :param base_class: The base class to check.
++    :raises ValueError: The actual base class is not the one provided
++
++    """
++    actual_base_class = base_class
++    for cls in base_class.mro():
++        if hasattr(cls, '_id'):
++            actual_base_class = cls
++
++    if actual_base_class != base_class:
++        raise(
++            ValueError(
++                WRONG_CPO_CLASS_MSG.format(
++                    passed=base_class,
++                    actual=actual_base_class
++                )
++            )
++        )
++
++
  def _make_proxy_object_async(
          data_source, emulator_base, reply_handler, error_handler):
      """Make a proxy object for a dbus backend.
 === modified file 'autopilot/tests/functional/test_introspection_features.py'
 --- autopilot/tests/functional/test_introspection_features.py	2015-02-26 00:40:54 +0000
 +++ autopilot/tests/functional/test_introspection_features.py	2015-05-06 10:03:54 +0000
@@ -24,7 +24,7 @@
  import subprocess
  import tempfile
  from tempfile import mktemp
--from testtools import skip, skipIf
++from testtools import skipIf
  from testtools.matchers import (
      Contains,
      Equals,
@@ -36,6 +36,7 @@
      StartsWith,
+ )
  from textwrap import dedent
++from unittest.mock import patch
  from io import StringIO
  from autopilot import platform
@@ -43,6 +44,8 @@
  from autopilot.testcase import AutopilotTestCase
  from autopilot.tests.functional.fixtures import TempDesktopFile
  from autopilot.introspection import CustomEmulatorBase
++from autopilot.introspection import _object_registry as object_registry
++from autopilot.introspection import _search
  from autopilot.introspection.qt import QtObjectProxyMixin
  from autopilot.display import Display
@@ -99,7 +102,6 @@
              Equals(WindowMockerApp)
+         )
--    @skip("Currently fails due to lp:1425721 (and lp:1376996)")
      def test_customised_proxy_classes_have_extension_classes(self):
          class WindowMockerApp(EmulatorBase):
              @classmethod
@@ -109,6 +111,49 @@
          app = self.start_mock_app(EmulatorBase)
          self.assertThat(app.__class__.__bases__, Contains(QtObjectProxyMixin))
++    def test_customised_proxy_classes_have_multiple_extension_classes(self):
++        with object_registry.patch_registry({}):
++            class SecondEmulatorBase(CustomEmulatorBase):
++                pass
++
++            class WindowMockerApp(EmulatorBase, SecondEmulatorBase):
++                @classmethod
++                def validate_dbus_object(cls, path, _state):
++                    return path == b'/window-mocker'
++
++            app = self.start_mock_app(EmulatorBase)
++            self.assertThat(app.__class__.__bases__, Contains(EmulatorBase))
++            self.assertThat(
++                app.__class__.__bases__,
++                Contains(SecondEmulatorBase)
++            )
++
++    def test_handles_using_app_cpo_base_class(self):
++        # This test replicates an issue found in an application test suite
++        # where using the App CPO caused an exception.
++        with object_registry.patch_registry({}):
++            class WindowMockerApp(CustomEmulatorBase):
++                @classmethod
++                def validate_dbus_object(cls, path, _state):
++                    return path == b'/window-mocker'
++
++            self.start_mock_app(WindowMockerApp)
++
++    def test_warns_when_using_incorrect_cpo_base_class(self):
++        # Ensure the warning method is called when launching a proxy.
++        with object_registry.patch_registry({}):
++            class TestCPO(CustomEmulatorBase):
++                pass
++
++            class WindowMockerApp(TestCPO):
++                @classmethod
++                def validate_dbus_object(cls, path, _state):
++                    return path == b'/window-mocker'
++
++            with patch.object(_search, 'logger') as p_logger:
++                self.start_mock_app(WindowMockerApp)
++                self.assertTrue(p_logger.warning.called)
++
      def test_can_select_custom_emulators_by_name(self):
          """Must be able to select a custom emulator type by name."""
          class MouseTestWidget(EmulatorBase):
 === modified file 'autopilot/tests/unit/test_introspection_object_registry.py'
 --- autopilot/tests/unit/test_introspection_object_registry.py	2014-07-31 04:42:08 +0000
 +++ autopilot/tests/unit/test_introspection_object_registry.py	2015-05-06 10:03:54 +0000
@@ -1,7 +1,7 @@
  # -*- Mode: Python; coding: utf-8; indent-tabs-mode: nil; tab-width: 4 -*-
+ #
  # Autopilot Functional Test Tool
--# Copyright (C) 2014 Canonical
++# Copyright (C) 2014, 2015 Canonical
+ #
  # This program is free software: you can redistribute it and/or modify
  # it under the terms of the GNU General Public License as published by
@@ -263,3 +263,149 @@
          with object_registry.patch_registry(dict()):
              object_registry._object_registry[token] = token
          self.assertFalse(token in object_registry._object_registry)
++
++
++class CombineBasesTests(TestCase):
++    def test_returns_original_if_no_extensions(self):
++        class Base():
++            pass
++
++        class Sub(Base):
++            pass
++
++        self.assertEqual(
++            object_registry._combine_base_and_extensions(Sub, ()),
++            (Base,)
++        )
++
++    def test_returns_addition_of_extension_classes(self):
++        class Base():
++            pass
++
++        class Sub(Base):
++            pass
++
++        class Ext1():
++            pass
++
++        class Ext2():
++            pass
++
++        mixed = object_registry._combine_base_and_extensions(Sub, (Ext1, Ext2))
++
++        self.assertIn(Ext1, mixed)
++        self.assertIn(Ext2, mixed)
++
++    def test_excludes_duplication_of_base_class_within_extension_classes(self):
++        class Base():
++            pass
++
++        class Sub(Base):
++            pass
++
++        class Ext1():
++            pass
++
++        class Ext2(Ext1):
++            pass
++
++        self.assertEqual(
++            object_registry._combine_base_and_extensions(Sub, (Ext2, Base)),
++            (Ext2, Base,)
++        )
++
++    def test_excludes_addition_of_extension_base_classes(self):
++        class Base():
++            pass
++
++        class Sub(Base):
++            pass
++
++        class Ext1():
++            pass
++
++        class Ext2(Ext1):
++            pass
++
++        self.assertNotIn(
++            object_registry._combine_base_and_extensions(Sub, (Ext2,)),
++            Ext1
++        )
++
++    def test_excludes_addition_of_subject_class(self):
++        class Base():
++            pass
++
++        class Sub(Base):
++            pass
++
++        class Ext1():
++            pass
++
++        self.assertEqual(
++            object_registry._combine_base_and_extensions(Sub, (Ext1, Sub)),
++            (Ext1, Base)
++        )
++
++    def test_maintains_mro_order(self):
++        class Base():
++            pass
++
++        class Sub1(Base):
++            pass
++
++        class Sub2(Sub1, Base):
++            pass
++
++        class Ext1():
++            pass
++
++        mixed = object_registry._combine_base_and_extensions(Sub2, (Ext1,))
++
++        self.assertLess(
++            mixed.index(Sub1),
++            mixed.index(Base)
++        )
++
++
++class MROSortOrderTests(TestCase):
++    def test_returns_lower_values_for_lower_classes_in_mro(self):
++        class Parent():
++            pass
++
++        class Child(Parent):
++            pass
++
++        class GrandChild(Child):
++            pass
++
++        self.assertGreater(
++            object_registry._get_mro_sort_order(Child),
++            object_registry._get_mro_sort_order(Parent),
++        )
++        self.assertGreater(
++            object_registry._get_mro_sort_order(GrandChild),
++            object_registry._get_mro_sort_order(Child),
++        )
++
++    def test_return_higher_values_for_promoted_collections_in_ties(self):
++        class Parent1():
++            pass
++
++        class Child1(Parent1):
++            pass
++
++        class Parent2():
++            pass
++
++        class Child2(Parent2):
++            pass
++
++        promoted_collection = (Parent1, Child1)
++
++        self.assertGreater(
++            object_registry._get_mro_sort_order(
++                Child1, promoted_collection),
++            object_registry._get_mro_sort_order(
++                Child2, promoted_collection),
++        )
 === modified file 'autopilot/tests/unit/test_introspection_search.py'
 --- autopilot/tests/unit/test_introspection_search.py	2015-04-02 08:27:42 +0000
 +++ autopilot/tests/unit/test_introspection_search.py	2015-05-06 10:03:54 +0000
@@ -34,6 +34,8 @@
  from autopilot.exceptions import ProcessSearchError
  from autopilot.utilities import sleep
  from autopilot.introspection import _search as _s
++
++from autopilot.introspection import CustomEmulatorBase
  from autopilot.introspection.constants import AUTOPILOT_PATH
@@ -730,3 +732,95 @@
                  _s._find_matching_connections(bus, lambda *args: True)
                  dedupe.assert_called_once_with(["conn1"], bus)
++
++
++class ActualBaseClassTests(TestCase):
++
++    def test_dont_raise_passed_base_when_is_only_base(self):
++        class ActualBase(CustomEmulatorBase):
++            pass
++
++        try:
++            _s._raise_if_base_class_not_actually_base(ActualBase)
++        except ValueError:
++            self.fail('Unexpected ValueError exception')
++
++    def test_raises_if_passed_incorrect_base_class(self):
++        class ActualBase(CustomEmulatorBase):
++            pass
++
++        class InheritedCPO(ActualBase):
++            pass
++
++        self.assertRaises(
++            ValueError,
++            _s._raise_if_base_class_not_actually_base,
++            InheritedCPO
++        )
++
++    def test_raises_parent_with_simple_non_ap_multi_inheritance(self):
++        """When mixing in non-customproxy classes must return the base."""
++
++        class ActualBase(CustomEmulatorBase):
++            pass
++
++        class InheritedCPO(ActualBase):
++            pass
++
++        class TrickyOne(object):
++            pass
++
++        class FinalForm(InheritedCPO, TrickyOne):
++            pass
++
++        self.assertRaises(
++            ValueError,
++            _s._raise_if_base_class_not_actually_base,
++            FinalForm
++        )
++
++    def test_raises_parent_with_non_ap_multi_inheritance(self):
++
++        class ActualBase(CustomEmulatorBase):
++            pass
++
++        class InheritedCPO(ActualBase):
++            pass
++
++        class TrickyOne(object):
++            pass
++
++        class FinalForm(TrickyOne, InheritedCPO):
++            pass
++
++        self.assertRaises(
++            ValueError,
++            _s._raise_if_base_class_not_actually_base,
++            FinalForm
++        )
++
++    def test_dont_raise_when_using_default_emulator_base(self):
++        # _make_proxy_object potentially creates a default base.
++        DefaultBase = _s._make_default_emulator_base()
++        try:
++            _s._raise_if_base_class_not_actually_base(DefaultBase)
++        except ValueError:
++            self.fail('Unexpected ValueError exception')
++
++    def test_exception_message_contains_useful_information(self):
++        class ActualBase(CustomEmulatorBase):
++            pass
++
++        class InheritedCPO(ActualBase):
++            pass
++
++        try:
++            _s._raise_if_base_class_not_actually_base(InheritedCPO)
++        except ValueError as err:
++            self.assertEqual(
++                str(err),
++                _s.WRONG_CPO_CLASS_MSG.format(
++                    passed=InheritedCPO,
++                    actual=ActualBase
++                )
++            )
 === removed file 'debian/autopilot-touch.dirs'
 --- debian/autopilot-touch.dirs	2013-09-20 19:06:30 +0000
 +++ debian/autopilot-touch.dirs	1970-01-01 00:00:00 +0000
@@ -1,1 +0,0 @@
--usr/share/autopilot-touch/apparmor
 === removed file 'debian/autopilot-touch.install'
 --- debian/autopilot-touch.install	2013-09-20 19:06:30 +0000
 +++ debian/autopilot-touch.install	1970-01-01 00:00:00 +0000
@@ -1,1 +0,0 @@
--apparmor/click.rules usr/share/autopilot-touch/apparmor
 === modified file 'debian/changelog'
 --- debian/changelog	2015-02-26 21:20:18 +0000
 +++ debian/changelog	2015-05-06 10:03:54 +0000
@@ -1,12 +1,19 @@
  autopilot (1.5.0) UNRELEASED; urgency=medium
++  [ Christopher Lee ]
    * Fix for desktop file name change (lp:1411096)
      Fix for config value containing '=' char (lp:1408317)
      Fix for skipped tests not appearing in log (lp:1414632)
      Add json docs build (for web publish) (lp:1409778)
      Documentation improvements for API, Tutorial, FAQ, and Guidelines
-- -- Christopher Lee <chris.lee@canonical.com>  Fri, 27 Feb 2015 10:16:42 +1300
++  [ Vincent Ladeuil ]
++  * Remove apparmor/click.rules which is now shipped by dbus-property-service.
++
++  [ Federico Gimenez ]
++  * Fixed CPO class resolution (lp:1425721, lp:1376996)
++
++ -- Federico Gimenez <fgimenez@canonical.com>  mon, 4 May 2015 12:12:42 +0200
  autopilot (1.5.0+15.04.20141031-0ubuntu1) vivid; urgency=low
 === modified file 'debian/rules' (properties changed: -x to +x)