Repository: google/mobly

Issue:
Flake in unit tests
Reported in #87 
https://travis-ci.org/google/mobly/jobs/197104959

=================================== FAILURES ===================================
____________________ UtilsTest.test_start_standing_subproc _____________________
self = <tests.mobly.utils_test.UtilsTest testMethod=test_start_standing_subproc>
    def test_start_standing_subproc(self):
        with self.assertRaisesRegexp(utils.Error,
                                     "Process .* has terminated"):
\>           utils.start_standing_subprocess("sleep 0", check_health_delay=0.1)
E           AssertionError: Error not raised
tests/mobly/utils_test.py:32: AssertionErro

Discussion:
xpconanfan: 



Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion.

---

*[tests/mobly/utils_test.py, line 33 at r1](https://reviewable.io:443/reviews/google/mobly/164#-Kft4IlohqXOZQbkgvwN:-Kft4IlpLhgfHHd6AJ8z:b-a1kill) ([raw file](https://github.com/google/mobly/blob/7b01a61aa1a22614e0e70eae04eb10370a4c8f4f/tests/mobly/utils_test.py#L33)):*
> ```Python
>                                      "Process .* has terminated"):
>             utils.start_standing_subprocess(
>                 ['sleep','0'], check_health_delay=0.5)
> ```

why the change from 0.1 to 0.5 and str to list?
this does not seem to be related to `exe_cmd`?

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/google/mobly/164)*
<!-- Sent from Reviewable.io -->

adorokhine: 



Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion.

---

*[tests/mobly/utils_test.py, line 33 at r1](https://reviewable.io:443/reviews/google/mobly/164#-Kft4IlohqXOZQbkgvwN:-Kft95spFobm0QYGR4uD:b-896fix) ([raw file](https://github.com/google/mobly/blob/454b556b53133176e6e1f02ae6e0b3f41739bcb1/tests/mobly/utils_test.py#L33)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

why the change from 0.1 to 0.5 and str to list?
this does not seem to be related to `exe_cmd`?
</blockquote></details>

Done.

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/google/mobly/164)*
<!-- Sent from Reviewable.io -->

dthkao: <img class="emoji" title=":lgtm:" alt=":lgtm:" align="absmiddle" src="https://reviewable.io/lgtm.png" height="20" width="61"/>

---

Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion.

---



