test: use pytest-xprocess to manage test servers

Remove our bespoke test server management, use pytest-xprocess instead.
Ensures that pytest doesn't hang if a test fails, and that the tests run
correctly in VS Code (maybe also on Windows, still to be determined).

Also fixes a couple of issues identified by pylint.

Author: apotterri

_appmap/metadata.py

@@ -1,7 +1,6 @@
 """Shared metadata gathering"""
 
 import platform
-import re
 from functools import lru_cache
 
 from . import utils

_appmap/test/__init__.py

@@ -0,0 +1,5 @@
+import pytest
+
+# Make sure assertions in web_framework get rewritten (e.g. to show
+# diffs in generated appmaps)
+pytest.register_assert_rewrite("_appmap.test.web_framework")

_appmap/test/conftest.py

@@ -1,4 +1,5 @@
 import importlib
+import os
 import sys
 from distutils.dir_util import copy_tree
 from functools import partialmethod
@@ -8,6 +9,7 @@
 
 import _appmap
 import appmap
+from _appmap.test.web_framework import _TestRecordRequests
 from appmap import generation
 
 from .. import utils
@@ -128,3 +130,17 @@ def _generate(check_fn, method_name):
         return generation.dump(rec)
 
     return _generate
+
+
+@pytest.fixture(name="server_base")
+def server_base_fixture(request):
+    marker = request.node.get_closest_marker("server")
+    debug = marker.kwargs.get("debug", False)
+    server_env = os.environ.copy()
+    server_env.update(marker.kwargs.get("env", {}))
+    if "PYTHONPATH" in server_env:
+        server_env["PYTHONPATH"] = f"{server_env['PYTHONPATH']}:./init"
+    else:
+        server_env["PYTHONPATH"] = "./init"
+
+    return (_TestRecordRequests.server_host, _TestRecordRequests.server_port, debug, server_env)

_appmap/test/helpers.py

@@ -15,3 +15,14 @@ class DictIncluding(dict):
 
     def __eq__(self, other):
         return other.items() >= self.items()
+
+
+class HeadersIncluding(dict):
+    """Like DictIncluding, but key comparison is case-insensitive."""
+
+    def __eq__(self, other):
+        for k in self.keys():
+            v = other.get(k, other.get(k.lower(), None))
+            if v is None:
+                return False
+        return True

_appmap/test/test_django.py

@@ -3,9 +3,10 @@
 
 import json
 import os
+import socket
 import sys
 from pathlib import Path
-from threading import Thread
+from types import SimpleNamespace as NS
 
 import django
 import django.core.handlers.exception
@@ -15,30 +16,47 @@
 import pytest
 from django.template.loader import render_to_string
 from django.test.client import MULTIPART_CONTENT
+from xprocess import ProcessStarter
 
 import appmap
 import appmap.django  # noqa: F401
 from _appmap.metadata import Metadata
 
 from ..test.helpers import DictIncluding
-
-# Make sure assertions in web_framework get rewritten (e.g. to show
-# diffs in generated appmaps)
-pytest.register_assert_rewrite("_appmap.test.web_framework")
-
-# pylint: disable=unused-import,wrong-import-position
-from .web_framework import TestRemoteRecording  # pyright:ignore
-from .web_framework import TestRequestCapture  # pyright: ignore
-from .web_framework import _TestRecordRequests, exec_cmd, wait_until_port_is
-
-# pylint: enable=unused-import
+from .web_framework import (
+    _TestFormCapture,
+    _TestFormData,
+    _TestRecordRequests,
+    _TestRemoteRecording,
+    _TestRequestCapture,
+)
 
 sys.path += [str(Path(__file__).parent / "data" / "django")]
 
 # Import app just for the side-effects. It must happen after sys.path has been modified.
 import djangoapp  # pyright: ignore pylint: disable=import-error, unused-import,wrong-import-order,wrong-import-position
 
 
+class TestFormCapture(_TestFormCapture):
+    pass
+
+
+class TestFormTest(_TestFormData):
+    pass
+
+
+class TestRecordRequests(_TestRecordRequests):
+    pass
+
+
+class TestRemoteRecording(_TestRemoteRecording):
+    pass
+
+
+class TestRequestCapture(_TestRequestCapture):
+    pass
+
+
 @pytest.mark.django_db
 @pytest.mark.appmap_enabled(appmap_enabled=False)
 def test_sql_capture(events):
