| Summary: | BackendTest::createSession() may fail to catch the ready() signal by m_session | ||
|---|---|---|---|
| Product: | [Applications] cantor | Reporter: | Tuukka Verho <tuukka.verho> |
| Component: | general | Assignee: | Alexander Rieder <alexanderrieder> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | ||
| Priority: | NOR | ||
| Version: | unspecified | ||
| Target Milestone: | --- | ||
| Platform: | unspecified | ||
| OS: | Linux | ||
| Latest Commit: | http://commits.kde.org/cantor/ab7c8302fe7c20e9b7956a4c886d706f14033ffe | Version Fixed In: | |
| Attachments: |
Fix the bug using QSignalSpy
A fix that keeps the event loop approach |
||
|
Description
Tuukka Verho
2014-02-02 12:35:36 UTC
Created attachment 84947 [details]
Fix the bug using QSignalSpy
Use QSignalSpy instead of a temporary event loop. For consistency, also use QSignalSpy in BackendTest::waitForSignal(). Furthermore, use the waitForSignal() method in evalExp() to avoid code duplication.
As an added feature, a timeout prevents hanging if the session fails to emit the ready() signal.
Hi, the idea of using QSignalspy is good, but I think the use of processEvent in a loop is bad, as it seems to waste a lot of cpu cycles waiting (my laptop goes to 100% usage and a short test with cachegrind reveals there a lot more commands being executed with your patch applied). If you look at the implementation of QSignalSpy::wait() in Qt5 you see that they also use an event loop for doing this. So I think this should be the way waitForSignal should be implemented. Also maybe you can share the code between the init and the waitForEvent by taking a QSignalSpy as parameter and doing the event-loop thing for that? Ok, in that case the simplest solution seems to be to stick with the old approach but use a signal spy in createSession() to catch the ready() signal if it is emitted immediately. Another approach would be to have a slot to call m_session->login() and invoke it with QTimer::singleShot(), but I don't think it would be any tidier... Somehow I don't like the idea of creating a new event loop just to wait for a signal, but it indeed seems to be the Qt way to do it... Created attachment 84979 [details]
A fix that keeps the event loop approach
the new patch seems to work well. Do you have commit access or should I do it for you? No, I don't have a dev account. Git commit ab7c8302fe7c20e9b7956a4c886d706f14033ffe by Alexander Rieder. Committed on 06/02/2014 at 21:36. Pushed by arieder into branch 'master'. use QSignalSpy to detect if a signal was emited in the unit tests. this way we won't miss signals when they are emitted immediately, i.e. before we create the temporary event loop. patch by Tuukka Verho, thanks! M +8 -12 src/lib/test/backendtest.cpp http://commits.kde.org/cantor/ab7c8302fe7c20e9b7956a4c886d706f14033ffe |