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

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)

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

pip install -e .
parse_log_pytest
pytest
pytest
apt-get update apt-get install -y gcc
3.9
pytest --no-header -rA --tb=line --color=no -p no:cacheprovider -W ignore::DeprecationWarning
exceptiongroup @ file:///croot/exceptiongroup_1706031385326/work
future==1.0.0
iniconfig @ file:///home/linux1/recipes/ci/iniconfig_1610983019677/work
-e git+https://github.com/google/mobly.git@464a16aee403100025085846f0b6a5e29620e5b6#egg=mobly
mock==1.0.1
packaging @ file:///croot/packaging_1734472117206/work
pluggy @ file:///croot/pluggy_1733169602837/work
psutil==7.0.0
pytest @ file:///croot/pytest_1738938843180/work
pytz==2025.2
PyYAML==6.0.2
tomli @ file:///opt/conda/conda-bld/tomli_1657175507142/work

name: mobly
channels:
  - defaults
  - https://repo.anaconda.com/pkgs/main
  - https://repo.anaconda.com/pkgs/r
  - conda-forge
dependencies:
  - _libgcc_mutex=0.1=main
  - _openmp_mutex=5.1=1_gnu
  - ca-certificates=2025.2.25=h06a4308_0
  - exceptiongroup=1.2.0=py39h06a4308_0
  - iniconfig=1.1.1=pyhd3eb1b0_0
  - ld_impl_linux-64=2.40=h12ee557_0
  - libffi=3.4.4=h6a678d5_1
  - libgcc-ng=11.2.0=h1234567_1
  - libgomp=11.2.0=h1234567_1
  - libstdcxx-ng=11.2.0=h1234567_1
  - ncurses=6.4=h6a678d5_0
  - openssl=3.0.16=h5eee18b_0
  - packaging=24.2=py39h06a4308_0
  - pip=25.0=py39h06a4308_0
  - pluggy=1.5.0=py39h06a4308_0
  - pytest=8.3.4=py39h06a4308_0
  - python=3.9.21=he870216_1
  - readline=8.2=h5eee18b_0
  - setuptools=75.8.0=py39h06a4308_0
  - sqlite=3.45.3=h5eee18b_0
  - tk=8.6.14=h39e8969_0
  - tomli=2.0.1=py39h06a4308_0
  - tzdata=2025a=h04d1e81_0
  - wheel=0.45.1=py39h06a4308_0
  - xz=5.6.4=h5eee18b_1
  - zlib=1.2.13=h5eee18b_1
  - pip:
      - future==1.0.0
      - mock==1.0.1
      - psutil==7.0.0
      - pytz==2025.2
      - pyyaml==6.0.2
prefix: /opt/conda/envs/mobly

tests/mobly/utils_test.py::UtilsTest::test_stop_standing_subproc
tests/mobly/controllers/android_device_test.py::AndroidDeviceTest::test_AndroidDevice_build_info tests/mobly/controllers/android_device_test.py::AndroidDeviceTest::test_AndroidDevice_cat_adb_log tests/mobly/controllers/android_device_test.py::AndroidDeviceTest::test_AndroidDevice_debug_tag tests/mobly/controllers/android_device_test.py::AndroidDeviceTest::test_AndroidDevice_instantiation tests/mobly/controllers/android_device_test.py::AndroidDeviceTest::test_AndroidDevice_load_snippet tests/mobly/controllers/android_device_test.py::AndroidDeviceTest::test_AndroidDevice_load_snippet_dup_attribute_name tests/mobly/controllers/android_device_test.py::AndroidDeviceTest::test_AndroidDevice_load_snippet_dup_package tests/mobly/controllers/android_device_test.py::AndroidDeviceTest::test_AndroidDevice_load_snippet_dup_snippet_name tests/mobly/controllers/android_device_test.py::AndroidDeviceTest::test_AndroidDevice_snippet_cleanup tests/mobly/controllers/android_device_test.py::AndroidDeviceTest::test_AndroidDevice_take_bug_report tests/mobly/controllers/android_device_test.py::AndroidDeviceTest::test_AndroidDevice_take_bug_report_fail tests/mobly/controllers/android_device_test.py::AndroidDeviceTest::test_AndroidDevice_take_bug_report_fallback tests/mobly/controllers/android_device_test.py::AndroidDeviceTest::test_AndroidDevice_take_logcat tests/mobly/controllers/android_device_test.py::AndroidDeviceTest::test_AndroidDevice_take_logcat_with_user_param tests/mobly/controllers/android_device_test.py::AndroidDeviceTest::test_create_with_dict_list tests/mobly/controllers/android_device_test.py::AndroidDeviceTest::test_create_with_empty_config tests/mobly/controllers/android_device_test.py::AndroidDeviceTest::test_create_with_no_valid_config tests/mobly/controllers/android_device_test.py::AndroidDeviceTest::test_create_with_not_list_config tests/mobly/controllers/android_device_test.py::AndroidDeviceTest::test_create_with_pickup_all tests/mobly/controllers/android_device_test.py::AndroidDeviceTest::test_create_with_string_list tests/mobly/controllers/android_device_test.py::AndroidDeviceTest::test_get_device_no_match tests/mobly/controllers/android_device_test.py::AndroidDeviceTest::test_get_device_success_with_serial tests/mobly/controllers/android_device_test.py::AndroidDeviceTest::test_get_device_success_with_serial_and_extra_field tests/mobly/controllers/android_device_test.py::AndroidDeviceTest::test_get_device_too_many_matches tests/mobly/controllers/android_device_test.py::AndroidDeviceTest::test_start_services_on_ads tests/mobly/utils_test.py::UtilsTest::test_is_port_available_negative tests/mobly/utils_test.py::UtilsTest::test_is_port_available_positive tests/mobly/utils_test.py::UtilsTest::test_start_standing_subproc tests/mobly/utils_test.py::UtilsTest::test_stop_standing_subproc_already_dead
mobly/controllers/sniffer_lib/local/local_base.py mobly/utils.py
mobly/controllers/sniffer_lib/local/local_base.py mobly/utils.py