@@ -200,55 +218,37 @@ def test_disabled(self, pytester, monkeypatch):
         assert not (pytester.path / "tmp").exists()
 
 
-class TestRecordRequestsDjango(_TestRecordRequests):
-    def server_start_thread(self, debug=True):
-        # Use appmap from our working copy, not the module installed by virtualenv. Add the init
-        # directory so the sitecustomize.py file it contains will be loaded on startup. This
-        # simulates a real installation.
-        settings = "settings_dev" if debug else "settings"
-        exec_cmd(
-            """
-export PYTHONPATH="$PWD"
-
-cd _appmap/test/data/django/
-PYTHONPATH="$PYTHONPATH:$PWD/init"
-"""
-            + f" APPMAP_OUTPUT_DIR=/tmp DJANGO_SETTINGS_MODULE=djangoapp.{settings}"
-            + " python manage.py runserver"
-            + f" 127.0.0.1:{_TestRecordRequests.server_port}"
-        )
-
-    def server_start(self, debug=True):
-        def start_with_debug():
-            self.server_start_thread(debug)
-
-        # start as background thread so running the tests can continue
-        thread = Thread(target=start_with_debug)
-        thread.start()
-        wait_until_port_is("127.0.0.1", _TestRecordRequests.server_port, "open")
-
-    def server_stop(self):
-        exec_cmd(
-            "ps -ef"
-            + "| grep -i 'manage.py runserver'"
-            + "| grep -v grep"
-            + "| awk '{ print $2 }'"
-            + "| xargs kill -9"
-        )
-        wait_until_port_is("127.0.0.1", _TestRecordRequests.server_port, "closed")
-
-    def test_record_request_appmap_enabled_requests_enabled_no_remote(self):
-        self.server_stop()  # ensure it's not running
-        self.server_start()
-        self.record_request(False)
-        self.server_stop()
-
-    def test_record_request_appmap_enabled_requests_enabled_and_remote(self):
-        self.server_stop()  # ensure it's not running
-        self.server_start()
-        self.record_request(True)
-        self.server_stop()
-
-    # it's not possible to test for
-    # appmap_not_enabled_requests_enabled_and_remote because when
-    # APPMAP=false the routes for remote recording are disabled.
+@pytest.fixture(name="server")
+def django_server(xprocess, server_base):
+    host, port, debug, server_env = server_base
+    settings = "settings_dev" if debug else "settings"
+
+    class Starter(ProcessStarter):
+        def startup_check(self):
+            try:
+                s = socket.socket()
+                s.connect((host, port))
+                return True
+            except ConnectionRefusedError:
+                pass
+            return False
+
+        pattern = f"server at http://{host}:{port}"
+        args = [
+            "bash",
+            "-ec",
+            f"cd {Path(__file__).parent / 'data'/ 'django'};"
+            + f" {sys.executable} manage.py runserver"
+            + f" {host}:{port}",
+        ]
+        env = {
+            "DJANGO_SETTINGS_MODULE": f"djangoapp.{settings}",
+            "PYTHONPATH": "./init",
+            "PYTHONUNBUFFERED": "1",
+            "APPMAP_OUTPUT_DIR": "/tmp",
+            **server_env,
+        }
+
+    xprocess.ensure("myserver", Starter)
+    yield NS(debug=debug, url=f"http://{host}:{port}")
+    xprocess.getinfo("myserver").terminate()

_appmap/test/test_flask.py

@@ -11,23 +11,40 @@
 
 import flask
 import pytest
+from xprocess import ProcessStarter
 
 from _appmap.env import Env
 from _appmap.metadata import Metadata
 from appmap.flask import AppmapFlask
 
 from ..test.helpers import DictIncluding
+from .web_framework import (
+    _TestFormCapture,
+    _TestFormData,
+    _TestRecordRequests,
+    _TestRemoteRecording,
+    _TestRequestCapture,
+)
 
-# Make sure assertions in web_framework get rewritten (e.g. to show
-# diffs in generated appmaps)
-pytest.register_assert_rewrite("_appmap.test.web_framework")
 
