Repository: google/mobly

Issue:
Include stack traces of extra errors
Right now only the stack trace for the main error is recorded in `TeatResultRecord`.
We should store the stack traces for extra errors as well.

Discussion:
dthkao: 



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

---

*[mobly/records.py, line 122 at r1](https://reviewable.io:443/reviews/google/mobly/284#-KqTgIHnBFx-nHQQcOAq:-KqTgIHnBFx-nHQQcOAr:b-acx593) ([raw file](https://github.com/google/mobly/blob/b8b32f7a4ec1519a61a320a6b3b2c5a1811fb48d/mobly/records.py#L122)):*
> ```Python
> 
> class TestResultRecord(object):
>     """A record that holds the information of a single test case.
> ```

"test case" nomenclature?

---

*[mobly/records.py, line 132 at r1](https://reviewable.io:443/reviews/google/mobly/284#-KqTgS0N6lQ8TLdFtNJ5:-KqTgS0OviyvCih_C-0R:bnc8sd2) ([raw file](https://github.com/google/mobly/blob/b8b32f7a4ec1519a61a320a6b3b2c5a1811fb48d/mobly/records.py#L132)):*
> ```Python
>         extras: User defined extra information of the test result, must be
>                 serializable.
>         details: string, description of the cause of the test's termination.
> ```

nit: is yapf formatting this way? the different alignments for each arg are kind of distracting.

---

*[mobly/records.py, line 180 at r1](https://reviewable.io:443/reviews/google/mobly/284#-KqTiCM4ZWPPVnwvXdjF:-KqTiCM4ZWPPVnwvXdjG:boxqafn) ([raw file](https://github.com/google/mobly/blob/b8b32f7a4ec1519a61a320a6b3b2c5a1811fb48d/mobly/records.py#L180)):*
> ```Python
>         # error as the signal if there is any.
>         failure_location = ''
>         if e is None and self.extra_errors:
> ```

Just for my understanding, is there never a scenario where we have extra errors but the result is something other than ERROR or FAIL?

---


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

xpconanfan: 



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

---

*[mobly/records.py, line 132 at r1](https://reviewable.io:443/reviews/google/mobly/284#-KqTgS0N6lQ8TLdFtNJ5:-KqTlZLblhxBIk6Sik2R:b-sa7406) ([raw file](https://github.com/google/mobly/blob/b8b32f7a4ec1519a61a320a6b3b2c5a1811fb48d/mobly/records.py#L132)):*
<details><summary><i>Previously, dthkao (David T.H. Kao) wrote…</i></summary><blockquote>

nit: is yapf formatting this way? the different alignments for each arg are kind of distracting.
</blockquote></details>

Our convention is to indent to after ": ".

---

*[mobly/records.py, line 180 at r1](https://reviewable.io:443/reviews/google/mobly/284#-KqTiCM4ZWPPVnwvXdjF:-KqTlkFZb7qqCuCXoPuR:b-j9rm3x) ([raw file](https://github.com/google/mobly/blob/b8b32f7a4ec1519a61a320a6b3b2c5a1811fb48d/mobly/records.py#L180)):*
<details><summary><i>Previously, dthkao (David T.H. Kao) wrote…</i></summary><blockquote>

Just for my understanding, is there never a scenario where we have extra errors but the result is something other than ERROR or FAIL?
</blockquote></details>

If extra error exists, the test should be ERROR, otherwise it's a bug.

---


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

xpconanfan: 



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

---

*[mobly/records.py, line 122 at r1](https://reviewable.io:443/reviews/google/mobly/284#-KqTgIHnBFx-nHQQcOAq:-KqTm-oNT1t1p-LYmAAk:b-896fix) ([raw file](https://github.com/google/mobly/blob/b8b32f7a4ec1519a61a320a6b3b2c5a1811fb48d/mobly/records.py#L122)):*
<details><summary><i>Previously, dthkao (David T.H. Kao) wrote…</i></summary><blockquote>

"test case" nomenclature?
</blockquote></details>

Done.

---


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

dthkao: 



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

---

*[mobly/records.py, line 180 at r1](https://reviewable.io:443/reviews/google/mobly/284#-KqTiCM4ZWPPVnwvXdjF:-KqTor4h_KJbD4nSbht7:by8gldi) ([raw file](https://github.com/google/mobly/blob/b8b32f7a4ec1519a61a320a6b3b2c5a1811fb48d/mobly/records.py#L180)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

If extra error exists, the test should be ERROR, otherwise it's a bug.
</blockquote></details>

That does not seem right. If a test fails and then an error occurs in teardown the result should be fail.

---


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

xpconanfan: 



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

---

*[mobly/records.py, line 180 at r1](https://reviewable.io:443/reviews/google/mobly/284#-KqTiCM4ZWPPVnwvXdjF:-KqTp4dZkO_WrQ2rWfOE:b-wh33nw) ([raw file](https://github.com/google/mobly/blob/b8b32f7a4ec1519a61a320a6b3b2c5a1811fb48d/mobly/records.py#L180)):*
<details><summary><i>Previously, dthkao (David T.H. Kao) wrote…</i></summary><blockquote>

That does not seem right. If a test fails and then an error occurs in teardown the result should be fail.
</blockquote></details>

No, the result should be error in that case, as it is today and in all other frameworks (junit, pyunit etc).

---


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

dthkao: 



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

---

*[mobly/records.py, line 180 at r1](https://reviewable.io:443/reviews/google/mobly/284#-KqTiCM4ZWPPVnwvXdjF:-KqTpbZVQM6Vi-tcjDXF:b1f97c1) ([raw file](https://github.com/google/mobly/blob/b8b32f7a4ec1519a61a320a6b3b2c5a1811fb48d/mobly/records.py#L180)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

No, the result should be error in that case, as it is today and in all other frameworks (junit, pyunit etc).
</blockquote></details>

1) Pyunit does not have a notion of an extra error,
2) It still records the failure
3) Your code does not seem to support that behavior: if there is a failure marked as failure that is not changed with the extra error message moved to details

$ cat pytesttest.py
davidkao@davidkao:~$ cat pytesttest.py 
import unittest

class TestErrorReporting(unittest.TestCase):

    def tearDown(self):
        raise Exception('ow')

    def test_fail(self):
        self.assertEqual('bar', 'FOO')

if __name__ == '__main__':
  unittest.main()

$ python pytesttest.py 
FE
======================================================================
ERROR: test_fail (__main__.TestErrorReporting)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "pytesttest.py", line 16, in tearDown
    raise Exception('ow')
Exception: ow

======================================================================
FAIL: test_fail (__main__.TestErrorReporting)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "pytesttest.py", line 23, in test_fail
    self.assertEqual('bar', 'FOO')
AssertionError: 'bar' != 'FOO'

----------------------------------------------------------------------
Ran 1 test in 0.001s

---


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

xpconanfan: 



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

---

*[mobly/records.py, line 180 at r1](https://reviewable.io:443/reviews/google/mobly/284#-KqTiCM4ZWPPVnwvXdjF:-KqTrz_Hc9lHr0GvZNIT:b-pl5y07) ([raw file](https://github.com/google/mobly/blob/b8b32f7a4ec1519a61a320a6b3b2c5a1811fb48d/mobly/records.py#L180)):*
<details><summary><i>Previously, dthkao (David T.H. Kao) wrote…</i></summary><blockquote>

1) Pyunit does not have a notion of an extra error,
2) It still records the failure
3) Your code does not seem to support that behavior: if there is a failure marked as failure that is not changed with the extra error message moved to details

$ cat pytesttest.py
davidkao@davidkao:~$ cat pytesttest.py 
import unittest

class TestErrorReporting(unittest.TestCase):

    def tearDown(self):
        raise Exception('ow')

    def test_fail(self):
        self.assertEqual('bar', 'FOO')

if __name__ == '__main__':
  unittest.main()

$ python pytesttest.py 
FE
======================================================================
ERROR: test_fail (__main__.TestErrorReporting)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "pytesttest.py", line 16, in tearDown
    raise Exception('ow')
Exception: ow

======================================================================
FAIL: test_fail (__main__.TestErrorReporting)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "pytesttest.py", line 23, in test_fail
    self.assertEqual('bar', 'FOO')
AssertionError: 'bar' != 'FOO'

----------------------------------------------------------------------
Ran 1 test in 0.001s

</blockquote></details>

Pyunit always says "FAILED" in the end as long as it's not pass.
But if you see the detailed output, it singles out errors.

For our purposes, if something threw in `teardown_test`, then something went unexpectedly wrong, which translates to ERROR. Otherwise, a failed test with or without teardown error would be reported the same, and somebody would have to click the summary to read the extra errors to discover that something went unexpectedly wrong.

That behavior would make teardown errors hard to discover, which is not good. Hence we intentionally change the test result to error.

I don't really know what 3 means. if the test body explicitly Failed, then we won't move extra error to main.

---


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

dthkao: 



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

---

*[mobly/records.py, line 180 at r1](https://reviewable.io:443/reviews/google/mobly/284#-KqTiCM4ZWPPVnwvXdjF:-KqTumLKX8fA_esUqlpn:b-kc55ub) ([raw file](https://github.com/google/mobly/blob/b8b32f7a4ec1519a61a320a6b3b2c5a1811fb48d/mobly/records.py#L180)):*

> Pyunit always says "FAILED" in the end as long as it's not pass.
> But if you see the detailed output, it singles out errors.

It records them yes, but dow not single them out. I encourage you to run the simple testcase above. Your suggestion here would be to allow a teardown error to hide a test failure?

> For our purposes, if something threw in teardown_test, then something went unexpectedly wrong, which translates to ERROR. Otherwise, a failed test with or without teardown error would be reported the same, and somebody would have to click the summary to read the extra errors to discover that something went unexpectedly wrong.

"Otherwise, a failed test with or without teardown error would be reported the same" <-- this is exactly what should happen. The failure occurred first and them potentially caused extra errors.

> That behavior would make teardown errors hard to discover, which is not good. Hence we intentionally change the test result to error. 

We should only change the result to error if it was Pass before teardown. Please see comment in https://github.com/google/mobly/issues/264

> I don't really know what 3 means. if the test body explicitly Failed, then we won't move extra error to main.
What happens if somehow we call `self._test_end(TestResultEnums.TEST_RESULT_FAIL, None)`?

---


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

xpconanfan: 



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

---

*[mobly/records.py, line 180 at r1](https://reviewable.io:443/reviews/google/mobly/284#-KqTiCM4ZWPPVnwvXdjF:-KqUJFOd69F66aABin7p:b-xxjkh1) ([raw file](https://github.com/google/mobly/blob/b8b32f7a4ec1519a61a320a6b3b2c5a1811fb48d/mobly/records.py#L180)):*
<details><summary><i>Previously, dthkao (David T.H. Kao) wrote…</i></summary><blockquote>


> Pyunit always says "FAILED" in the end as long as it's not pass.
> But if you see the detailed output, it singles out errors.

It records them yes, but dow not single them out. I encourage you to run the simple testcase above. Your suggestion here would be to allow a teardown error to hide a test failure?

> For our purposes, if something threw in teardown_test, then something went unexpectedly wrong, which translates to ERROR. Otherwise, a failed test with or without teardown error would be reported the same, and somebody would have to click the summary to read the extra errors to discover that something went unexpectedly wrong.

"Otherwise, a failed test with or without teardown error would be reported the same" <-- this is exactly what should happen. The failure occurred first and them potentially caused extra errors.

> That behavior would make teardown errors hard to discover, which is not good. Hence we intentionally change the test result to error. 

We should only change the result to error if it was Pass before teardown. Please see comment in https://github.com/google/mobly/issues/264

> I don't really know what 3 means. if the test body explicitly Failed, then we won't move extra error to main.
What happens if somehow we call `self._test_end(TestResultEnums.TEST_RESULT_FAIL, None)`?
</blockquote></details>

That comment only affects where the error description and stacktrace are recorded, it doesn't mention the final result of the test entry.
If error occurs in teardown, the test is ERROR; this is expected behavior.

---


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

xpconanfan: Made the change to properly record exception objects.
* Record stacktrace for each exception object.
* Record the actual exception object for the main termination signal.

This paves the road for better debug info in `on_fail` as required by #281 
dthkao: 



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

---

*[mobly/records.py, line 132 at r1](https://reviewable.io:443/reviews/google/mobly/284#-KqTgS0N6lQ8TLdFtNJ5:-Kr2ef8oe7Ird8oaa9l_:b7tfnuk) ([raw file](https://github.com/google/mobly/blob/b8b32f7a4ec1519a61a320a6b3b2c5a1811fb48d/mobly/records.py#L132)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

Our convention is to indent to after ": ".
</blockquote></details>

Discussed offline, filed https://github.com/google/mobly/issues/300. Acknowledged for now

---

*[mobly/records.py, line 180 at r1](https://reviewable.io:443/reviews/google/mobly/284#-KqTiCM4ZWPPVnwvXdjF:-Kr2fHm7qvD8Mu8EFaL-:b-9exoj3) ([raw file](https://github.com/google/mobly/blob/b8b32f7a4ec1519a61a320a6b3b2c5a1811fb48d/mobly/records.py#L180)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

That comment only affects where the error description and stacktrace are recorded, it doesn't mention the final result of the test entry.
If error occurs in teardown, the test is ERROR; this is expected behavior.
</blockquote></details>

It is not expected behavior. If a test fails in the test case an error in teardown should not cause that failure to vanish. This was one of the #1 reasons to reexamine test result recording. 

---


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

xpconanfan: 



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

---

*[mobly/records.py, line 180 at r1](https://reviewable.io:443/reviews/google/mobly/284#-KqTiCM4ZWPPVnwvXdjF:-Kr2yMMfCWPH9jvSHs6B:bbpfs5s) ([raw file](https://github.com/google/mobly/blob/b8b32f7a4ec1519a61a320a6b3b2c5a1811fb48d/mobly/records.py#L180)):*
<details><summary><i>Previously, dthkao (David T.H. Kao) wrote…</i></summary><blockquote>

It is not expected behavior. If a test fails in the test case an error in teardown should not cause that failure to vanish. This was one of the #1 reasons to reexamine test result recording. 
</blockquote></details>

This is intentional.

1. test fail, teardown pass -> FAIL
2. teat fail, teardown error -> ERROR

The "failure" is not "lost" in case 2. The termination signal is still the original failure, the details and stacktrace fields still use the original failure's info.
But an error in teardown test *should* change the status of the test result because something unexpected happened in teardown.

A test is tallied as FAIL only if an explicit test failure signal is thrown in test body and no other errors occurred in the test.
If anything unexpected happen, the status is ERROR.

---


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

xpconanfan: There's a bug where errors in functions like `on_fail` are not tallied properly.
Need another patch.
dthkao: 



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

---

*[mobly/records.py, line 180 at r1](https://reviewable.io:443/reviews/google/mobly/284#-KqTiCM4ZWPPVnwvXdjF:-Kr49v8Qlmz11BJJodd9:byltc7v) ([raw file](https://github.com/google/mobly/blob/b8b32f7a4ec1519a61a320a6b3b2c5a1811fb48d/mobly/records.py#L180)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

This is intentional.

1. test fail, teardown pass -> FAIL
2. teat fail, teardown error -> ERROR

The "failure" is not "lost" in case 2. The termination signal is still the original failure, the details and stacktrace fields still use the original failure's info.
But an error in teardown test *should* change the status of the test result because something unexpected happened in teardown.

A test is tallied as FAIL only if an explicit test failure signal is thrown in test body and no other errors occurred in the test.
If anything unexpected happen, the status is ERROR.
</blockquote></details>

1) I disagree entirely. See the pytest example above. When a test fails and an error is thrown in teardown a failure and error are BOTH counted: there is a failure result and an EXTRA error.  If the details and stacktrace reflect the failure then the result should also. Otherwise this will only lead to confusion as to why a result with information coming from a mode of failure is marked as error. 

2) The logic implemented here itself is inconsistent with your claim. It is possible to call test_fail with no error message:
`def test_fail(self, e=None)` and in this scenario the code explicitly subverts the result marked by the user. to replace it with an 'extra_error'.

---


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

xpconanfan: 



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

---

*[mobly/records.py, line 180 at r1](https://reviewable.io:443/reviews/google/mobly/284#-KqTiCM4ZWPPVnwvXdjF:-Kr7vy8js6-dUfo3Pu30:bsgoujm) ([raw file](https://github.com/google/mobly/blob/b8b32f7a4ec1519a61a320a6b3b2c5a1811fb48d/mobly/records.py#L180)):*
<details><summary><i>Previously, dthkao (David T.H. Kao) wrote…</i></summary><blockquote>

1) I disagree entirely. See the pytest example above. When a test fails and an error is thrown in teardown a failure and error are BOTH counted: there is a failure result and an EXTRA error.  If the details and stacktrace reflect the failure then the result should also. Otherwise this will only lead to confusion as to why a result with information coming from a mode of failure is marked as error. 

2) The logic implemented here itself is inconsistent with your claim. It is possible to call test_fail with no error message:
`def test_fail(self, e=None)` and in this scenario the code explicitly subverts the result marked by the user. to replace it with an 'extra_error'.
</blockquote></details>

Regardless of what you agree, can we get this PR in?
This PR addresses the issue of exception objects not being stored properly in records.
As the initial commit message says, this is the first of multiple PRs to come.

We can address other concerns in later PRs.
We are keeping that tracking issue open until all the PRs are in.

---


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

k2fong: 



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

---

*[mobly/records.py, line 180 at r1](https://reviewable.io:443/reviews/google/mobly/284#-KqTiCM4ZWPPVnwvXdjF:-Kr8PO87zIe_2us-T9V7:b-8lvomq) ([raw file](https://github.com/google/mobly/blob/b8b32f7a4ec1519a61a320a6b3b2c5a1811fb48d/mobly/records.py#L180)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

Regardless of what you agree, can we get this PR in?
This PR addresses the issue of exception objects not being stored properly in records.
As the initial commit message says, this is the first of multiple PRs to come.

We can address other concerns in later PRs.
We are keeping that tracking issue open until all the PRs are in.
</blockquote></details>

Just giving my 2 cents here, my recommendation is to follow how pytest handles since that'll probably be the user's expectation of mobly.  From a little test of mine, looks like a test failure (with teardown error) results in a reporting of  "FAILED (failures=1, errors=1)".  Can we do similar?  Also, it provided stack trace for the failure in test and the error in teardown.  From an offline chat, sounds like to do so might be a big task, what you guys think about the path forward to just continue however mobly is reporting currently today so as to minimize user impacted in the meantime.  Unless that was partly the reason for this PR and I'm assuming users are either okay with how Mobly is reporting or has created some workaround.

---

*[mobly/records.py, line 197 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3inUC5cHYesCg3hIN:-Kr3inUC5cHYesCg3hIO:b-bvf5k5) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L197)):*
>         if e:
>             self.termination_signal = e
no need for this 'if e' clause.  Just assign?

---

*[mobly/records.py, line 200 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3jiRas15KiEg1ApFP:-Kr3jiRas15KiEg1ApFQ:bd3d4ok) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L200)):*
> e is None
e should be None at this point already

---

*[mobly/records.py, line 203 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3kD9eWG2D5rUDEOU-:-Kr3kD9eWG2D5rUDEOU0:b4eguto) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L203)):*
>         if failure_location:
>             failure_location += ': '
simplify one less logic by adding this inside the failure_location assignment above.  Unless its possible that the popitem() above might return None or ''

---

*[mobly/records.py, line 205 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3lPLRAOw3SbbSD8V7:-Kr3lPLRAOw3SbbSD8V8:bh6yzzm) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L205)):*
>         if isinstance(e, signals.TestSignal):
>             self.details = failure_location + str(e.details)
>  
>  
>  
>             self.extras = e.extras
>         elif isinstance(e, Exception):
>             self.details = failure_location + str(e)
Seems like if the logic enters into either of these, failure_location will be empty string.  Is that intended?

