Merge lp:~thomir-deactivatedaccount/autopilot/trunk-fix-ProcessSearchError-message into lp:autopilot

[Thomi Richards]

Improve the error message in the ProcessSearchError exception.

Improve the error message in the ProcessSearchError exception.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:418
http://jenkins.qa.ubuntu.com/job/autopilot-ci/410/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-trusty-amd64-ci/136
        deb: http://jenkins.qa.ubuntu.com/job/autopilot-trusty-amd64-ci/136/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-trusty-armhf-ci/136
        deb: http://jenkins.qa.ubuntu.com/job/autopilot-trusty-armhf-ci/136/artifact/work/output/*zip*/output.zip
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty/2458
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-trusty/2146
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/2460
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/2460/artifact/work/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)

[lp:~thomir-deactivatedaccount/autopilot/trunk-fix-ProcessSearchError-message]

419. By Thomi Richards on 2014-01-20

Fix failing functional test.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:419
http://jenkins.qa.ubuntu.com/job/autopilot-ci/411/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-trusty-amd64-ci/137
        deb: http://jenkins.qa.ubuntu.com/job/autopilot-trusty-amd64-ci/137/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-trusty-armhf-ci/137
        deb: http://jenkins.qa.ubuntu.com/job/autopilot-trusty-armhf-ci/137/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty/2475
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-trusty/2162
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/2477
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/2477/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[lp:~thomir-deactivatedaccount/autopilot/trunk-fix-ProcessSearchError-message]

420. By Thomi Richards on 2014-01-20

Fixed issues from code review.

[Christopher Lee (veebers)]

LGTM

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:420
http://jenkins.qa.ubuntu.com/job/autopilot-ci/412/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-trusty-amd64-ci/138
        deb: http://jenkins.qa.ubuntu.com/job/autopilot-trusty-amd64-ci/138/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-trusty-armhf-ci/138
        deb: http://jenkins.qa.ubuntu.com/job/autopilot-trusty-armhf-ci/138/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty/2477
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-trusty/2164
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/2479
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/2479/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

=== modified file 'autopilot/introspection/__init__.py'
--- autopilot/introspection/__init__.py	2014-01-16 23:28:01 +0000
+++ autopilot/introspection/__init__.py	2014-01-20 20:59:59 +0000
@@ -33,6 +33,7 @@
 from functools import partial
 import os
 import psutil