*Comments from [Reviewable](https://reviewable.io:443/reviews/google/mobly/164#-:-KftHytyvUUIumc_97NM:bnfp4nl)*
<!-- Sent from Reviewable.io -->

xpconanfan: 



Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion.

---

*[tests/mobly/utils_test.py, line 33 at r1](https://reviewable.io:443/reviews/google/mobly/164#-Kft4IlohqXOZQbkgvwN:-Kg0s0bj_r8a4fdh3Uew:b-yz7gkv) ([raw file](https://github.com/google/mobly/blob/454b556b53133176e6e1f02ae6e0b3f41739bcb1/tests/mobly/utils_test.py#L33)):*
<details><summary><i>Previously, adorokhine (Alexander Dorokhine) wrote…</i></summary><blockquote>

Done.
</blockquote></details>

I still see this being changed to a list.
Is this a leak from the start standing subprocess change?

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/google/mobly/164)*
<!-- Sent from Reviewable.io -->

adorokhine: 



Reviewed 3 of 4 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

*[tests/mobly/utils_test.py, line 33 at r1](https://reviewable.io:443/reviews/google/mobly/164#-Kft4IlohqXOZQbkgvwN:-Kg1Ig0bDteHmFq-Yszy:brssb8g) ([raw file](https://github.com/google/mobly/blob/454b556b53133176e6e1f02ae6e0b3f41739bcb1/tests/mobly/utils_test.py#L33)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

I still see this being changed to a list.
Is this a leak from the start standing subprocess change?
</blockquote></details>

Thanks, good catch. Done.

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/google/mobly/164)*
<!-- Sent from Reviewable.io -->

xpconanfan: <img class="emoji" title=":lgtm:" alt=":lgtm:" align="absmiddle" src="https://reviewable.io/lgtm.png" height="20" width="61"/>

---

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---



*Comments from [Reviewable](https://reviewable.io:443/reviews/google/mobly/164#-:-KgFrMky1iYJR0yzvtbz:bnfp4nl)*
<!-- Sent from Reviewable.io -->


Patch:
diff --git a/mobly/controllers/sniffer_lib/local/local_base.py b/mobly/controllers/sniffer_lib/local/local_base.py
index 859c242..781e4b6 100644
--- a/mobly/controllers/sniffer_lib/local/local_base.py
+++ b/mobly/controllers/sniffer_lib/local/local_base.py
@@ -54,9 +54,10 @@ class SnifferLocalBase(sniffer.Sniffer):
         self._base_configs = base_configs
 
         try:
-            utils.exe_cmd("ifconfig", self._interface, "down")
-            utils.exe_cmd("iwconfig", self._interface, "mode", "monitor")
-            utils.exe_cmd("ifconfig", self._interface, "up")
+            subprocess.check_call(['ifconfig', self._interface, 'down'])
+            subprocess.check_call(
+                ['iwconfig', self._interface, 'mode', 'monitor'])
+            subprocess.check_call(['ifconfig', self._interface, 'up'])
         except Exception as err:
             raise sniffer.ExecutionError(err)
 
@@ -87,8 +88,11 @@ class SnifferLocalBase(sniffer.Sniffer):
 
         if sniffer.Sniffer.CONFIG_KEY_CHANNEL in final_configs:
             try:
-                utils.exe_cmd("iwconfig", self._interface, "channel",
-                        str(final_configs[sniffer.Sniffer.CONFIG_KEY_CHANNEL]))
+                subprocess.check_call([
+                    'iwconfig',
+                    self._interface,
+                    'channel',
+                    str(final_configs[sniffer.Sniffer.CONFIG_KEY_CHANNEL])])
             except Exception as err:
                 raise sniffer.ExecutionError(err)
 
diff --git a/mobly/utils.py b/mobly/utils.py
index ba47c60..c9be01e 100644
--- a/mobly/utils.py
+++ b/mobly/utils.py
@@ -280,27 +280,6 @@ def concurrent_exec(func, param_list):
         return return_vals
 
 
-def exe_cmd(*cmds):
-    """Executes commands in a new shell.
-
-    Args:
-        cmds: A sequence of commands and arguments.
-
-    Returns:
-        The output of the command run.
-
-    Raises:
-        OSError is raised if an error occurred during the command execution.
-    """
-    cmd = ' '.join(cmds)
-    proc = subprocess.Popen(
-        cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True)
-    (out, err) = proc.communicate()
-    if not err:
-        return out
-    raise OSError(err)
-
-
 def _assert_subprocess_running(proc):
     """Checks if a subprocess has terminated on its own.
 
@@ -368,14 +347,21 @@ def stop_standing_subprocess(proc, kill_signal=signal.SIGTERM):
     _assert_subprocess_running(proc)
     process = psutil.Process(pid)
     success = True
-    for child in process.children(recursive=True):
+    try:
+        children = process.children(recursive=True)
+    except AttributeError:
+        # Handle versions <3.0.0 of psutil.
+        children = process.get_children(recursive=True)
+    for child in children:
         try:
             child.kill()
+            child.wait(timeout=10)
         except:
             success = False
             logging.exception('Failed to kill standing subprocess %d', child.pid)
     try:
         process.kill()
+        process.wait(timeout=10)
     except:
         success = False
         logging.exception('Failed to kill standing subprocess %d', pid)


Test patch:
diff --git a/tests/mobly/controllers/android_device_test.py b/tests/mobly/controllers/android_device_test.py
index c5b3733..3a0940e 100755
--- a/tests/mobly/controllers/android_device_test.py
+++ b/tests/mobly/controllers/android_device_test.py
@@ -209,8 +209,7 @@ class AndroidDeviceTest(unittest.TestCase):
     @mock.patch('mobly.controllers.android_device_lib.fastboot.FastbootProxy',
                 return_value=mock_android_device.MockFastbootProxy(1))
     @mock.patch('mobly.utils.create_dir')
-    @mock.patch('mobly.utils.exe_cmd')
-    def test_AndroidDevice_take_bug_report(self, exe_mock, create_dir_mock,
+    def test_AndroidDevice_take_bug_report(self, create_dir_mock,
                                            FastbootProxy, MockAdbProxy):
         """Verifies AndroidDevice.take_bug_report calls the correct adb command
         and writes the bugreport file to the correct path.
@@ -227,8 +226,7 @@ class AndroidDeviceTest(unittest.TestCase):
     @mock.patch('mobly.controllers.android_device_lib.fastboot.FastbootProxy',
                 return_value=mock_android_device.MockFastbootProxy(1))
     @mock.patch('mobly.utils.create_dir')
-    @mock.patch('mobly.utils.exe_cmd')
-    def test_AndroidDevice_take_bug_report_fail(self, exe_mock, create_dir_mock,
+    def test_AndroidDevice_take_bug_report_fail(self, create_dir_mock,
                                                 FastbootProxy, MockAdbProxy):
         """Verifies AndroidDevice.take_bug_report writes out the correct message
         when taking bugreport fails.
@@ -245,8 +243,7 @@ class AndroidDeviceTest(unittest.TestCase):
     @mock.patch('mobly.controllers.android_device_lib.fastboot.FastbootProxy',
                 return_value=mock_android_device.MockFastbootProxy(1))
     @mock.patch('mobly.utils.create_dir')
-    @mock.patch('mobly.utils.exe_cmd')
-    def test_AndroidDevice_take_bug_report_fallback(self, exe_mock,
+    def test_AndroidDevice_take_bug_report_fallback(self,
         create_dir_mock, FastbootProxy, MockAdbProxy):
         """Verifies AndroidDevice.take_bug_report falls back to traditional
         bugreport on builds that do not have bugreportz.
diff --git a/tests/mobly/utils_test.py b/tests/mobly/utils_test.py
index fb2f519..de618a5 100755
--- a/tests/mobly/utils_test.py
+++ b/tests/mobly/utils_test.py
@@ -32,6 +32,11 @@ class UtilsTest(unittest.TestCase):
             utils.start_standing_subprocess("sleep 0", check_health_delay=0.1)
 
     def test_stop_standing_subproc(self):
+        p = utils.start_standing_subprocess('sleep 5')
+        utils.stop_standing_subprocess(p)
+        self.assertIsNotNone(p.poll())
+
+    def test_stop_standing_subproc_already_dead(self):
         p = utils.start_standing_subprocess("sleep 0")
         time.sleep(0.1)
         with self.assertRaisesRegexp(utils.Error,