Skip to content

test: Use QSignalSpy instead of QEventLoop #335

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 20, 2021
Merged

test: Use QSignalSpy instead of QEventLoop #335

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/qt/test/apptests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@
#endif

#include <QAction>
#include <QEventLoop>
#include <QLineEdit>
#include <QScopedPointer>
#include <QSignalSpy>
#include <QTest>
#include <QTextEdit>
#include <QtGlobal>
Expand All @@ -33,13 +33,14 @@ namespace {
//! Call getblockchaininfo RPC and check first field of JSON output.
void TestRpcCommand(RPCConsole* console)
{
QEventLoop loop;
QTextEdit* messagesWidget = console->findChild<QTextEdit*>("messagesWidget");
QObject::connect(messagesWidget, &QTextEdit::textChanged, &loop, &QEventLoop::quit);
QLineEdit* lineEdit = console->findChild<QLineEdit*>("lineEdit");
QSignalSpy mw_spy(messagesWidget, &QTextEdit::textChanged);
QVERIFY(mw_spy.isValid());
QTest::keyClicks(lineEdit, "getblockchaininfo");
QTest::keyClick(lineEdit, Qt::Key_Return);
loop.exec();
QVERIFY(mw_spy.wait(1000));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why 1000?

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member Author

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

Copy link
Contributor

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.

Copy link
Member Author

@jarolrod jarolrod May 20, 2021 •
edited
Loading

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Member Author

@jarolrod jarolrod May 21, 2021 •
edited
Loading

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.

QCOMPARE(mw_spy.count(), 2);
QString output = messagesWidget->toPlainText();
UniValue value;
value.read(output.right(output.size() - output.lastIndexOf(QChar::ObjectReplacementCharacter) - 1).toStdString());
Expand Down