baloo-widgets: Create test to assert metaDataRequestFinished is emitted once only
ClosedPublic

Authored by michaelh on Jan 26 2018, 4:26 PM.

Details

Diff Detail

Repository
R824 Baloo Widgets
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes

Made a mistake: updated D10113 with a diff supposed to update this.

michaelh planned changes to this revision.Jan 28 2018, 12:09 PM
michaelh updated this revision to Diff 26119.Jan 28 2018, 12:23 PM

Applied suggested changes

autotests/filemetadatawidgettest.cpp
27

We can #include <QSignalSpy> :)

michaelh updated this revision to Diff 26123.Jan 28 2018, 1:25 PM

Include <QSignalSpy>

This revision is now accepted and ready to land.Jan 28 2018, 2:20 PM
autotests/filemetadatawidgettest.cpp
74

(Current diff looks wrong)

This could be simplified with: QStringLiteral("%1/test.mp3").arg(TESTS_SAMPLE_FILES_PATH). Same below.

michaelh planned changes to this revision.Jan 29 2018, 2:10 PM
michaelh added inline comments.
autotests/filemetadatawidgettest.cpp
74

(Current diff looks wrong)

You're right, it is wrong.
Also since

Diff 3	26119	130ab80

the series of commits looks disrupted.

I'll sort it out when D10113 is accepted, because this branch is following it

michaelh updated this revision to Diff 26221.Jan 30 2018, 7:09 PM
  • Merge branch 'signalavailable' into unittest
  • Add test for empty file list
  • autotests:Simplify path construction
This revision is now accepted and ready to land.Jan 30 2018, 7:09 PM
michaelh added a comment.EditedJan 30 2018, 7:10 PM

Both test for empty failed because the spy timed out!
IMO they should not, because consumers rely on the metaDataRequestFinished signal.
Please tell me if you disagree.

I don't know why they fail, yet. See D10113

Patch doesn't apply here, can you update it?

$ arc patch D10119
 INFO  Base commit is not in local repository; trying to fetch.
Branch name arcpatch-D10119 already exists; trying a new name.
Created and checked out branch arcpatch-D10119_1.
Created and checked out branch arcpatch-D10143.
Checking patch src/filemetadataprovider.cpp...
Applied patch src/filemetadataprovider.cpp cleanly.

 Cherry Pick Failed!
 Exception 
Command failed with error #1!
COMMAND
git cherry-pick 'arcpatch-D10143'

STDOUT
(empty)

STDERR
error: could not apply 4c0d4f8... baloo-widgets: Apply coding style to filemetadataprovider
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'

(Run with `--trace` for a full exception trace.)
autotests/filemetadatawidgettest.cpp
34

Please remove this commented line.

michaelh updated this revision to Diff 26487.Feb 4 2018, 10:57 AM
michaelh updated this revision to Diff 26488.Feb 4 2018, 11:09 AM
  • Remove obsolete comment

arc patch D10119 still fails with the same error. Don't know why it is D10143 that fails. Hints welcome.

Anyway, I think it's best to do this

  • wait for KDE Applications 17.12.2 release.
  • then land D10113
  • fix the history of this one
  • create new diff for filemetadataprovider to make it pass these tests

error: could not apply 88a775b... baloo-widgets: Apply coding style to filemetadataprovider

It seems this is a local commit in your feature branch? Have you tried to rebase this branch on top of master?

michaelh updated this revision to Diff 26557.Feb 5 2018, 8:18 AM
  • Add test for empty file list
  • autotests:Simplify path construction
  • Remove obsolete comment
  • Merge branch 'signalavailable' into unittest
  • Make it compile
michaelh updated this revision to Diff 26560.Feb 5 2018, 8:39 AM
  • Adjust whitespace

This is the best I could accomplish to get arc patch D10119 working, sorry.

  • I had to resolve 35 conflicts during rebase to master.
  • After that the diff looked like this one. (Duplication of changes in D10113)
  • Then:
$ git status
Auf Branch unittest
Ihr Branch und 'signalavailable' sind divergiert,
und haben jeweils 26 und 19 unterschiedliche Commits.
  (benutzen Sie "git pull", um Ihren Branch mit dem Remote-Branch zusammenzuführen)

nichts zu committen, Arbeitsverzeichnis unverändert
  • $ git pull ( Resolved conflicts)
  • arc diff
  • ... same error during arc patch D10119
  • $ git checkout -b rebaseunittest b58d7b66bbf213eb15672d7ffa5346fa31c7243d (commit before doing $ git pull)
  • Manually adjusted two blank lines
  • arc diff => this diff

Connection to D10113 is lost, but least this one compiles after 'arc patch D10119` and the tests have the expect result (FAIL with empty list).

