Repository: google/mobly

Issue:
New summary output.
The current summary log has limited debug information and is in json.

We need to consider what other useful information we can include in the summary log.
Like information on the invocation status, and device monitoring info.

Also consider using a different format so it's possible to stream this file instead of only writing it out in the end.
This will make the summary log more crash proof and takes less memory.

Discussion:
dthkao: 



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

---

*[mobly/base_test.py, line 90 at r1](https://reviewable.io:443/reviews/google/mobly/246#-KolBHB_6qgNLwbiX5DP:-KolBHB_6qgNLwbiX5DQ:b-3ck3ax) ([raw file](https://github.com/google/mobly/blob/2a5db1715f86f0c48c840f38fc594733fba709d1/mobly/base_test.py#L90)):*
> ```Python
>         self.register_controller = configs.register_controller
>         self.results = records.TestResult()
>         self.summary_writer = configs.summary_writer
> ```

why not provide the option for someone to provide an alternate summary writer (e.g., xml)?

---

*[mobly/logger.py, line 172 at r1](https://reviewable.io:443/reviews/google/mobly/246#-KolAdOnosuYsrZtk8QR:-KolAdOnosuYsrZtk8QS:b43g1d3) ([raw file](https://github.com/google/mobly/blob/2a5db1715f86f0c48c840f38fc594733fba709d1/mobly/logger.py#L172)):*
> ```Python
>     log.addHandler(fh)
>     # Write logger output to files
>     fh_info = logging.FileHandler(os.path.join(log_path, 'test_log.INFO'))
> ```

filenames to module constants

---

*[mobly/test_runner.py, line 302 at r1](https://reviewable.io:443/reviews/google/mobly/246#-Kol979fZAxltFe1-WH_:-Kol979fZAxltFe1-WHa:b-tmmajb) ([raw file](https://github.com/google/mobly/blob/2a5db1715f86f0c48c840f38fc594733fba709d1/mobly/test_runner.py#L302)):*
> ```Python
>         log_path = os.path.join(self._log_dir, self._test_bed_name, start_time)
>         summary_writer = records.TestSummaryWriter(
>             os.path.join(log_path, 'test_summary.yml'))
> ```

official extension is '.yaml'. Also move string to constant?

---

*[mobly/test_runner.py, line 303 at r1](https://reviewable.io:443/reviews/google/mobly/246#-KolB5WY-9olsjILQnli:-KolB5WY-9olsjILQnlj:bnq3578) ([raw file](https://github.com/google/mobly/blob/2a5db1715f86f0c48c840f38fc594733fba709d1/mobly/test_runner.py#L303)):*
> ```Python
>         summary_writer = records.TestSummaryWriter(
>             os.path.join(log_path, 'test_summary.yml'))
>         print(log_path)
> ```

debugging? why print?

---


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

xpconanfan: 



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

---

*[mobly/base_test.py, line 90 at r1](https://reviewable.io:443/reviews/google/mobly/246#-KolBHB_6qgNLwbiX5DP:-KomaeyRNY3h__TUDDIn:b-wwysz7) ([raw file](https://github.com/google/mobly/blob/2a5db1715f86f0c48c840f38fc594733fba709d1/mobly/base_test.py#L90)):*
<details><summary><i>Previously, dthkao (David T.H. Kao) wrote…</i></summary><blockquote>

why not provide the option for someone to provide an alternate summary writer (e.g., xml)?
</blockquote></details>

That's out of the scope of this change, which focuses on output file changes.

---

*[mobly/logger.py, line 172 at r1](https://reviewable.io:443/reviews/google/mobly/246#-KolAdOnosuYsrZtk8QR:-KomcdAdKTVWxLwdeRa7:b-896fix) ([raw file](https://github.com/google/mobly/blob/2a5db1715f86f0c48c840f38fc594733fba709d1/mobly/logger.py#L172)):*
<details><summary><i>Previously, dthkao (David T.H. Kao) wrote…</i></summary><blockquote>

filenames to module constants
</blockquote></details>

Done.

---

*[mobly/test_runner.py, line 302 at r1](https://reviewable.io:443/reviews/google/mobly/246#-Kol979fZAxltFe1-WH_:-KomccE_WdEKHasrR22p:b-896fix) ([raw file](https://github.com/google/mobly/blob/2a5db1715f86f0c48c840f38fc594733fba709d1/mobly/test_runner.py#L302)):*
<details><summary><i>Previously, dthkao (David T.H. Kao) wrote…</i></summary><blockquote>

official extension is '.yaml'. Also move string to constant?
</blockquote></details>

Done.

---

*[mobly/test_runner.py, line 303 at r1](https://reviewable.io:443/reviews/google/mobly/246#-KolB5WY-9olsjILQnli:-Komb7m4w4kWy2Cxuhq5:b-896fix) ([raw file](https://github.com/google/mobly/blob/2a5db1715f86f0c48c840f38fc594733fba709d1/mobly/test_runner.py#L303)):*
<details><summary><i>Previously, dthkao (David T.H. Kao) wrote…</i></summary><blockquote>

debugging? why print?
</blockquote></details>

Done.

---


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

dthkao: 



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

---

*[mobly/base_test.py, line 90 at r1](https://reviewable.io:443/reviews/google/mobly/246#-KolBHB_6qgNLwbiX5DP:-Kos4vjDDTDnA_P0TY78:b-tatxse) ([raw file](https://github.com/google/mobly/blob/2a5db1715f86f0c48c840f38fc594733fba709d1/mobly/base_test.py#L90)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

That's out of the scope of this change, which focuses on output file changes.
</blockquote></details>

Is tat something we should track in a separate issue for followup?

---


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

xpconanfan: 



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

---

*[mobly/base_test.py, line 90 at r1](https://reviewable.io:443/reviews/google/mobly/246#-KolBHB_6qgNLwbiX5DP:-Kos5x8wgLOKHkRA4289:b-grw326) ([raw file](https://github.com/google/mobly/blob/2a5db1715f86f0c48c840f38fc594733fba709d1/mobly/base_test.py#L90)):*
<details><summary><i>Previously, dthkao (David T.H. Kao) wrote…</i></summary><blockquote>

Is tat something we should track in a separate issue for followup?
</blockquote></details>

I do not see a use case atm. Feel free to file a request if you have a use case.

---


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

dthkao: 



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

---

*[mobly/base_test.py, line 90 at r1](https://reviewable.io:443/reviews/google/mobly/246#-KolBHB_6qgNLwbiX5DP:-KosCErPuUc2M0VjbX4v:bud97ry) ([raw file](https://github.com/google/mobly/blob/2a5db1715f86f0c48c840f38fc594733fba709d1/mobly/base_test.py#L90)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

I do not see a use case atm. Feel free to file a request if you have a use case.
</blockquote></details>

#259  filed.

---


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

k2fong: 



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

---

*[mobly/base_test.py, line 90 at r2](https://reviewable.io:443/reviews/google/mobly/246#-KpqJ7C8fQWqsl9MIICt:-KpqJ5xoR-hXMaWo-8Gl:b-igzdwz) ([raw file](https://github.com/google/mobly/blob/6502b3ca6437dfc940de5dac8c31647cad271685/mobly/base_test.py#L90)):*
> self.summary_writer = configs.summary_writer
Since, there is a data object test_config to store the above.  Will it make sense for BaseTestClass to store a single test_config reference instead having its own duplicate set of config entries (log_path, summary_writer, ...).  Do so should reduce the need for the changes like summary_writer here.

---

*[mobly/logger.py, line 166 at r2](https://reviewable.io:443/reviews/google/mobly/246#-KpqJRO6K1z0lepTuRlf:-KpqJRO7iDVy9YCrxfyY:b-hedunp) ([raw file](https://github.com/google/mobly/blob/6502b3ca6437dfc940de5dac8c31647cad271685/mobly/logger.py#L166)):*
> ```Python
>         filename = get_log_file_timestamp()
>         utils.create_dir(log_path)
>     # TODO(angli): Deprecate `test_run_details.txt` when we remove old output
> ```

TODO(xpconanfan)?  else where as well

---

*[mobly/records.py, line 36 at r2](https://reviewable.io:443/reviews/google/mobly/246#-KpqNQUQ01G6rAxA3O2J:-KpqNQUQ01G6rAxA3O2K:b-247gvw) ([raw file](https://github.com/google/mobly/blob/6502b3ca6437dfc940de5dac8c31647cad271685/mobly/records.py#L36)):*
> Enums
'Enum' to 'Constants'?  But is there some reason why not make TestSummaryEntryType an enum instead?

---

*[mobly/records.py, line 88 at r2](https://reviewable.io:443/reviews/google/mobly/246#-KpqOY-Xmtwp8s4NfqVN:-KpqOY-Xmtwp8s4NfqVO:b3326yt) ([raw file](https://github.com/google/mobly/blob/6502b3ca6437dfc940de5dac8c31647cad271685/mobly/records.py#L88)):*
> ```Python
>         """
>         new_content = copy.deepcopy(content)
>         new_content['Type'] = entry_type
> ```

verify entry_type is a member of TestSummaryEntryType.  But I suggest making TestSummaryEntryType as an enum so this kind of validation can be omitted.

---

*[mobly/test_runner.py, line 446 at r2](https://reviewable.io:443/reviews/google/mobly/246#-KpqR-M0YcLjp6_SpPD6:-KpqR-M0YcLjp6_SpPD7:bmi5x2f) ([raw file](https://github.com/google/mobly/blob/6502b3ca6437dfc940de5dac8c31647cad271685/mobly/test_runner.py#L446)):*
> ```Python
>         """Writes out a json file with the test result info for easy parsing.
> 
>         # TODO(angli): Deprecate with old output format.
> ```

Seems like there is some inconsistency with the TODO() comment inside a docstring (with '#' vs  without '#')

---


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

xpconanfan: 



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

---

*[mobly/logger.py, line 166 at r2](https://reviewable.io:443/reviews/google/mobly/246#-KpqJRO6K1z0lepTuRlf:-KpqXqiBPjLL4G_vISDz:b-9kd1do) ([raw file](https://github.com/google/mobly/blob/6502b3ca6437dfc940de5dac8c31647cad271685/mobly/logger.py#L166)):*
<details><summary><i>Previously, k2fong wrote…</i></summary><blockquote>

TODO(xpconanfan)?  else where as well
</blockquote></details>

Done.
Tracking with #270 

---

*[mobly/records.py, line 36 at r2](https://reviewable.io:443/reviews/google/mobly/246#-KpqNQUQ01G6rAxA3O2J:-KpqY9PZPtCHRSznjEVb:b-896fix) ([raw file](https://github.com/google/mobly/blob/6502b3ca6437dfc940de5dac8c31647cad271685/mobly/records.py#L36)):*
<details><summary><i>Previously, k2fong wrote…</i></summary><blockquote>

> Enums
'Enum' to 'Constants'?  But is there some reason why not make TestSummaryEntryType an enum instead?
</blockquote></details>

Done.

---

*[mobly/records.py, line 88 at r2](https://reviewable.io:443/reviews/google/mobly/246#-KpqOY-Xmtwp8s4NfqVN:-Kpqdk5r3N90oVeoNLVt:b-896fix) ([raw file](https://github.com/google/mobly/blob/6502b3ca6437dfc940de5dac8c31647cad271685/mobly/records.py#L88)):*
<details><summary><i>Previously, k2fong wrote…</i></summary><blockquote>

verify entry_type is a member of TestSummaryEntryType.  But I suggest making TestSummaryEntryType as an enum so this kind of validation can be omitted.
</blockquote></details>

Done.

---

*[mobly/test_runner.py, line 446 at r2](https://reviewable.io:443/reviews/google/mobly/246#-KpqR-M0YcLjp6_SpPD6:-Kpqdw63XaQIRCDhC3U7:b-896fix) ([raw file](https://github.com/google/mobly/blob/6502b3ca6437dfc940de5dac8c31647cad271685/mobly/test_runner.py#L446)):*
<details><summary><i>Previously, k2fong wrote…</i></summary><blockquote>

Seems like there is some inconsistency with the TODO() comment inside a docstring (with '#' vs  without '#')
</blockquote></details>

Done.

---


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

k2fong: 



Review status: 0 of 8 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

*[mobly/records.py, line 88 at r2](https://reviewable.io:443/reviews/google/mobly/246#-KpqOY-Xmtwp8s4NfqVN:-KpucYmWwY5JOEaZa1EW:b-wfwycm) ([raw file](https://github.com/google/mobly/blob/6502b3ca6437dfc940de5dac8c31647cad271685/mobly/records.py#L88)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

Done.
</blockquote></details>

I don't believe entry_type of type string will yield what you are looking for with isinstance against TestSummaryEntryType.  Can dump() take in the new enum type instead?  Actually, looks like you are already using it as an enum, so maybe just need to update the Args comment section above.

---

*[mobly/records.py, line 94 at r3](https://reviewable.io:443/reviews/google/mobly/246#-KpudrOlxxutDjla1vtQ:-KpudrOlxxutDjla1vtR:bvim0xj) ([raw file](https://github.com/google/mobly/blob/2a7b849d78d34ba48dd1578579110af0ff33b589/mobly/records.py#L94)):*
>         if not isinstance(entry_type, TestSummaryEntryType):
>             raise Error('%s is not a valid entry type, see records.'
>                         'TestSummaryEntryType.' % entry_type)
I would suggest not having to check against the pass in type.  Not typical in Python from my experience.

---


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

xpconanfan: 



Review status: 0 of 8 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

*[mobly/records.py, line 88 at r2](https://reviewable.io:443/reviews/google/mobly/246#-KpqOY-Xmtwp8s4NfqVN:-KpuuHNDEs4vuOsKs5rt:b-896fix) ([raw file](https://github.com/google/mobly/blob/6502b3ca6437dfc940de5dac8c31647cad271685/mobly/records.py#L88)):*
<details><summary><i>Previously, k2fong wrote…</i></summary><blockquote>

I don't believe entry_type of type string will yield what you are looking for with isinstance against TestSummaryEntryType.  Can dump() take in the new enum type instead?  Actually, looks like you are already using it as an enum, so maybe just need to update the Args comment section above.
</blockquote></details>

Done.

---

*[mobly/records.py, line 94 at r3](https://reviewable.io:443/reviews/google/mobly/246#-KpudrOlxxutDjla1vtQ:-KpuuGv2qH_8WtEkdVEV:b-896fix) ([raw file](https://github.com/google/mobly/blob/2a7b849d78d34ba48dd1578579110af0ff33b589/mobly/records.py#L94)):*
<details><summary><i>Previously, k2fong wrote…</i></summary><blockquote>

>         if not isinstance(entry_type, TestSummaryEntryType):
>             raise Error('%s is not a valid entry type, see records.'
>                         'TestSummaryEntryType.' % entry_type)
I would suggest not having to check against the pass in type.  Not typical in Python from my experience.
</blockquote></details>

Done.

---


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

xpconanfan: 



Review status: 0 of 8 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

*[mobly/base_test.py, line 90 at r2](https://reviewable.io:443/reviews/google/mobly/246#-KpqJ7C8fQWqsl9MIICt:-KpuwkUm9_3K_UZKATEJ:b-8bmi7g) ([raw file](https://github.com/google/mobly/blob/6502b3ca6437dfc940de5dac8c31647cad271685/mobly/base_test.py#L90)):*
<details><summary><i>Previously, k2fong wrote…</i></summary><blockquote>

> self.summary_writer = configs.summary_writer
Since, there is a data object test_config to store the above.  Will it make sense for BaseTestClass to store a single test_config reference instead having its own duplicate set of config entries (log_path, summary_writer, ...).  Do so should reduce the need for the changes like summary_writer here.
</blockquote></details>

We actually intentionally set these attributes to `BaseTestClass` itself so the tests can use them directly via `self.xxx`

---


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

kdart: 



Review status: 0 of 8 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

*[mobly/logger.py, line 183 at r3](https://reviewable.io:443/reviews/google/mobly/246#-KpuxQZ7pl-EiB1TJPII:-KpuxQZ7pl-EiB1TJPIJ:b-b9j9jd) ([raw file](https://github.com/google/mobly/blob/2a7b849d78d34ba48dd1578579110af0ff33b589/mobly/logger.py#L183)):*
> ```Python
>     log.addHandler(ch)
>     log.addHandler(fh_info)
>     log.addHandler(fh_debug)
> ```

Could this splitting be made user configurable? I'm not sure everyone would want to split INFO and DEBUG this way. It makes some events harder to correlate. If they are combined into one stream then it's a little faster, and still easily filtered with reporting tools (a simple grep could be sufficient). 

---

*[mobly/records.py, line 94 at r3](https://reviewable.io:443/reviews/google/mobly/246#-KpudrOlxxutDjla1vtQ:-Kpuvmv9pS7D_gqvu6kJ:b5vf8xr) ([raw file](https://github.com/google/mobly/blob/2a7b849d78d34ba48dd1578579110af0ff33b589/mobly/records.py#L94)):*
<details><summary><i>Previously, k2fong wrote…</i></summary><blockquote>

>         if not isinstance(entry_type, TestSummaryEntryType):
>             raise Error('%s is not a valid entry type, see records.'
>                         'TestSummaryEntryType.' % entry_type)
I would suggest not having to check against the pass in type.  Not typical in Python from my experience.
</blockquote></details>

isinstance checks are actually pretty common. Over 1400 library modules in my system contain it. 

---


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

xpconanfan: 



Review status: 0 of 8 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

*[mobly/logger.py, line 183 at r3](https://reviewable.io:443/reviews/google/mobly/246#-KpuxQZ7pl-EiB1TJPII:-Kpuyq_zjebb3WgmDa6N:b-kzwhfq) ([raw file](https://github.com/google/mobly/blob/2a7b849d78d34ba48dd1578579110af0ff33b589/mobly/logger.py#L183)):*
<details><summary><i>Previously, kdart (Keith Dart) wrote…</i></summary><blockquote>

Could this splitting be made user configurable? I'm not sure everyone would want to split INFO and DEBUG this way. It makes some events harder to correlate. If they are combined into one stream then it's a little faster, and still easily filtered with reporting tools (a simple grep could be sufficient). 
</blockquote></details>

The current one dumps everything together.
What we've found is that it's very difficult to read when debugging an app, considering every single adb call is logged.

So the DEBUG one has all the lines, whereas the INFO one would only have the ones displayed in console (console outputs INFO level).

---


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

kdart: After talking to Ang, I see that in this context it's a reasonable short-term fix. 

---

Review status: 0 of 8 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

*[mobly/logger.py, line 183 at r3](https://reviewable.io:443/reviews/google/mobly/246#-KpuxQZ7pl-EiB1TJPII:-Kpv_5M7wjjKXJRVrQiw:b6dk7we) ([raw file](https://github.com/google/mobly/blob/2a7b849d78d34ba48dd1578579110af0ff33b589/mobly/logger.py#L183)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

The current one dumps everything together.
What we've found is that it's very difficult to read when debugging an app, considering every single adb call is logged.

So the DEBUG one has all the lines, whereas the INFO one would only have the ones displayed in console (console outputs INFO level).
</blockquote></details>

OK, now I see that our debug level log also has stderr and other cruft from Android. 

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/google/mobly/246#-:-KpvaQ_lvp_bs_HxQqsH:b-4n6nrl)*
<!-- Sent from Reviewable.io -->

xpconanfan: There is no Ang, there is only xpconanfan :P 

---

Review status: 0 of 8 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---



*Comments from [Reviewable](https://reviewable.io:443/reviews/google/mobly/246#-:-Kpvagv4qLXfIPkCXWlP:b-jgmvdo)*
<!-- Sent from Reviewable.io -->

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

---

Reviewed 1 of 7 files at r1, 3 of 6 files at r2, 2 of 4 files at r3, 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---



*Comments from [Reviewable](https://reviewable.io:443/reviews/google/mobly/246#-:-KpvclZAu9cDdf2S6Jt4:bnfp4nl)*
<!-- 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: 5 of 8 files reviewed at latest revision, 5 unresolved discussions.

---



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


Patch:
diff --git a/mobly/config_parser.py b/mobly/config_parser.py
index 094777a..9d84a3a 100644
--- a/mobly/config_parser.py
+++ b/mobly/config_parser.py
@@ -37,8 +37,8 @@ def _validate_test_config(test_config):
     """
     required_key = keys.Config.key_testbed.value
     if required_key not in test_config:
-        raise MoblyConfigError('Required key %s missing in test config.' %
-                               required_key)
+        raise MoblyConfigError(
+            'Required key %s missing in test config.' % required_key)
 
 
 def _validate_testbed_name(name):
@@ -109,8 +109,8 @@ def load_test_config_file(test_config_path, tb_filters=None):
         if len(tbs) != len(tb_filters):
             raise MoblyConfigError(
                 'Expect to find %d test bed configs, found %d. Check if'
-                ' you have the correct test bed names.' %
-                (len(tb_filters), len(tbs)))
+                ' you have the correct test bed names.' % (len(tb_filters),
+                                                           len(tbs)))
         configs[keys.Config.key_testbed.value] = tbs
     mobly_params = configs.get(keys.Config.key_mobly_params.value, {})
     # Decide log path.
@@ -166,6 +166,8 @@ class TestRunConfig(object):
         user_params: dict, all the parameters to be consumed by the test logic.
         register_controller: func, used by test classes to register controller
                              modules.
+        summary_writer: records.TestSummaryWriter, used to write elements to
+                        the test result summary file.
     """
 
     def __init__(self):
@@ -174,6 +176,7 @@ class TestRunConfig(object):
         self.controller_configs = None
         self.user_params = None
         self.register_controller = None
+        self.summary_writer = None
 
     def copy(self):
         """Returns a deep copy of the current config.
diff --git a/mobly/logger.py b/mobly/logger.py
index 1bd2eb4..de90d6a 100644
--- a/mobly/logger.py
+++ b/mobly/logger.py
@@ -20,6 +20,7 @@ import os
 import re
 import sys
 
+from mobly import records
 from mobly import utils
 
 log_line_format = '%(asctime)s.%(msecs).03d %(levelname)s %(message)s'
@@ -162,11 +163,24 @@ def _setup_test_logger(log_path, prefix=None, filename=None):
     if filename is None:
         filename = get_log_file_timestamp()
         utils.create_dir(log_path)
+    # TODO(#270): Deprecate `test_run_details.txt` when we remove old output
+    # format support.
     fh = logging.FileHandler(os.path.join(log_path, 'test_run_details.txt'))
     fh.setFormatter(f_formatter)
     fh.setLevel(logging.DEBUG)
-    log.addHandler(ch)
     log.addHandler(fh)
+    # Write logger output to files
+    fh_info = logging.FileHandler(
+        os.path.join(log_path, records.OUTPUT_FILE_INFO_LOG))
+    fh_info.setFormatter(f_formatter)
+    fh_info.setLevel(logging.INFO)
+    fh_debug = logging.FileHandler(
+        os.path.join(log_path, records.OUTPUT_FILE_DEBUG_LOG))
+    fh_debug.setFormatter(f_formatter)
+    fh_debug.setLevel(logging.DEBUG)
+    log.addHandler(ch)
+    log.addHandler(fh_info)
+    log.addHandler(fh_debug)
     log.log_path = log_path
     logging.log_path = log_path
 
@@ -224,4 +238,3 @@ def normalize_log_line_timestamp(log_line_timestamp):
     norm_tp = log_line_timestamp.replace(' ', '_')
     norm_tp = norm_tp.replace(':', '-')
     return norm_tp
-
diff --git a/mobly/records.py b/mobly/records.py
index 0b67c9d..2bf050e 100644
--- a/mobly/records.py
+++ b/mobly/records.py
@@ -11,19 +11,88 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
-"""This module is where all the record definitions and record containers live.
+"""This module has classes for test result collection, and test result output.
 """
 
 import itertools
+import copy
+import enum
 import json
 import logging
 import pprint
 import sys
 import traceback
+import yaml
 
 from mobly import signals
 from mobly import utils
 
+# File names for the default files output by 
+OUTPUT_FILE_INFO_LOG = 'test_log.INFO'
+OUTPUT_FILE_DEBUG_LOG = 'test_log.DEBUG'
+OUTPUT_FILE_SUMMARY = 'test_summary.yaml'
+
+
+class TestSummaryEntryType(enum.Enum):
+    """Constants used to identify the type of entries in test summary file.
+
+    Test summary file contains multiple yaml documents. In order to parse this
+    file efficiently, the write adds the type of each entry when it writes the
+    entry to the file.
+
+    The idea is similar to how `TestResult.json_str` categorizes different
+    sections of a `TestResult` object in the serialized format.
+    """
+    RECORD = 'Record'
+    SUMMARY = 'Summary'
+    CONTROLLER_INFO = 'ControllerInfo'
+
+
+class Error(Exception):
+    """Raised for errors in records."""
+
+
+class TestSummaryWriter(object):
+    """Writer for the test result summary file of a test run.
+
+    For each test run, a writer is created to stream test results to the
+    summary file on disk.
+
+    The serialization and writing of the `TestResult` object is intentionally
+    kept out of `TestResult` class and put in this class. Because `TestResult`
+    can be operated on by suites, like `+` operation, and it is difficult to
+    guarantee the consistency between `TestResult` in memory and the files on
+    disk. Also, this separation makes it easier to provide a more generic way
+    for users to consume the test summary, like via a database instead of a
+    file.
+    """
+
+    def __init__(self, path):
+        self._path = path
+
+    def dump(self, content, entry_type):
+        """Dumps a dictionary as a yaml document to the summary file.
+
+        Each call to this method dumps a separate yaml document to the same
+        summary file associated with a test run.
+
+        The content of the dumped dictionary has an extra field `TYPE` that
+        specifies the type of each yaml document, which is the flag for parsers
+        to identify each document.
+
+        Args:
+            content: dictionary, the content to serialize and write.
+            entry_type: a member of enum TestSummaryEntryType.
+
+        Raises:
+            recoreds.Error is raised if an invalid entry type is passed in.
+        """
+        new_content = copy.deepcopy(content)
+        new_content['Type'] = entry_type.value
+        content_str = yaml.dump(new_content, explicit_start=True, indent=4)
+        with open(self._path, 'a') as f:
+            f.write(content_str)
+
 
 class TestResultEnums(object):
     """Enums used for TestResultRecord class.
@@ -188,6 +257,8 @@ class TestResultRecord(object):
     def json_str(self):
         """Converts this test record to a string in json format.
 
+        TODO(#270): Deprecate with old output format.
+
         Format of the json string is:
             {
                 'Test Name': <test name>,
@@ -251,7 +322,7 @@ class TestResult(object):
                 # '+' operator for TestResult is only valid when multiple
                 # TestResult objs were created in the same test run, which means
                 # the controller info would be the same across all of them.
-                # TODO(angli): have a better way to validate this situation.
+                # TODO(xpconanfan): have a better way to validate this situation.
                 setattr(sum_result, name, l_value)
         return sum_result
 
@@ -276,9 +347,9 @@ class TestResult(object):
 
     def add_controller_info(self, name, info):
         try:
-            json.dumps(info)
+            yaml.dump(info)
         except TypeError:
-            logging.warning('Controller info for %s is not JSON serializable!'
+            logging.warning('Controller info for %s is not YAML serializable!'
                             ' Coercing it to string.' % name)
             self.controller_info[name] = str(info)
             return
@@ -323,6 +394,8 @@ class TestResult(object):
     def json_str(self):
         """Converts this test result to a string in json format.
 
+        TODO(#270): Deprecate with old output format.
+
         Format of the json string is:
             {
                 'Results': [


Test patch:
diff --git a/mobly/base_test.py b/mobly/base_test.py
index 7353afd..ece150a 100644
--- a/mobly/base_test.py
+++ b/mobly/base_test.py
@@ -87,6 +87,7 @@ class BaseTestClass(object):
         self.user_params = configs.user_params
         self.register_controller = configs.register_controller
         self.results = records.TestResult()
+        self.summary_writer = configs.summary_writer
         self.current_test_name = None
         self._generated_test_table = collections.OrderedDict()
 
@@ -383,6 +384,8 @@ class BaseTestClass(object):
             elif tr_record.result == records.TestResultEnums.TEST_RESULT_SKIP:
                 self._exec_procedure_func(self._on_skip, tr_record)
             self.results.add_record(tr_record)
+            self.summary_writer.dump(tr_record.to_dict(),
+                                     records.TestSummaryEntryType.RECORD)
 
     def _assert_function_name_in_stack(self, expected_func_name):
         """Asserts that the current stack contains the given function name."""
@@ -504,6 +507,8 @@ class BaseTestClass(object):
                 test_record = records.TestResultRecord(test_name, self.TAG)
                 test_record.test_skip(exception)
                 self.results.add_record(test_record)
+                self.summary_writer.dump(test_record.to_dict(),
+                                         records.TestSummaryEntryType.RECORD)
 
     def run(self, test_names=None):
         """Runs tests within a test class.
@@ -534,6 +539,8 @@ class BaseTestClass(object):
             class_record.test_begin()
             class_record.test_error(e)
             self.results.add_class_error(class_record)
+            self.summary_writer.dump(class_record.to_dict(),
+                                     records.TestSummaryEntryType.RECORD)
             return self.results
         logging.info('==========> %s <==========', self.TAG)
         # Devise the actual test methods to run in the test class.
@@ -565,6 +572,8 @@ class BaseTestClass(object):
             class_record.test_error(e)
             self._exec_procedure_func(self._on_fail, class_record)
             self.results.add_class_error(class_record)
+            self.summary_writer.dump(class_record.to_dict(),
+                                     records.TestSummaryEntryType.RECORD)
             self._skip_remaining_tests(e)
             self._safe_exec_func(self.teardown_class)
             return self.results
diff --git a/mobly/test_runner.py b/mobly/test_runner.py
index 5ae2767..8f9aba4 100644
--- a/mobly/test_runner.py
+++ b/mobly/test_runner.py
@@ -158,7 +158,7 @@ def verify_controller_module(module):
 
         def get_info(objects):
             [Optional] Gets info from the controller objects used in a test
-            run. The info will be included in test_result_summary.json under
+            run. The info will be included in test_summary.yaml under
             the key 'ControllerInfo'. Such information could include unique
             ID, version, or anything that could be useful for describing the
             test bed and debugging.
@@ -262,9 +262,7 @@ class TestRunner(object):
                 (self._test_bed_name, config.test_bed_name))
         self._test_run_infos.append(
             TestRunner._TestRunInfo(
-                config=config,
-                test_class=test_class,
-                tests=tests))
+                config=config, test_class=test_class, tests=tests))
 
     def _run_test_class(self, config, test_class, tests=None):
         """Instantiates and executes a test class.
@@ -300,6 +298,8 @@ class TestRunner(object):
             raise Error('No tests to execute.')
         start_time = logger.get_log_file_timestamp()
         log_path = os.path.join(self._log_dir, self._test_bed_name, start_time)
+        summary_writer = records.TestSummaryWriter(
+            os.path.join(log_path, records.OUTPUT_FILE_SUMMARY))
         logger.setup_test_logger(log_path, self._test_bed_name)
         try:
             for test_run_info in self._test_run_infos:
@@ -308,7 +308,7 @@ class TestRunner(object):
                 test_config.log_path = log_path
                 test_config.register_controller = functools.partial(
                     self._register_controller, test_config)
-
+                test_config.summary_writer = summary_writer
                 try:
                     self._run_test_class(test_config, test_run_info.test_class,
                                          test_run_info.tests)
@@ -319,6 +319,11 @@ class TestRunner(object):
                 finally:
                     self._unregister_controllers()
         finally:
+            # Write controller info and summary to summary file.
+            summary_writer.dump(self.results.controller_info,
+                                records.TestSummaryEntryType.CONTROLLER_INFO)
+            summary_writer.dump(self.results.summary_dict(),
+                                records.TestSummaryEntryType.SUMMARY)
             # Stop and show summary.
             msg = '\nSummary for test run %s@%s: %s\n' % (
                 self._test_bed_name, start_time, self.results.summary_str())
@@ -438,7 +443,7 @@ class TestRunner(object):
     def _write_results_json_str(self, log_path):
         """Writes out a json file with the test result info for easy parsing.
 
-        TODO(angli): This should be replaced by standard log record mechanism.
+        TODO(#270): Deprecate with old output format.
         """
         path = os.path.join(log_path, 'test_run_summary.json')
         with open(path, 'w') as f:
diff --git a/tests/mobly/base_test_test.py b/tests/mobly/base_test_test.py
index 35c29f2..d7f152b 100755
--- a/tests/mobly/base_test_test.py
+++ b/tests/mobly/base_test_test.py
@@ -40,6 +40,7 @@ class SomeError(Exception):
 class BaseTestTest(unittest.TestCase):
     def setUp(self):
         self.mock_test_cls_configs = config_parser.TestRunConfig()
+        self.mock_test_cls_configs.summary_writer = mock.Mock()
         self.mock_test_cls_configs.log_path = '/tmp'
         self.mock_test_cls_configs.user_params = {"some_param": "hahaha"}
         self.mock_test_cls_configs.reporter = mock.MagicMock()
diff --git a/tests/mobly/output_test.py b/tests/mobly/output_test.py
new file mode 100755
index 0000000..4bc8e0a
--- /dev/null
+++ b/tests/mobly/output_test.py
@@ -0,0 +1,94 @@
+# Copyright 2017 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.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import os
+import shutil
+import tempfile
+import unittest
+import yaml
+
+from mobly import config_parser
+from mobly import records
+from mobly import test_runner
+
+from tests.lib import mock_controller
+from tests.lib import integration_test
+
+
+class OutputTest(unittest.TestCase):
+    """This test class has unit tests for the implementation of Mobly's output
+    files.
+    """
+
+    def setUp(self):
+        self.tmp_dir = tempfile.mkdtemp()
+        self.base_mock_test_config = config_parser.TestRunConfig()
+        self.base_mock_test_config.test_bed_name = 'SampleTestBed'
+        self.base_mock_test_config.controller_configs = {}
+        self.base_mock_test_config.user_params = {
+            'icecream': 42,
+            'extra_param': 'haha'
+        }
+        self.base_mock_test_config.log_path = self.tmp_dir
+        self.log_dir = self.base_mock_test_config.log_path
+        self.test_bed_name = self.base_mock_test_config.test_bed_name
+
+    def tearDown(self):
+        shutil.rmtree(self.tmp_dir)
+
+    def test_output(self):
+        """Verifies the expected output files from a test run.
+
+        * Files are correctly created.
+        * Basic sanity checks of each output file.
+        """
+        mock_test_config = self.base_mock_test_config.copy()
+        mock_ctrlr_config_name = mock_controller.MOBLY_CONTROLLER_CONFIG_NAME
+        my_config = [{
+            'serial': 'xxxx',
+            'magic': 'Magic1'
+        }, {
+            'serial': 'xxxx',
+            'magic': 'Magic2'
+        }]
+        mock_test_config.controller_configs[mock_ctrlr_config_name] = my_config
+        tr = test_runner.TestRunner(self.log_dir, self.test_bed_name)
+        tr.add_test_class(mock_test_config, integration_test.IntegrationTest)
+        tr.run()
+        output_dir = os.path.join(self.log_dir, self.test_bed_name, 'latest')
+        summary_file_path = os.path.join(output_dir,
+                                         records.OUTPUT_FILE_SUMMARY)
+        debug_log_path = os.path.join(output_dir,
+                                      records.OUTPUT_FILE_DEBUG_LOG)
+        info_log_path = os.path.join(output_dir, records.OUTPUT_FILE_INFO_LOG)
+        self.assertTrue(os.path.isfile(summary_file_path))
+        self.assertTrue(os.path.isfile(debug_log_path))
+        self.assertTrue(os.path.isfile(info_log_path))
+        summary_entries = []
+        with open(summary_file_path) as f:
+            for entry in yaml.load_all(f):
+                self.assertTrue(entry['Type'])
+                summary_entries.append(entry)
+        with open(debug_log_path, 'r') as f:
+            content = f.read()
+            self.assertIn('DEBUG', content)
+            self.assertIn('INFO', content)
+        with open(info_log_path, 'r') as f:
+            content = f.read()
+            self.assertIn('INFO', content)
+            self.assertNotIn('DEBUG', content)
+
+
+if __name__ == "__main__":
+    unittest.main()
diff --git a/tests/mobly/records_test.py b/tests/mobly/records_test.py
index 51b9e28..a5f68db 100755
--- a/tests/mobly/records_test.py
+++ b/tests/mobly/records_test.py
@@ -12,6 +12,11 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import os
+import shutil
+import tempfile
+import yaml
+
 from future.tests.base import unittest
 
 from mobly import records
@@ -28,6 +33,10 @@ class RecordsTest(unittest.TestCase):
         self.details = "Some details about the test execution."
         self.float_extra = 12345.56789
         self.json_extra = {"ha": "whatever"}
+        self.tmp_path = tempfile.mkdtemp()
+
+    def tearDown(self):
+        shutil.rmtree(self.tmp_path)
 
     def verify_record(self, record, result, details, extras):
         # Verify each field.
@@ -295,6 +304,19 @@ class RecordsTest(unittest.TestCase):
         self.assertTrue(tr.is_test_executed(record1.test_name))
         self.assertFalse(tr.is_test_executed(self.tn + 'ha'))
 
+    def test_summary_write_dump(self):
+        s = signals.TestFailure(self.details, self.float_extra)
+        record1 = records.TestResultRecord(self.tn)
+        record1.test_begin()
+        record1.test_fail(s)
+        dump_path = os.path.join(self.tmp_path, 'ha.yaml')
+        writer = records.TestSummaryWriter(dump_path)
+        writer.dump(record1.to_dict(), records.TestSummaryEntryType.RECORD)
+        with open(dump_path, 'r') as f:
+            content = yaml.load(f)
+            self.assertEqual(content['Type'],
+                             records.TestSummaryEntryType.RECORD.value)
+
 
 if __name__ == "__main__":
     unittest.main()