D11604: kdirlistertest doesn't fail at random

[Jaime Torres Amate]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:0c8cd4076b7d: kdirlistertest doesnt fail at random 
(authored by jtamate).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11604?vs=39991=3

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

AFFECTED FILES
  autotests/kdirlistertest.cpp
  autotests/kdirlistertest.h

To: jtamate, #frameworks, dfaure
Cc: kde-frameworks-devel, apol, michaelh, ngraham, bruns

D11604: kdirlistertest doesn't fail at random

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  Those comments were all for a single line of code ;)
  But yes, your last change is exactly what I meant. "wait for nothing" is when 
waiting for a spy to get to 0.
  The line above that waits for it to get to 1, so TRY_ makes sense there.
  
  Thanks!

REPOSITORY
  R241 KIO

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

To: jtamate, #frameworks, dfaure
Cc: kde-frameworks-devel, apol, michaelh, ngraham, bruns

D11604: kdirlistertest doesn't fail at random

[Jaime Torres Amate]

jtamate updated this revision to Diff 39991.
jtamate marked an inline comment as done.
jtamate added a comment.


  I'm never sure at how many lines the comments affects, but the last one 
should be:
  
QTRY_COMPARE(m_dirLister.spyClear.count(), 1);
QCOMPARE(m_dirLister.spyClearQUrl.count(), 0);
  
  Otherwise the test fails for me.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11604?vs=39990=39991

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

AFFECTED FILES
  autotests/kdirlistertest.cpp
  autotests/kdirlistertest.h

To: jtamate, #frameworks, dfaure
Cc: kde-frameworks-devel, apol, michaelh, ngraham, bruns

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.


  Thanks. Almost there :-)

INLINE COMMENTS

> kdirlistertest.cpp:658
> +QTRY_COMPARE(dirLister2.spyStarted.count(), 1);
>  QCOMPARE(dirLister2.spyCompleted.count(), 1);
>  QCOMPARE(dirLister2.spyCompletedQUrl.count(), 1);

*That* one should probably be a QTRY_COMPARE, since dirLister2 will first emit 
started, and then completed later.

> kdirlistertest.cpp:1121
> +QTRY_VERIFY(m_dirLister.isFinished());
> +QTRY_VERIFY(m_items.isEmpty());
>  

No TRY_ here.

> kdirlistertest.cpp:1197
> +QTRY_VERIFY(m_dirLister.isFinished());
> +QTRY_COMPARE(m_items.count(), 0);
> +QTRY_COMPARE(m_dirLister.spyItemsDeleted.count(), 1);

No TRY_ here

> kdirlistertest.cpp:1292
> +QTRY_COMPARE(m_dirLister.spyClear.count(), 1);
> +QTRY_COMPARE(m_dirLister.spyClearQUrl.count(), 0);
>  QList deletedUrls;

No TRY_ here, we can't "wait for nothing to happen" ;)

REPOSITORY
  R241 KIO

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

To: jtamate, #frameworks, dfaure
Cc: kde-frameworks-devel, apol, michaelh, ngraham, bruns

D11604: kdirlistertest doesn't fail at random

[Jaime Torres Amate]

jtamate updated this revision to Diff 39990.
jtamate marked 11 inline comments as done.
jtamate added a comment.


  Fixed the inline comments.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11604?vs=39836=39990

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

AFFECTED FILES
  autotests/kdirlistertest.cpp
  autotests/kdirlistertest.h

To: jtamate, #frameworks, dfaure
Cc: kde-frameworks-devel, apol, michaelh, ngraham, bruns

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::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

D11604: kdirlistertest doesn't fail at random

[Jaime Torres Amate]

jtamate updated this revision to Diff 39836.
jtamate marked 3 inline comments as done.
jtamate added a comment.


  Restored all the signalSpy wait wrongly deleted.
  Removed the use of qmimedatabase.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11604?vs=39801=39836

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

AFFECTED FILES
  autotests/kdirlistertest.cpp
  autotests/kdirlistertest.h

To: jtamate, #frameworks, dfaure
Cc: kde-frameworks-devel, apol, michaelh, ngraham, bruns

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:530
> +const QMimeType htmlType = 
> db.mimeTypeForUrl(QUrl(QLatin1String("file:///index.html")));
> +qDebug() << "html mimeType=" << htmlType;
> +

Should be removed -- or QCOMPAREd.

> kdirlistertest.cpp:926
>  dirLister.openUrl(QUrl::fromLocalFile(path));
> -QVERIFY(spyCompleted.wait(1000));
> -QVERIFY(spyCompleted.wait(1000));

We no longer verify that completed is emitted twice...

> kdirlistertest.cpp:963
>  QSignalSpy spyCompleted(, SIGNAL(completed()));
> -QVERIFY(spyCompleted.wait(1000));
> -QVERIFY(secondDirLister.isFinished());

why was this removed?

> kdirlistertest.cpp:1107
>  QSignalSpy spyCompleted(_dirLister, SIGNAL(completed()));
> -QVERIFY(spyCompleted.wait(1000));
> -QVERIFY(m_dirLister.isFinished());

same here and elsewhere

REPOSITORY
  R241 KIO

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

