Repository: google/mobly

Issue:
`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`.

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


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


Test patch:
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(