<table><tr><td style="">dfaure requested changes to this revision.<br />dfaure added a comment.<br />This revision now requires changes to proceed.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D11604">View Revision</a></tr></table><br /><div><div><p>I'm all for making tests more stable, but changing what the test is testing, sounds dangerous to me (it might hide bugs in the cases that the test intended to be testing). Sometimes it's necessary to change what we're testing, if we're testing something unreliable, but e.g. creating one file should and must reliably trigger an update, we need to make sure of that.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D11604#inline-64163">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kdirlistertest.cpp:183</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "><span style="color: #74777d">// This test assumes testOpenUrl was run before. So m_dirLister is holding the items already.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d">// This test creates 1000 files in the temp directory</span>
</div><div style="padding: 0 8px; margin: 0 4px; "><span style="color: #aa4000">void</span> <span class="n">KDirListerTest</span><span style="color: #aa2211">::</span><span class="n">testNewItems</span><span class="p">()</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">That's testing a different use case. Isn't there a risk that this hides a bug when a single file is created by the user (which is more common than 1000...)?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D11604#inline-64162">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kdirlistertest.cpp:293</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);">    <span class="n">QTest</span><span style="color: #aa2211">::</span><span class="n">qWait</span><span class="p">(<span class="bright"></span></span><span class="bright"></span><span style="color: #601200"><span class="bright">1000</span></span><span class="bright"></span><span class="p"><span class="bright">);</span></span><span class="bright"> </span><span style="color: #74777d"><span class="bright">// We need a 1s timestamp difference on the dir, otherwise FAM won't notice anything</span>.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">QTest</span><span style="color: #aa2211">::</span><span class="n">qWait</span><span class="p">(<span class="bright"></span></span><span class="bright"></span><span style="color: #601200"><span class="bright">4000</span></span><span class="bright"></span><span class="p"><span class="bright">);</span></span><span class="bright"> </span><span style="color: #74777d"><span class="bright">// We need a 4s timestamp difference on the dir, otherwise FAM won't notice enough</span>.</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Why 4s? The comment sounds like it has to be 4 exactly, but I guess this is just "safety"? That's a lot, it makes the test quite slow. (Rule of thumb is that a unittest should last less than 5s)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D11604#inline-64164">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kdirlistertest.cpp:502</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">QMimeDatabase</span> <span class="n">db</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span style="color: #aa4000">const</span> <span class="n">QMimeType</span> <span class="n">htmlType</span> <span style="color: #aa2211">=</span> <span class="n">db</span><span class="p">.</span><span class="n">mimeTypeForUrl</span><span class="p">(</span><span class="n">QUrl</span><span class="p">(</span><span class="n">QLatin1String</span><span class="p">(</span><span style="color: #766510">"file://index.html"</span><span class="p">)));</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This URL is incorrect, it's missing a slash, you're referring to a hostname of "index.html" here.</p>

<p style="padding: 0; margin: 8px;">But if you fix it, how then could this ever be anything else than text/html? I don't see the point of this indirection.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R241 KIO</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D11604">https://phabricator.kde.org/D11604</a></div></div><br /><div><strong>To: </strong>jtamate, Frameworks, dfaure<br /><strong>Cc: </strong>apol, michaelh, bruns<br /></div>