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
