Merge lp:~canonical-platform-qa/autopilot/fix-cpo-having-different-name-1337004 into lp:autopilot

[Christopher Lee]

Allow Custom Proxy Object classes to have a different name to that of the underlying UI Type

Allow Custom Proxy Object classes to have a different name to that of the underlying UI Type (I.e. having a RedRect CPO for a QQuickRectangle).

[Thomi Richards (thomir-deactivatedaccount)]

Hi,

You asked for my feedback, so.... brace yourself :D

First, I think this will solve your immediate problem, so don't take anything else I write here as an immediate disapproval. However, I think you're painting yourself into a corner API-design-wise, and it can be avoided.

Let's consider the 60k ft view:

* Test authors want to select one or more objects from the object tree.
* From a data flow POV, that looks like:

 test query (in python)
   -> xpathselect query string (over dbus)
   -> libxpathselect
   -> query data structure (over dbus)
   -> proxy objects (in python)

Currently we give test authors a way to filter what proxy objects get created from the query data structure (via validate_dbus_object), and the long-standing bug you're trying to solve is that the API for generating the query is very limiting on the client-side.

Your solution below is to allow test authors to override the class name generated in the source query.

I think that doesn't go far enough.

Since autopilot 1.5 was released, we have a pretty good encapsulation of the xpathselect query protocol. Why not make the tweaks necessary so that test authors can:

 - create any valid xpathselect query object
 - filter the results as they come back
 - tell autopilot to create proxies with any given class or class mixin

The big, fundamental mistake we made in autopilot was to conflate "a connection to an application" with "a set of classes that can be used to represent application internals". I think that separating those wouldn't be too hard.

The specific issue I see with this merge proposal is, it'll only be a matter of time before Leo (sorry Leo, I'm totally picking on you here) says "I want to write a test where I only select QQuickRectangles that have property X set to Z, and filtering them on the client side is too costly". That's a valid use case, and the only way you'll be able to support that is by further extending an already bloated API.

I suggest you think about how you can separate the concerns mentioned above, and start writing a new query API, while leaving the old API in place for a release to give people time to migrate (hell, you can probably make it 99% backwards compatible).

Let me know if anything doesn't make sense. I'm not subscribed to this MP, so if you reply, you'll need to ping me on IRC...

[lp:~canonical-platform-qa/autopilot/fix-cpo-having-different-name-1337004]

562. By Christopher Lee on 2015-06-16

Remove errounous test fix pre-req.

563. By Christopher Lee on 2015-06-23

Pyflakes fix

564. By Christopher Lee on 2015-07-19

Merge trunk updates

Unmerged revisions

=== modified file 'autopilot/introspection/_search.py'
--- autopilot/introspection/_search.py	2015-06-16 02:39:44 +0000
+++ autopilot/introspection/_search.py	2015-05-06 10:03:43 +0000
@@ -430,6 +430,11 @@
     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
@@ -447,25 +452,15 @@
             actual_base_class = cls
 
     if actual_base_class != base_class:
-        error_message = _get_wrong_cpo_error_message(
-            base_class,
-            actual_base_class
+        raise(
+            ValueError(
+                WRONG_CPO_CLASS_MSG.format(
+                    passed=base_class,
+                    actual=actual_base_class
+                )
+            )
         )
 
-        raise ValueError(error_message)
-
-
-WRONG_CPO_CLASS_MSG = '''\
-base_class: {passed} does not appear to be the actual base CPO class.
-Perhaps you meant to use: {actual}.'''
-
-
-def _get_wrong_cpo_error_message(passed_base, actual_base):
-    return WRONG_CPO_CLASS_MSG.format(
-        passed=passed_base,
-        actual=actual_base
-    )
-
 
 def _make_proxy_object_async(
         data_source, emulator_base, reply_handler, error_handler):

=== modified file 'autopilot/introspection/dbus.py'
--- autopilot/introspection/dbus.py	2014-07-31 04:42:08 +0000
+++ autopilot/introspection/dbus.py	2015-06-16 02:39:44 +0000
@@ -544,6 +544,36 @@
         class_name = cls.__name__.encode('utf-8')
         return state_name == class_name
 
+    @classmethod
+    def get_type_query_name(cls):
+        """Return the Type node name to use within the search query.
+
+        This allows for a Custom Proxy Object to be named differently to the
+        underlying node type name.
+
+        For instance if you have a QML type defined in the file RedRect.qml::
+
+            import QtQuick 2.0
+            Rectangle {
+                color: red;
+            }
+
+        You can then define a Custom Proxy Object  for this type like so::
+
+        class RedRect(DBusIntrospectionObject):
+            @classmethod
+            def get_type_query_name(cls):
+                return 'QQuickRectangle'
+
+        This is due to the qml engine storing 'RedRect' as a QQuickRectangle in
+        the UI tree and the xpathquery query needs a node type to query for.
+        By default the query will use the class name (in this case RedRect) but
+        this will not match any node type in the tree.
+
+        """
+
+        return cls.__name__
+
 
 # TODO - can we add a deprecation warning around this somehow?
 CustomEmulatorBase = DBusIntrospectionObject
@@ -557,5 +587,11 @@
 
     """
     if not isinstance(maybe_string_or_class, str):
-        return maybe_string_or_class.__name__
+        return _get_class_type_name(maybe_string_or_class)
     return maybe_string_or_class
+
+def _get_class_type_name(maybe_cpo_class):
+    if hasattr(maybe_cpo_class, 'get_type_query_name'):
+        return maybe_cpo_class.get_type_query_name()
+    else:
+        return maybe_cpo_class.__name__

=== modified file 'autopilot/tests/functional/test_introspection_features.py'
--- autopilot/tests/functional/test_introspection_features.py	2015-06-16 02:39:44 +0000
+++ autopilot/tests/functional/test_introspection_features.py	2015-06-16 02:39:44 +0000
@@ -42,6 +42,7 @@
 from autopilot import platform
 from autopilot.matchers import Eventually
 from autopilot.testcase import AutopilotTestCase
+from autopilot.tests.functional import QmlScriptRunnerMixin
 from autopilot.tests.functional.fixtures import TempDesktopFile
 from autopilot.introspection import CustomEmulatorBase
 from autopilot.introspection import _object_registry as object_registry
@@ -139,8 +140,8 @@
 
             self.start_mock_app(WindowMockerApp)
 
-    def test_raises_exception_when_using_incorrect_cpo_base_class(self):
-        """Launching a proxy app with incorrect base must raise exception."""
+    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
@@ -150,8 +151,9 @@
                 def validate_dbus_object(cls, path, _state):
                     return path == b'/window-mocker'
 
-            self.assertRaises(ValueError, self.start_mock_app, WindowMockerApp)
-
+            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."""
@@ -396,3 +398,31 @@
                 [e[1] for e in result2.decorated.errors]
             )
         )