---

*[mobly/records.py, line 223 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3nm3ORrhOCtOJM6Km:-Kr3nm3ORrhOCtOJM6Kn:b1nwfy1) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L223)):*
>             if e and not hasattr(e, 'stacktrace_str'):
>                 e.stacktrace_str = stacktrace_str
should test end have side effect on e passed in?

---

*[mobly/records.py, line 284 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3oVzGwyFl-d3k9t2Q:-Kr3oVzGwyFl-d3k9t2R:bxfio11) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L284)):*
<details><summary><i alt="summary">Quoted 12 lines of code…</i></summary>
>         # This check cannot be based on try...except, which messes up
>         # `exc_info`.
>         if e and hasattr(e, '__traceback__'):
>             exc_traceback = e.__traceback__
>         else:
>             # In py2, exception objects don't have built-in traceback, so we
>             # have to immediately retrieve stacktrace from `sys.exc_info`.
>             _, _, exc_traceback = sys.exc_info()
>         if exc_traceback:
>             stacktrace_str = ''.join(traceback.format_tb(exc_traceback))
>             if not hasattr(e, 'stacktrace_str'):
>                 e.stacktrace_str = stacktrace_str
</details>
This look like a candidate for refactor.  Exact code above

---

*[mobly/records.py, line 322 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3ptASE2tC2CxyjWH7:-Kr3ptASE2tC2CxyjWH8:b-4oha6q) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L322)):*
<details><summary><i alt="summary">Quoted 4 lines of code…</i></summary>
>             try:
>                 exception_dict[TestResultEnums.RECORD_EXTRAS] = e.extras
>             except AttributeError:
>                 pass
</details>
can hasattr() be used here instead of relying on exception being raised?

