Test run exit status should be `0` if test retry eventually passed. (#755)

3 files changed, 43 insertions, 14 deletions

diff --git a/mobly/base_test.py b/mobly/base_test.py
index 2823a1a..d6e9faa 100644
--- a/mobly/base_test.py
+++ b/mobly/base_test.py
@@ -670,7 +670,7 @@ class BaseTestClass:
     for i in range(max_count - 1):
       retry_name = f'{test_name}_retry_{i+1}'
       new_record = records.TestResultRecord(retry_name, self.TAG)
-      new_record.retry_parent = previous_record.signature
+      new_record.retry_parent = previous_record
       previous_record = self.exec_one_test(retry_name, test_method, new_record)
       if not should_retry(previous_record):
         break
diff --git a/mobly/records.py b/mobly/records.py
index 981051e..f4ec4d5 100644
--- a/mobly/records.py
+++ b/mobly/records.py
@@ -311,9 +311,9 @@ class TestResultRecord:
     uid: User-defined unique identifier of the test.
     signature: string, unique identifier of a test record, the value is
       generated by Mobly.
-    retry_parent: string, only set for retry iterations. This is the signature
-      of the previous iteration of this retry. Parsers can use this field to
-      construct the chain of execution for each retried test.
+    retry_parent: TestResultRecord, only set for retry iterations. This is the
+      test result record of the previous retry iteration. Parsers can use this
+      field to construct the chain of execution for each retried 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.
@@ -470,7 +470,7 @@ class TestResultRecord:
   def __repr__(self):
     """This returns a short string representation of the test record."""
     t = utils.epoch_to_human_time(self.begin_time)
-    return '%s %s %s' % (t, self.test_name, self.result)
+    return f'{t} {self.test_name} {self.result}'
 
   def to_dict(self):
     """Gets a dictionary representating the content of this class.
@@ -486,7 +486,8 @@ class TestResultRecord:
     d[TestResultEnums.RECORD_RESULT] = self.result
     d[TestResultEnums.RECORD_UID] = self.uid
     d[TestResultEnums.RECORD_SIGNATURE] = self.signature
-    d[TestResultEnums.RECORD_RETRY_PARENT] = self.retry_parent
+    d[TestResultEnums.
+      RECORD_RETRY_PARENT] = self.retry_parent.signature if self.retry_parent else None
     d[TestResultEnums.RECORD_EXTRAS] = self.extras
     d[TestResultEnums.RECORD_DETAILS] = self.details
     d[TestResultEnums.RECORD_EXTRA_ERRORS] = {
@@ -607,11 +608,31 @@ class TestResult:
         return True
     return False
 
+  def _count_eventually_passing_retries(self):
+    """Counts the number of retry iterations that eventually passed.
+
+    If a test is retried and eventually passed, all the associated non-passing
+    iterations should not be considered when devising the final state of the
+    test run.
+
+    Returns:
+      Int, the number that should be subtracted from the result altering error
+      counts.
+    """
+    count = 0
+    for record in self.passed:
+      r = record
+      while r.retry_parent:
+        count += 1
+        r = r.retry_parent
+    return count
+
   @property
   def is_all_pass(self):
     """True if no tests failed or threw errors, False otherwise."""
-    num_of_failures = len(self.failed) + len(self.error)
-    if num_of_failures == 0:
+    num_of_result_altering_errors = (len(self.failed) + len(self.error) -
+                                     self._count_eventually_passing_retries())
+    if num_of_result_altering_errors == 0:
       return True
     return False
 
diff --git a/tests/mobly/base_test_test.py b/tests/mobly/base_test_test.py
index 7fcfb70..e9a7e80 100755
--- a/tests/mobly/base_test_test.py
+++ b/tests/mobly/base_test_test.py
@@ -2365,8 +2365,10 @@ class BaseTestTest(unittest.TestCase):
     max_consec_error = 2
     mock_action = mock.MagicMock()
     mock_action.side_effect = [
-        Exception('Error 1'), None,
-        Exception('Error 2'), None,
+        Exception('Error 1'),
+        None,
+        Exception('Error 2'),
+        None,
         Exception('Error 3'),
     ]
 
@@ -2406,6 +2408,8 @@ class BaseTestTest(unittest.TestCase):
 
     bt_cls = MockBaseTest(self.mock_test_cls_configs)
     bt_cls.run()
+    self.assertTrue(bt_cls.results.is_all_pass,
+                    'This test run should be considered pass.')
     self.assertEqual(1, len(bt_cls.results.executed))
     self.assertEqual(1, len(bt_cls.results.passed))
     pass_record = bt_cls.results.passed[0]
@@ -2425,6 +2429,8 @@ class BaseTestTest(unittest.TestCase):
 
     bt_cls = MockBaseTest(self.mock_test_cls_configs)
     bt_cls.run()
+    self.assertTrue(bt_cls.results.is_all_pass,
+                    'This test run should be considered pass.')
     self.assertEqual(3, len(bt_cls.results.executed))
     self.assertEqual(1, len(bt_cls.results.passed))
     pass_record = bt_cls.results.passed[0]
@@ -2433,8 +2439,8 @@ class BaseTestTest(unittest.TestCase):
     error_record_1, error_record_2 = bt_cls.results.error
     self.assertEqual(error_record_1.test_name, 'test_something')
     self.assertEqual(error_record_2.test_name, 'test_something_retry_1')
-    self.assertEqual(error_record_1.signature, error_record_2.retry_parent)
-    self.assertEqual(error_record_2.signature, pass_record.retry_parent)
+    self.assertIs(error_record_1, error_record_2.retry_parent)
+    self.assertIs(error_record_2, pass_record.retry_parent)
 
   def test_retry_all_fail(self):
     max_count = 3
@@ -2453,14 +2459,16 @@ class BaseTestTest(unittest.TestCase):
 
     bt_cls = MockBaseTest(self.mock_test_cls_configs)
     bt_cls.run()
+    self.assertFalse(bt_cls.results.is_all_pass,
+                     'This test run should be considered fail.')
     self.assertEqual(3, len(bt_cls.results.executed))
     self.assertEqual(3, len(bt_cls.results.error))
     error_record_1, error_record_2, error_record_3 = bt_cls.results.error
     self.assertEqual(error_record_1.test_name, 'test_something')
     self.assertEqual(error_record_2.test_name, 'test_something_retry_1')
     self.assertEqual(error_record_3.test_name, 'test_something_retry_2')
-    self.assertEqual(error_record_1.signature, error_record_2.retry_parent)
-    self.assertEqual(error_record_2.signature, error_record_3.retry_parent)
+    self.assertIs(error_record_1, error_record_2.retry_parent)
+    self.assertIs(error_record_2, error_record_3.retry_parent)
 
   def test_uid(self):