michaelh marked 8 inline comments as done.Feb 6 2018, 11:59 AM

@michaelh Why is D10113 not pushed yet? You need to rebase this patch on top of master *after* you push D10113.

michaelh updated this revision to Diff 26751.Feb 8 2018, 9:10 AM
  • Add test for empty file list
  • autotests:Simplify path construction
  • Remove obsolete comment
  • Make it compile
  • Reinsert lost curly brace

@elvisangelaccio: D10113 currently fails shouldSignalOnceWithoutFile() and shouldSignalOnceWithEmptyFile() for reasons I don't understand. The correct code path is taken, but no signal is emitted in src/filemetadatawidget.cpp:119
I hadn't pushed D10113 because I was waiting for your comment on this.

elvisangelaccio requested changes to this revision.Feb 8 2018, 9:20 PM

@elvisangelaccio: D10113 currently fails shouldSignalOnceWithoutFile() and shouldSignalOnceWithEmptyFile() for reasons I don't understand. The correct code path is taken, but no signal is emitted in src/filemetadatawidget.cpp:119
I hadn't pushed D10113 because I was waiting for your comment on this.

Ah sorry, I hadn't realized this.

autotests/filemetadatawidgettest.cpp
63

From the documentation of QSignalSpy::wait():

Starts an event loop that runs until the given signal is received. Optionally the event loop can return earlier on a timeout (in milliseconds).

Returns true if the signal was emitted at least once in timeout milliseconds, otherwise returns false.

The signal IS emitted, but before you create the nested event loop. So wait() returns false because "no signal was emitted at least once in 500 ms".

Just remove this QVERIFY(), we don't need it.

This revision now requires changes to proceed.Feb 8 2018, 9:20 PM
michaelh added a comment.EditedFeb 8 2018, 11:04 PM

Treated all test equally: QVERIFY(spy.wait(xxx)); -> spy.wait(xxx)
shouldSignalOnceWithoutFile() and shouldSignalOnceWithEmptyFile() still fail, the others pass.

Just to see what would happen I changed filemetadataprovider.cpp:465 to

KFileItemList FileMetaDataProvider::items() const
{
    return m_fileItems.isEmpty() 
        ? KFileItemList() << QUrl(QStringLiteral("file:///"))
        : m_fileItems;
}

Now all tests pass.

The crux must be in build/kde/applications/baloo-widgets/src/KF5BalooWidgets_autogen/include/moc_filemetadatawidget.cpp:

// SIGNAL 1
void Baloo::FileMetaDataWidget::metaDataRequestFinished(const KFileItemList & _t1)
{
    void *_a[] = { nullptr, const_cast<void*>(reinterpret_cast<const void*>(&_t1)) };
    QMetaObject::activate(this, &staticMetaObject, 1, _a);
}

I can't read this, but hopefully you can tell why there is no signal when _t1 is an empty list.

autotests/filemetadatawidgettest.cpp
63

Exactly that was the reason I had put this QEXPECT_FAIL in.
To understand how spy.count() can ever be > 1, will take a lot of reading and thinking.

michaelh updated this revision to Diff 26801.Feb 8 2018, 11:08 PM
  • Unwrap spy.wait() calls
elvisangelaccio requested changes to this revision.Feb 9 2018, 8:50 PM

Treated all test equally: QVERIFY(spy.wait(xxx)); -> spy.wait(xxx)
shouldSignalOnceWithoutFile() and shouldSignalOnceWithEmptyFile() still fail, the others pass.

Just to see what would happen I changed filemetadataprovider.cpp:465 to

KFileItemList FileMetaDataProvider::items() const
{
    return m_fileItems.isEmpty() 
        ? KFileItemList() << QUrl(QStringLiteral("file:///"))
        : m_fileItems;
}

Now all tests pass.

The crux must be in build/kde/applications/baloo-widgets/src/KF5BalooWidgets_autogen/include/moc_filemetadatawidget.cpp:

// SIGNAL 1
void Baloo::FileMetaDataWidget::metaDataRequestFinished(const KFileItemList & _t1)
{
    void *_a[] = { nullptr, const_cast<void*>(reinterpret_cast<const void*>(&_t1)) };
    QMetaObject::activate(this, &staticMetaObject, 1, _a);
}

I can't read this, but hopefully you can tell why there is no signal when _t1 is an empty list.

Sorry but I'm not following you. The metaDataRequestFinished() signal is emitted, as the QCOMPARE(spy.count(), 1) test proves.
The first two tests are currently failing for another reason:

FAIL!  : FileMetadataWidgetTest::shouldSignalOnceWithoutFile() Compared values are not the same
   Actual   (m_widget->items().count()): 0
   Expected (1)                        : 1
   Loc: [../autotests/filemetadatawidgettest.cpp(64)]