---


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

xpconanfan: 



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

---

*[mobly/records.py, line 180 at r1](https://reviewable.io:443/reviews/google/mobly/284#-KqTiCM4ZWPPVnwvXdjF:-Kr8VMvx7t5gc8BpMUR2:b-xqgz0k) ([raw file](https://github.com/google/mobly/blob/b8b32f7a4ec1519a61a320a6b3b2c5a1811fb48d/mobly/records.py#L180)):*
<details><summary><i>Previously, k2fong wrote…</i></summary><blockquote>

Just giving my 2 cents here, my recommendation is to follow how pytest handles since that'll probably be the user's expectation of mobly.  From a little test of mine, looks like a test failure (with teardown error) results in a reporting of  "FAILED (failures=1, errors=1)".  Can we do similar?  Also, it provided stack trace for the failure in test and the error in teardown.  From an offline chat, sounds like to do so might be a big task, what you guys think about the path forward to just continue however mobly is reporting currently today so as to minimize user impacted in the meantime.  Unless that was partly the reason for this PR and I'm assuming users are either okay with how Mobly is reporting or has created some workaround.
</blockquote></details>

The line "FAILED (failures=1, errors=1)" is the test run summary line of all tests in a run, we have a similar summary line at the end, and we are not really discussing this line here...

We are talking about the status of a single test record.

We are not a unit test framework, so we can't copy pyunit as it is. This is one of the differ points between unit tests and E2E tests.

If that's what you prefer, I'm ok with keeping the current behavior because there have been no complaints from users who run tests everyday.

---

*[mobly/records.py, line 200 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3jiRas15KiEg1ApFP:-Kr8WTTciz-tcCwsDzco:brza0n9) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L200)):*
<details><summary><i>Previously, k2fong wrote…</i></summary><blockquote>

> e is None
e should be None at this point already
</blockquote></details>

why? e is passed in, it is not always None.

---

*[mobly/records.py, line 203 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3kD9eWG2D5rUDEOU-:-Kr8XCyLsylMTQ0wha9y:bjxq2oh) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L203)):*
It actually can be...
```
>>> x = OrderedDict()
>>> x[None] = 'a'
>>> x
OrderedDict([(None, 'a')])
```

---

*[mobly/records.py, line 205 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3lPLRAOw3SbbSD8V7:-Kr8XHqZm-qbRsSkiSX0:baioqxy) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L205)):*
<details><summary><i>Previously, k2fong wrote…</i></summary><blockquote>

>         if isinstance(e, signals.TestSignal):
>             self.details = failure_location + str(e.details)
>  
>  
>  
>             self.extras = e.extras
>         elif isinstance(e, Exception):
>             self.details = failure_location + str(e)
Seems like if the logic enters into either of these, failure_location will be empty string.  Is that intended?
</blockquote></details>

Yes. E.g. there's no `failure_location` for the self.termination_signal.

---

*[mobly/records.py, line 223 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3nm3ORrhOCtOJM6Km:-Kr8XkXmbMJv_MgYYAZ_:b-h3ul1p) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L223)):*
<details><summary><i>Previously, k2fong wrote…</i></summary><blockquote>

>             if e and not hasattr(e, 'stacktrace_str'):
>                 e.stacktrace_str = stacktrace_str
should test end have side effect on e passed in?
</blockquote></details>

this e is used internally for recording test execution info, so it's kinda ok...
the thing is, if we don't save the stacktrace right here, we lose it.
it's sad but it's python...

---

*[mobly/records.py, line 284 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3oVzGwyFl-d3k9t2Q:-Kr8YNUnQM53SkOIRPXC:bs0rvn9) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L284)):*
<details><summary><i>Previously, k2fong wrote…</i></summary><blockquote>

>         # This check cannot be based on try...except, which messes up
>         # `exc_info`.
>         if e and hasattr(e, '__traceback__'):
>             exc_traceback = e.__traceback__
>         else:
>             # In py2, exception objects don't have built-in traceback, so we
>             # have to immediately retrieve stacktrace from `sys.exc_info`.
>             _, _, exc_traceback = sys.exc_info()
>         if exc_traceback:
>             stacktrace_str = ''.join(traceback.format_tb(exc_traceback))
>             if not hasattr(e, 'stacktrace_str'):
>                 e.stacktrace_str = stacktrace_str
This look like a candidate for refactor.  Exact code above
</blockquote></details>