To: jtamate, #frameworks, dfaure
Cc: kde-frameworks-devel, apol, michaelh, ngraham, bruns

D11604: kdirlistertest doesn't fail at random

[Jaime Torres Amate]

jtamate updated this revision to Diff 39801.
jtamate edited the summary of this revision.
jtamate added a comment.
Herald added a subscriber: kde-frameworks-devel.


  Removed the use of enterLoop and exitLoop in favor of QTRY_COMPARE and 
QTRY_VERIFY.
  Removed some waits
  Changed all the connections to the new syntax.
  Have run it several times and it hasn't failed here.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11604?vs=33623=39801

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

AFFECTED FILES
  autotests/kdirlistertest.cpp
  autotests/kdirlistertest.h

To: jtamate, #frameworks, dfaure
Cc: kde-frameworks-devel, apol, michaelh, ngraham, bruns

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.


  Please add QStandardPaths::setTestModeEnabled(true) in initTestCase() so that 
your (broken, thank you WinE) locally defined mimetypes don't interfer with the 
test. Then "hardcoding" text/html will be fine again.

INLINE COMMENTS

> kdirlistertest.cpp:222
> +
> +qDebug() << "Creating new 100 files";
> +for (int i = 50; i > 0; i--) {

100 new files, you mean? ;)

> kdirlistertest.cpp:236
> +
> +QVERIFY(m_dirLister.spyStarted.count() < 3); // Updates call started, 
> probably twice
> +QVERIFY(m_dirLister.spyCompleted.count() < 3); // and completed, 
> probably twice

(so 0 would be fine?)

> kdirlistertest.cpp:516
> +
> +connect(_dirLister, SIGNAL(itemsDeleted(KFileItemList)), this, 
> SLOT(exitLoop()));
> +

with QTRY_* and/or QSignalSpy::wait(), the enterLoop/exitLoop old solution 
isn't really necessary anymore

> kdirlistertest.cpp:534
> +m_items.clear();
> +connect(_dirLister, SIGNAL(newItems(KFileItemList)), this, 
> SLOT(slotNewItems(KFileItemList)));
> +m_dirLister.openUrl(QUrl::fromLocalFile(path), KDirLister::NoFlags);

convert to new style connect, as you did for the other code you touched?

> kdirlistertest.cpp:608
>  if (m_refreshedItems.isEmpty()) {
> -QVERIFY(waitForRefreshedItems()); // refreshItems could come from 
> KDirWatch or KDirNotify.
> +QTRY_COMPARE(m_refreshedItems.isEmpty(), false);  // could come from 
> KDirWatch or KDirNotify.
>  }

QTRY_VERIFY(!m_refreshedItems.isEmpty());

(same for all other instances)

REPOSITORY
  R241 KIO

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

To: jtamate, #frameworks, dfaure
Cc: apol, michaelh, ngraham, bruns

D11604: kdirlistertest doesn't fail at random

[Jaime Torres Amate]

jtamate updated this revision to Diff 33623.
jtamate edited the summary of this revision.
jtamate added a comment.


  Add the new tests instead of replace.
  Use QTRY_COMPARE instead of custom loops.
  Reduce the number of files to 100.
  
  In my machine, the mime type of file.html is "application/x-extension-html"
  because of F5833260: file_associations.png 


REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11604?vs=30323=33623

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

AFFECTED FILES
  autotests/kdirlistertest.cpp
  autotests/kdirlistertest.h

To: jtamate, #frameworks, dfaure
Cc: apol, michaelh, bruns

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

D11604: kdirlistertest doesn't fail at random

[Jaime Torres Amate]

jtamate updated this revision to Diff 30323.
jtamate added a comment.


  QStringLiteral instead of QString.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11604?vs=30285=30323

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

AFFECTED FILES
  autotests/kdirlistertest.cpp
  autotests/kdirlistertest.h

To: jtamate, #frameworks, dfaure
Cc: apol, michaelh, ngraham

D11604: kdirlistertest doesn't fail at random

[Aleix Pol Gonzalez]

apol added inline comments.

INLINE COMMENTS

> kdirlistertest.cpp:194
> +for (int i = 0; i < 1000 ; i++) {
> +createSimpleFile(path + QString("toplevelfile_new_%1").arg(i));
> +}

Should be QStringLiteral("") instead of QString("").

REPOSITORY
  R241 KIO

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

To: jtamate, #frameworks, dfaure
Cc: apol, michaelh, ngraham

D11604: kdirlistertest doesn't fail at random

[Jaime Torres Amate]

jtamate created this revision.
jtamate added reviewers: Frameworks, dfaure.
Restricted Application added a project: Frameworks.
jtamate requested review of this revision.

REVISION SUMMARY
  - Instead of testing the creation and deletion of one file, do it with 1000 
files.
  - test the mime type with the on in the QMimeDatabase for an .html file.
  - increase some wait times.

TEST PLAN
  Repeating the test several times, it failed sometimes due to timing problems, 
doesn't fail any more for me.

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  autotests/kdirlistertest.cpp
  autotests/kdirlistertest.h

To: jtamate, #frameworks, dfaure
Cc: michaelh, ngraham