<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/102098/">http://git.reviewboard.kde.org/r/102098/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 28th, 2011, 10:26 a.m., <b>Aurélien Gâteau</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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 don't think it is good for unit-tests to test different things depending on environment variables (
@David, what is your opinion on the subject?)
I was actually about to commit a simpler fix:
@@ -26,11 +26,11 @@ QTEST_KDEMAIN( KGlobalSettingsTest, GUI )
#include <kglobalsettings.h>
#include <kdebug.h>
#include <kprocess.h>
#include <QtCore/QEventLoop>
#include <QtDBus/QtDBus>
-
+#include <stdlib.h>
/**
* The strategy of this test is:
* We install QSignalSpy instances on many signals from KGlobalSettings::self(),
* and then we get another process (kglobalsettingsclient) to call emitChange(),
* and we check that the corresponding signals are emitted, i.e. that our process
@@ -39,10 +39,15 @@ QTEST_KDEMAIN( KGlobalSettingsTest, GUI )
* As a nice side-effect we automatically test a bit of KProcess as well :)
*/
void KGlobalSettingsTest::initTestCase()
{
+ // Some signals are only emitted when we are running a full KDE session. If
+ // we are not then KDE applications follow the platform palette and font
+ // settings.
+ setenv("KDE_FULL_SESSION", "1", 1);
+
QDBusConnectionInterface *bus = 0;
if (!QDBusConnection::sessionBus().isConnected() || !(bus = QDBusConnection::sessionBus().interface())) {
QFAIL("Session bus not found");
}
}</pre>
</blockquote>
<p>On July 28th, 2011, 11:33 a.m., <b>Frank Reininghaus</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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 fully agree that the things that are tested should better not depend on environment variables, and I can confirm that the test passes here with your patch applied. I hadn't tried this myself earlier because I had assumed that the variable has to be set *before* the test executable is run, but that's obviously wrong ;-)
Maybe it would be even better to always test both things (i.e., that signals are emitted when KDE_FULL_SESSION is set and that they aren't when it's not set) if we find a good way to do it? That way, we would also test that the things that your recent commit changed work as they should.</pre>
</blockquote>
<p>On July 28th, 2011, 12:48 p.m., <b>Aurélien Gâteau</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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 agree testing both would be even nicer. Maybe you can mix your patch with mine to do it, calling setenv("KDE_FULL_SESSION", "1", 1) to fake the "run in KDE session" situation and unsetenv("KDE_FULL_SESSION") to fake the 'run outside KDE session' situation?</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The problem with that is that the value of KDE_FULL_SESSION is read only once by KGlobalSettings (in the constructor of its private class, which is invoked the first time KGlobalSettings::self() is called). So setting and unsetting the variable in different parts of the test does not work. The only possible way I can think of at the moment would be to set/unset the variable in kglobalsettingstest and then do the actual testing in another executable, but maybe that's not worth the effort.
I'll commit your patch (with qputenv instead of setenv) to fix the test failure.</pre>
<br />
<p>- Frank</p>
<br />
<p>On July 27th, 2011, 4:31 p.m., Frank Reininghaus wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kdelibs, Aurélien Gâteau and David Faure.</div>
<div>By Frank Reininghaus.</div>
<p style="color: grey;"><i>Updated July 27, 2011, 4:31 p.m.</i></p>
<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;">Since commit b064749a754ec358170ecb7f19828e4216f6e965, KDE palette and font settings are only used when running KDE apps in a full KDE session. This makes KGlobalSettingsTest fail if the test is not run in a full KDE session, see
http://my.cdash.org/testSummary.php?project=16&name=kdeui-kglobalsettingstest&date=2011-07-27
This commit changes KGlobalSettings' unit test to reflect that change. My first idea was to change the unit test such that it verifies the expected behaviour for both situations, i.e., apps run in a full KDE session and apps run in some other kind of session, but I could not figure out a way to do this without changing the KDE_FULL_SESSION environment variable before the unit test executable is run.
In the case that the signal is not expected, I reduced the kWaitForSignal timeout to prevent wasting too much time each time the test is run.</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;">The test passes here (run by my kde-devel user in a Konsole inside the regular user's KDE 4.6 session).</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>kdeui/tests/kglobalsettingstest.h <span style="color: grey">(69ed5bf)</span></li>
<li>kdeui/tests/kglobalsettingstest.cpp <span style="color: grey">(464825d)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/102098/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>