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

---

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved.

---



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

dthkao: THANKS!

---

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved.

---



*Comments from [Reviewable](https://reviewable.io:443/reviews/google/mobly/165#-:-KftLtV6H66npyt40ciz:bp121n7)*
<!-- 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..338c4e8 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.
 
@@ -339,10 +318,16 @@ def start_standing_subprocess(cmd, check_health_delay=0):
     """
     proc = subprocess.Popen(
         cmd,
+        stdin=subprocess.PIPE,
         stdout=subprocess.PIPE,
         stderr=subprocess.PIPE,
         shell=True)
     logging.debug('Start standing subprocess with cmd: %s', cmd)
+    # Leaving stdin open causes problems for input, e.g. breaking the
+    # code.inspect() shell (http://stackoverflow.com/a/25512460/1612937), so
+    # explicitly close it assuming it is not needed for standing subprocesses.
+    proc.stdin.close()
+    proc.stdin = None
     if check_health_delay > 0:
         time.sleep(check_health_delay)
         _assert_subprocess_running(proc)
@@ -368,14 +353,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..73ea149 100755
--- a/tests/mobly/utils_test.py
+++ b/tests/mobly/utils_test.py
@@ -29,11 +29,16 @@ class UtilsTest(unittest.TestCase):
     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)
+            utils.start_standing_subprocess("sleep 0", check_health_delay=0.5)
 
     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)
+        time.sleep(0.5)
         with self.assertRaisesRegexp(utils.Error,
                                      "Process .* has terminated"):
             utils.stop_standing_subprocess(p)