Fix and clean up timer handling in signal/callback blockers

CHANGELOG.rst

@@ -1,9 +1,20 @@
 UNRELEASED
 ----------
 
-* Added official support for Python 3.13.
-* Dropped support for EOL Python 3.8.
-* Dropped support for EOL PySide 2.
+- Added official support for Python 3.13.
+- Dropped support for EOL Python 3.8.
+- Dropped support for EOL PySide 2.
+- Fixed PySide6 exceptions / warnings about being unable to disconnect signals
+  with ``qtbot.waitSignal`` (`#552`_, `#558`_).
+- Reduced the likelyhood of trouble when using ``qtbot.waitSignal(s)`` and
+  ``qtbot.waitCallback`` where the signal/callback is emitted from a non-main
+  thread. In theory, more problems remain and this isn't a proper fix yet. In
+  practice, it seems impossible to provoke any problems in pytest-qt's testsuite.
+  (`#586`_)
+
+..  _#552: https://github.com/pytest-dev/pytest-qt/issues/552
+..  _#558: https://github.com/pytest-dev/pytest-qt/issues/558
+..  _#586: https://github.com/pytest-dev/pytest-qt/issues/586
 
 4.4.0 (2024-02-07)
 ------------------

pytest.ini

@@ -2,5 +2,4 @@
 testpaths = tests
 addopts = --strict-markers --strict-config
 xfail_strict = true
-markers =
-  filterwarnings: pytest's filterwarnings marker
+filterwarnings = error

src/pytestqt/wait_signal.py

@@ -24,12 +24,12 @@ def __init__(self, timeout=5000, raising=True):
         self.raising = raising
         self._signals = None  # will be initialized by inheriting implementations
         self._timeout_message = ""
-        if timeout is None or timeout == 0:
-            self._timer = None
-        else:
-            self._timer = qt_api.QtCore.QTimer(self._loop)
-            self._timer.setSingleShot(True)
+
+        self._timer = qt_api.QtCore.QTimer(self._loop)
+        self._timer.setSingleShot(True)
+        if timeout is not None:
             self._timer.setInterval(timeout)
+        self._timer.timeout.connect(self._quit_loop_by_timeout)
 
     def wait(self):
         """
@@ -43,11 +43,13 @@ def wait(self):
             return
         if self.timeout is None and not self._signals:
             raise ValueError("No signals or timeout specified.")
-        if self._timer is not None:
-            self._timer.timeout.connect(self._quit_loop_by_timeout)
-            self._timer.start()
 
         if self.timeout != 0:
+            if self.timeout is not None:
+                # asserts as a stop-gap for possible multithreading issues
+                assert not self.signal_triggered
+                self._timer.start()
+                assert not self.signal_triggered
             qt_api.exec(self._loop)
 
         if not self.signal_triggered and self.raising:
@@ -62,10 +64,7 @@ def _quit_loop_by_timeout(self):
     def _cleanup(self):
         # store timeout message before the data to construct it is lost
         self._timeout_message = self._get_timeout_error_message()
-        if self._timer is not None:
-            _silent_disconnect(self._timer.timeout, self._quit_loop_by_timeout)
-            self._timer.stop()
-            self._timer = None
+        self._timer.stop()
 
     def _get_timeout_error_message(self):
         """Subclasses have to implement this, returning an appropriate error message for a TimeoutError."""
@@ -649,12 +648,12 @@ def __init__(self, timeout=5000, raising=True):
         self.kwargs = None
         self.called = False
         self._loop = qt_api.QtCore.QEventLoop()
-        if timeout is None:
-            self._timer = None
-        else:
-            self._timer = qt_api.QtCore.QTimer(self._loop)
-            self._timer.setSingleShot(True)
+
+        self._timer = qt_api.QtCore.QTimer(self._loop)
+        self._timer.setSingleShot(True)
+        if timeout is not None:
             self._timer.setInterval(timeout)
+        self._timer.timeout.connect(self._quit_loop_by_timeout)
 
     def wait(self):
         """
@@ -664,8 +663,7 @@ def wait(self):
         __tracebackhide__ = True
         if self.called:
             return
-        if self._timer is not None:
-            self._timer.timeout.connect(self._quit_loop_by_timeout)
+        if self.timeout is not None:
             self._timer.start()
         qt_api.exec(self._loop)
         if not self.called and self.raising:
@@ -687,10 +685,7 @@ def _quit_loop_by_timeout(self):
             self._loop.quit()
 
     def _cleanup(self):
-        if self._timer is not None:
-            _silent_disconnect(self._timer.timeout, self._quit_loop_by_timeout)
-            self._timer.stop()
-            self._timer = None
+        self._timer.stop()
 
     def __call__(self, *args, **kwargs):
         # Not inside the try: block, as if self.called is True, we did quit the

tests/conftest.py

@@ -1,4 +1,3 @@
-import functools
 import time
 
 import pytest
@@ -63,10 +62,9 @@ def shutdown(self):
         def single_shot(self, signal, delay):
             t = qt_api.QtCore.QTimer(self)
             t.setSingleShot(True)
-            slot = functools.partial(self._emit, signal)
-            t.timeout.connect(slot)
+            t.timeout.connect(signal)
             t.start(delay)
