D11604: kdirlistertest doesn't fail at random

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  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.

INLINE COMMENTS

> kdirlistertest.cpp:183
>  // This test assumes testOpenUrl was run before. So m_dirLister is holding the items already.
> +// This test creates 1000 files in the temp directory
>  void KDirListerTest::testNewItems()

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...)?

> kdirlistertest.cpp:293
>  
> -    QTest::qWait(1000); // We need a 1s timestamp difference on the dir, otherwise FAM won't notice anything.
> +    QTest::qWait(4000); // We need a 4s timestamp difference on the dir, otherwise FAM won't notice enough.
>  

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)

> kdirlistertest.cpp:502
> +    QMimeDatabase db;
> +    const QMimeType htmlType = db.mimeTypeForUrl(QUrl(QLatin1String("file://index.html")));
> +

This URL is incorrect, it's missing a slash, you're referring to a hostname of "index.html" here.

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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D11604

To: jtamate, #frameworks, dfaure
Cc: apol, michaelh, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180428/67493931/attachment.html>