not sure what you mean. can you clarify.
also it's really hard to read this post due to formatting...

---

*[mobly/records.py, line 322 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3ptASE2tC2CxyjWH7:-Kr8Y4l2nK2TlcupyFHX:b1undhl) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L322)):*
<details><summary><i>Previously, k2fong wrote…</i></summary><blockquote>

>             try:
>                 exception_dict[TestResultEnums.RECORD_EXTRAS] = e.extras
>             except AttributeError:
>                 pass
can hasattr() be used here instead of relying on exception being raised?
</blockquote></details>

Don't have a strong preference.

i thought you liked pythonic style?
"it's better to ask for forgiveness than permission" :P

---


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

k2fong: 



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

---

*[mobly/records.py, line 200 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3jiRas15KiEg1ApFP:-Kr9NlBZLx3BU5371B2y:b1r6lk5) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L200)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

why? e is passed in, it is not always None.
</blockquote></details>

because of the 'if e' on above

---

*[mobly/records.py, line 203 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3kD9eWG2D5rUDEOU-:-Kr9To43i_0oHdx3dDI3:bg7zopp) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L203)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

It actually can be...
```
>>> x = OrderedDict()
>>> x[None] = 'a'
>>> x
OrderedDict([(None, 'a')])
```
</blockquote></details>

can it be like below?  just trying to reduce the number of conditional logics here.

elif e is None and self.extra_errors:
  failure_location, e = self.extra_errors.popitem(last=False)
  failure_location += ': '

---

*[mobly/records.py, line 205 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3lPLRAOw3SbbSD8V7:-Kr9Osv8BYHFtQlRGpHY:buvaev8) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L205)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

Yes. E.g. there's no `failure_location` for the self.termination_signal.
</blockquote></details>

if that's the case then I'll suggest not using failure_location here at all

---

*[mobly/records.py, line 223 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3nm3ORrhOCtOJM6Km:-Kr9PNuYPYR77a1qfcyD:b-tg73hw) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L223)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

this e is used internally for recording test execution info, so it's kinda ok...
the thing is, if we don't save the stacktrace right here, we lose it.
it's sad but it's python...
</blockquote></details>

if so, might be worthwhile to indicate so in the header

---

*[mobly/records.py, line 284 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3oVzGwyFl-d3k9t2Q:-Kr9PbW-PVzlWsD5ShSQ:bsgxs7k) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L284)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

not sure what you mean. can you clarify.
also it's really hard to read this post due to formatting...
</blockquote></details>

sorry about that, I was expecting it'll highlight code similar to critique :p my comment is that line 219-233 and 288-300 are pretty much identical beside 2 lines that I'm not sure if they should be different either.  can it be refactored to be combine into some method?

---

*[mobly/records.py, line 322 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3ptASE2tC2CxyjWH7:-Kr9S1FKj2qHXgu0nPcN:b-qna566) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L322)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

Don't have a strong preference.

i thought you liked pythonic style?
"it's better to ask for forgiveness than permission" :P
</blockquote></details>

hmm not me.  but is this style really the pythonic style?  I prefer hasattr() especially since its use else where in this file as well.

---


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

xpconanfan: 



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

---

*[mobly/records.py, line 200 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3jiRas15KiEg1ApFP:-KrCGa4Aefw6mqIKkqST:bjgacef) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L200)):*
<details><summary><i>Previously, k2fong wrote…</i></summary><blockquote>

because of the 'if e' on above
</blockquote></details>

Ok, but I fail to see the point here...
If e is None, we want to assign it a not None value from extra errors.

---

*[mobly/records.py, line 203 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3kD9eWG2D5rUDEOU-:-KrCGo05U8oXQTCWbjPt:bnoc1ps) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L203)):*
<details><summary><i>Previously, k2fong wrote…</i></summary><blockquote>

can it be like below?  just trying to reduce the number of conditional logics here.

elif e is None and self.extra_errors:
  failure_location, e = self.extra_errors.popitem(last=False)
  failure_location += ': '
</blockquote></details>

No, because failure_location could have be overwritten to None by the pop and the `+=` would fail with type error.

---


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

xpconanfan: 



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

---

*[mobly/records.py, line 205 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3lPLRAOw3SbbSD8V7:-KrCGyyoTtcsammxe0LQ:bxu03q6) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L205)):*
<details><summary><i>Previously, k2fong wrote…</i></summary><blockquote>

if that's the case then I'll suggest not using failure_location here at all
</blockquote></details>

But we do need to prepend it here if failure_location is not empty string.
This is just a shortcut to reduce `if` statements.

---


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

xpconanfan: 



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

---

*[mobly/records.py, line 284 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3oVzGwyFl-d3k9t2Q:-KrCHAAK5pYCavnPI-bj:bc4us8f) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L284)):*
<details><summary><i>Previously, k2fong wrote…</i></summary><blockquote>

sorry about that, I was expecting it'll highlight code similar to critique :p my comment is that line 219-233 and 288-300 are pretty much identical beside 2 lines that I'm not sure if they should be different either.  can it be refactored to be combine into some method?
</blockquote></details>

No, because we are taking stacktraces here so another func stack would create noise in the stacktrace

---


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

xpconanfan: 



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

---

*[mobly/records.py, line 322 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3ptASE2tC2CxyjWH7:-KrCHYXuXXUVNDbXHZd6:b-5caqun) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L322)):*
<details><summary><i>Previously, k2fong wrote…</i></summary><blockquote>

hmm not me.  but is this style really the pythonic style?  I prefer hasattr() especially since its use else where in this file as well.
</blockquote></details>

Yes. It is quite common and somewhat recommended to use try...except in place of `if` in Python.
https://stackoverflow.com/questions/7604636/better-to-try-something-and-catch-the-exception-or-test-if-its-possible-first

---


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

xpconanfan: 



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

---

*[mobly/records.py, line 197 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3inUC5cHYesCg3hIN:-KrCqIigbI--dMXyrhd3:b3vi6kr) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L197)):*
<details><summary><i>Previously, k2fong wrote…</i></summary><blockquote>

>         if e:
>             self.termination_signal = e
no need for this 'if e' clause.  Just assign?
</blockquote></details>

Will refactor

---

*[mobly/records.py, line 200 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3jiRas15KiEg1ApFP:-KrCq90ebf4aJ56LV1cE:b9ty8wm) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L200)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

Ok, but I fail to see the point here...
If e is None, we want to assign it a not None value from extra errors.
</blockquote></details>

Oh, i see.
`if e` doesn't gurantee `e is not None`
e can be '' or other values that evaluate to false.

---


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

xpconanfan: Done with the fixes.
Here's an example of a grand failure where test body, `teardown_test`, and `on_fail` all failed.

The entry looks like:
```
--- 
"Begin Time": 1502406537536
Details: "fail in body."
"End Time": 1502406537537
"Extra Errors": 
  - 
    Details: "teardown test error."
    Extras: null
    Position: teardown_test
    Stacktrace: |
        Traceback (most recent call last):
          File "/Users/angli/Developer/mobly/mobly/base_test.py", line 341, in exec_one_test
            self._teardown_test(test_name)
          File "/Users/angli/Developer/mobly/mobly/base_test.py", line 205, in _teardown_test
            self.teardown_test()
          File "tests/lib/integration_test.py", line 37, in teardown_test
            raise Exception('teardown test error.')
        Exception: teardown test error.
  - 
    Details: "on_fail error."
    Extras: null
    Position: _on_fail
    Stacktrace: |
        Traceback (most recent call last):
          File "/Users/angli/Developer/mobly/mobly/base_test.py", line 301, in _exec_procedure_func
            func(tr_record)
          File "/Users/angli/Developer/mobly/mobly/base_test.py", line 226, in _on_fail
            self.on_fail(test_name, begin_time)
          File "tests/lib/integration_test.py", line 40, in on_fail
            raise Exception('on_fail error.')
        Exception: on_fail error.
Extras: null
Result: FAIL
Stacktrace: |
    Traceback (most recent call last):
      File "/Users/angli/Developer/mobly/mobly/base_test.py", line 332, in exec_one_test
        test_method()
      File "tests/lib/integration_test.py", line 33, in test_hello_world
        asserts.fail('fail in body.')
      File "/Users/angli/Developer/mobly/mobly/asserts.py", line 241, in fail
        raise signals.TestFailure(msg, extras)
    TestFailure: Details=fail in body., Extras=None
"Test Class": IntegrationTest
"Test Name": test_hello_world
Type: Record
UID: null
```

xpconanfan: 



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

---