-            self.timers_and_slots.append((t, slot))
+            self.timers_and_slots.append((t, signal))
 
         def single_shot_callback(self, callback, delay):
             t = qt_api.QtCore.QTimer(self)
@@ -75,9 +73,6 @@ def single_shot_callback(self, callback, delay):
             t.start(delay)
             self.timers_and_slots.append((t, callback))
 
-        def _emit(self, signal):
-            signal.emit()
-
     timer = Timer()
     yield timer
     timer.shutdown()

tests/test_wait_signal.py

@@ -1364,3 +1364,132 @@ def test_timeout_not_raising(self, qtbot):
         assert not callback.called
         assert callback.args is None
         assert callback.kwargs is None
+
+
+@pytest.mark.parametrize(
+    "check_stderr, count",
+    [
+        # Checking stderr messages
+        pytest.param(
+            True,  # check stderr
+            200,  # gets output reliably even with only few runs (often the first)
+            id="stderr",
+        ),
+        # Triggering AttributeError
+        pytest.param(
+            False,  # don't check stderr
+            # Hopefully enough to trigger the AttributeError race condition reliably.
+            # With 500 runs, only 1 of 5 Windows PySide6 CI jobs triggered it (but all
+            # Ubuntu/macOS jobs did).  With 1500 runs, Windows jobs still only triggered
+            # it 0-2 times.
+            #
+            # On my machine (Linux, Intel Core Ultra 9 185H), 500 runs trigger it
+            # reliably and take ~1s in total.
+            2500 if sys.platform == "win32" else 500,
+            id="attributeerror",
+        ),
+    ],
+)
+@pytest.mark.parametrize("multi_blocker", [True, False])
+def test_signal_raised_from_thread(
+    pytester: pytest.Pytester, check_stderr: bool, multi_blocker: bool, count: int
+) -> None:
+    """Wait for a signal with a thread.
+
+    Extracted from https://github.com/pytest-dev/pytest-qt/issues/586
+    """
+    pytester.makepyfile(
+        f"""
+        import pytest
+        from pytestqt.qt_compat import qt_api
+
+
+        class Worker(qt_api.QtCore.QObject):
+            signal = qt_api.Signal()
+
+
+        @pytest.mark.parametrize("_", range({count}))
+        def test_thread(qtbot, capfd, _):
+            worker = Worker()
+            thread = qt_api.QtCore.QThread()
+            worker.moveToThread(thread)
+            thread.start()
+
+            try:
+                if {multi_blocker}:  # multi_blocker
+                    with qtbot.waitSignals([worker.signal], timeout=500) as blocker:
+                        worker.signal.emit()
+                else:
+                    with qtbot.waitSignal(worker.signal, timeout=500) as blocker:
+                        worker.signal.emit()
+            finally:
+                thread.quit()
+                thread.wait()
+
+            if {check_stderr}:  # check_stderr
+                out, err = capfd.readouterr()
+                assert not err
+    """
+    )
+
+    res = pytester.runpytest_subprocess("-x", "-s")
+    outcomes = res.parseoutcomes()
+
+    if outcomes.get("failed", 0) and check_stderr and qt_api.pytest_qt_api == "pyside6":
+        # The test succeeds on PyQt (unsure why!), but we can't check
+        # qt_api.pytest_qt_api at import time, so we can't use
+        # pytest.mark.xfail conditionally.
+        pytest.xfail(
+            "Qt error: QObject::killTimer: "
+            "Timers cannot be stopped from another thread"
+        )
+
+    res.assert_outcomes(passed=outcomes["passed"])  # no failed/error
+
+
+@pytest.mark.skip(reason="Runs ~1min to reproduce bug reliably")
+def test_callback_in_thread(pytester: pytest.Pytester) -> None:
+    """Wait for a callback with a thread.
+
+    Inspired by https://github.com/pytest-dev/pytest-qt/issues/586
+    """
+    # Hopefully enough to trigger the bug reliably.
+    #
+    # On my machine (Linux, Intel Core Ultra 9 185H), sometimes the bug only
+    # triggers after ~30k runs (~44s). Thus, we skip this test by default.
+    count = 50_000
+
+    pytester.makepyfile(
+        f"""
+        import pytest
+        from pytestqt.qt_compat import qt_api
+
+
+        class Worker(qt_api.QtCore.QObject):
+            def __init__(self, callback):
+                super().__init__()
+                self.callback = callback
+
+            def call_callback(self):
+                self.callback()
+
+
+        @pytest.mark.parametrize("_", range({count}))
+        def test_thread(qtbot, _):
+            thread = qt_api.QtCore.QThread()
+
+            try:
+                with qtbot.waitCallback() as callback:
+                    worker = Worker(callback)
+                    worker.moveToThread(thread)
+                    thread.started.connect(worker.call_callback)
+                    thread.start()
+            finally:
+                thread.quit()
+                thread.wait()
+    """
+    )
+
+    res = pytester.runpytest_subprocess("-x")
+    outcomes = res.parseoutcomes()
+    res.assert_outcomes(passed=outcomes["passed"])  # no failed/error