and:

FAIL!  : FileMetadataWidgetTest::shouldSignalOnceWithEmptyFile() Compared values are not the same
   Actual   (m_widget->items().count()): 0
   Expected (1) 
   Loc: [../autotests/filemetadatawidgettest.cpp(74)]

(see inline comments below).

autotests/filemetadatawidgettest.cpp
63

I meant that this line should be removed. The test passes even without this nested event loop.

65

Why are you expecting one element in items()? You are passing a list with no valid local urls to setItems(), so we don't append anything to localItemsList. Just replace the 1 with a 0 and the test will pass.

73

remove

75

same as above, 0 rather than 1

85

Here we do need the nested event loop (because metaDataRequestFinished is emitted insied a slot).

So please wrap it inside a QVERIFY().

99

QVERIFY() also here

This revision now requires changes to proceed.Feb 9 2018, 8:50 PM
michaelh updated this revision to Diff 26846.Feb 9 2018, 9:50 PM
michaelh marked 6 inline comments as done.
  • Apply requested changes
michaelh marked 2 inline comments as done.Feb 9 2018, 9:53 PM
michaelh added inline comments.
autotests/filemetadatawidgettest.cpp
65

You're right. I didn't expect that but overlooked that line all the time, stupid.

Almost there, just coding style issues now :)

autotests/filemetadatawidgettest.cpp
26

#include <KFileItem> (and remove the duplicate include above)

32

This comment does not add much, please remove it

34

Not needed

39

Not needed

42–45

empty function -> remove it

(initTestCase() and cleanupTestCase() definitions are not mandatory).

49

This comment does not add much, please remove it

55

This comment does not add much, please remove it

autotests/filemetadatawidgettest.h
26

Please move this include in the .cpp file

27

Not needed

29

Not needed

52

Please remove

michaelh updated this revision to Diff 26847.Feb 9 2018, 10:48 PM
michaelh marked 5 inline comments as done.
  • Remove obsolete comments and includes
michaelh updated this revision to Diff 26848.Feb 9 2018, 10:53 PM
  • Moved include to .cpp
michaelh updated this revision to Diff 26850.Feb 9 2018, 10:58 PM
michaelh marked an inline comment as done.
  • More cleanup
michaelh marked 5 inline comments as done.Feb 9 2018, 11:03 PM

Thank you for your patience.

autotests/filemetadatawidgettest.cpp
39
********* Start testing of FileMetadataWidgetTest *********
Config: Using QtTest library 5.10.0, Qt 5.10.0 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 7.3.0)
PASS   : FileMetadataWidgetTest::initTestCase()
QWARN  : FileMetadataWidgetTest::shouldSignalOnceWithoutFile() QSignalSpy: Unable to handle parameter 'items' of type 'KFileItemList' of method 'metaDataRequestFinished', use qRegisterMetaType to register it.
42–45
filemetadatawidgettest_autogen/EWIEGA46WW/moc_filemetadatawidgettest.cpp:97: undefined reference to `FileMetadataWidgetTest::cleanupTestCase()'
collect2: error: ld returned 1 exit status

My inline comments do not appear where they should.

  • Deleting cleanupTestCase() gave a linker error.
  • Deleting qRegisterMetaType 4 Debug warnings

My inline comments do not appear where they should.

  • Deleting cleanupTestCase() gave a linker error.

Did you also remove the declaration in filemetadatawidgettest.h?

  • Deleting qRegisterMetaType 4 Debug warnings

Weird, works here without it. Oh well, let's put it then (better safe than sorry).

michaelh updated this revision to Diff 26853.Feb 9 2018, 11:26 PM
  • Remove cleanupTestCase()
  • Remove white space
michaelh marked 2 inline comments as done.Feb 9 2018, 11:28 PM

Did you also remove the declaration in filemetadatawidgettest.h?

Good question :)

autotests/filemetadatawidgettest.cpp
89

Please remove the moc include. It is only needed when a Q_OBJECT class is declared in a .cpp file.

autotests/filemetadatawidgettest.h
26

Not done, #include "config.h" should be in the .cpp file (since we only need it for the TESTS_SAMPLE_FILES_PATH macro).

michaelh updated this revision to Diff 26881.Feb 10 2018, 5:08 PM
  • Correct includes
michaelh added inline comments.Feb 10 2018, 5:13 PM
autotests/filemetadatawidgettest.cpp
89

Thanks, your explanations are very helpful. From the examples I saw I got the impression including the moc was sort of "conventional".

This revision is now accepted and ready to land.Feb 10 2018, 5:26 PM
This revision was automatically updated to reflect the committed changes.