*[mobly/records.py, line 180 at r1](https://reviewable.io:443/reviews/google/mobly/284#-KqTiCM4ZWPPVnwvXdjF:-KrDR-RS0xcanNO7EY7o:b-896fix) ([raw file](https://github.com/google/mobly/blob/b8b32f7a4ec1519a61a320a6b3b2c5a1811fb48d/mobly/records.py#L180)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

The line "FAILED (failures=1, errors=1)" is the test run summary line of all tests in a run, we have a similar summary line at the end, and we are not really discussing this line here...

We are talking about the status of a single test record.

We are not a unit test framework, so we can't copy pyunit as it is. This is one of the differ points between unit tests and E2E tests.

If that's what you prefer, I'm ok with keeping the current behavior because there have been no complaints from users who run tests everyday.
</blockquote></details>

Done.

---

*[mobly/records.py, line 197 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3inUC5cHYesCg3hIN:-KrDR-lwuqUrfUCxzh88:b-896fix) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L197)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

Will refactor
</blockquote></details>

Done.

---

*[mobly/records.py, line 200 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3jiRas15KiEg1ApFP:-KrDR0LNJcYqMPQj2_AW:b-896fix) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L200)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

Oh, i see.
`if e` doesn't gurantee `e is not None`
e can be '' or other values that evaluate to false.
</blockquote></details>

Done.

---

*[mobly/records.py, line 203 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3kD9eWG2D5rUDEOU-:-KrDR2JLvD3aQz1wBbuv:b-896fix) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L203)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

No, because failure_location could have be overwritten to None by the pop and the `+=` would fail with type error.
</blockquote></details>

Done.

---

*[mobly/records.py, line 205 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3lPLRAOw3SbbSD8V7:-KrDR2xwa7grak5QWfX1:b-ruqkw2) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L205)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

But we do need to prepend it here if failure_location is not empty string.
This is just a shortcut to reduce `if` statements.
</blockquote></details>

Obsolete

---

*[mobly/records.py, line 223 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3nm3ORrhOCtOJM6Km:-KrDR4rUDgRX4uXbHfhK:b-ruqkw2) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L223)):*
<details><summary><i>Previously, k2fong wrote…</i></summary><blockquote>

if so, might be worthwhile to indicate so in the header
</blockquote></details>

Obsolete

---

*[mobly/records.py, line 284 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3oVzGwyFl-d3k9t2Q:-KrDR5PkwfBzTeyiP04y:b-ruqkw2) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L284)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

No, because we are taking stacktraces here so another func stack would create noise in the stacktrace
</blockquote></details>

Obsolete

---

