Bug report: `event_loop_until_condition` raises when `ConditionTimeoutError` event loop terminated by something else

[corranwebster]

Environment

Toolkit: Qt

Description

With #1168 no longer checking the condition after event loop termination, we have a number of tests in downstream projects which are now failing because the event loop terminates via a mechanism other than (a) the condition evaluating to True in the event loop, or (b) timing out; but after the event loop has finished the condition evaluates to True.

When this happens code generates a ConditionTimeoutError, which is misleading because there was no timeout.

While this may sometimes (or even most of the time) indicate an issue with the test or other code, there could be situations where the exit via another path can happen via an issue with underlying code that the tester doesn't control, or may even be intentional. In any case, it feels like #1168 may also be too large a behaviour change for a bugfix release.

I think it may make sense to do a check for the case: condition_result is False, loop didn't time out, and condition evaluates to True and warn instead of raising. Whatever the fix is, it certainly shouldn't raise a ConditionTimeoutError in this case.

[corranwebster]

I think I want event_loop_until_condition to look something like this:

    def event_loop_until_condition(self, condition, timeout=10.0):
        condition_result = False
        timed_out = False

        def handler():
            nonlocal condition_result

            condition_result = condition_result or bool(condition())
            if condition_result:
                self.qt_app.exit()

        def do_timeout():
            nonlocal timed_out

            timed_out = True
            self.qt_app.exit()

        # Make sure we don't get a premature exit from the event loop.
        with dont_quit_when_last_window_closed(self.qt_app):
            condition_timer = QtCore.QTimer()
            condition_timer.setInterval(50)
            condition_timer.timeout.connect(handler)
            timeout_timer = QtCore.QTimer()
            timeout_timer.setSingleShot(True)
            timeout_timer.setInterval(round(timeout * 1000))
            timeout_timer.timeout.connect(do_timeout)
            timeout_timer.start()
            condition_timer.start()
            try:
                self.qt_app.exec_()
                if not condition_result:
                    if not timed_out:
                        if condition():
                            warnings.warn(
                                RuntimeWarning(
                                    ... # appropriate warning message
                                )
                            )
                        else:
                            raise ... # some other error
                    else:
                        raise ConditionTimeoutError(
                            "Timed out waiting for condition"
                        )
            finally:
                timeout_timer.stop()
                condition_timer.stop()

[mdickinson]

Agreed, with one suggested modification: one of the issues that was solved by the earlier PR was not to evaluate the condition outside the scope of the event loop; I think we want to keep that fix. So I'd suggest dropping the if condition() clause from your warning, and warn unconditionally if we're exiting the event loop as a result of neither the timeout nor the condition becoming true.

[mdickinson]

And if we're changing this again, I'd also suggest initialising condition_result to None rather than False, just so that in interactive debugging it's possible to distinguish the case where the condition_result was never set (e.g., because calling the condition raised an exception) from the case where the condition was successfully evaluated and gave a result of False.

[corranwebster]

one of the issues that was solved by the earlier PR was not to evaluate the condition outside the scope of the event loop; I think we want to keep that fix

I was hoping by guarding with the initial check of condition_result we'd avoid the problematic cases that #1168 was trying to fix: condition() is only called in special cases and avoids the "evaluated True in loop by failed at the end".

But warning unconditionally is probably a reasonable thing to do if only for simplicity. Tests which care can potentially double-check the condition if appropriate.

I agree about using None - the warning message should then probably differ based on whether the condition was ever evaluated.