QSignalSpy.wait() doesn't behave like the docs suggest

[gonzalo-bulnes]

I think we've got a problem (Please find what I'm doing wrong! ><')

It seems like the wait() method of QSignalSpy in Python doesn't behave like the C++ docs suggest. Namely:

🍎 It doesn't return True when the signal was received at least once before the timeout.
🍎 It doesn't return early once the signal was received (increasing the timeout of tests/test_qsignalspy.py::TestService::test_wait_without_assert to a few seconds makes that obvious - try it locally).

Minimal example: a7f7868
CI: https://app.circleci.com/pipelines/github/freedomofpress/securedrop-client/3043/workflows/42f33676-a8cf-47af-9b1f-c4b1a551f291/jobs/18248
Docs: https://doc.qt.io/qt-5/qsignalspy.html#wait

If this is indeed the case, then it is problematic for a few reasons:

• we use signals to communicate between GUI components in a thread-safe way
• we use threads to run longer operations without blocking the GUI
  signals are asynchronous: they are processed by Qt event loop
• means that in a small test, without wait we can easily reach the end of the test before the signal was processed and assert on meaningless state
• means that without wait, every test contains a race condition :nightmare: :corn_flakes:

If the wait() method was behaving as advertised:

• the test "arrange" stage can set up a QSignalSpy
• the "act" stage can emit the relevant signal
• the "assert" stage can wait for the relevant signal before asserting on state
• there is no race condition (:warning: as long as signal are idempotent, because wait doesn't pretend to guarantee that only one signal was processed by the event loop - that's a different story :sadness_though_no_nightmares:)

Can someone take a look at the code above? I'd like to discuss it to confirm/infirm my suspicions 🍐 :female-detective:.

[gonzalo-bulnes]

Note: It seems to me that asserting on spy.siValid() is pointless, since spying on an invalid signal already causes an error in Python.

[gonzalo-bulnes]

@cfm - we talked briefly about it today, I think my fears are founded.

@zekehuntergreen I think you may be interested in this conversation. It would clearly have implication on how we test some of the code we write. Some of the recommendations I made in #1604 might actually be ineffective, and we might only have been lucky so far. (I'm being less lucky in some new code that relies more heavily on that test pattern, that's how I noticed. #1615)

[cfm]

@gonzalo-bulnes, you may have already done all of this research, but just in case....

https://stackoverflow.com/a/55123043 seems to describe a similar race condition in which "QSignalSpy would catch the emitted signal before QSignalSpy::wait() was called". If QSignalSpy.wait() "starts an event loop that runs until the given signal is received" (my emphasis), then in the example—

securedrop-client/tests/test_qsignalspy.py

Lines 43 to 45 in a7f7868

	client.query.emit() # Act.
	assert query_emissions.wait(200)
	assert response_emissions.wait(200)

—neither QSignalSpy is wait()ing at the time of client.query.emit().

https://forum.qt.io/post/445592 suggests delaying the emission with a single-shot timer. That strikes me as just racing the race condition. :-(