This PR refactors our GUI apptests to use QSignalSpy instead of QEventLoop.
QSignalSpy is more appropriate for our GUI test’s as it is purpose-built for testing emission of signals and sets up its own QEventLoop when the wait function is called.
39 QLineEdit* lineEdit = console->findChild<QLineEdit*>("lineEdit");
40+ QSignalSpy mw_spy(messagesWidget, &QTextEdit::textChanged);
41 QTest::keyClicks(lineEdit, "getblockchaininfo");
42 QTest::keyClick(lineEdit, Qt::Key_Return);
43- loop.exec();
44+ QVERIFY(mw_spy.wait(1000));
32@@ -33,13 +33,12 @@ namespace {
33 //! Call getblockchaininfo RPC and check first field of JSON output.
34 void TestRpcCommand(RPCConsole* console)
35 {
36- QEventLoop loop;
37 QTextEdit* messagesWidget = console->findChild<QTextEdit*>("messagesWidget");
38- QObject::connect(messagesWidget, &QTextEdit::textChanged, &loop, &QEventLoop::quit);
39 QLineEdit* lineEdit = console->findChild<QLineEdit*>("lineEdit");
40+ QSignalSpy mw_spy(messagesWidget, &QTextEdit::textChanged);
QSignalSpy class.
updated from c519b41 -> 7eea659
Changes:
textChanged signal (it is received twice because the console outputs the command which was entered, then outputs the response to that command)
40+ QSignalSpy mw_spy(messagesWidget, &QTextEdit::textChanged);
41+ QVERIFY(mw_spy.isValid());
42 QTest::keyClicks(lineEdit, "getblockchaininfo");
43 QTest::keyClick(lineEdit, Qt::Key_Return);
44- loop.exec();
45+ QVERIFY(mw_spy.wait(1000));
exec call would process all events while the new signal spy wait will only wait for the text changed event, which could potentially trigger after the getblockchaininfo event and before the Key_Return event, or time out before either event. The PR description says using signal spy is more appropriate without explaining why, but it seems to make the test more verbose and less robust.
The wait call will process all events just like the exec call does. The wait call sets up a QEventLoop which calls exec, but it’s bounded by a timer which is the value provided to wait
Looking at the source code for the qsignalspy wait function, we can see the following line:
0m_loop.enterLoopMSecs(timeout);
Going to the source for the enterLoopMSecs function, we can observe that
QEventLoop is being setupexec function is being calledQEventLoop will end after a certain amount of ms, which is our provided valueAs I see it, if there is a downside, it is that this part of the test will always wait for 1000ms. So if the signals are received in 1ms, the loop will continue waiting for 999 more ms. At the same time, it is possible that the test fails because the signals weren’t received within 1000ms because the CI is slow.
QSignalSpy more intuitive than to use an event loop and connect the signal to QEventLoop::quit.
Sounds good if test isn’t failing. Just if the test turns out to be unreliable, I’d recommend reverting.
I see only downsides and no upsides to using signal spy in this instance. It makes the test more fragile, more verbose, and less straightforward, waiting indirectly though a hidden event loop in an inflexible and poorly documented test class, instead of just running the code we need to run directly in a deterministic way.
wait it doesn’t diminish the use of QSignalSpy to test the proper emission of signals and count. So a hybrid approach of both setting up a QEventLoop to process events and a QSignalSpy as a window into what is happening signal-wise can be adapted.
Not sure why a “signal spy” would be more intuitive than an “event loop”. The previous code is telling the event loop to quit when the textChanged signal happens. The new code is telling the signal spy to wait until the textChanged signal happens.
I’d think both versions should be about as intuitive at the test behavior level. Only an event loop is a well known construct that I think you need to understand anyway to use Qt, while a signal spy is a specialized test class (with a hidden event loop inside) that makes you worry about timeouts and leaves you “going to the source for the enterLoopMSecs function” to find out how they work.
Sorry I’m probably sounding too cranky about this.
😀 no worries. Please always be as vocal as possible, we all just want to ensure good changes occur.
There is one hidden bug case that the original code wouldn’t detect. And another that this version does a (very slightly) better job of diagnosing.
Scenario A: No Actual console response
The QEventLoop will quit when the textChanged signal is received. But, this is supposed to be received twice. When you enter a command, the textChanged signal will be emitted because the command you entered will now be present. Then the console will present the output to your command and another textChanged signal will be emitted.
Now, let’s say we introduce a bug where the console fails to present output to your command. The old version will pass on the QEventLoop but detect this when it searches for the current chain value and compares it with “regtest”. The new version will fail earlier because the signal wasn’t received twice. This makes it (very slightly) easier to diagnose what we did wrong because we are counting the signal emissions.
Scenario B: Double console response If we introduce a bug where the console starts to output responses twice, the old version cannot detect this. The chain value will be found and the old test will say everything looks good. The new version will know something is wrong because the signal would have been received three times instead of 2 and the test will fail.
Furthermore, let’s say we introduce a bug where a textChanged signal is never emitted. The old test will hang indeterminately. The new test will time out and the CI can continue to test other things.