Repository: google/mobly

Issue:
`signals.TestAbortClass` does not work in `setup_test` and `on_xxxx` methods
It is valid to abort the class at any stage of a test.

E.g. If we find out in `on_fail` that a test failed due to a fatal error like snippet_client losing socket connection, it is reasonable for the test to abort all subsequent tests in the class instead of continue running and failing more tests when we know they will fail.

Discussion:
dthkao: 



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

---

*[mobly/base_test.py, line 321 at r1](https://reviewable.io:443/reviews/google/mobly/323#-KsW8dICenAvZlCo5EPy:-KsW8dICenAvZlCo5EPz:bc0xqwv) ([raw file](https://github.com/google/mobly/blob/4fb6d1a9cee058cf24bf5a7c8d76f355862f0388/mobly/base_test.py#L321)):*
> ```Python
>             # will not be modified.
>             func(copy.deepcopy(tr_record))
>         except (signals.TestAbortAll, signals.TestAbortClass):
> ```

How about having one of these subclass the other or both of them subclassing from something like `signals.TestAbort`. It seems their handling is often very similar.

---

*[mobly/base_test.py, line 349 at r1](https://reviewable.io:443/reviews/google/mobly/323#-KsW89wWCGfYzGYozVN4:-KsW89wWCGfYzGYozVN5:b-pgdkr0) ([raw file](https://github.com/google/mobly/blob/4fb6d1a9cee058cf24bf5a7c8d76f355862f0388/mobly/base_test.py#L349)):*
> ```Python
>                 try:
>                     self._setup_test(test_name)
>                 except (signals.TestAbortAll, signals.TestAbortClass):
> ```

Why is this needed? Wouldn't they be raised anyway?

---

*[mobly/base_test.py, line 368 at r1](https://reviewable.io:443/reviews/google/mobly/323#-KsW8_r_xwSxCOA91u5X:-KsW8_r_xwSxCOA91u5Y:bxthl54) ([raw file](https://github.com/google/mobly/blob/4fb6d1a9cee058cf24bf5a7c8d76f355862f0388/mobly/base_test.py#L368)):*
> ```Python
>                 try:
>                     self._teardown_test(test_name)
>                 except signals.TestAbortAll:
> ```

Does this need the same treatment?

---


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

k2fong: 



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

---

*[tests/mobly/base_test_test.py, line 780 at r1](https://reviewable.io:443/reviews/google/mobly/323#-KsLbUjtTowbfMVKVkYq:-KsLbUjuEJrAru-PfJfg:bbi2jmz) ([raw file](https://github.com/google/mobly/blob/4fb6d1a9cee058cf24bf5a7c8d76f355862f0388/tests/mobly/base_test_test.py#L780)):*
> ```Python
>         bt_cls.run(test_names=["test_1", "test_2", "test_3"])
>         self.assertEqual(len(bt_cls.results.skipped), 2)
>         self.assertEqual(bt_cls.results.summary_str(),
> ```

consider asserting on values from bt_cls.summary_dict() instead or similar len comparison above on the other summary results.  plus 'skipped' check is redundant and test will be more susceptible to format changes.

---


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

xpconanfan: 



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

---

*[mobly/base_test.py, line 321 at r1](https://reviewable.io:443/reviews/google/mobly/323#-KsW8dICenAvZlCo5EPy:-KsfESx857NKFKpEj7F0:b-896fix) ([raw file](https://github.com/google/mobly/blob/4fb6d1a9cee058cf24bf5a7c8d76f355862f0388/mobly/base_test.py#L321)):*
<details><summary><i>Previously, dthkao (David T.H. Kao) wrote…</i></summary><blockquote>

How about having one of these subclass the other or both of them subclassing from something like `signals.TestAbort`. It seems their handling is often very similar.
</blockquote></details>

Done.

---

*[mobly/base_test.py, line 349 at r1](https://reviewable.io:443/reviews/google/mobly/323#-KsW89wWCGfYzGYozVN4:-Ksf7OVznst44Y5kUKvg:b-7xle7c) ([raw file](https://github.com/google/mobly/blob/4fb6d1a9cee058cf24bf5a7c8d76f355862f0388/mobly/base_test.py#L349)):*
<details><summary><i>Previously, dthkao (David T.H. Kao) wrote…</i></summary><blockquote>

Why is this needed? Wouldn't they be raised anyway?
</blockquote></details>

So they don't fall to the next `except` and get converted to TestError.

---

*[mobly/base_test.py, line 368 at r1](https://reviewable.io:443/reviews/google/mobly/323#-KsW8_r_xwSxCOA91u5X:-KsfERkA9Kclo_q7SjRj:b-896fix) ([raw file](https://github.com/google/mobly/blob/4fb6d1a9cee058cf24bf5a7c8d76f355862f0388/mobly/base_test.py#L368)):*
<details><summary><i>Previously, dthkao (David T.H. Kao) wrote…</i></summary><blockquote>

Does this need the same treatment?
</blockquote></details>

Done.

---

*[tests/mobly/base_test_test.py, line 780 at r1](https://reviewable.io:443/reviews/google/mobly/323#-KsLbUjtTowbfMVKVkYq:-Ksf73Vo1RhZ2v9ji8if:b-1fg0vl) ([raw file](https://github.com/google/mobly/blob/4fb6d1a9cee058cf24bf5a7c8d76f355862f0388/tests/mobly/base_test_test.py#L780)):*
<details><summary><i>Previously, k2fong wrote…</i></summary><blockquote>

consider asserting on values from bt_cls.summary_dict() instead or similar len comparison above on the other summary results.  plus 'skipped' check is redundant and test will be more susceptible to format changes.
</blockquote></details>

We specifically chose to verify this string as it is user-facing.
Since the string has all the info, we don't need a separate check for the dict.

---


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

dthkao: 



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

---

*[mobly/base_test.py, line 349 at r1](https://reviewable.io:443/reviews/google/mobly/323#-KsW89wWCGfYzGYozVN4:-KsgL-Sv8l0YmluAy2mm:bit11dr) ([raw file](https://github.com/google/mobly/blob/4fb6d1a9cee058cf24bf5a7c8d76f355862f0388/mobly/base_test.py#L349)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

So they don't fall to the next `except` and get converted to TestError.
</blockquote></details>

The next except only catches TestFailure though?

---


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

k2fong: 



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

---

*[tests/mobly/base_test_test.py, line 780 at r1](https://reviewable.io:443/reviews/google/mobly/323#-KsLbUjtTowbfMVKVkYq:-KsioSRefUXtm5KaI4Rf:b-wtk41s) ([raw file](https://github.com/google/mobly/blob/4fb6d1a9cee058cf24bf5a7c8d76f355862f0388/tests/mobly/base_test_test.py#L780)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

We specifically chose to verify this string as it is user-facing.
Since the string has all the info, we don't need a separate check for the dict.
</blockquote></details>

If that's the case, it will seem like verification for this summary string should be its own test that properly test out the various values reported from this summary string (if it doesn't exist already).  But probably outside the scope of this review. 

---


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

xpconanfan: 



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

---

*[mobly/base_test.py, line 349 at r1](https://reviewable.io:443/reviews/google/mobly/323#-KsW89wWCGfYzGYozVN4:-KsivUyUSfXVYTMfozsQ:b31vzk8) ([raw file](https://github.com/google/mobly/blob/4fb6d1a9cee058cf24bf5a7c8d76f355862f0388/mobly/base_test.py#L349)):*
<details><summary><i>Previously, dthkao (David T.H. Kao) wrote…</i></summary><blockquote>

The next except only catches TestFailure though?
</blockquote></details>

Done.

Oops, somehow I thought it was TestSignal...

---

*[tests/mobly/base_test_test.py, line 780 at r1](https://reviewable.io:443/reviews/google/mobly/323#-KsLbUjtTowbfMVKVkYq:-KsivZ92ZwohGN6R-Jzq:bvadmfs) ([raw file](https://github.com/google/mobly/blob/4fb6d1a9cee058cf24bf5a7c8d76f355862f0388/tests/mobly/base_test_test.py#L780)):*
<details><summary><i>Previously, k2fong wrote…</i></summary><blockquote>

If that's the case, it will seem like verification for this summary string should be its own test that properly test out the various values reported from this summary string (if it doesn't exist already).  But probably outside the scope of this review. 
</blockquote></details>

No, each test expects a different summaryve string with different info.
It's a lot easier to just verify the final product than each field every time here in `base_test_test`.

We have per-field checks in `record_test`.

---


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


Patch:
diff --git a/mobly/controllers/android_device.py b/mobly/controllers/android_device.py
index 2be180b..5d7027e 100644
--- a/mobly/controllers/android_device.py
+++ b/mobly/controllers/android_device.py
@@ -725,7 +725,17 @@ class AndroidDevice(object):
                     ' "%s".' % (package, client_name))
         client = snippet_client.SnippetClient(
             package=package, adb_proxy=self.adb, log=self.log)
-        client.start_app_and_connect()
+        try:
+            client.start_app_and_connect()
+        except Exception as e:
+            # If errors happen, make sure we clean up before raising.
+            try:
+                client.stop_app()
+            except:
+                self.log.exception(
+                    'Failed to stop app after failure to launch.')
+            # Raise the error from start app failure.
+            raise e
         self._snippet_clients[name] = client
         setattr(self, name, client)
 
diff --git a/mobly/signals.py b/mobly/signals.py
index 85bdc30..e367a8f 100644
--- a/mobly/signals.py
+++ b/mobly/signals.py
@@ -62,13 +62,18 @@ class TestSkip(TestSignal):
     """Raised when a test has been skipped."""
 
 
-class TestAbortClass(TestSignal):
+class TestAbortSignal(TestSignal):
+    """Base class for abort signals.
+    """
+
+
+class TestAbortClass(TestAbortSignal):
     """Raised when all subsequent tests within the same test class should
     be aborted.
     """
 
 
-class TestAbortAll(TestSignal):
+class TestAbortAll(TestAbortSignal):
     """Raised when all subsequent tests should be aborted."""
 
 


Test patch:
diff --git a/mobly/base_test.py b/mobly/base_test.py
index 0c540a5..24290de 100644
--- a/mobly/base_test.py
+++ b/mobly/base_test.py
@@ -318,7 +318,7 @@ class BaseTestClass(object):
             # 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:
+        except signals.TestAbortSignal:
             raise
         except Exception as e:
             logging.exception('Exception happened when executing %s for %s.',
@@ -363,7 +363,7 @@ class BaseTestClass(object):
             finally:
                 try:
                     self._teardown_test(test_name)
-                except signals.TestAbortAll:
+                except signals.TestAbortSignal:
                     raise
                 except Exception as e:
                     logging.exception(e)
@@ -374,7 +374,7 @@ class BaseTestClass(object):
         except signals.TestSkip as e:
             # Test skipped.
             tr_record.test_skip(e)
-        except (signals.TestAbortClass, signals.TestAbortAll) as e:
+        except signals.TestAbortSignal as e:
             # Abort signals, pass along.
             tr_record.test_fail(e)
             raise e
@@ -389,17 +389,20 @@ class BaseTestClass(object):
                 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)
-            elif tr_record.result == records.TestResultEnums.TEST_RESULT_PASS:
-                self._exec_procedure_func(self._on_pass, tr_record)
-            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)
-            self.current_test_name = None
+            try:
+                if tr_record.result in (
+                        records.TestResultEnums.TEST_RESULT_ERROR,
+                        records.TestResultEnums.TEST_RESULT_FAIL):
+                    self._exec_procedure_func(self._on_fail, tr_record)
+                elif tr_record.result == records.TestResultEnums.TEST_RESULT_PASS:
+                    self._exec_procedure_func(self._on_pass, tr_record)
+                elif tr_record.result == records.TestResultEnums.TEST_RESULT_SKIP:
+                    self._exec_procedure_func(self._on_skip, tr_record)
+            finally:
+                self.results.add_record(tr_record)
+                self.summary_writer.dump(tr_record.to_dict(),
+                                         records.TestSummaryEntryType.RECORD)
+                self.current_test_name = None
 
     def _assert_function_name_in_stack(self, expected_func_name):
         """Asserts that the current stack contains the given function name."""
@@ -577,7 +580,7 @@ class BaseTestClass(object):
         # Setup for the class.
         try:
             self._setup_class()
-        except signals.TestAbortClass as e:
+        except signals.TestAbortSignal as e:
             # The test class is intentionally aborted.
             # Skip all tests peacefully.
             e.details = 'setup_class aborted due to: %s' % e.details
diff --git a/tests/mobly/base_test_test.py b/tests/mobly/base_test_test.py
index d615f3f..29781af 100755
--- a/tests/mobly/base_test_test.py
+++ b/tests/mobly/base_test_test.py
@@ -739,6 +739,48 @@ class BaseTestTest(unittest.TestCase):
                          ("Error 0, Executed 0, Failed 0, Passed 0, "
                           "Requested 3, Skipped 3"))
 
+    def test_abort_class_in_setup_test(self):
+        class MockBaseTest(base_test.BaseTestClass):
+            def setup_test(self):
+                asserts.abort_class(MSG_EXPECTED_EXCEPTION)
+
+            def test_1(self):
+                never_call()
+
+            def test_2(self):
+                never_call()
+
+            def test_3(self):
+                never_call()
+
+        bt_cls = MockBaseTest(self.mock_test_cls_configs)
+        bt_cls.run(test_names=["test_1", "test_2", "test_3"])
+        self.assertEqual(len(bt_cls.results.skipped), 2)
+        self.assertEqual(bt_cls.results.summary_str(),
+                         ("Error 0, Executed 1, Failed 1, Passed 0, "
+                          "Requested 3, Skipped 2"))
+
+    def test_abort_class_in_on_fail(self):
+        class MockBaseTest(base_test.BaseTestClass):
+            def test_1(self):
+                asserts.fail(MSG_EXPECTED_EXCEPTION)
+
+            def test_2(self):
+                never_call()
+
+            def test_3(self):
+                never_call()
+
+            def on_fail(self, record):
+                asserts.abort_class(MSG_EXPECTED_EXCEPTION)
+
+        bt_cls = MockBaseTest(self.mock_test_cls_configs)
+        bt_cls.run(test_names=["test_1", "test_2", "test_3"])
+        self.assertEqual(len(bt_cls.results.skipped), 2)
+        self.assertEqual(bt_cls.results.summary_str(),
+                         ("Error 0, Executed 1, Failed 1, Passed 0, "
+                          "Requested 3, Skipped 2"))
+
     def test_setup_and_teardown_execution_count(self):
         class MockBaseTest(base_test.BaseTestClass):
             def test_func(self):