dthkao: 



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

---

*[mobly/controllers/android_device.py, line 839 at r1](https://reviewable.io:443/reviews/google/mobly/328#-KszMvOnUpIQ_wuRiftL:-KszMvOnUpIQ_wuRiftM:b-dddf90) ([raw file](https://github.com/google/mobly/blob/41979c89e8b741071f4d90fe81cc0f8724c9dec1/mobly/controllers/android_device.py#L839)):*
> ```Python
>         except AttributeError:
>             extra_params = ''
>         cmd = '"%s" -s %s logcat -v threadtime %s >> %s' % (
> ```

Can we quote the file path in the literal rather than edit the string?

---


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

xpconanfan: 



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

---

*[mobly/controllers/android_device.py, line 839 at r1](https://reviewable.io:443/reviews/google/mobly/328#-KszMvOnUpIQ_wuRiftL:-KszOwCoUGOHTmyHnR4c:b-896fix) ([raw file](https://github.com/google/mobly/blob/41979c89e8b741071f4d90fe81cc0f8724c9dec1/mobly/controllers/android_device.py#L839)):*
<details><summary><i>Previously, dthkao (David T.H. Kao) wrote…</i></summary><blockquote>

Can we quote the file path in the literal rather than edit the string?
</blockquote></details>

Done.

---


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

dthkao: 



Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.

---

*[mobly/controllers/android_device.py, line 832 at r2](https://reviewable.io:443/reviews/google/mobly/328#-KszPom4LWZUJNxFsMzj:-KszPom4LWZUJNxFsMzk:bqebhii) ([raw file](https://github.com/google/mobly/blob/fb26406eb9181bf644ace98857e9f54bd2f53c5f/mobly/controllers/android_device.py#L832)):*
> ```Python
>             self.adb.shell('logpersist.stop --clear')
>             self.adb.shell('logpersist.start')
>         f_name = 'adblog,%s,%s.txt' % ('model a c', self.serial)
> ```

why this change?

---


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

xpconanfan: 



Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.

---

*[mobly/controllers/android_device.py, line 832 at r2](https://reviewable.io:443/reviews/google/mobly/328#-KszPom4LWZUJNxFsMzj:-KszR96tRrUELpuNC-IO:b-896fix) ([raw file](https://github.com/google/mobly/blob/fb26406eb9181bf644ace98857e9f54bd2f53c5f/mobly/controllers/android_device.py#L832)):*
<details><summary><i>Previously, dthkao (David T.H. Kao) wrote…</i></summary><blockquote>

why this change?
</blockquote></details>

Done.

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/google/mobly/328)*
<!-- 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 2 files reviewed at latest revision, 2 unresolved discussions.

---



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

k2fong: 



Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.

---

*[mobly/controllers/android_device.py, line 839 at r1](https://reviewable.io:443/reviews/google/mobly/328#-KszMvOnUpIQ_wuRiftL:-KtHnpJS_gLK3fm0aWke:b-4nkc0v) ([raw file](https://github.com/google/mobly/blob/41979c89e8b741071f4d90fe81cc0f8724c9dec1/mobly/controllers/android_device.py#L839)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

Done.
</blockquote></details>

why not have the quote inside the string literal?  seems like that'll follow how adb is done.

---


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

xpconanfan: 



Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.

---

*[mobly/controllers/android_device.py, line 839 at r1](https://reviewable.io:443/reviews/google/mobly/328#-KszMvOnUpIQ_wuRiftL:-KtHrazK6XDU2xpFjRls:b-896fix) ([raw file](https://github.com/google/mobly/blob/41979c89e8b741071f4d90fe81cc0f8724c9dec1/mobly/controllers/android_device.py#L839)):*
<details><summary><i>Previously, k2fong wrote…</i></summary><blockquote>

why not have the quote inside the string literal?  seems like that'll follow how adb is done.
</blockquote></details>

Done.

---


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

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

---

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.

---



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

diff --git a/mobly/controllers/android_device.py b/mobly/controllers/android_device.py
index 5d7027e..e52e5fd 100644
--- a/mobly/controllers/android_device.py
+++ b/mobly/controllers/android_device.py
@@ -836,10 +836,8 @@ class AndroidDevice(object):
             extra_params = self.adb_logcat_param
         except AttributeError:
             extra_params = ''
-        cmd = '"%s" -s %s logcat -v threadtime %s >> %s' % (adb.ADB,
-                                                            self.serial,
-                                                            extra_params,
-                                                            logcat_file_path)
+        cmd = '"%s" -s %s logcat -v threadtime %s >> "%s"' % (
+            adb.ADB, self.serial, extra_params, logcat_file_path)
         process = utils.start_standing_subprocess(cmd, shell=True)
         self._adb_logcat_process = process
         self.adb_logcat_file_path = logcat_file_path

`AndroidDevice` adb logcat file name gets truncated
If device model name includes space, the output file name of adb logcat is incorrect.
For example, for model "aqua power hd-4g", we expect `adblog,aqua power hd-4g,usb:3-5.2.3.txt`, but we actually get `adblog,aqua`.
diff --git a/tests/mobly/controllers/android_device_test.py b/tests/mobly/controllers/android_device_test.py
index 1849496..59c5529 100755
--- a/tests/mobly/controllers/android_device_test.py
+++ b/tests/mobly/controllers/android_device_test.py
@@ -326,7 +326,7 @@ class AndroidDeviceTest(unittest.TestCase):
         creat_dir_mock.assert_called_with(os.path.dirname(expected_log_path))
         adb_cmd = '"adb" -s %s logcat -v threadtime  >> %s'
         start_proc_mock.assert_called_with(
-            adb_cmd % (ad.serial, expected_log_path), shell=True)
+            adb_cmd % (ad.serial, '"%s"' % expected_log_path), shell=True)
         self.assertEqual(ad.adb_logcat_file_path, expected_log_path)
         expected_msg = (
             'Logcat thread is already running, cannot start another'
@@ -373,7 +373,7 @@ class AndroidDeviceTest(unittest.TestCase):
         creat_dir_mock.assert_called_with(os.path.dirname(expected_log_path))
         adb_cmd = '"adb" -s %s logcat -v threadtime -b radio >> %s'
         start_proc_mock.assert_called_with(
-            adb_cmd % (ad.serial, expected_log_path), shell=True)
+            adb_cmd % (ad.serial, '"%s"' % expected_log_path), shell=True)
         self.assertEqual(ad.adb_logcat_file_path, expected_log_path)
 
     @mock.patch(

pip install -e .[dev]
parse_log_pytest
pytest
pytest
apt-get update apt-get install -y gcc apt-get install -y adb
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@51c912d1a8ffd5ace4a9a744a46498515cf5c145#egg=mobly
mock==1.0.1
packaging @ file:///croot/packaging_1734472117206/work
pluggy @ file:///croot/pluggy_1733169602837/work
portpicker==1.6.0
psutil==7.0.0
pytest @ file:///croot/pytest_1738938843180/work
pytz==2025.2
PyYAML==6.0.2
timeout-decorator==0.5.0
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
      - portpicker==1.6.0
      - psutil==7.0.0
      - pytz==2025.2
      - pyyaml==6.0.2
      - timeout-decorator==0.5.0
prefix: /opt/conda/envs/mobly

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_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_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_create_with_usb_id 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
mobly/controllers/android_device.py
mobly/controllers/android_device.py