D11604: kdirlistertest doesn't fail at random

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kdirlistertest.cpp:374
> +    QTRY_COMPARE(m_dirLister.spyStarted.count(), 0); // fast path: no directory listing needed
> +    QTRY_VERIFY(m_dirLister.spyCompleted.count() < 2);
> +    QTRY_VERIFY(m_dirLister.spyCompletedQUrl.count() < 2);

that's immediately true since it's 0 initially, so the TRY_ brings nothing. It's not like it's going to down if we wait ;)

Same for the lines before and after, actually.

> kdirlistertest.cpp:453
>      org::kde::KDirNotify::emitFilesChanged(QList<QUrl>() << QUrl::fromLocalFile(path));
> -    QVERIFY(waitForRefreshedItems());
> +    QTRY_COMPARE(m_refreshedItems.isEmpty(), false);
> +

I keep thinking that this would be more readable as

  QTRY_VERIFY(!m_refreshedItems.isEmpty())

But YMMV.

> kdirlistertest.cpp:480
>  
> -    QCOMPARE(m_dirLister.spyItemsDeleted.count(), 1);
> +    // The signal could be emited 1 time with all the deleted files or more times
> +    QTRY_VERIFY(m_dirLister.spyItemsDeleted.count() > 0);

How? This test is deleting a single file... I don't get it.

> kdirlistertest.cpp:513
> +    // The signal could be emited 1 time with all the deleted files or more times
> +    QTRY_VERIFY(m_dirLister.spyItemsDeleted.count() > 0);
> +

The problem is, this will likely move on after the first signal is emitted.

I think we need to sum up the number of items emitted by the deleted signals and wait until we reach 100.

> kdirlistertest.cpp:923
> +    QTRY_VERIFY(dirLister.isFinished());
> +    QTRY_VERIFY(m_items.isEmpty());
>  

there's nothing to wait for here, no TRY_ needed.

> kdirlistertest.cpp:940
>          // this is why this unittest needs to create a test file in the subdir.
> -        QTest::qWait(1000);
> -        QVERIFY(m_items.isEmpty());
> +        QTRY_VERIFY(m_items.isEmpty());
>  

This is one case where the new code isn't equivalent to the old code.

Starting with an empty list, "wait until it's empty" will move on immediately, while the old code was doing "wait a second and ensure the list is still empty afterwards".

> kdirlistertest.cpp:976
>      QVERIFY(spyCompleted.wait(1000));
> -    QVERIFY(secondDirLister.isFinished());
> -    QVERIFY(m_items.empty());
> +    QTRY_VERIFY(secondDirLister.isFinished());
> +    QTRY_VERIFY(m_items.empty());

We just waited for the completed signal, why wait again? I don't think this TRY_ is needed.

> kdirlistertest.cpp:977
> +    QTRY_VERIFY(secondDirLister.isFinished());
> +    QTRY_VERIFY(m_items.empty());
>      QCOMPARE(secondDirLister.rootItem().url().toLocalFile(), path);

no TRY_ needed

> kdirlistertest.cpp:987
>      // Check that the URL of the root item got updated
> -    QCOMPARE(secondDirLister.rootItem().url().toLocalFile(), newPath);
> +    QTRY_COMPARE(secondDirLister.rootItem().url().toLocalFile(), newPath);
>  

same here, this isn't async, it's done by the redirection handling, which we just waited for.

> kdirlistertest.cpp:1011
>      QVERIFY(spyCompleted.wait(1000));
> -    QVERIFY(m_dirLister.isFinished());
> +    QTRY_VERIFY(m_dirLister.isFinished());
>  

as above

> kdirlistertest.cpp:1054
> +    QTRY_COMPARE(m_dirLister.spyCompleted.count(), 0); // we stopped before the listing.
> +    QTRY_COMPARE(m_dirLister.spyCompletedQUrl.count(), 0);
> +    QTRY_COMPARE(m_dirLister.spyCanceled.count(), 0);

any QTRY_COMPARE(spy.count(), 0) is pretty useless to me. It's 0 already, we'll never wait. (or if it's not 0, we'll wait and fail, instead of failing right away)

REPOSITORY
  R241 KIO

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

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