-# pylint: disable=unused-import,wrong-import-position
-from .web_framework import TestRemoteRecording  # pyright:ignore
-from .web_framework import TestRequestCapture  # pyright: ignore
-from .web_framework import _TestRecordRequests, exec_cmd, wait_until_port_is
+class TestFormCapture(_TestFormCapture):
+    pass
 
-# pylint: enable=unused-import
+
+class TestFormTest(_TestFormData):
+    pass
+
+
+class TestRecordRequests(_TestRecordRequests):
+    pass
+
+
+class TestRemoteRecording(_TestRemoteRecording):
+    pass
+
+
+class TestRequestCapture(_TestRequestCapture):
+    pass
 
 
 @pytest.fixture(name="app")
@@ -44,7 +61,7 @@ def flask_app(data_dir, monkeypatch):
 
     # Add the AppmapFlask extension to the app. This now happens automatically when a Flask app is
     # started from the command line, but must be done manually otherwise.
-    AppmapFlask().init_app(flaskapp.app)
+    AppmapFlask(flaskapp.app).init_app()
 
     return flaskapp.app
 
@@ -93,58 +110,41 @@ def test_template(app, events):
     )
 
 
-class TestRecordRequestsFlask(_TestRecordRequests):
-    def server_start_thread(self, debug=True):
-        # Use appmap from our working copy, not the module installed by virtualenv. Add the init
-        # directory so the sitecustomize.py file it contains will be loaded on startup. This
-        # simulates a real installation.
-        flask_debug = "FLASK_DEBUG=1" if debug else ""
-
-        exec_cmd(
-            """
-export PYTHONPATH="$PWD"
-
-cd _appmap/test/data/flask/
-PYTHONPATH="$PYTHONPATH:$PWD/init"
-"""
-            + f" APPMAP_OUTPUT_DIR=/tmp {flask_debug} FLASK_APP=flaskapp.py flask run -p "
-            + str(_TestRecordRequests.server_port)
-        )
+@pytest.fixture(name="server")
+def flask_server(xprocess, server_base):
+    host, port, debug, server_env = server_base
+    flask_debug = "1" if debug else "0"
+
+    class Starter(ProcessStarter):
+        def startup_check(self):
+            try:
+                s = socket.socket()
+                s.connect((host, port))
+                return True
+            except ConnectionRefusedError:
+                pass
+            return False
+
+        pattern = f"Running on http://{host}:{port}"
+        args = [
+            "bash",
+            "-ec",
+            f"cd {Path(__file__).parent / 'data'/ 'flask'};"
+            + f" {sys.executable} -m flask run"
+            + f" -p {port}",
+        ]
+        print(args)
+        env = {
+            "FLASK_APP": "flaskapp.py",
+            "FLASK_DEBUG": flask_debug,
+            "PYTHONUNBUFFERED": "1",
+            "APPMAP_OUTPUT_DIR": "/tmp",
+            **server_env,
+        }
 
-    def server_start(self, debug=True):
-        # start as background thread so running the tests can continue
-        def start_with_debug():
-            self.server_start_thread(debug)
-
-        thread = Thread(target=start_with_debug)
-        thread.start()
-        wait_until_port_is("127.0.0.1", _TestRecordRequests.server_port, "open")
-
-    def server_stop(self):
-        exec_cmd(
-            "ps -ef"
-            + "| grep -i 'flask run'"
-            + "| grep -v grep"
-            + "| awk '{ print $2 }'"
-            + "| xargs kill -9"
-        )
-        wait_until_port_is("127.0.0.1", _TestRecordRequests.server_port, "closed")
-
-    def test_record_request_appmap_enabled_requests_enabled_no_remote(self):
-        self.server_stop()  # ensure it's not running
-        self.server_start()
-        self.record_request(False)
-        self.server_stop()
-
-    def test_record_request_appmap_enabled_requests_enabled_and_remote(self):
-        self.server_stop()  # ensure it's not running
-        self.server_start()
-        self.record_request(True)
-        self.server_stop()
-
-    # it's not possible to test for
-    # appmap_not_enabled_requests_enabled_and_remote because when
-    # APPMAP=false the routes for remote recording are disabled.
+    xprocess.ensure("myserver", Starter)
+    yield NS(debug=debug, url=f"http://{host}:{port}")
+    xprocess.getinfo("myserver").terminate()
 
 
 class TestFlaskApp: