Repository: google/mobly

Issue:
AdbLogExcerpts filename is invalid on Windows
Windows filenames should not have colons in them:
https://support.microsoft.com/en-us/help/905231/information-about-the-characters-that-you-cannot-use-in-site-names-fol

Discussion:
xpconanfan: 



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

---

*[mobly/logger.py, line 30 at r1](https://beta.reviewable.io/reviews/google/mobly/433#-L9rUD-H8deRk8kCggOb:-L9rUD-H8deRk8kCggOc:b-si4ejl) ([raw file](https://github.com/google/mobly/blob/ed7f9460e71e3f266925802c8007b4a044b64dab/mobly/logger.py#L30)):*
> ```Python
> # The micro seconds are added by the format string above,
> # so the time format does not include ms.
> log_line_time_format = '%m-%d %H:%M:%S'
> ```

why are there so many changes needed?
can't you simply change this template to use `-` instead of `:` on win?
I'd expect this to be a fairly small change.

---


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

winterfroststrom: 



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

---

*[mobly/logger.py, line 30 at r1](https://beta.reviewable.io/reviews/google/mobly/433#-L9rUD-H8deRk8kCggOb:-L9rVeGL144Q0V0yjdW8:b-ylpfxz) ([raw file](https://github.com/google/mobly/blob/ed7f9460e71e3f266925802c8007b4a044b64dab/mobly/logger.py#L30)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

why are there so many changes needed?
can't you simply change this template to use `-` instead of `:` on win?
I'd expect this to be a fairly small change.
</blockquote></details>

Wouldn't changing that would change all logging output?
If you think that's an okay thing to change, then I can go with that approach instead.

Also, I don't really want to special case this for Windows because that would mean that anything that consumes this stuff would also need to special case for Windows, which seems really non-ideal.

---


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

xpconanfan: 



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

---

*[mobly/controllers/android_device.py, line 940 at r1](https://beta.reviewable.io/reviews/google/mobly/433#-L9rY7nx5JR9tU-NmjVE:-L9rY7nx5JR9tU-NmjVF:b-sujf6i) ([raw file](https://github.com/google/mobly/blob/ed7f9460e71e3f266925802c8007b4a044b64dab/mobly/controllers/android_device.py#L940)):*
> ```Python
>         begin_time_file_format = mobly_logger.log_line_to_log_file_timestamp(
>             begin_time, year=year)
>         out_name = f_name.replace('adblog,', '').replace('.txt', '')
> ```

Let's just add another line here to replace the `:` with `-`, non-platform-specific?

---


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

winterfroststrom: 



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

---

*[mobly/controllers/android_device.py, line 940 at r1](https://beta.reviewable.io/reviews/google/mobly/433#-L9rY7nx5JR9tU-NmjVE:-L9rYoFK7tUkGZS_O9Of:b-feviw3) ([raw file](https://github.com/google/mobly/blob/ed7f9460e71e3f266925802c8007b4a044b64dab/mobly/controllers/android_device.py#L940)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

Let's just add another line here to replace the `:` with `-`, non-platform-specific?
</blockquote></details>

In that case, these excerpt log files won't match the general format and any format changes could be a bit haphazard?
Is that okay?

---


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

xpconanfan: 



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

---

*[mobly/controllers/android_device.py, line 940 at r1](https://beta.reviewable.io/reviews/google/mobly/433#-L9rY7nx5JR9tU-NmjVE:-L9raa-d54WwVx0DHq-I:bhtxqid) ([raw file](https://github.com/google/mobly/blob/ed7f9460e71e3f266925802c8007b4a044b64dab/mobly/controllers/android_device.py#L940)):*
<details><summary><i>Previously, winterfroststrom wrote…</i></summary><blockquote>

In that case, these excerpt log files won't match the general format and any format changes could be a bit haphazard?
Is that okay?
</blockquote></details>

what's the general format? do we have other files that have `:` in their names?

---


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

winterfroststrom: 



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

---

*[mobly/controllers/android_device.py, line 940 at r1](https://beta.reviewable.io/reviews/google/mobly/433#-L9rY7nx5JR9tU-NmjVE:-L9rbv9W4xRGxh8CWLxf:bcvkn56) ([raw file](https://github.com/google/mobly/blob/ed7f9460e71e3f266925802c8007b4a044b64dab/mobly/controllers/android_device.py#L940)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

what's the general format? do we have other files that have `:` in their names?
</blockquote></details>

I basically mean anything with get_log_file_timestamp, which seems to just be the logging directory and the sniffer client aside.

I'm not aware of any other filenames with ":" in their name atm.

---


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

xpconanfan: 



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

---

*[mobly/controllers/android_device.py, line 940 at r1](https://beta.reviewable.io/reviews/google/mobly/433#-L9rY7nx5JR9tU-NmjVE:-L9rf-5v2dhh9z7pAkVX:b-tflkmf) ([raw file](https://github.com/google/mobly/blob/ed7f9460e71e3f266925802c8007b4a044b64dab/mobly/controllers/android_device.py#L940)):*
<details><summary><i>Previously, winterfroststrom wrote…</i></summary><blockquote>

I basically mean anything with get_log_file_timestamp, which seems to just be the logging directory and the sniffer client aside.

I'm not aware of any other filenames with ":" in their name atm.
</blockquote></details>

You mean the sniffer client would have the same problem on win?

---


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

winterfroststrom: 



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

---

*[mobly/controllers/android_device.py, line 940 at r1](https://beta.reviewable.io/reviews/google/mobly/433#-L9rY7nx5JR9tU-NmjVE:-L9rfz0CBFLiuW8syDME:b-t4khsn) ([raw file](https://github.com/google/mobly/blob/ed7f9460e71e3f266925802c8007b4a044b64dab/mobly/controllers/android_device.py#L940)):*
> get_log_file_timestamp
No, I mean the design would be that anything that uses get_log_file_timestamp should be fine with any OS, which seems to be the case as is,
which makes for a more robust solution and standardized solution.

However, if that's not valuable, then I can go with the simpler solution.

---


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

xpconanfan: 



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

---

*[mobly/controllers/android_device.py, line 940 at r1](https://beta.reviewable.io/reviews/google/mobly/433#-L9rY7nx5JR9tU-NmjVE:-L9rhRqi9nO2gbTCait2:b-3gkpqu) ([raw file](https://github.com/google/mobly/blob/ed7f9460e71e3f266925802c8007b4a044b64dab/mobly/controllers/android_device.py#L940)):*
<details><summary><i>Previously, winterfroststrom wrote…</i></summary><blockquote>

> get_log_file_timestamp
No, I mean the design would be that anything that uses get_log_file_timestamp should be fine with any OS, which seems to be the case as is,
which makes for a more robust solution and standardized solution.

However, if that's not valuable, then I can go with the simpler solution.
</blockquote></details>

The `get_log_file_timestamp` works as expected, which is to get the timestamp in the format used *inside* adb logcat files.
The mistake here is `cat_adb_log` uses that timestamp in its file names....

---


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

winterfroststrom: 



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

---

*[mobly/controllers/android_device.py, line 940 at r1](https://beta.reviewable.io/reviews/google/mobly/433#-L9rY7nx5JR9tU-NmjVE:-L9rit0lEh-vWhiZ-LVu:b6uj05l) ([raw file](https://github.com/google/mobly/blob/ed7f9460e71e3f266925802c8007b4a044b64dab/mobly/controllers/android_device.py#L940)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

The `get_log_file_timestamp` works as expected, which is to get the timestamp in the format used *inside* adb logcat files.
The mistake here is `cat_adb_log` uses that timestamp in its file names....
</blockquote></details>

I'm confused.

I was under the impression that:
get_log_file_timestamp is used for human readable timestamps on log files
get_log_line_timestamp is used for logging inside of files and getting from them

I thought the mistake was using get_log_line_timestamp for the file name when get_log_file_timestamp is what should've been used;
hence all the logic for the conversion.

---


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

xpconanfan: 



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

---

*[mobly/controllers/android_device.py, line 940 at r1](https://beta.reviewable.io/reviews/google/mobly/433#-L9rY7nx5JR9tU-NmjVE:-L9rk7Kx5a0qYq5wnsgc:bkonyb3) ([raw file](https://github.com/google/mobly/blob/ed7f9460e71e3f266925802c8007b4a044b64dab/mobly/controllers/android_device.py#L940)):*
<details><summary><i>Previously, winterfroststrom wrote…</i></summary><blockquote>

I'm confused.

I was under the impression that:
get_log_file_timestamp is used for human readable timestamps on log files
get_log_line_timestamp is used for logging inside of files and getting from them

I thought the mistake was using get_log_line_timestamp for the file name when get_log_file_timestamp is what should've been used;
hence all the logic for the conversion.
</blockquote></details>

Sorry I misread...

Where did the `:` come from then?
Seems like it was supplied by the caller?

---


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

winterfroststrom: 



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

---

*[mobly/controllers/android_device.py, line 940 at r1](https://beta.reviewable.io/reviews/google/mobly/433#-L9rY7nx5JR9tU-NmjVE:-L9rkW7dAA8m5K5LqMdT:b-mes44j) ([raw file](https://github.com/google/mobly/blob/ed7f9460e71e3f266925802c8007b4a044b64dab/mobly/controllers/android_device.py#L940)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

Sorry I misread...

Where did the `:` come from then?
Seems like it was supplied by the caller?
</blockquote></details>

It was supplied by the caller via begin_time, which must match the format of get_log_line_timestamp.
The issue is that the format of get_log_line_timestamp includes a ":", which is not a valid character on Windows.

---


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

xpconanfan: 



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

---

*[mobly/controllers/android_device.py, line 940 at r1](https://beta.reviewable.io/reviews/google/mobly/433#-L9rY7nx5JR9tU-NmjVE:-L9rl1q08d5p9FESjY67:bt97oki) ([raw file](https://github.com/google/mobly/blob/ed7f9460e71e3f266925802c8007b4a044b64dab/mobly/controllers/android_device.py#L940)):*
<details><summary><i>Previously, winterfroststrom wrote…</i></summary><blockquote>

It was supplied by the caller via begin_time, which must match the format of get_log_line_timestamp.
The issue is that the format of get_log_line_timestamp includes a ":", which is not a valid character on Windows.
</blockquote></details>

Ok, so the actual bug is we shouldn't let users supply part of file name really...
Why do you have to match the timestamp format

---


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

winterfroststrom: 



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

---

*[mobly/controllers/android_device.py, line 940 at r1](https://beta.reviewable.io/reviews/google/mobly/433#-L9rY7nx5JR9tU-NmjVE:-L9rlJ698ZP3_p_dImhI:b-c95xsa) ([raw file](https://github.com/google/mobly/blob/ed7f9460e71e3f266925802c8007b4a044b64dab/mobly/controllers/android_device.py#L940)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

Ok, so the actual bug is we shouldn't let users supply part of file name really...
Why do you have to match the timestamp format
</blockquote></details>

Because the logic/purpose of this function is to create a shortened version of the adblog which only contains log lines from begin_time to current.
In the current logic, begin_time is used with _is_timestamp_in_range to determine whether or not a particular line should be included.
The logic of _is_timestamp_in_range uses logline_timestamp_comparator, which expects the log line format.

Users supplying begin_time should be fine because the logic depends on that.
The issue is that begin_time's format is not compatible with file systems, so it needs to be changed.
If a user gives a malformed begin_time, then that's just user error, which I think should be okay.
But since the log line format is not a valid file name format, this isn't a user error issue.

---


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

xpconanfan: 



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

---

*[mobly/controllers/android_device.py, line 940 at r1](https://beta.reviewable.io/reviews/google/mobly/433#-L9rY7nx5JR9tU-NmjVE:-L9rnj7oB1DbQLYv4DR3:b-v9o03b) ([raw file](https://github.com/google/mobly/blob/ed7f9460e71e3f266925802c8007b4a044b64dab/mobly/controllers/android_device.py#L940)):*
<details><summary><i>Previously, winterfroststrom wrote…</i></summary><blockquote>

Because the logic/purpose of this function is to create a shortened version of the adblog which only contains log lines from begin_time to current.
In the current logic, begin_time is used with _is_timestamp_in_range to determine whether or not a particular line should be included.
The logic of _is_timestamp_in_range uses logline_timestamp_comparator, which expects the log line format.

Users supplying begin_time should be fine because the logic depends on that.
The issue is that begin_time's format is not compatible with file systems, so it needs to be changed.
If a user gives a malformed begin_time, then that's just user error, which I think should be okay.
But since the log line format is not a valid file name format, this isn't a user error issue.
</blockquote></details>

I see.
I really want to get rid of this function eventually...

---


*Comments from [Reviewable](https://beta.reviewable.io/reviews/google/mobly/433)*
<!-- 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: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

---



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


Patch:
diff --git a/mobly/controllers/android_device.py b/mobly/controllers/android_device.py
index f1a4636..14828a4 100644
--- a/mobly/controllers/android_device.py
+++ b/mobly/controllers/android_device.py
@@ -436,9 +436,8 @@ class AndroidDevice(object):
         self._log_path = os.path.join(self._log_path_base,
                                       'AndroidDevice%s' % self._serial)
         self._debug_tag = self._serial
-        self.log = AndroidDeviceLoggerAdapter(logging.getLogger(), {
-            'tag': self.debug_tag
-        })
+        self.log = AndroidDeviceLoggerAdapter(logging.getLogger(),
+                                              {'tag': self.debug_tag})
         self.sl4a = None
         self.ed = None
         self._adb_logcat_process = None
@@ -937,6 +936,7 @@ class AndroidDevice(object):
         f_name = os.path.basename(self.adb_logcat_file_path)
         out_name = f_name.replace('adblog,', '').replace('.txt', '')
         out_name = ',%s,%s.txt' % (begin_time, out_name)
+        out_name = out_name.replace(':', '-')
         tag_len = utils.MAX_FILENAME_LEN - len(out_name)
         tag = tag[:tag_len]
         out_name = tag + out_name


Test patch:
diff --git a/tests/mobly/controllers/android_device_test.py b/tests/mobly/controllers/android_device_test.py
index b1428ec..f175f17 100755
--- a/tests/mobly/controllers/android_device_test.py
+++ b/tests/mobly/controllers/android_device_test.py
@@ -630,7 +630,7 @@ class AndroidDeviceTest(unittest.TestCase):
         ad.cat_adb_log('some_test', MOCK_ADB_LOGCAT_BEGIN_TIME)
         cat_file_path = os.path.join(
             ad.log_path, 'AdbLogExcerpts',
-            ('some_test,02-29 14:02:20.123,%s,%s.txt') % (ad.model, ad.serial))
+            ('some_test,02-29 14-02-20.123,%s,%s.txt') % (ad.model, ad.serial))
         with open(cat_file_path, 'r') as f:
             actual_cat = f.read()
         self.assertEqual(actual_cat, ''.join(MOCK_ADB_LOGCAT_CAT_RESULT))
diff --git a/tests/mobly/logger_test.py b/tests/mobly/logger_test.py
index b1cf839..1ac9f1d 100755
--- a/tests/mobly/logger_test.py
+++ b/tests/mobly/logger_test.py
@@ -1,11 +1,11 @@
 # Copyright 2016 Google Inc.
-# 
+#
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
 # You may obtain a copy of the License at
-# 
+#
 #     http://www.apache.org/licenses/LICENSE-2.0
-# 
+#
 # Unless required by applicable law or agreed to in writing, software
 # distributed under the License is distributed on an "AS IS" BASIS,
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.