Merge lp:~canonical-platform-qa/autopilot/fixing_pid_search_1569079 into lp:autopilot

[Christopher Lee]

Add a final filter that ignores any children processes if the parent process matches and the supplied pid is the parents.

It's possible that a parent process spawns a child process that will match an attempt to get a proxy object for the parent.

Add a final filter that ignores any children processes if the parent process matches and the supplied pid is the parents.

[Max Brustkern (nuclearbob)]

Seems reasonable to me, once we get a good test of it.

[platform-qa-bot (platform-qa-bot)]

FAILED: Continuous integration, rev:577
https://platform-qa-jenkins.ubuntu.com/job/autopilot-ci/17/
Executed test runs:
    FAILURE: https://platform-qa-jenkins.ubuntu.com/job/build-wily-amd64-package/80/console
    FAILURE: https://platform-qa-jenkins.ubuntu.com/job/build-wily-i386-package/80/console
    None: https://platform-qa-jenkins.ubuntu.com/job/generic-update-mp/79/console

Click here to trigger a rebuild:
https://platform-qa-jenkins.ubuntu.com/job/autopilot-ci/17/rebuild

review: Needs Fixing (continuous-integration)

[lp:~canonical-platform-qa/autopilot/fixing_pid_search_1569079]

578. By Christopher Lee on 2016-04-11

Fix minor flake8 issue.

[platform-qa-bot (platform-qa-bot)]

PASSED: Continuous integration, rev:578
https://platform-qa-jenkins.ubuntu.com/job/autopilot-ci/18/
Executed test runs:
    SUCCESS: https://platform-qa-jenkins.ubuntu.com/job/build-wily-amd64-package/81
        deb: https://platform-qa-jenkins.ubuntu.com/job/build-wily-amd64-package/81/artifact/work/output/*zip*/output.zip
    SUCCESS: https://platform-qa-jenkins.ubuntu.com/job/build-wily-i386-package/81
        deb: https://platform-qa-jenkins.ubuntu.com/job/build-wily-i386-package/81/artifact/work/output/*zip*/output.zip
    None: https://platform-qa-jenkins.ubuntu.com/job/generic-update-mp/80/console

Click here to trigger a rebuild:
https://platform-qa-jenkins.ubuntu.com/job/autopilot-ci/18/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:577
http://jenkins.qa.ubuntu.com/job/autopilot-ci/1147/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-wily-amd64-ci/77
        deb: http://jenkins.qa.ubuntu.com/job/autopilot-wily-amd64-ci/77/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-wily-armhf-ci/76
        deb: http://jenkins.qa.ubuntu.com/job/autopilot-wily-armhf-ci/76/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-wily-i386-ci/77
        deb: http://jenkins.qa.ubuntu.com/job/autopilot-wily-i386-ci/77/artifact/work/output/*zip*/output.zip
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/6809
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-touch/5160
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/6820
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/6820/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/28068

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:578
http://jenkins.qa.ubuntu.com/job/autopilot-ci/1148/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-wily-amd64-ci/78
        deb: http://jenkins.qa.ubuntu.com/job/autopilot-wily-amd64-ci/78/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-wily-armhf-ci/77
        deb: http://jenkins.qa.ubuntu.com/job/autopilot-wily-armhf-ci/77/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-wily-i386-ci/78
        deb: http://jenkins.qa.ubuntu.com/job/autopilot-wily-i386-ci/78/artifact/work/output/*zip*/output.zip
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/6810
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-touch/5161
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/6821
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/6821/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/28069

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

review: Approve (continuous-integration)

[lp:~canonical-platform-qa/autopilot/fixing_pid_search_1569079]

579. By Christopher Lee on 2016-04-12

Refactor to remove mock in test and potentially reduce calls to get pid details.

[platform-qa-bot (platform-qa-bot)]

PASSED: Continuous integration, rev:579
https://platform-qa-jenkins.ubuntu.com/job/autopilot-ci/19/
Executed test runs:
    SUCCESS: https://platform-qa-jenkins.ubuntu.com/job/build-wily-amd64-package/82
        deb: https://platform-qa-jenkins.ubuntu.com/job/build-wily-amd64-package/82/artifact/work/output/*zip*/output.zip
    SUCCESS: https://platform-qa-jenkins.ubuntu.com/job/build-wily-i386-package/82
        deb: https://platform-qa-jenkins.ubuntu.com/job/build-wily-i386-package/82/artifact/work/output/*zip*/output.zip
    None: https://platform-qa-jenkins.ubuntu.com/job/generic-update-mp/81/console

Click here to trigger a rebuild:
https://platform-qa-jenkins.ubuntu.com/job/autopilot-ci/19/rebuild

review: Approve (continuous-integration)

[Thomi Richards (thomir-deactivatedaccount)]

LGTM

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:579
http://jenkins.qa.ubuntu.com/job/autopilot-ci/1149/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-wily-amd64-ci/79
        deb: http://jenkins.qa.ubuntu.com/job/autopilot-wily-amd64-ci/79/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-wily-armhf-ci/78
        deb: http://jenkins.qa.ubuntu.com/job/autopilot-wily-armhf-ci/78/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-wily-i386-ci/79
        deb: http://jenkins.qa.ubuntu.com/job/autopilot-wily-i386-ci/79/artifact/work/output/*zip*/output.zip
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/6811
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-touch/5162
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/6822
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/6822/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/28070

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

