<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/124779/">https://git.reviewboard.kde.org/r/124779/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 17th, 2015, 2:23 p.m. CEST, <b>Kevin Funk</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/124779/diff/1/?file=395249#file395249line80" style="color: black; font-weight: bold; text-decoration: underline;">shell/tests/test_filteredproblemstore.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">80</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cp">#define MYCOMPARE(actual, expected) \</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Is this really needed?</p></pre>
</blockquote>
<p>On August 17th, 2015, 2:26 p.m. CEST, <b>Laszlo Kis-Adam</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">QCOMPARE simply returns, so it cannot be used in a function that retuns a value (bool for example). So yes, it's neccessary.</p></pre>
</blockquote>
<p>On August 17th, 2015, 2:42 p.m. CEST, <b>Kevin Funk</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Your test-methods should use void. There should be no branching in unit test code. => MYCOMPARE could be dropped</p></pre>
</blockquote>
<p>On August 17th, 2015, 2:44 p.m. CEST, <b>Laszlo Kis-Adam</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That will lead to lots of repeated code...</p></pre>
</blockquote>
<p>On August 17th, 2015, 3 p.m. CEST, <b>Kevin Funk</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Hm? Not sure I'm missing something; I'm just asking you to turn <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">bool checkFoo(...)</code> into <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">void checkFoo(...)</code> (and use QCOMPARE instead of MYCOMPARE inside <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">checkFoo</code>). Does that lead to code duplication?</p></pre>
</blockquote>
<p>On August 17th, 2015, 3:06 p.m. CEST, <b>Laszlo Kis-Adam</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That way the testcase doesn't get interrupted if the check fails (since it only returns from checkFoo, not the testcase). So to make it interrupt the testcase I'd have to copypasta the code in checkFoo.</p></pre>
</blockquote>
<p>On August 17th, 2015, 3:27 p.m. CEST, <b>Kevin Funk</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">should</em> interrupt if the first assumption is wrong. If a an assumption is wrong, then any following failing assumption (which is based on the preceeding assumption (cf. <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QVERIFY(ptr1); QCOMPARE(ptr1, ptr2)</code>)) has no informative value.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">You only do that in external functions, so it's not too bad. Still, I'm not considering it "best-style". (I don't have a too-strong opinion on that, so whatever...)</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Well since it only returns from the external function it doesn't interrupt the test case. That's the entire point...</p></pre>
<br />
<p>- Laszlo</p>
<br />
<p>On August 18th, 2015, 12:25 a.m. CEST, Laszlo Kis-Adam wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDevelop.</div>
<div>By Laszlo Kis-Adam.</div>
<p style="color: grey;"><i>Updated Aug. 18, 2015, 12:25 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdevplatform
</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;">Tests for the following classes:
* DetectedProblem
* ProblemStoreNode
* ProblemStore
* FilteredProblemStore
* ProblemModel
* ProblemModelSet
* ProblemsView</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>plugins/problemreporter/CMakeLists.txt <span style="color: grey">(e179232)</span></li>
<li>plugins/problemreporter/tests/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>plugins/problemreporter/tests/test_problemsview.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>shell/filteredproblemstore.h <span style="color: grey">(d6cc909)</span></li>
<li>shell/filteredproblemstore.cpp <span style="color: grey">(a27b9ac)</span></li>
<li>shell/problemstore.h <span style="color: grey">(97f34f3)</span></li>
<li>shell/problemstore.cpp <span style="color: grey">(4b114b2)</span></li>
<li>shell/tests/CMakeLists.txt <span style="color: grey">(6b18470)</span></li>
<li>shell/tests/test_detectedproblem.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>shell/tests/test_filteredproblemstore.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>shell/tests/test_problemmodel.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>shell/tests/test_problemmodelset.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>shell/tests/test_problemstore.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>shell/tests/test_problemstorenode.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/124779/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>