<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/113877/">http://git.reviewboard.kde.org/r/113877/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I'm not a KWindowSystem expert, but anyway, here are some more things that I noticed, and that might or might not be worth considering:
1. I think that you can simplify the code in KStartupInfo::Private::startups_cleanup_internal(bool) slightly by using
it->age++;
if (it->silent() == Data::Yes)
etc. Might be a matter of taste though ;-) I think that this is slightly easier to read, but the compiler will most likely generate the same code for both variants.
2. Since your three test functions share the vast majority of their code, one could use data-driven testing:
http://qt-project.org/doc/qt-5.1/qttestlib/tutorial2.html
That could make future maintenance of the test functions easier, and it also makes it easier to see what the difference between the three tests is.
3. If I read the test code correctly, the unit test will have a runtime of at least 27 seconds because of the QTest::qWait(int) calls, right? I think that tests should run as fast as possible - if they don't, it gives people an excellent excuse for not running them, which greatly increases the risk of regressions.
For the test that checks if the QSignalSpy finally has a certain count(), one could use a loop over a qWait(int) with a smaller timeout that terminates if the count() has reached the expected value. (For the case that the expected count() was 1, we had kWaitForSignal, but this is probably not an option for tier 1. Internally, that function uses a QEventLoop which has its quit() slot connected to the expected signal, I think).
I'm not sure about the test that waits for 21 seconds and passes if the count() is zero though. I guess that the 21 seconds are required for safety, such that the test has a predictable outcome even if runs on a slow system with heavy load? Some knowledge of the code to be tested might be required to come up with a less time-consuming solution. If there was a signal that was emitted if everything went OK, one could simply wait for it.
</pre>
<br />
<p>- Frank Reininghaus</p>
<br />
<p>On November 15th, 2013, 10:42 a.m. UTC, Martin Gräßlin wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Frameworks.</div>
<div>By Martin Gräßlin.</div>
<p style="color: grey;"><i>Updated Nov. 15, 2013, 10:42 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdelibs
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">If there were multiple KStartupInfoIds which were removed at the same time, it was possible that the map got changed while being iterated, thus the data corrupted and we hit a crash later on. This changes to a QMutableMapIterator for checking the cleanup and allowing to safely remove ids during the iteration.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">When running the unit tests on the old code base it isn't guaranteed that it crashes as it's a race and also depends on X. Just run a few times and it will certainly hit the abort condition.
For reference the backtrace of the crash before fix:
#0 0x00007ffff50131e5 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1 0x00007ffff5016398 in __GI_abort () at abort.c:90
#2 0x00007ffff5c3d402 in qt_message_fatal (context=..., message=...) at global/qlogging.cpp:979
#3 0x00007ffff5c3afce in QMessageLogger::fatal (this=0x7fffffffbbc0, msg=0x7ffff5f8d9f8 "ASSERT: \"%s\" in file %s, line %d") at global/qlogging.cpp:384
#4 0x00007ffff5c369e6 in qt_assert (assertion=0x7ffff5f8b7f0 "size == 0 || offset < 0 || size_t(offset) >= sizeof(QArrayData)", file=0x7ffff5f8b7b0 "../../include/QtCore/../../src/corelib/tools/qarraydata.h", line=62)
at global/qglobal.cpp:2088
#5 0x00007ffff5c29bf8 in QArrayData::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:61
#6 0x00007ffff5c3015e in QTypedArrayData<char>::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:207
#7 0x00007ffff5c2fde7 in QByteArray::constData (this=0x6f4760) at ../../include/QtCore/../../src/corelib/tools/qbytearray.h:433
#8 0x00007ffff5c53333 in qstrcmp (str1=..., str2=...) at tools/qbytearray.cpp:347
#9 0x00007ffff7ba4625 in operator< (a1=..., a2=...) at /opt/qt5/include/QtCore/qbytearray.h:550
#10 0x00007ffff7ba0eb9 in KStartupInfoId::operator< (this=0x693858, id_P=...) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:1175
#11 0x00007ffff7ba67b5 in qMapLessThanKey<KStartupInfoId> (key1=..., key2=...) at /opt/qt5/include/QtCore/qmap.h:75
#12 0x00007ffff7ba6ef3 in QMapNode<KStartupInfoId, KStartupInfo::Data>::lowerBound (this=0x693840, akey=...) at /opt/qt5/include/QtCore/qmap.h:145
#13 0x00007ffff7ba66b4 in QMapData<KStartupInfoId, KStartupInfo::Data>::findNode (this=0x700870, akey=...) at /opt/qt5/include/QtCore/qmap.h:292
#14 0x00007ffff7ba5a73 in QMap<KStartupInfoId, KStartupInfo::Data>::remove (this=0x65acc0, akey=...) at /opt/qt5/include/QtCore/qmap.h:897
#15 0x00007ffff7b9dd42 in KStartupInfo::Private::remove_startup_info_internal (this=0x65acb0, id_P=...) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:399
#16 0x00007ffff7ba039b in KStartupInfo::Private::startups_cleanup_internal (this=0x65acb0, age_P=true) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:984
#17 0x00007ffff7ba02bf in KStartupInfo::Private::startups_cleanup (this=0x65acb0) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:965
#18 0x00007ffff7ba3d1c in KStartupInfo::qt_static_metacall (_o=0x7fffffffdc60, _c=QMetaObject::InvokeMetaMethod, _id=3, _a=0x7fffffffbf70) at /opt/build/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/moc_kstartupinfo.cpp:106
#19 0x00007ffff5ef8c9f in QMetaObject::activate (sender=0x684d50, signalOffset=3, local_signal_index=0, argv=0x0) at kernel/qobject.cpp:3530
#20 0x00007ffff5ef84f0 in QMetaObject::activate (sender=0x684d50, m=0x7ffff63ab980 <QTimer::staticMetaObject>, local_signal_index=0, argv=0x0) at kernel/qobject.cpp:3405
#21 0x00007ffff5f88fc3 in QTimer::timeout (this=0x684d50) at .moc/moc_qtimer.cpp:189
#22 0x00007ffff5f03bc5 in QTimer::timerEvent (this=0x684d50, e=0x7fffffffca30) at kernel/qtimer.cpp:255
#23 0x00007ffff5ef2314 in QObject::event (this=0x684d50, e=0x7fffffffca30) at kernel/qobject.cpp:1101
#24 0x00007ffff710e24e in QApplicationPrivate::notify_helper (this=0x60f010, receiver=0x684d50, e=0x7fffffffca30) at kernel/qapplication.cpp:3467
#25 0x00007ffff710b805 in QApplication::notify (this=0x7fffffffdc90, receiver=0x684d50, e=0x7fffffffca30) at kernel/qapplication.cpp:2888
#26 0x00007ffff5eb6908 in QCoreApplication::notifyInternal (this=0x7fffffffdc90, receiver=0x684d50, event=0x7fffffffca30) at kernel/qcoreapplication.cpp:878
#27 0x00007ffff5eba4f1 in QCoreApplication::sendEvent (receiver=0x684d50, event=0x7fffffffca30) at ../../include/QtCore/../../src/corelib/kernel/qcoreapplication.h:232
#28 0x00007ffff5f2c450 in QTimerInfoList::activateTimers (this=0x654ec0) at kernel/qtimerinfo_unix.cpp:643
#29 0x00007ffff5f2d764 in timerSourceDispatch (source=0x654e60) at kernel/qeventdispatcher_glib.cpp:185
#30 0x00007ffff25a8ea6 in g_main_context_dispatch () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#31 0x00007ffff25a91f8 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#32 0x00007ffff25a929c in g_main_context_iteration () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#33 0x00007ffff5f2e0f9 in QEventDispatcherGlib::processEvents (this=0x64ed40, flags=...) at kernel/qeventdispatcher_glib.cpp:426
#34 0x00007fffefcca744 in QPAEventDispatcherGlib::processEvents (this=0x64ed40, flags=...) at eventdispatchers/qeventdispatcher_glib.cpp:123
#35 0x00007ffff5eb6e46 in QCoreApplication::processEvents (flags=..., maxtime=11000) at kernel/qcoreapplication.cpp:1076
#36 0x0000000000403ecc in QTest::qWait (ms=11000) at /opt/qt5/include/QtTest/qtestsystem.h:66
#37 0x000000000040690c in KStartupInfo_UnitTest::dontCrashCleanup (this=0x7fffffffdc50) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/autotests/kstartupinfo_unittest.cpp:128
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>tier1/kwindowsystem/autotests/kstartupinfo_unittest.cpp <span style="color: grey">(17890ff)</span></li>
<li>tier1/kwindowsystem/src/kstartupinfo.cpp <span style="color: grey">(402cc97)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/113877/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>