+
+
+class CustomCPOTest(AutopilotTestCase, QmlScriptRunnerMixin):
+
+    def launch_simple_qml_script(self):
+        simple_script = dedent("""
+        import QtQuick 2.0
+        Rectangle {
+            objectName: "ExampleRectangle"
+        }
+        """)
+        return self.start_qml_script(simple_script)
+
+    def test_cpo_can_be_named_different_to_underlying_type(self):
+        """A CPO with the correct name match method must be matched if the
+        class name is different to the Type name.
+
+        """
+        with object_registry.patch_registry({}):
+            class RandomNamedCPORectangle(CustomEmulatorBase):
+                @classmethod
+                def get_type_query_name(cls):
+                    return 'QQuickRectangle'
+
+            app = self.launch_simple_qml_script()
+            rectangle = app.select_single(RandomNamedCPORectangle)
+
+            self.assertThat(rectangle.objectName, Equals('ExampleRectangle'))

=== modified file 'autopilot/tests/unit/test_introspection_dbus.py'
--- autopilot/tests/unit/test_introspection_dbus.py	2014-07-22 02:30:19 +0000
+++ autopilot/tests/unit/test_introspection_dbus.py	2015-06-16 02:39:44 +0000
@@ -118,6 +118,28 @@
                 ),
             )
 
+    def test_base_class_provides_correct_query_name(self):
+        self.assertThat(
+            dbus.DBusIntrospectionObject.get_type_query_name(),
+            Equals('ProxyBase')
+        )
+
+    def test_inherited_uses_default_get_node_name(self):
+        class TestCPO(dbus.DBusIntrospectionObject):
+            pass
+
+        self.assertThat(
+            TestCPO.get_type_query_name(),
+            Equals('TestCPO')
+        )
+
+    def test_inherited_overwrites_node_name_is_correct(self):
+        class TestCPO(dbus.DBusIntrospectionObject):
+            @classmethod
+            def get_type_query_name(cls):
+                return "TestCPO"
+        self.assertThat(TestCPO.get_type_query_name(), Equals("TestCPO"))
+
 
 class ProxyObjectPrintTreeTests(TestCase):
 
@@ -222,3 +244,23 @@
         class FooBarBaz(object):
             pass
         self.assertEqual("FooBarBaz", dbus.get_type_name(FooBarBaz))
+
+    def test_get_type_name_returns_classname(self):
+        class CustomCPO(dbus.DBusIntrospectionObject):
+            pass
+
+        type_name = dbus.get_type_name(CustomEmulatorBase)
+        self.assertThat(type_name, Equals('ProxyBase'))
+
+    def test_get_type_name_returns_custom_node_name(self):
+        class CustomCPO(dbus.DBusIntrospectionObject):
+            @classmethod
+            def get_type_query_name(cls):
+                return 'TestingCPO'
+        type_name = dbus.get_type_name(CustomCPO)
+        self.assertThat(type_name, Equals('TestingCPO'))
+
+    def test_get_type_name_returns_classname_of_non_proxybase_classes(self):
+        class Foo(object):
+            pass
+        self.assertEqual('Foo', dbus.get_type_name(Foo))

=== modified file 'autopilot/tests/unit/test_introspection_search.py'
--- autopilot/tests/unit/test_introspection_search.py	2015-06-16 02:39:44 +0000
+++ autopilot/tests/unit/test_introspection_search.py	2015-05-06 10:00:49 +0000
@@ -740,7 +740,10 @@
         class ActualBase(CustomEmulatorBase):
             pass
 
-        _s._raise_if_base_class_not_actually_base(ActualBase)
+        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):
@@ -797,23 +800,27 @@
         )
 
     def test_dont_raise_when_using_default_emulator_base(self):
+        # _make_proxy_object potentially creates a default base.
         DefaultBase = _s._make_default_emulator_base()
-        _s._raise_if_base_class_not_actually_base(DefaultBase)
+        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):
-        actual_base = 'ActualBase'
-
-        passed_base = 'InheritedCPO'
-
-        error_message = _s._get_wrong_cpo_error_message(
-            passed_base,
-            actual_base
-        )
-
-        self.assertEqual(
-            error_message,
-            _s.WRONG_CPO_CLASS_MSG.format(
-                passed=passed_base,
-                actual=actual_base
+        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
+                )
             )
-        )