-
Notifications
You must be signed in to change notification settings - Fork 261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: Use QSignalSpy instead of QEventLoop #335
Conversation
QTest::keyClicks(lineEdit, "getblockchaininfo"); | ||
QTest::keyClick(lineEdit, Qt::Key_Return); | ||
loop.exec(); | ||
QVERIFY(mw_spy.wait(1000)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 1000?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No specific reason, just an arbitrary number. Would you suggest leaving it as the default value of 5000 milliseconds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok if CI passes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
focal CI failure seems unrelated: https://cirrus-ci.com/task/5251590773800960?logs=ci#L3432
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK on moving to the QSignalSpy
class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 7eea659, tested on Linux Mint 20.1 (Qt 5.12.8).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK 7eea659.
QTest::keyClicks(lineEdit, "getblockchaininfo"); | ||
QTest::keyClick(lineEdit, Qt::Key_Return); | ||
loop.exec(); | ||
QVERIFY(mw_spy.wait(1000)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't switching from exec to wait(1000) make the test non-deterministic? The previous 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
m_loop.enterLoopMSecs(timeout);
Going to the source for the enterLoopMSecs function, we can observe that
- a
QEventLoop
is being setup - The
exec
function is being called - the
QEventLoop
will end after a certain amount of ms, which is our provided value
As 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find QSignalSpy
more intuitive than to use an event loop and connect the signal to QEventLoop::quit
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to review! I think if this turns out to be unreliable on the part of 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any benefit to the test counting its own signal emissions either. The goal of the test is to trigger an rpc, wait for the rpc, and check the rpc result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I'm probably sounding too cranky about this. I definitely appreciate any work on these tests, and think this change is basically fine, but just wanted to point out some drawbacks I see and point to a resolution just in case there do turn out to be reliability problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
7eea659 qt, test: use qsignalspy instead of qeventloop (Jarol Rodriguez) Pull request description: This PR refactors our GUI `apptests` to use [QSignalSpy](https://doc.qt.io/qt-5/qsignalspy.html) instead of [QEventLoop](https://doc.qt.io/qt-5/qeventloop.html). `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. ACKs for top commit: hebasto: ACK 7eea659, tested on Linux Mint 20.1 (Qt 5.12.8). promag: Code review ACK 7eea659. Tree-SHA512: 3adddbcc5efd726302b606980c9923025c44bb8ee16cb8a183e633e423179c0822db66de9ccba20dc5124fff34af4151a379c9cd18130625c60789ce809ee6fd
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 ownQEventLoop
when thewait
function is called.