diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 2a4fa735f..0aa741ebb 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -52,6 +52,25 @@ jobs: sudo apt install libxkbcommon-x11-0 xvfb libxcb-icccm4 libxcb-image0 libxcb-keysyms1 libxcb-randr0 libxcb-render-util0 libxcb-xinerama0 libxcb-xfixes0 x11-utils /sbin/start-stop-daemon --start --quiet --pidfile /tmp/custom_xvfb_99.pid --make-pidfile --background --exec /usr/bin/Xvfb -- :99 -screen 0 1920x1200x24 -ac +extension GLX - name: Run the tests - # test_HighContentScreeningGui.py: PySide6 QObject double-init bug (separate issue) + # SQUID_PYTEST_HARD_EXIT: belt-and-braces against segfaults during + # interpreter shutdown AFTER all tests pass (Qt destructor order vs + # Python GC — the same crash main_hcs.py avoids with os._exit()). The + # conftest hook hard-exits with pytest's real status once the session + # is complete, so real test failures still fail the step. run: python3 -m pytest --ignore=tests/control/test_HighContentScreeningGui.py working-directory: ./software + env: + SQUID_PYTEST_HARD_EXIT: "1" + - name: Run the GUI test in its own process + # Own invocation: the full-application GUI test leaves Qt/thread state + # behind that deterministically segfaults later Qt tests (e.g. + # test_channel_sequence) when run in the same pytest process. + # Step-scoped pyqt5 pin: without it, pytest-qt picks PySide6 for qtbot + # while the app code (qtpy) uses PyQt5, and the mixed bindings fail + # with a QObject double-init error. + run: python3 -m pytest tests/control/test_HighContentScreeningGui.py + working-directory: ./software + env: + QT_API: pyqt5 + PYTEST_QT_API: pyqt5 + SQUID_PYTEST_HARD_EXIT: "1" diff --git a/software/control/microscope.py b/software/control/microscope.py index 5ee8b3e8e..94e4ef186 100644 --- a/software/control/microscope.py +++ b/software/control/microscope.py @@ -443,6 +443,7 @@ def __init__( skip_init: bool = False, ): self._log = squid.logging.get_logger(self.__class__.__name__) + self._closed = False self.stage: AbstractStage = stage self.camera: AbstractCamera = camera @@ -1046,7 +1047,12 @@ def close(self) -> None: Attempts to cleanly shut down all hardware components. Errors during shutdown are logged but do not prevent other components from being closed. + Calling close() more than once is a no-op. """ + if self._closed: + return + self._closed = True + try: self.stop_live() except Exception as e: diff --git a/software/tests/conftest.py b/software/tests/conftest.py new file mode 100644 index 000000000..253574ea5 --- /dev/null +++ b/software/tests/conftest.py @@ -0,0 +1,122 @@ +""" +Suite-wide pytest fixtures. + +Ensures hardware-simulation objects created during a test (Microscope, +MultiPointController, Microcontroller) are closed at test teardown. Leaked +instances keep daemon threads (camera streaming, laser-engine tick, slack +notifier) and JobRunner child processes alive; those have caused CI segfaults +both mid-suite (a leftover thread touching a destroyed Qt object) and at +interpreter shutdown (daemon threads frozen inside C code during +finalization). +""" + +import logging +import os +import sys +import tempfile +from unittest.mock import patch + +import pytest + +import control.microcontroller +import control.microscope +from control.core.multi_point_controller import MultiPointController + +logger = logging.getLogger(__name__) + +# Junk dir for watchdog breadcrumbs written by leaked acquisitions while +# cleanup_leaked_hardware closes them; nothing reads it. A plain string, not +# pytest's tmp_path — tmp_path would cost a mkdir per test suite-wide, and the +# breadcrumb writer creates parent dirs itself. +_CLEANUP_STATE_DIR = os.path.join(tempfile.gettempdir(), f"squid-test-watchdog-cleanup-{os.getpid()}") + + +def pytest_sessionfinish(session, exitstatus): + session.config._squid_exitstatus = int(exitstatus) + + +def pytest_unconfigure(config): + """Optionally skip interpreter teardown after the test session. + + A pytest process that constructed the full HCS GUI segfaults during + interpreter shutdown (Qt C++ destructor order conflicts with Python GC) + even though every test passed. main_hcs.py sidesteps the same crash with + os._exit(); SQUID_PYTEST_HARD_EXIT=1 lets CI do likewise, preserving + pytest's exit status so real test failures still fail the step. + """ + if os.environ.get("SQUID_PYTEST_HARD_EXIT") == "1": + sys.stdout.flush() + sys.stderr.flush() + # Default 1, not 0: if pytest_sessionfinish never ran (e.g. a + # sessionstart failure), an unrecorded status must fail the step. + os._exit(getattr(config, "_squid_exitstatus", 1)) + + +def _make_tracking_init(original_init, instances_list): + """Create an __init__ wrapper that records constructed instances.""" + + def _tracking_init(self, *args, **kwargs): + original_init(self, *args, **kwargs) + instances_list.append(self) + + return _tracking_init + + +def _close_quietly(obj, label): + try: + obj.close() + except Exception: + logger.exception(f"Failed to close {label} in test cleanup") + + +@pytest.fixture(autouse=True) +def cleanup_leaked_hardware(monkeypatch): + """ + Automatically close hardware-simulation objects created during each test. + + Teardown order matters: + 1. MultiPointControllers first — joins the acquisition thread and shuts + down JobRunner child processes while the microcontroller is still + alive, so the worker's stage-return move can complete instead of + timing out. + 2. Microscopes next — stops camera streaming threads and closes the + microcontroller and addons. + 3. Any Microcontrollers created standalone (skipped if a Microscope + already closed them). + """ + microscopes = [] + controllers = [] + microcontrollers = [] + + with patch.object( + control.microscope.Microscope, + "__init__", + _make_tracking_init(control.microscope.Microscope.__init__, microscopes), + ), patch.object( + MultiPointController, + "__init__", + _make_tracking_init(MultiPointController.__init__, controllers), + ), patch.object( + control.microcontroller.Microcontroller, + "__init__", + _make_tracking_init(control.microcontroller.Microcontroller.__init__, microcontrollers), + ): + yield + + # A breadcrumb written by a leaked acquisition while it is closed below + # must not land in the real user state dir. Setting the env var here, at + # teardown start, is deterministic regardless of what the test body or + # other fixtures did with it: their monkeypatches are already undone (they + # were set up after this fixture), raw os.environ writes are overwritten, + # and our own monkeypatch reverts this value after the fixture finishes. + monkeypatch.setenv("SQUID_WATCHDOG_STATE_DIR", _CLEANUP_STATE_DIR) + + for controller in reversed(controllers): + _close_quietly(controller, "MultiPointController") + + for microscope in reversed(microscopes): + _close_quietly(microscope, "Microscope") + + for micro in reversed(microcontrollers): + if not micro.terminate_reading_received_packet_thread: + _close_quietly(micro, "Microcontroller") diff --git a/software/tests/control/conftest.py b/software/tests/control/conftest.py index 485098188..ff41628ef 100644 --- a/software/tests/control/conftest.py +++ b/software/tests/control/conftest.py @@ -1,60 +1,14 @@ """ Pytest fixtures for control module tests. -This module provides fixtures to ensure proper cleanup of Microcontroller instances, -preventing background threads from causing segfaults in subsequent tests. +Microcontroller/Microscope/MultiPointController cleanup is handled suite-wide +by the autouse fixture in tests/conftest.py. """ -import logging -from unittest.mock import patch - import pytest -import control.microcontroller from control.firmware_sim_serial import FirmwareSimSerial -logger = logging.getLogger(__name__) - - -def _make_tracking_init(original_init, instances_list): - """Create a wrapper that tracks Microcontroller instances.""" - - def _tracking_init(self, *args, **kwargs): - original_init(self, *args, **kwargs) - instances_list.append(self) - - return _tracking_init - - -@pytest.fixture(autouse=True) -def cleanup_microcontrollers(): - """ - Fixture that automatically cleans up all Microcontroller instances after each test. - - This prevents background threads from causing segfaults when subsequent tests run, - especially those involving Qt event loops. The Microcontroller.read_received_packet - method runs in a background thread that must be stopped via close(). - """ - # Track instances created during this test (scoped to this fixture invocation) - active_microcontrollers = [] - - # Capture original __init__ at fixture runtime, not module load time - original_init = control.microcontroller.Microcontroller.__init__ - - with patch.object( - control.microcontroller.Microcontroller, "__init__", _make_tracking_init(original_init, active_microcontrollers) - ): - yield - - # Clean up all tracked instances - for micro in active_microcontrollers: - try: - if hasattr(micro, "terminate_reading_received_packet_thread"): - if not micro.terminate_reading_received_packet_thread: - micro.close() - except Exception as e: - logger.warning(f"Failed to close Microcontroller in test cleanup: {e}") - @pytest.fixture def firmware_sim(): diff --git a/software/tests/control/test_watchdog_breadcrumbs.py b/software/tests/control/test_watchdog_breadcrumbs.py index fc9b27daa..e539a7092 100644 --- a/software/tests/control/test_watchdog_breadcrumbs.py +++ b/software/tests/control/test_watchdog_breadcrumbs.py @@ -15,5 +15,8 @@ def test_run_acquisition_writes_running_breadcrumb(qtbot): assert rec["status"] == "running" assert rec["pid"] == os.getpid() assert rec["expected"]["timepoints"] >= 1 - mpc.request_abort_aquisition() + # close() aborts the acquisition and joins its thread; that must happen + # before closing the scope, otherwise the thread keeps running against a + # closed microcontroller and dies with a TimeoutError. + mpc.close() scope.close() diff --git a/software/tests/control/test_watchdog_integration.py b/software/tests/control/test_watchdog_integration.py index 1cf35bd78..54d2be8df 100644 --- a/software/tests/control/test_watchdog_integration.py +++ b/software/tests/control/test_watchdog_integration.py @@ -33,4 +33,8 @@ def test_simulated_acquisition_writes_ended_breadcrumb(qtbot): assert rec["status"] == "ended" assert rec["reason"] in {"completed", "completed_with_errors", "user_abort", "error"} assert rec["ended_at"] is not None + # The "ended" breadcrumb is written before the worker's final stage-return + # move; join the acquisition thread before closing the scope so it doesn't + # run against a closed microcontroller. + mpc.close() scope.close()