review: Approve (continuous-integration)

=== modified file 'autopilot/introspection/_search.py'
--- autopilot/introspection/_search.py	2016-01-22 17:21:36 +0000
+++ autopilot/introspection/_search.py	2016-04-12 03:02:16 +0000
@@ -171,6 +171,16 @@
         process
     )
 
+    if pid is not None:
+        # Due to the filtering including children parents, if there exists a
+        # top-level pid, take that instead of any children that may have
+        # matched.
+        connections = _filter_parent_pids_from_children(
+            pid,
+            connections,
+            dbus_bus
+        )
+
     _raise_if_not_single_result(
         connections,
         _get_search_criteria_string_representation(**kwargs)
@@ -184,6 +194,38 @@
     )
 
 
+def _map_connection_to_pid(connection, dbus_bus):
+    try:
+        return _get_bus_connections_pid(dbus_bus, connection)
+    except dbus.DBusException as e:
+        logger.info(
+            "dbus.DBusException while attempting to get PID for %s: %r" %
+            (connection, e))
+
+
+def _filter_parent_pids_from_children(
+        pid, connections, dbus_bus, _connection_pid_fn=_map_connection_to_pid):
+    """Return any connections that have an actual pid matching the requested
+    and aren't just a child of that requested pid.
+
+    :param pid: Pid passed in for matching
+    :param connections: List of connections to filter
+    :param dbus_bus: Dbus object that the connections are contained.
+    :param _connection_pid_fn: Function that takes 2 args 'connection' 'dbus
+      object' that returns the pid (or None if not found) of the connection on
+      that dbus bus.
+      (Note: Useful for testing.)
+    :returns: List of suitable connections (e.g. returns what was passed if no
+      connections match the pid (i.e. all matches are children)).
+
+    """
+    for conn in connections:
+        if pid == _connection_pid_fn(conn, dbus_bus):
+            logger.info('Found the parent pid, ignoring any others.')
+            return [conn]
+    return connections
+
+
 def _get_dbus_bus_from_string(dbus_string):
     if dbus_string == 'session':
         return dbus_handler.get_session_bus()

=== modified file 'autopilot/tests/unit/test_introspection_search.py'
--- autopilot/tests/unit/test_introspection_search.py	2015-06-16 02:39:23 +0000
+++ autopilot/tests/unit/test_introspection_search.py	2016-04-12 03:02:16 +0000
@@ -19,7 +19,7 @@
 
 from dbus import DBusException
 import os
-from unittest.mock import patch, Mock
+from unittest.mock import call, patch, Mock
 from testtools import TestCase
 from testtools.matchers import (
     Contains,
@@ -581,6 +581,82 @@
         self.assertEqual(fake_process.pid, observed)
 
 
+class FilterParentPidsFromChildrenTests(TestCase):
+
+    def test_returns_all_connections_with_no_parent_match(self):
+        search_pid = 123
+        connections = ['1:0', '1:3']
+        dbus_bus = Mock()
+        pid_mapping = Mock(side_effect=[111, 222])
+        self.assertThat(
+            _s._filter_parent_pids_from_children(
+                search_pid,
+                connections,
+                dbus_bus,
+                _connection_pid_fn=pid_mapping
+            ),
+            Equals(connections)
+        )
+
+    def test_calls_connection_pid_fn_in_order(self):
+        search_pid = 123
+        connections = ['1:3', '1:0']
+        dbus_bus = Mock()
+        pid_mapping = Mock(side_effect=[222, 123])
+        _s._filter_parent_pids_from_children(
+            search_pid,
+            connections,
+            dbus_bus,
+            _connection_pid_fn=pid_mapping
+        )
+
+        self.assertTrue(
+            pid_mapping.call_args_list == [
+                call('1:3', dbus_bus),
+                call('1:0', dbus_bus)
+            ]
+        )
+
+    def test_returns_just_parent_connection_with_pid_match(self):
+        search_pid = 123
+        # connection '1.0' has pid 123.
+        connections = ['1:3', '1:0']
+        dbus_bus = Mock()
+        # Mapping returns parent pid on second call.
+        pid_mapping = Mock(side_effect=[222, 123])
+        self.assertThat(
+            _s._filter_parent_pids_from_children(
+                search_pid,
+                connections,
+                dbus_bus,
+                _connection_pid_fn=pid_mapping
+            ),
+            Equals(['1:0'])
+        )
+
+        self.assertTrue(
+            pid_mapping.call_args_list == [
+                call('1:3', dbus_bus),
+                call('1:0', dbus_bus)
+            ]
+        )
+
+    def test_returns_all_connections_with_no_pids_returned_in_search(self):
+        search_pid = 123
+        connections = ['1:3', '1:0']
+        dbus_bus = Mock()
+        pid_mapping = Mock(side_effect=[None, None])
+        self.assertThat(
+            _s._filter_parent_pids_from_children(
+                search_pid,
+                connections,
+                dbus_bus,
+                _connection_pid_fn=pid_mapping
+            ),
+            Equals(connections)
+        )
+
+
 class ProcessSearchErrorStringRepTests(TestCase):
 
     """Various tests for the _get_search_criteria_string_representation