fixes

Author: nycrat

src/software/gameplay_tests/field_test_runner.py

@@ -74,8 +74,9 @@ def _runner(
         while time_elapsed_s < test_timeout_s:
             processing_start_time = time.time()
 
-            # Check for new GC commands at this time step
-            for delay, cmd, team in gc_cmd_with_delay:
+            # Check for new GC commands at this time step. Iterate over a copy
+            # since we remove from the list while iterating.
+            for delay, cmd, team in list(gc_cmd_with_delay):
                 # If delay matches time
                 if delay <= time_elapsed_s:
                     # send command

src/software/gameplay_tests/simulated_test_runner.py

@@ -71,8 +71,10 @@ def _pre_run_setup(self, setup: (lambda: None)):
         :param setup: Function that sets up the world state
         """
         world_state_received_buffer = ThreadSafeBuffer(1, WorldStateReceivedTrigger)
-        self.simulator_proto_unix_io.register_observer(
-            WorldStateReceivedTrigger, world_state_received_buffer
+        self._register_observer(
+            self.simulator_proto_unix_io,
+            WorldStateReceivedTrigger,
+            world_state_received_buffer,
         )
 
         while True:
@@ -104,8 +106,9 @@ def _runner(
         while time_elapsed_s < test_timeout_s:
             processing_start_time = time.time()
 
-            # Check for new GC commands at this time step
-            for delay, cmd, team in gc_cmd_with_delay:
+            # Check for new GC commands at this time step. Iterate over a copy
+            # since we remove from the list while iterating.
+            for delay, cmd, team in list(gc_cmd_with_delay):
                 # If delay matches time
                 if delay <= time_elapsed_s:
                     # send command

src/software/gameplay_tests/tbots_test_runner.py

@@ -56,28 +56,60 @@ def __init__(
             buffer_size=1, protobuf_type=RobotStatus
         )
 
-        self.blue_full_system_proto_unix_io.register_observer(
-            SSL_WrapperPacket, self.ssl_wrapper_buffer
+        # Track every observer we register so we can clean them up later. This
+        # matters when the runner is short-lived but the ProtoUnixIOs outlive it
+        # (e.g. running tests repeatedly from the test selector widget).
+        self._registered_observers = []
+
+        self._register_observer(
+            self.blue_full_system_proto_unix_io,
+            SSL_WrapperPacket,
+            self.ssl_wrapper_buffer,
         )
-        self.blue_full_system_proto_unix_io.register_observer(
-            RobotStatus, self.robot_status_buffer
+        self._register_observer(
+            self.blue_full_system_proto_unix_io, RobotStatus, self.robot_status_buffer
         )
         if self.is_yellow_friendly:
-            self.yellow_full_system_proto_unix_io.register_observer(
-                World, self.world_buffer
+            self._register_observer(
+                self.yellow_full_system_proto_unix_io, World, self.world_buffer
             )
-            self.yellow_full_system_proto_unix_io.register_observer(
-                PrimitiveSet, self.primitive_set_buffer
+            self._register_observer(
+                self.yellow_full_system_proto_unix_io,
+                PrimitiveSet,
+                self.primitive_set_buffer,
             )
         # Only validate on the blue worlds
         else:
-            self.blue_full_system_proto_unix_io.register_observer(
-                World, self.world_buffer
+            self._register_observer(
+                self.blue_full_system_proto_unix_io, World, self.world_buffer
             )
-            self.blue_full_system_proto_unix_io.register_observer(
-                PrimitiveSet, self.primitive_set_buffer
+            self._register_observer(
+                self.blue_full_system_proto_unix_io,
+                PrimitiveSet,
+                self.primitive_set_buffer,
             )
 
+    def _register_observer(self, proto_unix_io, proto_class, buffer):
+        """Register an observer on a ProtoUnixIO and record it for cleanup.
+
+        :param proto_unix_io: the ProtoUnixIO to register on
+        :param proto_class: the protobuf class to observe
+        :param buffer: the buffer to push observed protos onto
+        """
+        proto_unix_io.register_observer(proto_class, buffer)
+        self._registered_observers.append((proto_unix_io, proto_class, buffer))
+
+    def unregister_observers(self):
+        """Unregister every observer this runner registered.
+
+        Call this when the runner is done but its ProtoUnixIOs live on, to avoid
+        leaking stale buffers that keep receiving protos (e.g. when running tests
+        repeatedly from the test selector widget in --test_mode).
+        """
+        for proto_unix_io, proto_class, buffer in self._registered_observers:
+            proto_unix_io.unregister_observer(proto_class, buffer)
+        self._registered_observers = []
+
     def send_gamecontroller_command(
         self,
         gc_command: proto.ssl_gc_state_pb2.Command,

src/software/thunderscope/common/test_bridge.py

@@ -101,12 +101,21 @@ def _execute_test(self, item):
             else:
                 test_func(gameplay_test_runner=runner)
 
+            # run_test() starts the test in a background thread (Thunderscope is
+            # owned externally), so block here until it actually finishes before
+            # reporting the result.
+            runner.wait_for_test()
+            if runner.last_exception is not None:
+                raise runner.last_exception
+
             if self._on_test_completed:
                 self._on_test_completed(node_id, True, "Passed")
         except Exception as e:
             if self._on_test_completed:
                 self._on_test_completed(node_id, False, str(e))
         finally:
+            if self._current_runner is not None:
+                self._current_runner.unregister_observers()
             self._running = False
             self._current_runner = None
 

src/software/thunderscope/common/test_discovery.py

@@ -34,12 +34,30 @@ def discover_test_targets(workspace_root: str = "") -> list[TestItem]:
     # If we are inside ``bazel run``/``bazel test`` we cannot recursively
     # invoke ``bazel query`` because it will block on the parent lock.
     inside_bazel = bool(os.environ.get("BUILD_WORKSPACE_DIRECTORY"))
+    items = None
     if not inside_bazel:
         try:
-            return _query_bazel_tests(workspace_root)
+            items = _query_bazel_tests(workspace_root)
         except (subprocess.TimeoutExpired, FileNotFoundError):
-            pass
-    return _walk_test_files(workspace_root)
+            items = None
+    if items is None:
+        items = _walk_test_files(workspace_root)
+
+    # Both discovery methods see every py_test in the workspace. Keep only the
+    # gameplay tests, identified by their use of the gameplay_test_runner fixture.
+    return [it for it in items if _uses_gameplay_fixture(it.src_file)]
+
+
+def _uses_gameplay_fixture(src_file: str) -> bool:
+    """Return True if the test file uses the ``gameplay_test_runner`` fixture.
+
+    :param src_file: Path to the test file
+    """
+    try:
+        with open(src_file) as f:
+            return "gameplay_test_runner" in f.read()
+    except OSError:
+        return False
 
 
 def _query_bazel_tests(workspace_root: str) -> list[TestItem]:
@@ -91,16 +109,16 @@ def _walk_test_files(workspace_root: str) -> list[TestItem]:
 
 
 def _make_item(label: str, name: str, src_file: str) -> TestItem:
-    """Build a TestItem from a test label/name, classifying as field vs simulated."""
-    if "field_test" in name.lower():
-        test_type = "field"
-    else:
-        test_type = "simulated"
+    """Build a TestItem from a test label/name.
+
+    All gameplay tests are classified as ``simulated`` -- even ``*_field_test.py``
+    files -- since they can all be run against the in-Thunderscope simulator.
+    """
     return TestItem(
         target_label=label,
         test_name=name,
         src_file=src_file,
-        test_type=test_type,
+        test_type="simulated",
     )
 
 

src/software/thunderscope/common/test_selector_widget.py

@@ -1,6 +1,6 @@
 import threading
 
-from pyqtgraph.Qt.QtCore import QTimer, Qt
+from pyqtgraph.Qt.QtCore import Qt, pyqtSignal
 from pyqtgraph.Qt.QtWidgets import *
 
 from software.thunderscope.common.test_discovery import (
@@ -14,6 +14,12 @@
 class TestSelectorWidget(QWidget):
     """Widget that lets users discover and run gameplay tests within Thunderscope."""
 
+    # Signals used to marshal updates from worker threads (test scanning and the
+    # test bridge) back onto the GUI thread. Touching widgets off the GUI thread
+    # is undefined behaviour in Qt.
+    _status_signal = pyqtSignal(str)
+    _scan_finished_signal = pyqtSignal()
+
     def __init__(self, parent=None):
         super().__init__(parent)
         self._all_tests: list[TestItem] = []
@@ -25,6 +31,8 @@ def __init__(self, parent=None):
         self._expanded_targets: dict[str, list[TestItem]] = {}
 
         self._setup_ui()
+        self._status_signal.connect(self.status_label.setText)
+        self._scan_finished_signal.connect(self._on_scan_finished)
         self._start_scan()
 
     def _setup_ui(self):
@@ -81,6 +89,7 @@ def _start_scan(self):
         thread.start()
 
     def _scan_tests(self):
+        # Runs on a worker thread: only touch plain data here, never widgets.
         items = discover_test_targets()
         seen_targets = set()
         deduped = []
@@ -90,13 +99,17 @@ def _scan_tests(self):
                 deduped.append(it)
         self._all_tests = deduped
         self._scanning = False
+        self._scan_finished_signal.emit()
+
+    def _on_scan_finished(self):
+        # Runs on the GUI thread (queued from _scan_tests).
         self.refresh_button.setEnabled(True)
-        if not deduped:
+        self._rebuild_tree()
+        if not self._all_tests:
             self.status_label.setText(
                 "Failed to discover tests (bazel query blocked, "
                 "used filesystem fallback but found nothing)"
             )
-        QTimer.singleShot(0, self._rebuild_tree)
 
     def _on_search(self, text):
         self._rebuild_tree()
@@ -173,7 +186,9 @@ def _run_test(self, item: TestItem):
         self._bridge.run_test(item)
 
     def _on_test_completed(self, node_id: str, passed: bool, message: str):
+        # Invoked from the test bridge's worker thread, so route the status
+        # update through a signal to reach the GUI thread safely.
         if passed:
-            self.status_label.setText(f"{node_id} \u2713 PASSED")
+            self._status_signal.emit(f"{node_id} \u2713 PASSED")
         else:
-            self.status_label.setText(f"{node_id} \u2717 FAILED: {message}")
+            self._status_signal.emit(f"{node_id} \u2717 FAILED: {message}")

src/software/thunderscope/proto_unix_io.py

@@ -96,6 +96,26 @@ def register_observer(
         else:
             self.proto_observers[proto_class.DESCRIPTOR.full_name] = [buffer]
 
+    def unregister_observer(
+        self, proto_class: Type[Message], buffer: ThreadSafeBuffer
+    ) -> None:
+        """Unregister a previously registered buffer for a given protobuf class.
+
+        Used by short-lived consumers (e.g. test runners launched repeatedly from
+        the test selector widget) to avoid leaking stale buffers on a ProtoUnixIO
+        that outlives them.
+
+        :param proto_class: Class of protobuf the buffer was registered to consume
+        :param buffer: the buffer to remove
+        """
+        observers = self.proto_observers.get(proto_class.DESCRIPTOR.full_name)
+        if observers:
+            # Replace the list rather than mutating it in place so that a
+            # concurrent __send_proto_to_observers iteration stays safe.
+            self.proto_observers[proto_class.DESCRIPTOR.full_name] = [
+                b for b in observers if b is not buffer
+            ]
+
     def register_to_observe_everything(self, buffer: ThreadSafeBuffer) -> None:
         """Register a buffer to observe all incoming protobufs