+from six import u
 
 from autopilot.dbus_handler import (
     get_session_bus,
@@ -143,14 +144,7 @@
         ``process.pid != pid``.
 
     """
-    if process is not None:
-        if pid is None:
-            pid = process.pid
-        elif pid != process.pid:
-            raise RuntimeError("Supplied PID and process.pid do not match.")
-
-    if pid is not None and not _pid_is_running(pid):
-        raise ProcessSearchError("PID %d could not be found" % pid)
+    pid = _check_process_and_pid_details(process, pid)
 
     dbus_addresses = _get_dbus_addresses_from_search_parameters(
         pid,
@@ -160,19 +154,109 @@
         process
     )
 
-    if application_name:
-        app_name_check_fn = lambda i: get_classname_from_path(
-            i.introspection_iface.GetState('/')[0][0]) == application_name
-        dbus_addresses = list(filter(app_name_check_fn, dbus_addresses))
+    dbus_addresses = _maybe_filter_connections_by_app_name(
+        application_name,
+        dbus_addresses
+    )
 
     if dbus_addresses is None or len(dbus_addresses) == 0:
-        raise ProcessSearchError("Search criteria returned no results")
+        criteria_string = _get_search_criteria_string_representation(
+            pid,
+            dbus_bus,
+            connection_name,
+            process,
+            object_path,
+            application_name
+        )
+        message_string = "Search criteria (%s) returned no results" % \
+            (criteria_string)
+        raise ProcessSearchError(message_string)
     if len(dbus_addresses) > 1:
-        raise RuntimeError("Search criteria returned multiple results")
+        criteria_string = _get_search_criteria_string_representation(
+            pid,
+            dbus_bus,
+            connection_name,
+            process,
+            object_path,
+            application_name
+        )
+        message_string = "Search criteria (%s) returned multiple results" % \
+            (criteria_string)
+        raise RuntimeError(message_string)
 
     return _make_proxy_object(dbus_addresses[0], emulator_base)
 
 
+def _check_process_and_pid_details(process=None, pid=None):
+    """Do error checking on process and pid specification.
+
+    :raises RuntimeError: if both process and pid are specified, but the
+        process's 'pid' attribute is different to the pid attribute specified.
+    :raises ProcessSearchError: if the process specified is not running.
+    :returns: the pid to use in all search queries.
+
+    """
+    if process is not None:
+        if pid is None:
+            pid = process.pid
+        elif pid != process.pid:
+            raise RuntimeError("Supplied PID and process.pid do not match.")
+
+    if pid is not None and not _pid_is_running(pid):
+        raise ProcessSearchError("PID %d could not be found" % pid)
+    return pid
+
+
+def _maybe_filter_connections_by_app_name(application_name, dbus_addresses):
+    """Filter `dbus_addresses` by the application name exported, if
+    `application_name` has been specified.
+
+    :returns: a filtered list of connections.
+    """
+
+    if application_name:
+        dbus_addresses = [
+            a for a in dbus_addresses
+            if _get_application_name_from_dbus_address(a) == application_name
+        ]
+    return dbus_addresses
+
+
+def _get_application_name_from_dbus_address(dbus_address):
+    """Return the application name from a dbus_address object."""
+    return get_classname_from_path(
+        dbus_address.introspection_iface.GetState('/')[0][0]
+    )
+
+
+def _get_search_criteria_string_representation(
+        pid=None, dbus_bus=None, connection_name=None, process=None,
+        object_path=None, application_name=None):
+    """Get a string representation of the search criteria.
+
+    Used to represent the search criteria to users in error messages.
+    """
+    description_parts = []
+    if pid is not None:
+        description_parts.append(u('pid = %d') % pid)
+    if dbus_bus is not None:
+        description_parts.append(u("dbus bus = '%s'") % dbus_bus)
+    if connection_name is not None:
+        description_parts.append(
+            u("connection name = '%s'") % connection_name
+        )
+    if object_path is not None:
+        description_parts.append(u("object path = '%s'") % object_path)
+    if application_name is not None:
+        description_parts.append(
+            u("application name = '%s'") % application_name
+        )
+    if process is not None:
+        description_parts.append(u("process object = '%r'") % process)
+
+    return ", ".join(description_parts)
+
+
 def _get_dbus_addresses_from_search_parameters(
         pid, dbus_bus, connection_name, object_path, process):
     """Returns a list of :py:class: `DBusAddress` for all successfully matched

=== modified file 'autopilot/tests/functional/test_ap_apps.py'
--- autopilot/tests/functional/test_ap_apps.py	2013-12-16 03:55:22 +0000
+++ autopilot/tests/functional/test_ap_apps.py	2014-01-20 20:59:59 +0000
@@ -105,10 +105,9 @@
                 sleep(1)
         """ % sys.executable))
 
-        expected_error = "Search criteria returned no results"
         self.assertThat(
             lambda: self.launch_test_application(path, app_type='qt'),
-            raises(ProcessSearchError(expected_error))
+            raises(ProcessSearchError)
         )
 
     def test_creating_app_for_non_running_app_fails(self):

=== modified file 'autopilot/tests/unit/test_introspection_features.py'
--- autopilot/tests/unit/test_introspection_features.py	2013-11-07 22:15:29 +0000
+++ autopilot/tests/unit/test_introspection_features.py	2014-01-20 20:59:59 +0000
@@ -33,9 +33,22 @@
     raises,
 )
 from testscenarios import TestWithScenarios
-from six import StringIO
-
-
+from six import StringIO, u, PY3
+from contextlib import contextmanager
+if PY3:
+    from contextlib import ExitStack
+else:
+    from contextlib2 import ExitStack
+
+
+from autopilot.introspection import (
+    _check_process_and_pid_details,
+    _get_application_name_from_dbus_address,
+    _get_search_criteria_string_representation,
+    _maybe_filter_connections_by_app_name,
+    get_proxy_object_for_existing_process,
+    ProcessSearchError,
+)
 from autopilot.introspection.dbus import (
     _get_filter_string_for_key_value_pair,
     _is_valid_server_side_filter_param,
@@ -279,3 +292,304 @@
             path: '/some/path'
             text: 'Hello'
             """))
+
+
+class ProcessSearchErrorStringRepTests(TestCase):
+
+    """Various tests for the _get_search_criteria_string_representation
+    function.
+
+    """
+
+    def test_get_string_rep_defaults_to_empty_string(self):
+        observed = _get_search_criteria_string_representation()
+        self.assertEqual("", observed)
+
+    def test_pid(self):
+        self.assertEqual(
+            u('pid = 123'),
+            _get_search_criteria_string_representation(pid=123)
+        )
+
+    def test_dbus_bus(self):
+        self.assertEqual(
+            u("dbus bus = 'foo'"),
+            _get_search_criteria_string_representation(dbus_bus='foo')
+        )
+
+    def test_connection_name(self):
+        self.assertEqual(
+            u("connection name = 'foo'"),
+            _get_search_criteria_string_representation(connection_name='foo')
+        )
+
+    def test_object_path(self):
+        self.assertEqual(
+            u("object path = 'foo'"),
+            _get_search_criteria_string_representation(object_path='foo')
+        )
+
+    def test_application_name(self):
+        self.assertEqual(
+            u("application name = 'foo'"),
+            _get_search_criteria_string_representation(application_name='foo')
+        )
+
+    def test_process_object(self):
+        class FakeProcess(object):
+
+            def __repr__(self):
+                return 'foo'
+        process = FakeProcess()
+        self.assertEqual(
+            u("process object = 'foo'"),
+            _get_search_criteria_string_representation(process=process)
+        )
+
+    def test_all_parameters_combined(self):
+        class FakeProcess(object):
+
+            def __repr__(self):
+                return 'foo'
+        process = FakeProcess()
+        observed = _get_search_criteria_string_representation(
+            pid=123,
+            dbus_bus='session_bus',
+            connection_name='com.Canonical.Unity',
+            object_path='/com/Canonical/Autopilot',
+            application_name='MyApp',
+            process=process
+        )
+        expected = "pid = 123, dbus bus = 'session_bus', " \
+            "connection name = 'com.Canonical.Unity', " \
+            "object path = '/com/Canonical/Autopilot', " \
+            "application name = 'MyApp', process object = 'foo'"
+        self.assertEqual(expected, observed)
+
+
+class ProcessAndPidErrorCheckingTests(TestCase):
+
+    def test_raises_ProcessSearchError_when_process_is_not_running(self):
+        with patch('autopilot.introspection._pid_is_running') as pir:
+            pir.return_value = False
+
+            self.assertThat(
+                lambda: _check_process_and_pid_details(pid=123),
+                raises(ProcessSearchError("PID 123 could not be found"))
+            )
+
+    def test_raises_RuntimeError_when_pid_and_process_disagree(self):
+        mock_process = Mock()
+        mock_process.pid = 1
+
+        self.assertThat(
+            lambda: _check_process_and_pid_details(mock_process, 2),
+            raises(RuntimeError("Supplied PID and process.pid do not match."))
+        )
+
+    def test_returns_pid_when_specified(self):
+        expected = self.getUniqueInteger()
+        with patch('autopilot.introspection._pid_is_running') as pir:
+            pir.return_value = True
+
+            observed = _check_process_and_pid_details(pid=expected)
+
+        self.assertEqual(expected, observed)
+
+    def test_returns_process_pid_attr_when_specified(self):
+        fake_process = Mock()
+        fake_process.pid = self.getUniqueInteger()
+
+        with patch('autopilot.introspection._pid_is_running') as pir:
+            pir.return_value = True
+            observed = _check_process_and_pid_details(fake_process)
+
+        self.assertEqual(fake_process.pid, observed)
+
+    def test_returns_None_when_neither_parameters_present(self):
+        self.assertEqual(
+            None,
+            _check_process_and_pid_details()
+        )
+
+    def test_returns_pid_when_both_specified(self):
+        fake_process = Mock()
+        fake_process.pid = self.getUniqueInteger()
+        with patch('autopilot.introspection._pid_is_running') as pir:
+            pir.return_value = True
+            observed = _check_process_and_pid_details(
+                fake_process,
+                fake_process.pid
+            )
+        self.assertEqual(fake_process.pid, observed)
+
+
+class ApplicationFilteringTests(TestCase):
+
+    def get_mock_dbus_address_with_application_name(slf, app_name):
+        mock_dbus_address = Mock()
+        mock_dbus_address.introspection_iface.GetState.return_value = (
+            ('/' + app_name, {}),
+        )
+        return mock_dbus_address
+
+    def test_can_extract_application_name(self):
+        mock_connection = self.get_mock_dbus_address_with_application_name(
+            'SomeAppName'
+        )
+        self.assertEqual(
+            'SomeAppName',
+            _get_application_name_from_dbus_address(mock_connection)
+        )
+
+    def test_maybe_filter_returns_addresses_when_app_name_not_specified(self):
+        self.assertEqual(
+            [],
+            _maybe_filter_connections_by_app_name(None, [])
+        )
+
+    def test_maybe_filter_works_with_partial_match(self):
+        mock_connections = [
+            self.get_mock_dbus_address_with_application_name('Match'),
+            self.get_mock_dbus_address_with_application_name('Mismatch'),
+        ]
+        expected = mock_connections[:1]
+        observed = _maybe_filter_connections_by_app_name(
+            'Match',
+            mock_connections
+        )
+        self.assertEqual(expected, observed)
+
+    def test_maybe_filter_works_with_no_match(self):
+        mock_connections = [
+            self.get_mock_dbus_address_with_application_name('Mismatch1'),
+            self.get_mock_dbus_address_with_application_name('Mismatch2'),
+        ]
+        expected = []
+        observed = _maybe_filter_connections_by_app_name(
+            'Match',
+            mock_connections
+        )
+        self.assertEqual(expected, observed)
+
+    def test_maybe_filter_works_with_full_match(self):
+        mock_connections = [
+            self.get_mock_dbus_address_with_application_name('Match'),
+            self.get_mock_dbus_address_with_application_name('Match'),
+        ]
+        expected = mock_connections
+        observed = _maybe_filter_connections_by_app_name(
+            'Match',
+            mock_connections
+        )
+        self.assertEqual(expected, observed)
+
+
+class ProxyObjectGenerationTests(TestCase):
+
+    @contextmanager
+    def mock_all_child_calls(self):
+        mock_dict = {}
+        with ExitStack() as all_the_mocks:
+            mock_dict['check_process'] = all_the_mocks.enter_context(
+                patch(
+                    'autopilot.introspection._check_process_and_pid_details'
+                )
+            )
+            mock_dict['get_addresses'] = all_the_mocks.enter_context(
+                patch(
+                    'autopilot.introspection.'
+                    '_get_dbus_addresses_from_search_parameters'
+                )
+            )
+            mock_dict['filter_addresses'] = all_the_mocks.enter_context(
+                patch(
+                    'autopilot.introspection.'
+                    '_maybe_filter_connections_by_app_name'
+                )
+            )
+            mock_dict['make_proxy_object'] = all_the_mocks.enter_context(
+                patch(
+                    'autopilot.introspection._make_proxy_object'
+                )
+            )
+            yield mock_dict
+
+    def test_makes_child_calls(self):
+        """Mock out all child functions, and assert that they're called.
+
+        This test is somewhat ugly, and should be refactored once the search
+        criteria has been refactored into a separate object, rather than a
+        bunch of named parameters.
+
+        """
+        with self.mock_all_child_calls() as mocks:
+            fake_address_list = [Mock()]
+            mocks['get_addresses'].return_value = fake_address_list
+            mocks['filter_addresses'].return_value = fake_address_list
+
+            get_proxy_object_for_existing_process()
+
+            self.assertEqual(
+                1,
+                mocks['check_process'].call_count
+            )
+            self.assertEqual(
+                1,
+                mocks['get_addresses'].call_count
+            )
+            self.assertEqual(
+                1,
+                mocks['make_proxy_object'].call_count
+            )
+
+    def test_raises_ProcessSearchError(self):
+        """Function must raise ProcessSearchError if no addresses are found.
+
+        This test is somewhat ugly, and should be refactored once the search
+        criteria has been refactored into a separate object, rather than a
+        bunch of named parameters.
+
+        """
+        with self.mock_all_child_calls() as mocks:
+            fake_address_list = [Mock()]
+            mocks['check_process'].return_value = 123
+            mocks['get_addresses'].return_value = fake_address_list
+            mocks['filter_addresses'].return_value = []
+
+            self.assertThat(
+                lambda: get_proxy_object_for_existing_process(),
+                raises(
+                    ProcessSearchError(
+                        "Search criteria (pid = 123, dbus bus = 'session', "
+                        "object path = "
+                        "'/com/canonical/Autopilot/Introspection') returned "
+                        "no results"
+                    )
+                )
+            )
+
+    def test_raises_RuntimeError(self):
+        """Function must raise RuntimeError if several addresses are found.
+
+        This test is somewhat ugly, and should be refactored once the search
+        criteria has been refactored into a separate object, rather than a
+        bunch of named parameters.
+
+        """
+        with self.mock_all_child_calls() as mocks:
+            fake_address_list = [Mock(), Mock()]
+            mocks['get_addresses'].return_value = fake_address_list
+            mocks['filter_addresses'].return_value = fake_address_list
+
+            self.assertThat(
+                lambda: get_proxy_object_for_existing_process(),
+                raises(
+                    RuntimeError(
+                        "Search criteria (pid = 1, dbus bus = 'session', "
+                        "object path = "
+                        "'/com/canonical/Autopilot/Introspection') "
+                        "returned multiple results"
+                    )
+                )
+            )