Refactor system tests to use the new FTP server and to run in pytest

.github/workflows/main.yml

@@ -212,12 +212,6 @@ jobs:
     timeout-minutes: 30
     steps:
       - uses: actions/checkout@v3
-      - name: Prune docker volumes
-        # seems like we're getting an error from ftp-server exiting with zero status code
-        # this is a workaround that resolves it locally 🤞
-        run: |
-          docker volume prune --force
-
       - uses: docker/setup-buildx-action@v3
       # pre-build and cache the postgres container as installing python3 takes a while, doesn't push
       - name: Cache Docker layers
@@ -247,6 +241,12 @@ jobs:
           python-version: 3.10.6
           cache: "pip"
 
+      - name: Install Python dependencies
+        run: |
+          pip install pixl_core/
+          pip install cli/[test]
+          pip install pytest-pixl/
+
       - name: Build test services
         working-directory: test
         run: |
@@ -260,3 +260,7 @@ jobs:
         working-directory: test
         run: |
           ./run-system-test.sh
+
+      - name: Dump docker logs on failure
+        if: failure()
+        uses: jwalton/gh-docker-logs@v2

cli/src/pixl_cli/main.py

@@ -125,7 +125,10 @@ def extract_radiology_reports(parquet_dir: Path) -> None:
 
     success_code = 200
     if response.status_code != success_code:
-        msg = f"Failed to run export-patient-data due to: {response.text}"
+        msg = (
+            f"Failed to run export-patient-data due to: "
+            f"error code {response.status_code} {response.text}"
+        )
         raise RuntimeError(msg)
 
 

docker-compose.yml

@@ -253,6 +253,8 @@ services:
       retries: 5
     networks:
       - pixl-net
+    extra_hosts:  # needed for linux
+      - "host.docker.internal:host-gateway"
     volumes:
       - ${PWD}/exports:/run/exports
 

pixl_core/src/core/upload.py

@@ -74,6 +74,7 @@ def upload_dicom_image(zip_content: BinaryIO, pseudo_anon_id: str) -> None:
 
     # Store the file using a binary handler
     try:
+        logger.info("Running command %s", command)
         ftp.storbinary(command, zip_content)
     except ftplib.all_errors as ftp_error:
         ftp.quit()
@@ -140,8 +141,8 @@ def _connect_to_ftp() -> FTP_TLS:
         ftp.login(ftp_user, ftp_password)
         ftp.prot_p()
     except ftplib.all_errors as ftp_error:
-        error_msg = "Failed to connect to FTPS server: '%s'"
-        raise ConnectionError(error_msg, ftp_error) from ftp_error
+        error_msg = f"Failed to connect to FTPS server: {ftp_user}@{ftp_host}:{ftp_port}"
+        raise ConnectionError(error_msg) from ftp_error
     return ftp
 
 

pytest-pixl/src/pytest_pixl/ftpserver.py

@@ -73,19 +73,20 @@ class PixlFTPServer:
     :type home_dir: str
     """
 
-    def __init__(self, home_dir: str) -> None:
+    def __init__(self, home_root: Path) -> None:
         """
         Initialise the FTPS server. Sets the hostname, port, username and password
         from the corresponding environment variables.
-        :param home_dir: The home directory for the FTP server. This is the directory where the
-            user will be placed after login.
+        :param home_root: The directory where the user's home directory will be created.
+        The home dir is the directory where the user will be placed after login.
         """
-        self.host = config("FTP_HOST", default="127.0.0.1")
+        self.host = "127.0.0.1"
         self.port = int(config("FTP_PORT", default=20021))
 
         self.user_name = config("FTP_USER_NAME", default="pixl_user")
         self.user_password = config("FTP_USER_PASSWORD", default="longpassword")
-        self.home_dir = home_dir
+        self.home_dir: Path = home_root / self.user_name
+        self.home_dir.mkdir()
 
         self.certfile = Path(__file__).parents[1] / "resources" / "ssl" / "localhost.crt"
         self.keyfile = Path(__file__).parents[1] / "resources" / "ssl" / "localhost.key"
@@ -103,7 +104,7 @@ def _add_user(self) -> None:
         will be a directory on the local filesystem!
         """
         self.authorizer.add_user(
-            self.user_name, self.user_password, self.home_dir, perm=USER_PERMISSIONS
+            self.user_name, self.user_password, str(self.home_dir), perm=USER_PERMISSIONS
         )
 
     def _setup_TLS_handler(self) -> None:

pytest-pixl/src/pytest_pixl/plugin.py

@@ -13,7 +13,6 @@
 #  limitations under the License.
 """Pytest fixtures."""
 
-import os
 import threading
 from collections.abc import Generator
 
@@ -22,15 +21,14 @@
 from pytest_pixl.ftpserver import PixlFTPServer
 
 
-@pytest.fixture(scope="module")
+@pytest.fixture(scope="session")
 def ftps_server(tmp_path_factory) -> Generator[PixlFTPServer, None, None]:
     """
     Spins up an FTPS server in a separate process for testing. Configuration is controlled by the
     FTP_* environment variables.
     """
-    tmp_home_dir = tmp_path_factory.mktemp("ftps_server") / os.environ["FTP_USER_NAME"]
-    tmp_home_dir.mkdir()
-    ftps_server = PixlFTPServer(home_dir=str(tmp_home_dir))
+    tmp_home_dir_root = tmp_path_factory.mktemp("ftps_server")
+    ftps_server = PixlFTPServer(home_root=tmp_home_dir_root)
     thread = threading.Thread(target=ftps_server.server.serve_forever)
     thread.start()
     yield ftps_server

ruff.toml

@@ -48,5 +48,5 @@ mccabe.max-complexity = 18
 exclude=["scripts"]
 
 [extend-per-file-ignores]
-"**/tests/*" = ["PLR2004"] #  Magic value used in comparison
+"**/test*/*" = ["PLR2004"] #  Magic value used in comparison
 "hasher/tests/*" = ["ARG001"] #  unused function argument

test/.env

@@ -29,7 +29,7 @@ PIXL_EHR_API_PORT=7006
 PIXL_IMAGING_API_PORT=7007
 RABBITMQ_PORT=7008
 RABBITMQ_ADMIN_PORT=7009
-FTP_PORT=21
+FTP_PORT=20021
 
 # PIXL Orthanc raw instance
 ORTHANC_RAW_USERNAME=orthanc_raw_username
@@ -62,6 +62,6 @@ RABBITMQ_USERNAME=rabbitmq_username
 RABBITMQ_PASSWORD=rabbitmq_password
 
 # FTP server
-FTP_HOST=ftp-server
-FTP_USER_NAME=ftp_username
+FTP_HOST=host.docker.internal
+FTP_USER_NAME=pixl_user
 FTP_USER_PASSWORD=longpassword

test/README.md

@@ -18,6 +18,20 @@ You can run the system test with:
 ./run-system-test.sh
 ```
 
+Or to do all the setup but not run any tests:
+```bash
+./run-system-test.sh setup
+```
+
+You can then develop and run tests repeatedly with `pytest` or through your IDE.
+But you are responsible for knowing
+when to re-run the setup if something it depends on has changed.
+
+Run the following to teardown:
+```bash
+./run-system-test.sh teardown
+```
+
 ## File organisation
 
 ### PIXL configuration

test/scripts/check_entry_in_pixl_anon.sh → test/check_utils.py

@@ -1,4 +1,3 @@
-#!/bin/bash
 #  Copyright (c) University College London Hospitals NHS Foundation Trust
 #
 #  Licensed under the Apache License, Version 2.0 (the "License");
@@ -12,9 +11,5 @@
 #  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 #  See the License for the specific language governing permissions and
 #  limitations under the License.
-set -euxo pipefail
 
-_sql_command="select * from emap_data.ehr_anon"
-_result=$(docker exec system-test-postgres-1 /bin/bash -c \
-  "PGPASSWORD=pixl_db_password psql -U pixl_db_username -d pixl -c \"$_sql_command\"")
-echo "$_result" | grep -q "2 row"
+"""Utilities for the system test"""

test/conftest.py

@@ -0,0 +1,89 @@
+#  Copyright (c) University College London Hospitals NHS Foundation Trust
+#
+#  Licensed under the Apache License, Version 2.0 (the "License");
+#  you may not use this file except in compliance with the License.
+#  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+#  Unless required by applicable law or agreed to in writing, software
+#  distributed under the License is distributed on an "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#  See the License for the specific language governing permissions and
+#  limitations under the License.
+"""System/E2E test setup"""
+import logging
+import subprocess
+from pathlib import Path
+
+import pytest
+
+pytest_plugins = "pytest_pixl"
+
+
+@pytest.fixture()
+def host_export_root_dir():
+    """Intermediate export dir as seen from the host"""
+    return Path(__file__).parents[1] / "exports"
+
+
+def run_subprocess(
+    cmd: list[str], working_dir: Path, *, shell=False, timeout=360
+) -> subprocess.CompletedProcess[bytes]:
+    """
+    Run a command but capture the stderr and stdout better than the CalledProcessError
+    string representation does
+    """
+    try:
+        logging.info("Running command %s", cmd)
+        return subprocess.run(
+            cmd,
+            check=True,
+            cwd=working_dir,
+            shell=shell,  # noqa: S603 input is trusted
+            timeout=timeout,
+            capture_output=True,
+        )
+    except subprocess.CalledProcessError as exception:
+        logging.error("*** exception occurred running: '%s'", cmd)  # noqa: TRY400 will raise anyway
+        logging.error("*** stdout:\n%s", exception.stdout.decode())  # noqa: TRY400
+        logging.error("*** stderr:\n%s", exception.stderr.decode())  # noqa: TRY400
+        raise
+
+
+TEST_DIR = Path(__file__).parent
+RESOURCES_DIR = TEST_DIR / "resources"
+RESOURCES_OMOP_DIR = RESOURCES_DIR / "omop"
+
+
+@pytest.fixture(scope="session")
+def _setup_pixl_cli(ftps_server) -> None:
+    """Run pixl populate/start. Cleanup intermediate export dir on exit."""
+    # CLI calls need to have CWD = test dir so they can find the pixl_config.yml file
+    run_subprocess(["pixl", "populate", str(RESOURCES_OMOP_DIR.absolute())], TEST_DIR)
+    run_subprocess(["pixl", "start"], TEST_DIR)
+    # poll here for two minutes to check for imaging to be processed, printing progress
+    yield
+    run_subprocess(
+        [
+            "docker",
+            "exec",
+            "system-test-ehr-api-1",
+            "rm",
+            "-r",
+            "/run/exports/test-extract-uclh-omop-cdm/",
+        ],
+        TEST_DIR,
+    )
+
+
+@pytest.fixture(scope="session")
+def _extract_radiology_reports(_setup_pixl_cli) -> None:
+    """
+    run pixl extract-radiology-reports.
+    TODO: should then poll for two minutes to ensure that the database is populated,
+     as per ./scripts/check_entry_in_pixl_anon.sh
+    """
+    run_subprocess(
+        ["pixl", "extract-radiology-reports", str(RESOURCES_OMOP_DIR.absolute())], TEST_DIR
+    )

test/docker-compose.yml

@@ -74,26 +74,3 @@ services:
             retries: 5
         networks:
             pixl-net:
-
-    ftp-server:
-        container_name: system-test-ftp-server
-        build:
-            context: ./dummy-services/ftp-server
-        ports:
-            - "127.0.0.1:20021:21"
-            - "21000-21010:21000-21010"
-        volumes:
-            # Mount for uploaded data
-            - "./dummy-services/ftp-server/mounts/data/:/home/${FTP_USER_NAME}/"
-        environment:
-            ADDRESS: ${FTP_HOST}
-            USERS: ${FTP_USER_NAME}|${FTP_USER_PASSWORD}|/home/${FTP_USER_NAME}
-            TLS_KEY: /etc/ssl/private/localhost.key
-            TLS_CERT: /etc/ssl/private/localhost.crt
-        healthcheck:
-            test: ping localhost:21 -w 2
-            interval: 10s
-            timeout: 5s
-            retries: 5
-        networks:
-            pixl-net: