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/bin/server_runner

@@ -0,0 +1,12 @@
+#!/usr/bin/env bash
+
+set -x
+
+cd "$1"; shift
+
+set -a
+PYTHONUNBUFFERED=1
+APPMAP_OUTPUT_DIR=/tmp
+PYTHONPATH=./init
+
+exec $@

_appmap/test/conftest.py

@@ -1,13 +1,21 @@
 import importlib
+import os
+import socket
 import sys
 from distutils.dir_util import copy_tree
-from functools import partialmethod
+from functools import partial, partialmethod
+from pathlib import Path
+from typing import Any
 
 import pytest
 import yaml
+from attr import dataclass
+from xprocess import ProcessStarter
 
 import _appmap
 import appmap
+from _appmap.env import Env
+from _appmap.test.web_framework import TEST_HOST, TEST_PORT
 from appmap import generation
 
 from .. import utils
@@ -128,3 +136,76 @@ def _generate(check_fn, method_name):
         return generation.dump(rec)
 
     return _generate
+
+
+@dataclass
+class ServerInfo:
+    name: str = ""
+    debug: bool = False
+    host: str = ""
+    port: int = 0
+    cmd: str = ""
+    pattern: str = ""
+    env: dict = {}
+    factory: Any = None
+
+
+class _ServerStarter(ProcessStarter):
+    @property
+    def args(self):
+        return self._args
+
+    @property
+    def pattern(self):
+        return self._pattern
+
+    def startup_check(self):
+        try:
+            s = socket.socket()
+            s.connect((self._host, self._port))
+            return True
+        except ConnectionRefusedError:
+            pass
+        return False
+
+    def __init__(self, info: ServerInfo, controldir, xprocess):
+        super().__init__(controldir, xprocess)
+        self._host = info.host
+        self._port = info.port
+        # Can't set popen_kwargs["cwd"] on a ProcessStarter until
+        # https://github.com/pytest-dev/pytest-xprocess/issues/89 is fixed.
+        #
+        # In the meantime, pass the desired directory to server_runner, which
+        # will handle changing the working directory.
+        self._args = [
+            (Path(__file__).parent / "bin" / "server_runner").as_posix(),
+            (Path(__file__).parent / "data" / info.name).as_posix(),
+            f"{Path(sys.executable).as_posix()} {info.cmd}",
+        ]
+        self._pattern = info.pattern
+        self.env = {**info.env}
+        self.terminate_on_interrupt = True
+
+
+def server_starter(info, name, cmd, pattern, env=None):
+    def _starter(controldir, xprocess):
+        info.name = name
+        info.cmd = cmd
+        if env is not None:
+            info.env = {**env, **info.env}
+        info.pattern = pattern
+        return _ServerStarter(info, controldir, xprocess)
+
+    return _starter
+
+
+@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", {}))
+
+    info = ServerInfo(debug=debug, host=TEST_HOST, port=TEST_PORT, env=server_env)
+    info.factory = partial(server_starter, info)
+    return info

_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,21 @@ 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):
+    debug = server_base.debug
+    host = server_base.host
+    port = server_base.port
+    settings = "settings_dev" if debug else "settings"
+
+    name = "django"
+    pattern = f"server at http://{host}:{port}"
+    cmd = f"manage.py runserver {host}:{port}"
+    env = {"DJANGO_SETTINGS_MODULE": f"djangoapp.{settings}"}
+
+    xprocess.ensure(name, server_base.factory(name, cmd, pattern, env))
+
+    url = f"http://{server_base.host}:{port}"
+    yield NS(debug=debug, url=url)
+
+    xprocess.getinfo(name).terminate()