From 234f7a7621ac869d37c77867f8458829f6dff4ec Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Wed, 9 Jul 2025 11:58:02 +0100 Subject: [PATCH 1/6] Start writing python-integration test that starts simulation server --- integration-tests/testfile.py | 134 ++++++++++++++++++++++++++++++++++ 1 file changed, 134 insertions(+) create mode 100755 integration-tests/testfile.py diff --git a/integration-tests/testfile.py b/integration-tests/testfile.py new file mode 100755 index 00000000..d767945d --- /dev/null +++ b/integration-tests/testfile.py @@ -0,0 +1,134 @@ +#! /usr/bin/env python3 +"""This module fires up a server in a subprocess to allow for integration tests."" + +These are seperated from the unit tests to stop them artificially inflating the test +coverage. For now this file should be run directly not with a test framework +""" + +import subprocess +import os +import shutil +from time import sleep + + +THIS_DIR = os.path.dirname(os.path.realpath(__file__)) +WORKING_DIR = os.path.join(THIS_DIR, "working_dir") +REPO_DIR = os.path.dirname(THIS_DIR) +CONFIG_FILE = os.path.join(REPO_DIR, "ofm_config_simulation.json") +CMD = ["openflexure-microscope-server", "--fallback", "-c", CONFIG_FILE] + + +def main(): + """Set up the server, run basic checks, shutdown, check for graceful exit""" + set_up_working_dir() + os.chdir(WORKING_DIR) + + try: + print("Starting server") + server_process = start_server() + print("Waiting 15s to ensure started up") + sleep(15) + error_if_server_not_started(server_process) + + finally: + # Ensure the server process is really dead + if server_process.poll() is None: + print("Server still running, shutting down.") + server_process.terminate() + sleep(2) + if server_process.poll() is None: + server_process.kill() + raise RuntimeError("Server needed killing at end of test.") + + +def set_up_working_dir() -> None: + """If working dir exists, delete it and make a new one.""" + if os.path.exists(WORKING_DIR): + shutil.rmtree(WORKING_DIR) + os.makedirs(WORKING_DIR) + + +def start_server() -> subprocess.Popen: + """ + Start the server in a subprocess. + + The server is started in a background thread and all outputs are + buffered. + + :return: Popen object for the ongoing process + """ + process = subprocess.Popen( + CMD, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + bufsize=1, + universal_newlines=True, + ) + os.set_blocking(process.stdout.fileno(), False) + os.set_blocking(process.stderr.fileno(), False) + return process + + +def error_if_server_not_started(server_process: subprocess.Popen) -> None: + """Check the server started up as expected. + + Check the subprocess is running + Check the logs have no errors + Read the Logs to check uvicorn is running on http://127.0.0.1:5000 + """ + + stdout, stderr = read_process_buffers(server_process) + output = format_stdout_stderr(stdout, stderr) + if server_process.poll() is not None: + raise RuntimeError(f"Server process is not running!\n{output}") + + confirmed_uvicorn_is_running = False + # Note that logs go to stderr once OFM logging process stars, but if there is an + # error very early (such as on import) on logs may go to stdout + for line in stdout + stderr: + if line.startswith("ERROR:"): + raise RuntimeError(f"Server started up with error\n{line}\n\n{output}") + if "Uvicorn running on http://127.0.0.1:5000" in line: + confirmed_uvicorn_is_running = True + + if not confirmed_uvicorn_is_running: + raise RuntimeError( + f"Cannot confirm Uvicorn is running on http://127.0.0.1:5000\n\n{output}" + ) + + # If we reached here Everything is fine! print the logs! + print("Server is running with following outputs:\n\n") + print(output) + + +def read_process_buffers(process: subprocess.Popen) -> tuple[list[str], list[str]]: + """Return STDOUT and STDERR from a process""" + stdout = [] + stderr = [] + + while line := process.stdout.readline(): + stdout.append(line) + + while line := process.stderr.readline(): + stderr.append(line) + + return stdout, stderr + + +def format_stdout_stderr(stdout: list[str], stderr: list[str]) -> str: + """Return a ready to print format for stdout and stderr""" + text = "" + if stdout: + text += "**STDOUT**\n" + "".join(stdout) + "\n" + else: + text += "**STDOUT IS EMPTY**\n\n" + + if stderr: + text += "**STDERR**\n" + "".join(stderr) + "\n" + else: + text += "**STDERR IS EMPTY**\n\n" + return text + + +if __name__ == "__main__": + main() From bd96585825e3793879a091d3ebc16bcf7d32b19f Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Wed, 9 Jul 2025 12:59:14 +0100 Subject: [PATCH 2/6] Create failing test for graceful shutdown. --- integration-tests/testfile.py | 110 ++++++++++++++++++++++++++++++---- 1 file changed, 99 insertions(+), 11 deletions(-) diff --git a/integration-tests/testfile.py b/integration-tests/testfile.py index d767945d..d07ce7ef 100755 --- a/integration-tests/testfile.py +++ b/integration-tests/testfile.py @@ -2,7 +2,10 @@ """This module fires up a server in a subprocess to allow for integration tests."" These are seperated from the unit tests to stop them artificially inflating the test -coverage. For now this file should be run directly not with a test framework +coverage. For now this file should be run directly not with a test framework. + +These tests are designed to run on CI. They should work on Linux or WSL for local +debugging. """ import subprocess @@ -10,12 +13,15 @@ import os import shutil from time import sleep +from PIL import Image + +import labthings_fastapi as lt THIS_DIR = os.path.dirname(os.path.realpath(__file__)) WORKING_DIR = os.path.join(THIS_DIR, "working_dir") REPO_DIR = os.path.dirname(THIS_DIR) CONFIG_FILE = os.path.join(REPO_DIR, "ofm_config_simulation.json") -CMD = ["openflexure-microscope-server", "--fallback", "-c", CONFIG_FILE] +SERVER_CMD = ["openflexure-microscope-server", "--fallback", "-c", CONFIG_FILE] def main(): @@ -30,15 +36,71 @@ def main(): sleep(15) error_if_server_not_started(server_process) + test_client_connection() + + # This also sleeps for 2s and checks it is still connected + subscriber_process = subscribe_to_mjpeg_stream() + + server_process.terminate() + sleep(3) + + check_for_graceful_shudown(server_process) + finally: - # Ensure the server process is really dead + # Ensure the server and subscriber processes are really dead + if subscriber_process.poll() is None: + subscriber_process.kill() if server_process.poll() is None: - print("Server still running, shutting down.") - server_process.terminate() - sleep(2) - if server_process.poll() is None: - server_process.kill() - raise RuntimeError("Server needed killing at end of test.") + server_process.kill() + raise RuntimeError("Server needed killing at end of test.") + + +def test_client_connection() -> None: + """Check a ThingClient can interact with the simulation microscope camera""" + print("Connecting Python client to microscope, and capturing image") + cam_client = lt.ThingClient.from_url("http://localhost:5000/camera/") + img = Image.open(cam_client.grab_jpeg().open()) + print(f"Successfully grabbed image of size {img.size}") + assert img.size == (800, 600) + print("Successfully grabbed image from camera mjpeg stream") + + +def subscribe_to_mjpeg_stream() -> subprocess.Popen: + """Start a background process subscribed to the mjpeg stream. + + :return: The Popen object for the ongoing process. + + :raises: RuntimeError if the stream is not still connected after 2s + """ + # Use nohup to stop process hanging up unexpectedly. Explicitly forward stream + # to /dev/null to keep curl connected. + + curl_command = [ + "nohup", + "curl", + "-s", + "http://localhost:5000/camera/mjpeg_stream", + ">", + "/dev/null", + "2>&1", + ] + + process = subprocess.Popen( + curl_command, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + bufsize=1, + universal_newlines=True, + ) + os.set_blocking(process.stdout.fileno(), False) + + sleep(2) + if process.poll() is not None: + raise RuntimeError( + "MJPEG Subscriber process is not running. This likley means it could not" + " stay connected to the stream.\n" + ) + return process def set_up_working_dir() -> None: @@ -58,7 +120,7 @@ def start_server() -> subprocess.Popen: :return: Popen object for the ongoing process """ process = subprocess.Popen( - CMD, + SERVER_CMD, stdout=subprocess.PIPE, stderr=subprocess.PIPE, bufsize=1, @@ -75,6 +137,10 @@ def error_if_server_not_started(server_process: subprocess.Popen) -> None: Check the subprocess is running Check the logs have no errors Read the Logs to check uvicorn is running on http://127.0.0.1:5000 + + :param server_process: The Popen object for the the server process. + + :rasies RuntimeError: If the server is not running as expected. """ stdout, stderr = read_process_buffers(server_process) @@ -83,7 +149,7 @@ def error_if_server_not_started(server_process: subprocess.Popen) -> None: raise RuntimeError(f"Server process is not running!\n{output}") confirmed_uvicorn_is_running = False - # Note that logs go to stderr once OFM logging process stars, but if there is an + # Note that logs go to stderr once OFM logging process starts, but if there is an # error very early (such as on import) on logs may go to stdout for line in stdout + stderr: if line.startswith("ERROR:"): @@ -101,6 +167,28 @@ def error_if_server_not_started(server_process: subprocess.Popen) -> None: print(output) +def check_for_graceful_shudown(server_process: subprocess.Popen) -> None: + """Check the server shutdown gracefully + + Check the subprocess is not running + Check the logs have no errors + Read the Logs to check the shutdown was graceful, rather than killed on Uvicorn + timeout + + :param server_process: The Popen object for the the server process. + + :rasies RuntimeError: If the server didn't shutdown gracefully. + """ + stdout, stderr = read_process_buffers(server_process) + output = format_stdout_stderr(stdout, stderr) + for line in stdout + stderr: + if line.startswith("ERROR:"): + raise RuntimeError(f"Server encountered error\n{line}\n\n{output}") + if "graceful shutdown exceeded" in line: + # This should be logged as an ERROR. This check is belts and braces. + raise RuntimeError(f"Server failed to shutdown gracefully.\n\n{output}") + + def read_process_buffers(process: subprocess.Popen) -> tuple[list[str], list[str]]: """Return STDOUT and STDERR from a process""" stdout = [] From 190766cd1c1a125dc7545a5a3759486bdc71d5c5 Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Wed, 9 Jul 2025 13:01:05 +0100 Subject: [PATCH 3/6] Add CI job for integration tests --- .gitlab-ci.yml | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index eb21c9a4..3acc09a5 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -3,6 +3,7 @@ stages: - analysis - testing - build + - integration - package - deploy @@ -167,6 +168,18 @@ build: paths: - "src/openflexure_microscope_server/static/" +server_integration_tests: + extends: .python + stage: integration + # This is allowed to fail for now until graceful shutdown is resolved + allow_failure: true + needs: + - job: build + artifacts: true + script: + - integration-tests/testfile.py + + # # Package application into distribution tarball # package: # stage: package From 4e54d9ac06fea9e5b6da39be1bbff55ba29ec52f Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Wed, 9 Jul 2025 13:24:31 +0100 Subject: [PATCH 4/6] Tidy integration test code and docstrings --- integration-tests/testfile.py | 46 +++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 18 deletions(-) diff --git a/integration-tests/testfile.py b/integration-tests/testfile.py index d07ce7ef..6fe1cf6b 100755 --- a/integration-tests/testfile.py +++ b/integration-tests/testfile.py @@ -1,13 +1,14 @@ #! /usr/bin/env python3 -"""This module fires up a server in a subprocess to allow for integration tests."" +"""This module fires up a server in a subprocess to allow for integration tests. -These are seperated from the unit tests to stop them artificially inflating the test +These are separated from the unit tests to stop them artificially inflating the test coverage. For now this file should be run directly not with a test framework. These tests are designed to run on CI. They should work on Linux or WSL for local debugging. """ +from typing import Optional import subprocess import os import shutil @@ -17,18 +18,28 @@ from PIL import Image import labthings_fastapi as lt -THIS_DIR = os.path.dirname(os.path.realpath(__file__)) -WORKING_DIR = os.path.join(THIS_DIR, "working_dir") -REPO_DIR = os.path.dirname(THIS_DIR) -CONFIG_FILE = os.path.join(REPO_DIR, "ofm_config_simulation.json") -SERVER_CMD = ["openflexure-microscope-server", "--fallback", "-c", CONFIG_FILE] +THIS_DIR: str = os.path.dirname(os.path.realpath(__file__)) +WORKING_DIR: str = os.path.join(THIS_DIR, "working_dir") +REPO_DIR: str = os.path.dirname(THIS_DIR) +CONFIG_FILE: str = os.path.join(REPO_DIR, "ofm_config_simulation.json") +SERVER_CMD: list[str] = ["openflexure-microscope-server", "--fallback", "-c", CONFIG_FILE] -def main(): - """Set up the server, run basic checks, shutdown, check for graceful exit""" +def main() -> None: + """Set up the server, run basic checks, shutdown, check for graceful exit + + The basic checks include checks that: + - The server boots + - The server reports the expected IP and port + - A ThingClient can connect to the camera + - The client grab a frame from the stream, and it is the expected size + - A background process can subscribe to the stream + """ set_up_working_dir() os.chdir(WORKING_DIR) + server_process: Optional[subprocess.Popen] = None + subscriber_process: Optional[subprocess.Popen] = None try: print("Starting server") server_process = start_server() @@ -44,13 +55,13 @@ def main(): server_process.terminate() sleep(3) - check_for_graceful_shudown(server_process) + check_for_graceful_shutdown(server_process) finally: # Ensure the server and subscriber processes are really dead - if subscriber_process.poll() is None: + if subscriber_process is not None and subscriber_process.poll() is None: subscriber_process.kill() - if server_process.poll() is None: + if server_process is not None and server_process.poll() is None: server_process.kill() raise RuntimeError("Server needed killing at end of test.") @@ -97,7 +108,7 @@ def subscribe_to_mjpeg_stream() -> subprocess.Popen: sleep(2) if process.poll() is not None: raise RuntimeError( - "MJPEG Subscriber process is not running. This likley means it could not" + "MJPEG Subscriber process is not running. This likely means it could not" " stay connected to the stream.\n" ) return process @@ -114,8 +125,7 @@ def start_server() -> subprocess.Popen: """ Start the server in a subprocess. - The server is started in a background thread and all outputs are - buffered. + The server is started in a subprocess and all outputs are buffered. :return: Popen object for the ongoing process """ @@ -140,7 +150,7 @@ def error_if_server_not_started(server_process: subprocess.Popen) -> None: :param server_process: The Popen object for the the server process. - :rasies RuntimeError: If the server is not running as expected. + :raises RuntimeError: If the server is not running as expected. """ stdout, stderr = read_process_buffers(server_process) @@ -167,7 +177,7 @@ def error_if_server_not_started(server_process: subprocess.Popen) -> None: print(output) -def check_for_graceful_shudown(server_process: subprocess.Popen) -> None: +def check_for_graceful_shutdown(server_process: subprocess.Popen) -> None: """Check the server shutdown gracefully Check the subprocess is not running @@ -177,7 +187,7 @@ def check_for_graceful_shudown(server_process: subprocess.Popen) -> None: :param server_process: The Popen object for the the server process. - :rasies RuntimeError: If the server didn't shutdown gracefully. + :raises RuntimeError: If the server didn't shutdown gracefully. """ stdout, stderr = read_process_buffers(server_process) output = format_stdout_stderr(stdout, stderr) From 76c21e42f7a7a1ee50d8596e1975a5ba6510978c Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Wed, 9 Jul 2025 13:40:52 +0100 Subject: [PATCH 5/6] Improve integration test startup with polling --- integration-tests/testfile.py | 87 +++++++++++++++-------------------- 1 file changed, 36 insertions(+), 51 deletions(-) diff --git a/integration-tests/testfile.py b/integration-tests/testfile.py index 6fe1cf6b..30bbc290 100755 --- a/integration-tests/testfile.py +++ b/integration-tests/testfile.py @@ -12,7 +12,7 @@ from typing import Optional import subprocess import os import shutil -from time import sleep +from time import sleep, time from PIL import Image @@ -22,7 +22,12 @@ THIS_DIR: str = os.path.dirname(os.path.realpath(__file__)) WORKING_DIR: str = os.path.join(THIS_DIR, "working_dir") REPO_DIR: str = os.path.dirname(THIS_DIR) CONFIG_FILE: str = os.path.join(REPO_DIR, "ofm_config_simulation.json") -SERVER_CMD: list[str] = ["openflexure-microscope-server", "--fallback", "-c", CONFIG_FILE] +SERVER_CMD: list[str] = [ + "openflexure-microscope-server", + "--fallback", + "-c", + CONFIG_FILE, +] def main() -> None: @@ -43,9 +48,7 @@ def main() -> None: try: print("Starting server") server_process = start_server() - print("Waiting 15s to ensure started up") - sleep(15) - error_if_server_not_started(server_process) + error_if_server_not_started(server_process, timeout=15) test_client_connection() @@ -132,16 +135,17 @@ def start_server() -> subprocess.Popen: process = subprocess.Popen( SERVER_CMD, stdout=subprocess.PIPE, - stderr=subprocess.PIPE, + stderr=subprocess.STDOUT, bufsize=1, universal_newlines=True, ) os.set_blocking(process.stdout.fileno(), False) - os.set_blocking(process.stderr.fileno(), False) return process -def error_if_server_not_started(server_process: subprocess.Popen) -> None: +def error_if_server_not_started( + server_process: subprocess.Popen, timeout: float = 15.0 +) -> None: """Check the server started up as expected. Check the subprocess is running @@ -153,28 +157,27 @@ def error_if_server_not_started(server_process: subprocess.Popen) -> None: :raises RuntimeError: If the server is not running as expected. """ - stdout, stderr = read_process_buffers(server_process) - output = format_stdout_stderr(stdout, stderr) - if server_process.poll() is not None: - raise RuntimeError(f"Server process is not running!\n{output}") - confirmed_uvicorn_is_running = False - # Note that logs go to stderr once OFM logging process starts, but if there is an - # error very early (such as on import) on logs may go to stdout - for line in stdout + stderr: - if line.startswith("ERROR:"): - raise RuntimeError(f"Server started up with error\n{line}\n\n{output}") - if "Uvicorn running on http://127.0.0.1:5000" in line: - confirmed_uvicorn_is_running = True + + t_start = time() + while not confirmed_uvicorn_is_running and time() - t_start < timeout: + sleep(0.2) + if stdout := read_process_buffers(server_process): + print("".join(stdout), end="") + if server_process.poll() is not None: + raise RuntimeError("Server process is not running!") + + for line in stdout: + if line.startswith("ERROR:"): + raise RuntimeError(f"Server started up with error\n{line}") + if "Uvicorn running on http://127.0.0.1:5000" in line: + confirmed_uvicorn_is_running = True if not confirmed_uvicorn_is_running: - raise RuntimeError( - f"Cannot confirm Uvicorn is running on http://127.0.0.1:5000\n\n{output}" - ) + raise RuntimeError("Cannot confirm Uvicorn is running on http://127.0.0.1:5000") # If we reached here Everything is fine! print the logs! print("Server is running with following outputs:\n\n") - print(output) def check_for_graceful_shutdown(server_process: subprocess.Popen) -> None: @@ -189,43 +192,25 @@ def check_for_graceful_shutdown(server_process: subprocess.Popen) -> None: :raises RuntimeError: If the server didn't shutdown gracefully. """ - stdout, stderr = read_process_buffers(server_process) - output = format_stdout_stderr(stdout, stderr) - for line in stdout + stderr: + stdout = read_process_buffers(server_process) + print("".join(stdout)) + print("\n\n") + for line in stdout: if line.startswith("ERROR:"): - raise RuntimeError(f"Server encountered error\n{line}\n\n{output}") + raise RuntimeError(f"Server encountered error\n{line}") if "graceful shutdown exceeded" in line: # This should be logged as an ERROR. This check is belts and braces. - raise RuntimeError(f"Server failed to shutdown gracefully.\n\n{output}") + raise RuntimeError("Server failed to shutdown gracefully.") -def read_process_buffers(process: subprocess.Popen) -> tuple[list[str], list[str]]: - """Return STDOUT and STDERR from a process""" +def read_process_buffers(process: subprocess.Popen) -> list[str]: + """Return STDOUT from a process""" stdout = [] - stderr = [] while line := process.stdout.readline(): stdout.append(line) - while line := process.stderr.readline(): - stderr.append(line) - - return stdout, stderr - - -def format_stdout_stderr(stdout: list[str], stderr: list[str]) -> str: - """Return a ready to print format for stdout and stderr""" - text = "" - if stdout: - text += "**STDOUT**\n" + "".join(stdout) + "\n" - else: - text += "**STDOUT IS EMPTY**\n\n" - - if stderr: - text += "**STDERR**\n" + "".join(stderr) + "\n" - else: - text += "**STDERR IS EMPTY**\n\n" - return text + return stdout if __name__ == "__main__": From 72c4dbee32abea10e57f0e0548c46a0fe136ca51 Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Wed, 9 Jul 2025 15:13:25 +0000 Subject: [PATCH 6/6] Apply suggestions from code review of branch python-integration-tests --- integration-tests/testfile.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/integration-tests/testfile.py b/integration-tests/testfile.py index 30bbc290..959e24cc 100755 --- a/integration-tests/testfile.py +++ b/integration-tests/testfile.py @@ -176,8 +176,8 @@ def error_if_server_not_started( if not confirmed_uvicorn_is_running: raise RuntimeError("Cannot confirm Uvicorn is running on http://127.0.0.1:5000") - # If we reached here Everything is fine! print the logs! - print("Server is running with following outputs:\n\n") + # If we reached here Everything is fine! + print("Server is running as expected\n\n") def check_for_graceful_shutdown(server_process: subprocess.Popen) -> None: