fix: catch BaseException from instrumented code

Catch all exceptions thrown from instrumented code by catching
BaseException, rather than Exception. This ensures that the appropriate
event gets added to the recording.

This problem was originally found when running instrumented tests in
pytest-dev/pytest, so the changes include a test copied from there.
pytest's OutcomeException (and its subclasses, like Skipped) inherits
from BaseException. pytest raises an OutcomeException internally to
indicate how a test case finished.

_appmap/instrument.py

@@ -101,7 +101,12 @@ def call_instrumented(f, instance, args, kwargs):
         return ret
     except AppMapLimitExceeded:
         raise
-    except Exception:  # noqa: E722
+    # Some applications make use of exceptions that aren't descended from Exception. For example,
+    # pytest's OutcomeException, used to indicate the outcome of a test case, is a child of
+    # BaseException.
+    #
+    # We need to catch *any* exception raised, to ensure that we add the appropriate ExceptionEvent.
+    except BaseException:  # noqa: E722
         elapsed_time = time.time() - start_time
         Recorder.add_event(
             event.ExceptionEvent(

_appmap/test/data/example_class.py

@@ -5,6 +5,7 @@
 
 import time
 from functools import lru_cache, wraps
+from typing import NoReturn
 
 import appmap
 
@@ -155,6 +156,8 @@ def del_write_only(self):
 
     write_only = property(None, set_write_only, del_write_only, "Write-only")
 
+    def raise_base_exception(self) -> NoReturn:
+        raise BaseException("not derived from Exception") # pylint: disable=broad-exception-raised
 
 def modfunc():
     return "Hello world!"

_appmap/test/data/pytest-instrumented/appmap.yml

@@ -0,0 +1,12 @@
+name: Simple
+packages:
+- path: simple
+- path: _pytest
+  exclude:
+  # - _py.path
+  - compat.safe_getattr
+  - config.PytestPluginManager
+  - fixtures.getfixturemarker
+  - config.argparsing
+  - config.Config.rootpath
+- path: pytest

_appmap/test/data/pytest-instrumented/init/sitecustomize.py

@@ -0,0 +1 @@
+import appmap

_appmap/test/data/pytest-instrumented/test_instrumented.py

@@ -0,0 +1,16 @@
+import pytest
+
+# Copied from pytest-dev/pytest. When recorded, this test case will raise an OutcomeException
+# (specifically _pytest.outcomes.Skipped).
+def test_skipped(pytester):
+    pytester.makeconftest(
+        """
+        import pytest
+        def pytest_ignore_collect():
+            pytest.skip("intentional")
+    """
+    )
+    pytester.makepyfile("def test_hello(): pass")
+    result = pytester.runpytest_inprocess()
+    assert result.ret == pytest.ExitCode.NO_TESTS_COLLECTED
+    result.stdout.fnmatch_lines(["*1 skipped*"])

_appmap/test/helpers.py

@@ -43,7 +43,19 @@ def check_call_stack(events):
         if e.get("event") == "call":
             stack.append(e)
         elif e.get("event") == "return":
-            assert len(stack) > 0, "return without call"
+            assert len(stack) > 0, f"return without call, {e.get('id')}"
             call = stack.pop()
-            assert call.get("id") == e.get("parent_id")
-    assert len(stack) == 0, "leftover events"
+            assert call.get("id") == e.get(
+                "parent_id"
+            ), f"parent mismatch, {call.get('id')} != {e.get('parent_id')}"
+    assert len(stack) == 0, f"leftover events, {len(stack)}"
+
+
+if __name__ == "__main__":
+    import json
+    from pathlib import Path
+    import sys
+
+    with Path(sys.argv[1]).open(encoding="utf-8") as f:
+        appmap = json.load(f)
+        check_call_stack(appmap["events"])

_appmap/test/test_events.py

@@ -117,3 +117,36 @@ def test_describe_return_value_recursion_protection(self):
         assert [e.method_id for e in r.events if e.event == "call" and hasattr(e, "method_id")] == [
             "return_self"
         ]
+
+    # There should be an exception return event generated even when the raised exception is a
+    # BaseException.
+    def test_exception_event_with_base_exception(self):
+        r = appmap.Recording()
+        with r:
+            # pylint: disable=import-outside-toplevel
+            from example_class import ExampleClass
+
+            try:
+                ExampleClass().raise_base_exception()
+            except BaseException: # pylint: disable=broad-exception-caught
+                pass
+        assert check_call_return_stack_order(r.events), "Unbalanced call stack"
+
+
+def check_call_return_stack_order(events):
+    stack = []
+    for e in events:
+        if e.event == "call":
+            stack.append(e)
+        elif e.event == "return":
+            if len(stack) > 0:
+                call = stack.pop()
+                if call.id != e.parent_id:
+                    return False
+            else:
+                return False
+    if len(stack) == 0:
+        return True
+
+    return False
+

_appmap/test/test_test_frameworks.py

@@ -10,10 +10,10 @@
 from pathlib import Path
 
 import pytest
+
 from _appmap import recording
-from _appmap.test.helpers import package_version
 
-from ..test.helpers import DictIncluding
+from .helpers import DictIncluding, check_call_stack, package_version
 from .normalize import normalize_appmap
 
 
@@ -155,6 +155,17 @@ def test_write_appmap(recorder_outdir):
     expected_shortname = longname[:235] + "-5d6e10d.appmap.json"
     assert (recorder_outdir / expected_shortname).read_text().startswith('{"version"')
 
+@pytest.mark.example_dir("pytest-instrumented")
+@pytest.mark.appmap_enabled
+def test_pytest_instrumented(testdir):
+    result = testdir.runpytest("-svv", "-p", "pytester", "test_instrumented.py")
+    result.assert_outcomes(passed=1)
+    appmap_file = testdir.path / "tmp" / "appmap" / "pytest" / "test_skipped.appmap.json"
+    appmap = json.load(appmap_file.open())
+    events = appmap["events"]
+    assert len(events) > 0
+    check_call_stack(events)
+
 
 def verify_expected_appmap(testdir, suffix=""):
     appmap_json = list(testdir.output().glob("*test_hello_world.appmap.json"))