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

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:562
http://jenkins.qa.ubuntu.com/job/autopilot-ci/1087/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/autopilot-wily-amd64-ci/18/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/autopilot-wily-armhf-ci/17/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/autopilot-wily-i386-ci/18/console

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

review: Needs Fixing (continuous-integration)

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

563. By Christopher Lee on 2015-06-23

Pyflakes fix

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:563
http://jenkins.qa.ubuntu.com/job/autopilot-ci/1088/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-wily-amd64-ci/19
        deb: http://jenkins.qa.ubuntu.com/job/autopilot-wily-amd64-ci/19/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-wily-armhf-ci/18
        deb: http://jenkins.qa.ubuntu.com/job/autopilot-wily-armhf-ci/18/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-wily-i386-ci/19
        deb: http://jenkins.qa.ubuntu.com/job/autopilot-wily-i386-ci/19/artifact/work/output/*zip*/output.zip
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/3278
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/2735
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/3276
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/3276/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21377

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:562
http://jenkins.qa.ubuntu.com/job/autopilot-ci/1089/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-wily-amd64-ci/20
        deb: http://jenkins.qa.ubuntu.com/job/autopilot-wily-amd64-ci/20/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-wily-armhf-ci/19
        deb: http://jenkins.qa.ubuntu.com/job/autopilot-wily-armhf-ci/19/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-wily-i386-ci/20
        deb: http://jenkins.qa.ubuntu.com/job/autopilot-wily-i386-ci/20/artifact/work/output/*zip*/output.zip
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/3279
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/2736
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/3277
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/3277/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21378

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

review: Approve (continuous-integration)

[Max Brustkern (nuclearbob)]

Looks reasonable to me.

review: Approve

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

564. By Christopher Lee on 2015-07-19

Merge trunk updates

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:564
http://jenkins.qa.ubuntu.com/job/autopilot-ci/1119/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-wily-amd64-ci/49
        deb: http://jenkins.qa.ubuntu.com/job/autopilot-wily-amd64-ci/49/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-wily-armhf-ci/48
        deb: http://jenkins.qa.ubuntu.com/job/autopilot-wily-armhf-ci/48/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-wily-i386-ci/49
        deb: http://jenkins.qa.ubuntu.com/job/autopilot-wily-i386-ci/49/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/3451/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/2875/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/3448
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/3448/artifact/work/output/*zip*/output.zip
    None: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21997/console

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

review: Approve (continuous-integration)

[Nicholas Skaggs (nskaggs)]

 balloons: sorry was out, hey so what if an inelegant solution where I can land the CPO any name change to 1.5 and then you could: if sdk.ver == a: return "MainView" else: return "MainVi12"
This is for now, purely a workaround and not a fix
That would be this MP and I could get that released ASAP as it's already reviewed and tested etc. it'll just be merging to 1.5 and not 1.6 https://code.launchpad.net/~canonical-platform-qa/autopilot/fix-cpo-having-different-name-1337004/+merge/262047

veebers, This would be better than it is, so yes, it would help!

=== modified file 'autopilot/introspection/dbus.py'
--- autopilot/introspection/dbus.py	2014-07-31 04:42:08 +0000
+++ autopilot/introspection/dbus.py	2015-07-20 22:38:37 +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,12 @@
 
     """
     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_input_stack.py'
--- autopilot/tests/functional/test_input_stack.py	2015-07-15 21:18:42 +0000
+++ autopilot/tests/functional/test_input_stack.py	2015-07-20 22:38:37 +0000
@@ -379,7 +379,7 @@
         screen_geometry = Display.create().get_screen_geometry(0)
         target_x = screen_geometry[0] + 10
         target_y = screen_geometry[1] + 10.6
-        self.device.move(target_x, target_y)
+        device.move(target_x, target_y)
         self.assertEqual(device.position(), (target_x, int(target_y)))
 
     @patch('autopilot.platform.model', new=lambda *args: "Not Desktop", )

=== modified file 'autopilot/tests/functional/test_introspection_features.py'
--- autopilot/tests/functional/test_introspection_features.py	2015-04-30 03:34:13 +0000
+++ autopilot/tests/functional/test_introspection_features.py	2015-07-20 22:38:37 +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
@@ -397,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-07-20 22:38:37 +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))