*[mobly/records.py, line 322 at r3](https://reviewable.io:443/reviews/google/mobly/284#-Kr3ptASE2tC2CxyjWH7:-KrDR7V21Wjo1AyvM3sm:b-896fix) ([raw file](https://github.com/google/mobly/blob/d3fd8b09c1d6875e09c2aba36aea69c2b15aa67d/mobly/records.py#L322)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

Yes. It is quite common and somewhat recommended to use try...except in place of `if` in Python.
https://stackoverflow.com/questions/7604636/better-to-try-something-and-catch-the-exception-or-test-if-its-possible-first
</blockquote></details>

Done.

---


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

xpconanfan: Rebased
k2fong: 



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

---

*[mobly/records.py, line 260 at r8](https://reviewable.io:443/reviews/google/mobly/284#-KrDywQj97Vemgr_3o27:-KrDywQj97Vemgr_3o28:b-ce0f4g) ([raw file](https://github.com/google/mobly/blob/be96f35805b0015c8d1ae1456be4c4c133b7e167/mobly/records.py#L260)):*
> ```Python
>             if self.result in (TestResultEnums.TEST_RESULT_PASS,
>                                TestResultEnums.TEST_RESULT_SKIP):
>                 self.result = TestResultEnums.TEST_RESULT_ERROR
> ```

Nit. if pass, skip, error, and fail are all the possible result value, will it read better if its if self.result != TestResultEnums.TEST_RESULT_FAIL?  I think there might be one more similar down below.

---

*[mobly/records.py, line 268 at r8](https://reviewable.io:443/reviews/google/mobly/284#-KrE-3-Q6Zo1E-oKKmX6:-KrE-3-Q6Zo1E-oKKmX7:b-r67v17) ([raw file](https://github.com/google/mobly/blob/be96f35805b0015c8d1ae1456be4c4c133b7e167/mobly/records.py#L268)):*
> ```Python
>             self.details = self.termination_signal.details
>             self.stacktrace = self.termination_signal.stacktrace
>             self.extras = self.termination_signal.extras
> ```

another nit.  instead of keeping more states within this class, how about defining properties for details, stacktrace, and extras and just return the results from termination_signal?

---

*[mobly/records.py, line 326 at r8](https://reviewable.io:443/reviews/google/mobly/284#-KrEUJVSrNSl4QdD03V0:-KrEUJVSrNSl4QdD03V1:bt2g9lo) ([raw file](https://github.com/google/mobly/blob/be96f35805b0015c8d1ae1456be4c4c133b7e167/mobly/records.py#L326)):*
> ```Python
>                            TestResultEnums.TEST_RESULT_SKIP):
>             self.result = TestResultEnums.TEST_RESULT_ERROR
>         self.extra_errors[position] = ExceptionRecord(e, position=position)
> ```

I don't believe its used that way but if in case the same position is passed in, the extra error will be overridden for that position.  is that intended?  if that is the intend, how about call this set_error()?  

---


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

xpconanfan: 



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

---

*[mobly/records.py, line 260 at r8](https://reviewable.io:443/reviews/google/mobly/284#-KrDywQj97Vemgr_3o27:-KrEs1w5TtPCylkxPY6e:b-896fix) ([raw file](https://github.com/google/mobly/blob/be96f35805b0015c8d1ae1456be4c4c133b7e167/mobly/records.py#L260)):*
<details><summary><i>Previously, k2fong wrote…</i></summary><blockquote>

Nit. if pass, skip, error, and fail are all the possible result value, will it read better if its if self.result != TestResultEnums.TEST_RESULT_FAIL?  I think there might be one more similar down below.
</blockquote></details>

Done.

---

*[mobly/records.py, line 268 at r8](https://reviewable.io:443/reviews/google/mobly/284#-KrE-3-Q6Zo1E-oKKmX6:-KrEtbz8tzWeG6-LZ139:b-896fix) ([raw file](https://github.com/google/mobly/blob/be96f35805b0015c8d1ae1456be4c4c133b7e167/mobly/records.py#L268)):*
<details><summary><i>Previously, k2fong wrote…</i></summary><blockquote>

another nit.  instead of keeping more states within this class, how about defining properties for details, stacktrace, and extras and just return the results from termination_signal?
</blockquote></details>

Done.

---

*[mobly/records.py, line 326 at r8](https://reviewable.io:443/reviews/google/mobly/284#-KrEUJVSrNSl4QdD03V0:-KrEuE6uo4SkpLsupxmH:boji8w4) ([raw file](https://github.com/google/mobly/blob/be96f35805b0015c8d1ae1456be4c4c133b7e167/mobly/records.py#L326)):*
<details><summary><i>Previously, k2fong wrote…</i></summary><blockquote>

I don't believe its used that way but if in case the same position is passed in, the extra error will be overridden for that position.  is that intended?  if that is the intend, how about call this set_error()?  
</blockquote></details>

Added a check.
This is not a huge concern as only the framework itself sets the position.

---


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

dthkao: 



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

---

*[mobly/records.py, line 132 at r1](https://reviewable.io:443/reviews/google/mobly/284#-KqTgS0N6lQ8TLdFtNJ5:-KrFCajBpfuFPCe6Dw20:b46bouh) ([raw file](https://github.com/google/mobly/blob/b8b32f7a4ec1519a61a320a6b3b2c5a1811fb48d/mobly/records.py#L132)):*
<details><summary><i>Previously, dthkao (David T.H. Kao) wrote…</i></summary><blockquote>

Discussed offline, filed https://github.com/google/mobly/issues/300. Acknowledged for now
</blockquote></details>

indentation pr is in so bringing this back up.

---

*[mobly/records.py, line 250 at r9](https://reviewable.io:443/reviews/google/mobly/284#-KrFDkoRafxNWkrjewJw:-KrFDkoRafxNWkrjewJx:b-3csec2) ([raw file](https://github.com/google/mobly/blob/cda0a0c386c5a562c6a7c513748620304480760a/mobly/records.py#L250)):*
> ```Python
>             result: One of the TEST_RESULT enums in TestResultEnums.
>             e: A test termination signal (usually an exception object). It can
>                be any exception instance or of any subclass of
> ```

indent?

---


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

xpconanfan: 



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

---

*[mobly/records.py, line 132 at r1](https://reviewable.io:443/reviews/google/mobly/284#-KqTgS0N6lQ8TLdFtNJ5:-KrFFqqLFyTZgRBmidyB:b-896fix) ([raw file](https://github.com/google/mobly/blob/b8b32f7a4ec1519a61a320a6b3b2c5a1811fb48d/mobly/records.py#L132)):*
<details><summary><i>Previously, dthkao (David T.H. Kao) wrote…</i></summary><blockquote>

indentation pr is in so bringing this back up.
</blockquote></details>

Done.

---

*[mobly/records.py, line 250 at r9](https://reviewable.io:443/reviews/google/mobly/284#-KrFDkoRafxNWkrjewJw:-KrFENweh8wyOPOsTBtJ:b-896fix) ([raw file](https://github.com/google/mobly/blob/cda0a0c386c5a562c6a7c513748620304480760a/mobly/records.py#L250)):*
<details><summary><i>Previously, dthkao (David T.H. Kao) wrote…</i></summary><blockquote>

indent?
</blockquote></details>

Done.

---


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

---



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

xpconanfan: 



Reviewed 1 of 3 files at r8, 1 of 1 files at r10.
Review status: 2 of 4 files reviewed at latest revision, 3 unresolved discussions.

---



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


Patch:
diff --git a/mobly/records.py b/mobly/records.py
index 62f130b..16072c1 100644
--- a/mobly/records.py
+++ b/mobly/records.py
@@ -14,6 +14,7 @@
 """This module has classes for test result collection, and test result output.
 """
 
+import collections
 import itertools
 import copy
 import enum
@@ -119,23 +120,86 @@ class TestResultEnums(object):
     RECORD_EXTRA_ERRORS = 'Extra Errors'
     RECORD_DETAILS = 'Details'
     RECORD_STACKTRACE = 'Stacktrace'
+    RECORD_POSITION = 'Position'
     TEST_RESULT_PASS = 'PASS'
     TEST_RESULT_FAIL = 'FAIL'
     TEST_RESULT_SKIP = 'SKIP'
     TEST_RESULT_ERROR = 'ERROR'
 
 
+class ExceptionRecord(object):
+    """A wrapper class for representing exception objects in TestResultRecord.
+
+    Attributes:
+        exception: Exception object, the original Exception.
+        stacktrace: string, stacktrace of the Exception.
+        extras: optional serializable, this corresponds to the
+            `TestSignal.extras` field.
+        position: string, an optional label specifying the position where the
+            Exception ocurred.
+    """
+
+    def __init__(self, e, position=None):
+        self.exception = e
+        self.stacktrace = None
+        self.extras = None
+        self.position = position
+        self.is_test_signal = isinstance(e, signals.TestSignal)
+        # Record stacktrace of the exception.
+        # This check cannot be based on try...except, which messes up
+        # `exc_info`.
+        if hasattr(e, '__traceback__'):
+            exc_traceback = e.__traceback__
+        else:
+            # In py2, exception objects don't have built-in traceback, so we
+            # have to immediately retrieve stacktrace from `sys.exc_info`.
+            _, _, exc_traceback = sys.exc_info()
+        if exc_traceback:
+            self.stacktrace = ''.join(
+                traceback.format_exception(e.__class__, e, exc_traceback))
+        # Populate fields based on the type of the termination signal.
+        if self.is_test_signal:
+            self.details = str(e.details)
+            self.extras = e.extras
+        else:
+            self.details = str(e)
+
+    def to_dict(self):
+        result = {}
+        result[TestResultEnums.RECORD_DETAILS] = self.details
+        result[TestResultEnums.RECORD_POSITION] = self.position
+        result[TestResultEnums.RECORD_STACKTRACE] = self.stacktrace
+        result[TestResultEnums.RECORD_EXTRAS] = copy.deepcopy(self.extras)
+        return result
+
+
 class TestResultRecord(object):
-    """A record that holds the information of a test execution.
+    """A record that holds the information of a single test.
+
+    The record object holds all information of a test, including all the
+    exceptions occurred during the test.
+
+    A test can terminate for two reasons:
+      1. the test function executes to the end and completes naturally.
+      2. the test is terminated by an exception, which we call
+         "termination signal".
+
+    The termination signal is treated differently. Its content are extracted
+    into first-tier attributes of the record object, like `details` and
+    `stacktrace`, for easy consumption.
+
+    Note the termination signal is not always an error, it can also be explicit
+    pass signal or abort/skip signals.
 
     Attributes:
-        test_name: A string representing the name of the test method.
+        test_name: string, the name of the test.
         begin_time: Epoch timestamp of when the test started.
         end_time: Epoch timestamp of when the test ended.
-        self.uid: Unique identifier of a test.
-        self.result: Test result, PASS/FAIL/SKIP.
-        self.extras: User defined extra information of the test result.
-        self.details: A string explaining the details of the test.
+        uid: Unique identifier of a test.
+        termination_signal: ExceptionRecord, the main exception of the test.
+        extra_errors: OrderedDict, all exceptions occurred during the entire
+            test lifecycle. The order of occurrence is preserved.
+        result: TestResultEnum.TEAT_RESULT_*, PASS/FAIL/SKIP.
     """
 
     def __init__(self, t_name, t_class=None):
@@ -144,11 +208,35 @@ class TestResultRecord(object):
         self.begin_time = None
         self.end_time = None
         self.uid = None
+        self.termination_signal = None
+        self.extra_errors = collections.OrderedDict()
         self.result = None
-        self.extras = None
-        self.details = None
-        self.stacktrace = None
-        self.extra_errors = {}
+
+    @property
+    def details(self):
+        """String description of the cause of the test's termination.
+
+        Note a passed test can have this as well due to the explicit pass
+        signal. If the test passed implicitly, this field would be None.
+        """
+        if self.termination_signal:
+            return self.termination_signal.details
+
+    @property
+    def stacktrace(self):
+        """The stacktrace string for the exception that terminated the test.
+        """
+        if self.termination_signal:
+            return self.termination_signal.stacktrace
+
+    @property
+    def extras(self):
+        """User defined extra information of the test result.
+
+        Must be serializable.
+        """
+        if self.termination_signal:
+            return self.termination_signal.extras
 
     def test_begin(self):
         """Call this when the test begins execution.
@@ -158,7 +246,7 @@ class TestResultRecord(object):
         self.begin_time = utils.get_current_epoch_time()
 
     def _test_end(self, result, e):
-        """Class internal function to signal the end of a test execution.
+        """Marks the end of the test logic.
 
         Args:
             result: One of the TEST_RESULT enums in TestResultEnums.
@@ -169,19 +257,25 @@ class TestResultRecord(object):
         if self.begin_time is not None:
             self.end_time = utils.get_current_epoch_time()
         self.result = result
+        if e:
+            self.termination_signal = ExceptionRecord(e)
+
+    def update_record(self):
+        """Updates the content of a record.
+
+        Several display fields like "details" and "stacktrace" need to be
+        updated based on the content of the record object.
+
+        As the content of the record change, call this method to update all
+        the appropirate fields.
+        """
         if self.extra_errors:
-            self.result = TestResultEnums.TEST_RESULT_ERROR
-        if isinstance(e, signals.TestSignal):
-            self.details = e.details
-            _, _, exc_traceback = sys.exc_info()
-            if exc_traceback:
-                self.stacktrace = ''.join(traceback.format_tb(exc_traceback))
-            self.extras = e.extras
-        elif isinstance(e, Exception):
-            self.details = str(e)
-            _, _, exc_traceback = sys.exc_info()
-            if exc_traceback:
-                self.stacktrace = ''.join(traceback.format_tb(exc_traceback))
+            if self.result != TestResultEnums.TEST_RESULT_FAIL:
+                self.result = TestResultEnums.TEST_RESULT_ERROR
+        # If no termination signal is provided, use the first exception
+        # occurred as the termination signal.
+        if not self.termination_signal and self.extra_errors:
+            _, self.termination_signal = self.extra_errors.popitem(last=False)
 
     def test_pass(self, e=None):
         """To mark the test as passed in this record.
@@ -219,19 +313,29 @@ class TestResultRecord(object):
         """
         self._test_end(TestResultEnums.TEST_RESULT_ERROR, e)
 
-    def add_error(self, tag, e):
-        """Add extra error happened during a test mark the test result as
+    def add_error(self, position, e):
+        """Add extra error happened during a test.
+
+        If the test has passed or skipped, this will mark the test result as
         ERROR.
 
         If an error is added the test record, the record's result is equivalent
         to the case where an uncaught exception happened.
 
+        If the test record has not recorded any error, the newly added error
+        would be the main error of the test record. Otherwise the newly added
+        error is added to the record's extra errors.
+
         Args:
-            tag: A string describing where this error came from, e.g. 'on_pass'.
+            position: string, where this error occurred, e.g. 'teardown_test'.
             e: An exception object.
         """
-        self.result = TestResultEnums.TEST_RESULT_ERROR
-        self.extra_errors[tag] = str(e)
+        if self.result != TestResultEnums.TEST_RESULT_FAIL:
+            self.result = TestResultEnums.TEST_RESULT_ERROR
+        if position in self.extra_errors:
+            raise Error('An exception is already recorded with position "%s",'
+                        ' cannot reuse.' % position)
+        self.extra_errors[position] = ExceptionRecord(e, position=position)
 
     def __str__(self):
         d = self.to_dict()
@@ -259,7 +363,9 @@ class TestResultRecord(object):
         d[TestResultEnums.RECORD_UID] = self.uid
         d[TestResultEnums.RECORD_EXTRAS] = self.extras
         d[TestResultEnums.RECORD_DETAILS] = self.details
-        d[TestResultEnums.RECORD_EXTRA_ERRORS] = self.extra_errors
+        d[TestResultEnums.RECORD_EXTRA_ERRORS] = [
+            e.to_dict() for e in self.extra_errors.values()
+        ]
         d[TestResultEnums.RECORD_STACKTRACE] = self.stacktrace
         return d
 
@@ -340,9 +446,13 @@ class TestResult(object):
 
         A record is considered executed once it's added to the test result.
 
+        Adding the record finalizes the content of a record, so no change
+        should be made to the record afterwards.
+
         Args:
             record: A test record object to add.
         """
+        record.update_record()
         if record.result == TestResultEnums.TEST_RESULT_SKIP:
             self.skipped.append(record)
             return
@@ -375,6 +485,7 @@ class TestResult(object):
         Args:
             test_record: A TestResultRecord object for the test class.
         """
+        test_record.update_record()
         self.error.append(test_record)
 
     def is_test_executed(self, test_name):


Test patch:
diff --git a/mobly/base_test.py b/mobly/base_test.py
index a483a83..40f1bc4 100644
--- a/mobly/base_test.py
+++ b/mobly/base_test.py
@@ -301,9 +301,9 @@ class BaseTestClass(object):
                 executed.
         """
         try:
-          # Pass a copy of the record instead of the actual object so that it
-          # will not be modified.
-          func(copy.deepcopy(tr_record))
+            # Pass a copy of the record instead of the actual object so that it
+            # will not be modified.
+            func(copy.deepcopy(tr_record))
         except signals.TestAbortAll:
             raise
         except Exception as e:
@@ -327,6 +327,7 @@ class BaseTestClass(object):
         tr_record = records.TestResultRecord(test_name, self.TAG)
         tr_record.test_begin()
         logging.info('%s %s', TEST_CASE_TOKEN, test_name)
+        teardown_test_failed = False
         try:
             try:
                 self._setup_test(test_name)
@@ -348,6 +349,7 @@ class BaseTestClass(object):
                 except Exception as e:
                     logging.exception(e)
                     tr_record.add_error('teardown_test', e)
+                    teardown_test_failed = True
         except (signals.TestFailure, AssertionError) as e:
             tr_record.test_fail(e)
         except signals.TestSkip as e:
@@ -364,8 +366,10 @@ class BaseTestClass(object):
             # Exception happened during test.
             tr_record.test_error(e)
         else:
-            tr_record.test_pass()
+            if not teardown_test_failed:
+                tr_record.test_pass()
         finally:
+            tr_record.update_record()
             if tr_record.result in (records.TestResultEnums.TEST_RESULT_ERROR,
                                     records.TestResultEnums.TEST_RESULT_FAIL):
                 self._exec_procedure_func(self._on_fail, tr_record)
diff --git a/tests/mobly/base_test_test.py b/tests/mobly/base_test_test.py
index 1cc1078..bd7dce9 100755
--- a/tests/mobly/base_test_test.py
+++ b/tests/mobly/base_test_test.py
@@ -250,7 +250,7 @@ class BaseTestTest(unittest.TestCase):
     def test_teardown_test_assert_fail(self):
         class MockBaseTest(base_test.BaseTestClass):
             def teardown_test(self):
-                asserts.assert_true(False, MSG_EXPECTED_EXCEPTION)
+                asserts.fail(MSG_EXPECTED_EXCEPTION)
 
             def test_something(self):
                 pass
@@ -259,7 +259,7 @@ class BaseTestTest(unittest.TestCase):
         bt_cls.run()
         actual_record = bt_cls.results.error[0]
         self.assertEqual(actual_record.test_name, self.mock_test_name)
-        self.assertIsNone(actual_record.details)
+        self.assertEqual(actual_record.details, MSG_EXPECTED_EXCEPTION)
         self.assertIsNone(actual_record.extras)
         expected_summary = ("Error 1, Executed 1, Failed 0, Passed 0, "
                             "Requested 1, Skipped 0")
@@ -277,10 +277,9 @@ class BaseTestTest(unittest.TestCase):
         bt_cls.run()
         actual_record = bt_cls.results.error[0]
         self.assertEqual(actual_record.test_name, self.mock_test_name)
-        self.assertIsNone(actual_record.details)
+        self.assertEqual(actual_record.details, MSG_EXPECTED_EXCEPTION)
         self.assertIsNone(actual_record.extras)
-        expected_extra_error = {"teardown_test": MSG_EXPECTED_EXCEPTION}
-        self.assertEqual(actual_record.extra_errors, expected_extra_error)
+        self.assertFalse(actual_record.extra_errors)
         expected_summary = ("Error 1, Executed 1, Failed 0, Passed 0, "
                             "Requested 1, Skipped 0")
         self.assertEqual(bt_cls.results.summary_str(), expected_summary)
@@ -372,10 +371,10 @@ class BaseTestTest(unittest.TestCase):
 
         bt_cls = MockBaseTest(self.mock_test_cls_configs)
         bt_cls.run()
-        my_mock.assert_called_once_with("on_fail")
+        my_mock.assert_called_once_with('on_fail')
         actual_record = bt_cls.results.error[0]
         self.assertEqual(actual_record.test_name, self.mock_test_name)
-        self.assertIsNone(actual_record.details)
+        self.assertEqual(actual_record.details, MSG_EXPECTED_EXCEPTION)
         self.assertIsNone(actual_record.extras)
         expected_summary = ("Error 1, Executed 1, Failed 0, Passed 0, "
                             "Requested 1, Skipped 0")
@@ -463,8 +462,8 @@ class BaseTestTest(unittest.TestCase):
         actual_record = bt_cls.results.error[0]
         self.assertEqual(actual_record.test_name, self.mock_test_name)
         self.assertEqual(actual_record.details, MSG_EXPECTED_EXCEPTION)
-        self.assertEqual(actual_record.extra_errors,
-                         {'teardown_test': 'This is an expected exception.ha'})
+        self.assertEqual(actual_record.extra_errors['teardown_test'].details,
+                         'This is an expected exception.ha')
         self.assertIsNone(actual_record.extras)
         expected_summary = ("Error 1, Executed 1, Failed 0, Passed 0, "
                             "Requested 1, Skipped 0")
@@ -504,12 +503,12 @@ class BaseTestTest(unittest.TestCase):
 
         bt_cls = MockBaseTest(self.mock_test_cls_configs)
         bt_cls.run()
-        actual_record = bt_cls.results.error[0]
+        actual_record = bt_cls.results.failed[0]
         self.assertIn('_on_fail', actual_record.extra_errors)
         self.assertEqual(actual_record.test_name, self.mock_test_name)
         self.assertEqual(actual_record.details, MSG_EXPECTED_EXCEPTION)
         self.assertIsNone(actual_record.extras)
-        expected_summary = ("Error 1, Executed 1, Failed 0, Passed 0, "
+        expected_summary = ("Error 0, Executed 1, Failed 1, Passed 0, "
                             "Requested 1, Skipped 0")
         self.assertEqual(bt_cls.results.summary_str(), expected_summary)
 
@@ -526,8 +525,8 @@ class BaseTestTest(unittest.TestCase):
         bt_cls = MockBaseTest(self.mock_test_cls_configs)
         bt_cls.run()
         actual_record = bt_cls.results.error[0]
-        expected_extra_error = {'_on_pass': expected_msg}
-        self.assertEqual(actual_record.extra_errors, expected_extra_error)
+        self.assertEqual(actual_record.extra_errors['_on_pass'].details,
+                         expected_msg)
         self.assertEqual(actual_record.test_name, self.mock_test_name)
         self.assertEqual(actual_record.details, MSG_EXPECTED_EXCEPTION)
         self.assertIsNone(actual_record.extras)
@@ -549,12 +548,58 @@ class BaseTestTest(unittest.TestCase):
         self.assertEqual(actual_record.test_name, self.mock_test_name)
         self.assertEqual(actual_record.details, "Test Body Exception.")
         self.assertIsNone(actual_record.extras)
-        self.assertEqual(actual_record.extra_errors["teardown_test"],
-                         "Details=This is an expected exception., Extras=None")
+        self.assertEqual(actual_record.extra_errors['teardown_test'].details,
+                         MSG_EXPECTED_EXCEPTION)
+        self.assertIsNone(actual_record.extra_errors['teardown_test'].extras)
         expected_summary = ("Error 1, Executed 1, Failed 0, Passed 0, "
                             "Requested 1, Skipped 0")
         self.assertEqual(bt_cls.results.summary_str(), expected_summary)
 
+    def test_exception_objects_in_record(self):
+        """Checks that the exception objects are correctly tallied.
+        """
+        expected_termination_signal = Exception('Test Body Exception.')
+        expected_extra_error = Exception('teardown_test Exception.')
+
+        class MockBaseTest(base_test.BaseTestClass):
+            def teardown_test(self):
+                raise expected_extra_error
+
+            def test_something(self):
+                raise expected_termination_signal
+
+        bt_cls = MockBaseTest(self.mock_test_cls_configs)
+        bt_cls.run()
+        actual_record = bt_cls.results.error[0]
+        self.assertIs(actual_record.termination_signal.exception,
+                      expected_termination_signal)
+        self.assertIsNotNone(actual_record.termination_signal.stacktrace)
+        self.assertEqual(len(actual_record.extra_errors), 1)
+        extra_error = actual_record.extra_errors['teardown_test']
+        self.assertIs(extra_error.exception, expected_extra_error)
+        self.assertIsNotNone(extra_error.stacktrace)
+        self.assertIsNone(actual_record.extras)
+
+    def test_promote_extra_errors_to_termination_signal(self):
+        """If no termination singal is specified, use the first extra error as
+        the termination signal.
+        """
+        expected_extra_error = Exception('teardown_test Exception.')
+
+        class MockBaseTest(base_test.BaseTestClass):
+            def teardown_test(self):
+                raise expected_extra_error
+
+            def test_something(self):
+                pass
+
+        bt_cls = MockBaseTest(self.mock_test_cls_configs)
+        bt_cls.run()
+        actual_record = bt_cls.results.error[0]
+        self.assertFalse(actual_record.extra_errors)
+        self.assertEqual(actual_record.details, 'teardown_test Exception.')
+        self.assertIsNotNone(actual_record.stacktrace)
+
     def test_explicit_pass_but_teardown_test_raises_an_exception(self):
         """Test record result should be marked as ERROR as opposed to PASS.
         """
@@ -564,16 +609,17 @@ class BaseTestTest(unittest.TestCase):
                 asserts.assert_true(False, MSG_EXPECTED_EXCEPTION)
 
             def test_something(self):
-                asserts.explicit_pass("Test Passed!")
+                asserts.explicit_pass('Test Passed!')
 
         bt_cls = MockBaseTest(self.mock_test_cls_configs)
         bt_cls.run()
         actual_record = bt_cls.results.error[0]
         self.assertEqual(actual_record.test_name, self.mock_test_name)
-        self.assertEqual(actual_record.details, "Test Passed!")
+        self.assertEqual(actual_record.details, 'Test Passed!')
         self.assertIsNone(actual_record.extras)
-        self.assertEqual(actual_record.extra_errors["teardown_test"],
-                         "Details=This is an expected exception., Extras=None")
+        self.assertEqual(actual_record.extra_errors['teardown_test'].details,
+                         MSG_EXPECTED_EXCEPTION)
+        self.assertIsNone(actual_record.extra_errors['teardown_test'].extras)
         expected_summary = ("Error 1, Executed 1, Failed 0, Passed 0, "
                             "Requested 1, Skipped 0")
         self.assertEqual(bt_cls.results.summary_str(), expected_summary)
@@ -606,8 +652,8 @@ class BaseTestTest(unittest.TestCase):
         self.assertEqual(actual_record.test_name, self.mock_test_name)
         self.assertEqual(actual_record.details, MSG_EXPECTED_EXCEPTION)
         self.assertEqual(actual_record.extras, MOCK_EXTRA)
-        self.assertEqual(actual_record.extra_errors,
-                         {'_on_pass': MSG_EXPECTED_EXCEPTION})
+        self.assertEqual(actual_record.extra_errors['_on_pass'].details,
+                         MSG_EXPECTED_EXCEPTION)
         expected_summary = ("Error 1, Executed 1, Failed 0, Passed 0, "
                             "Requested 1, Skipped 0")
         self.assertEqual(bt_cls.results.summary_str(), expected_summary)
@@ -622,14 +668,13 @@ class BaseTestTest(unittest.TestCase):
 
         bt_cls = MockBaseTest(self.mock_test_cls_configs)
         bt_cls.run()
-        actual_record = bt_cls.results.error[0]
-        self.assertEqual(bt_cls.results.failed, [])
+        actual_record = bt_cls.results.failed[0]
         self.assertEqual(actual_record.test_name, self.mock_test_name)
         self.assertEqual(actual_record.details, MSG_EXPECTED_EXCEPTION)
         self.assertEqual(actual_record.extras, MOCK_EXTRA)
-        self.assertEqual(actual_record.extra_errors,
-                         {'_on_fail': MSG_EXPECTED_EXCEPTION})
-        expected_summary = ("Error 1, Executed 1, Failed 0, Passed 0, "
+        self.assertEqual(actual_record.extra_errors['_on_fail'].details,
+                         MSG_EXPECTED_EXCEPTION)
+        expected_summary = ("Error 0, Executed 1, Failed 1, Passed 0, "
                             "Requested 1, Skipped 0")
         self.assertEqual(bt_cls.results.summary_str(), expected_summary)
 
diff --git a/tests/mobly/records_test.py b/tests/mobly/records_test.py
index a5f68db..719f0c8 100755
--- a/tests/mobly/records_test.py
+++ b/tests/mobly/records_test.py
@@ -28,6 +28,7 @@ from tests.lib import utils
 class RecordsTest(unittest.TestCase):
     """This test class tests the implementation of classes in mobly.records.
     """
+
     def setUp(self):
         self.tn = "test_name"
         self.details = "Some details about the test execution."
@@ -38,7 +39,8 @@ class RecordsTest(unittest.TestCase):
     def tearDown(self):
         shutil.rmtree(self.tmp_path)
 
-    def verify_record(self, record, result, details, extras):
+    def verify_record(self, record, result, details, extras, stacktrace=None):
+        record.update_record()
         # Verify each field.
         self.assertEqual(record.test_name, self.tn)
         self.assertEqual(record.result, result)
@@ -58,9 +60,15 @@ class RecordsTest(unittest.TestCase):
         d[records.TestResultEnums.RECORD_END_TIME] = record.end_time
         d[records.TestResultEnums.RECORD_UID] = None
         d[records.TestResultEnums.RECORD_CLASS] = None
-        d[records.TestResultEnums.RECORD_EXTRA_ERRORS] = {}
-        d[records.TestResultEnums.RECORD_STACKTRACE] = None
+        d[records.TestResultEnums.RECORD_EXTRA_ERRORS] = []
+        d[records.TestResultEnums.RECORD_STACKTRACE] = stacktrace
         actual_d = record.to_dict()
+        # Verify stacktrace partially match as stacktraces often have file path
+        # in them.
+        if stacktrace:
+            stacktrace_key = records.TestResultEnums.RECORD_STACKTRACE
+            self.assertTrue(
+                d.pop(stacktrace_key) in actual_d.pop(stacktrace_key))
         self.assertDictEqual(actual_d, d)
         # Verify that these code paths do not cause crashes and yield non-empty
         # results.
@@ -123,16 +131,14 @@ class RecordsTest(unittest.TestCase):
         # Verify stacktrace separately if we expect a non-None value.
         # Because stacktrace includes file names and line numbers, we can't do
         # a simple equality check.
-        self.assertTrue('Something failed.' in record.stacktrace)
-        self.assertTrue(
-            'in test_result_record_fail_stacktrace\n    raise Exception' in
-            record.stacktrace)
-        record.stacktrace = None
         self.verify_record(
             record=record,
             result=records.TestResultEnums.TEST_RESULT_FAIL,
             details='Something failed.',
-            extras=None)
+            extras=None,
+            stacktrace='in test_result_record_fail_stacktrace\n    '
+            'raise Exception(\'Something failed.\')\nException: '
+            'Something failed.\n')
 
     def test_result_record_fail_with_float_extra(self):
         record = records.TestResultRecord(self.tn)