Details
- Reviewers
elvisangelaccio - Group Reviewers
Dolphin - Commits
- R824:9973f0a3ddf7: baloo-widgets: Add autotest for asychronously extracted data
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.
- filemetadatawidgettest: Use QSKIP instead of QEXPECT_FAIL
- Set objectName in createLabel()
| autotests/filemetadatawidgettest.cpp | ||
|---|---|---|
| 146 | Due to my locale this is "MP3-Audio" when testing in Konsole unless I do LC_ALL=C ctest. | |
FileMetadataWidgetTest::shouldShowProperties() is crashing for me.
Here's the backtrace:
Thread 1 "filemetadatawid" received signal SIGSEGV, Segmentation fault.
0x00007ffff4169b30 in QLabel::text() const () from /usr/lib/libQt5Widgets.so.5
(gdb) bt
#0 0x00007ffff4169b30 in QLabel::text() const () from /usr/lib/libQt5Widgets.so.5
#1 0x000055555555a7f8 in FileMetadataWidgetTest::shouldShowProperties()::$_3::operator()() const (this=0x555555819770)
at ../autotests/filemetadatawidgettest.cpp:148
#2 0x000055555555a446 in QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, FileMetadataWidgetTest::shouldShowProperties()::$_3>::call(FileMetadataWidgetTest::shouldShowProperties()::$_3&, void**) (f=..., arg=0x7fffffffc8b0)
at /usr/include/qt/QtCore/qobjectdefs_impl.h:130
#3 0x000055555555a411 in _ZN9QtPrivate7FunctorIZN22FileMetadataWidgetTest20shouldShowPropertiesEvE3$_3Li0EE4callINS_4ListIJEEEvEEvRS2_PvPS8_
(f=..., arg=0x7fffffffc8b0) at /usr/include/qt/QtCore/qobjectdefs_impl.h:240
#4 0x000055555555a3bd in _ZN9QtPrivate18QFunctorSlotObjectIZN22FileMetadataWidgetTest20shouldShowPropertiesEvE3$_3Li0ENS_4ListIJEEEvE4implEiPNS_15QSlotObjectBaseEP7QObjectPPvPb (which=1, this_=0x555555819760, r=0x555555862040, a=0x7fffffffc8b0, ret=0x0)
at /usr/include/qt/QtCore/qobjectdefs_impl.h:423
#5 0x00007ffff2b317ef in QMetaObject::activate(QObject*, int, int, void**) () from /usr/lib/libQt5Core.so.5
#6 0x00007ffff787f9c6 in Baloo::FileMetaDataWidget::metaDataRequestFinished (this=0x555555862040, _t1=...)
at src/KF5BalooWidgets_autogen/include/moc_filemetadatawidget.cpp:216
#7 0x00007ffff787f46a in Baloo::FileMetaDataWidget::Private::slotLoadingFinished (this=0x555555923760) at ../src/filemetadatawidget.cpp:126
#8 0x00007ffff78808f5 in Baloo::FileMetaDataWidget::qt_static_metacall (_o=0x555555862040, _c=QMetaObject::InvokeMetaMethod, _id=2,
_a=0x7fffffffca90) at src/KF5BalooWidgets_autogen/include/moc_filemetadatawidget.cpp:106
#9 0x00007ffff2b316c6 in QMetaObject::activate(QObject*, int, int, void**) () from /usr/lib/libQt5Core.so.5
#10 0x00007ffff7896572 in Baloo::FileMetaDataProvider::loadingFinished (this=0x5555558ee440)
at src/KF5BalooWidgets_autogen/EWIEGA46WW/moc_filemetadataprovider.cpp:170
#11 0x00007ffff7885a27 in Baloo::FileMetaDataProvider::slotFileFetchFinished (this=0x5555558ee440, job=0x555555809580)
at ../src/filemetadataprovider.cpp:146
#12 0x00007ffff78963af in Baloo::FileMetaDataProvider::qt_static_metacall (_o=0x5555558ee440, _c=QMetaObject::InvokeMetaMethod, _id=3,
_a=0x7fffffffccd0) at src/KF5BalooWidgets_autogen/EWIEGA46WW/moc_filemetadataprovider.cpp:91
#13 0x00007ffff2b316c6 in QMetaObject::activate(QObject*, int, int, void**) () from /usr/lib/libQt5Core.so.5
#14 0x00007ffff2fc265b in KJob::finished (this=0x555555809580, _t1=0x555555809580, _t2=...)
at src/lib/KF5CoreAddons_autogen/include/moc_kjob.cpp:548
#15 0x00007ffff2fc2846 in KJob::finishJob (this=0x555555809580, emitResult=true) at ../src/lib/jobs/kjob.cpp:106
#16 0x00007ffff2fc362a in KJob::emitResult (this=0x555555809580) at ../src/lib/jobs/kjob.cpp:293
#17 0x00007ffff7895b52 in Baloo::FileFetchJob::doStart (this=0x555555809580) at ../src/filefetchjob.cpp:86
#18 0x00007ffff7896131 in Baloo::FileFetchJob::qt_static_metacall (_o=0x555555809580, _c=QMetaObject::InvokeMetaMethod, _id=0,
_a=0x55555582cf30) at src/KF5BalooWidgets_autogen/EWIEGA46WW/moc_filefetchjob.cpp:71
#19 0x00007ffff2b32112 in QObject::event(QEvent*) () from /usr/lib/libQt5Core.so.5
#20 0x00007ffff402cfec in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /usr/lib/libQt5Widgets.so.5
#21 0x00007ffff40349c6 in QApplication::notify(QObject*, QEvent*) () from /usr/lib/libQt5Widgets.so.5| autotests/filemetadataitemcounttest.cpp | ||
|---|---|---|
| 26–38 | Please keep Qt includes and KF5 includes separated. | |
| 51–53 | The C++11 for loop should be used only on const containers, otherwise the container will detach. For example we could do: const auto keys = settings.keyList(); for (const auto &key : keys) { settings.writeEntry(key, true); } | |
| src/filemetadatawidget.cpp | ||
| 129–139 ↗ | (On Diff #27294) | Please make this function a member of FileMetaDataWidget::Private. |
| autotests/filemetadatawidgettest.cpp | ||
|---|---|---|
| 146 | We can use http://doc.qt.io/qt-5/qtglobal.html#qputenv | |
| autotests/filemetadatawidgettest.cpp | ||
|---|---|---|
| 146 | No luck with qputenv. Environment (LC_ALL or LANG) is set, but without effect. | |
| 158 | This should have been there anyway. | |
| src/filemetadatawidget.cpp | ||
|---|---|---|
| 129–139 ↗ | (On Diff #27294) | Avoid free functions in general or is there a specific reason? |
| autotests/filemetadatawidgettest.cpp | ||
|---|---|---|
| 146 | Weird, it should be possible (see for example kio/autotests/favicontest.cpp line 86). How did you try to do it? | |
| src/filemetadatawidget.cpp | ||
| 129–139 ↗ | (On Diff #27294) | In this case because we have the private class where we can put things. In general free functions are fine, but they should be within the anonymous namespace for clarity. |
Crash is gone, but now I get two failures:
FAIL! : FileMetadataWidgetTest::shouldShowProperties() 'value' returned FALSE. (albumArtist data was not found)
Loc: [../autotests/filemetadatawidgettest.cpp(151)]
PASS : FileMetadataWidgetTest::shouldShowCommonProperties()
PASS : FileMetadataWidgetTest::cleanupTestCase()
Totals: 7 passed, 1 failed, 0 skipped, 0 blacklisted, 175ms
********* Finished testing of FileMetadataWidgetTest *********
Start 4: filemetadataitemcounttest
4/4 Test #4: filemetadataitemcounttest ........***Failed 0.30 sec
********* Start testing of FileMetadataItemCountTest *********
Config: Using QtTest library 5.10.1, Qt 5.10.1 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 7.3.0)
PASS : FileMetadataItemCountTest::initTestCase()
FAIL! : FileMetadataItemCountTest::testItemCount() Compared values are not the same
Actual (items.count()) : 16
Expected (expectedItems * widgetsPerItem): 38
Loc: [../autotests/filemetadataitemcounttest.cpp(73)]| src/filemetadatawidget.cpp | ||
|---|---|---|
| 72 ↗ | (On Diff #27464) | Pass by const reference also itemLabel. |
| 120 ↗ | (On Diff #27464) | Isn't this already called in slotDataAvailable() ? |
| 130 ↗ | (On Diff #27464) | code style: opening brace should go to the next line |
| 152 ↗ | (On Diff #27464) | This information should probably go in the documentation of filter() in metadatafilter.h |
| 159 ↗ | (On Diff #27464) | Same detaching issue as before. Don't remove the const QStringList keys = sortedKeys(data); line and we'll be fine. |
WTR tests failing on your machine.
16 Items, that is the number of synchronously extracted items + labels. Apparently there are no data pulled out of the testfiles.
Probably more tweaking of baloo config has to be done.
I don't quite understand this, though. On my machine all tests pass regardless of initial baloo config and with file indexing enabled or disabled.
Could you post ~/.config/baloofilerc and ~/.config/baloofileinforc of your test environment somewhere?
Also, if possible, could you please visually inspect the testfiles with infopanel or tooltips for properties like channels or bitrate?
I also re-checked my test environment running the dolphin binary in the debugger. I can set break points in filemetadatawidget so it is used.
| autotests/filemetadatawidgettest.cpp | ||
|---|---|---|
| 146 | Nearly like in kio/autotests/favicontest.cpp. I tried
in initTestCase() and some other places. en_US.UTF-8 does not work either. | |
| src/filemetadatawidget.cpp | ||
| 72 ↗ | (On Diff #27464) | We can't, we have to append a colon to the string. |
| 120 ↗ | (On Diff #27464) | The answer is no. But it made me think. |
| 152 ↗ | (On Diff #27464) | Marked it with a FIXME. |
| 129–139 ↗ | (On Diff #27294) | Thanks |
My best guess for the failing tests is a missing extractor lib. I'm fiddling around with find_package and the like taking kfilemetadata as example, but without success so far.
| autotests/filemetadataitemcounttest.cpp | ||
|---|---|---|
| 52 | I finally could reproduce the test fails (Had to delete ~/qttest. This works for me now. | |
We are doing to many things at once here. I suggest to split this diff.
- Remove dataavailable signal (affected: FileMetaDataWidget and FileMetaDataProvider)
- Use object names (affected: FileMetaDataWidget and WidgetFactory)
- Base this diff on 1) and 2)
But first the tests have to pass for you too.
Tests now pass for me! But I agree that splitting this diff would be good.
| autotests/filemetadataitemcounttest.cpp | ||
|---|---|---|
| 52 | I'm not sure there is a way to add escaped entries from the API. I think the users are supposed to manually write $e in their config files. But I could be wrong. | |
Ok, plenty of chances to mess with arc again. :-)
| autotests/filemetadataitemcounttest.cpp | ||
|---|---|---|
| 52 | That comment is outdated. See comment in code. .writePath() does it correctly, but refuses to add empty entries. We should stick with` .writeEntry() instead of .writePath("/tmp")` or similar. | |
Hmm, needs more rebase?
$ arc patch D10149 INFO Base commit is not in local repository; trying to fetch. Created and checked out branch arcpatch-D10149. Created and checked out branch arcpatch-D10833. Checking patch src/filemetadatawidget.h... Checking patch src/filemetadatawidget.cpp... Checking patch src/filemetadataprovider.h... Checking patch src/filemetadataprovider.cpp... Applied patch src/filemetadatawidget.h cleanly. Applied patch src/filemetadatawidget.cpp cleanly. Applied patch src/filemetadataprovider.h cleanly. Applied patch src/filemetadataprovider.cpp cleanly. Cherry Pick Failed! Exception Command failed with error #1! COMMAND git cherry-pick 'arcpatch-D10833' STDOUT (empty) STDERR error: could not apply 8c143f1... Remove dataavailable signal 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/filemetadataitemcounttest.cpp | ||
|---|---|---|
| 76–77 | const | |
- Copy src/ from master
- Merge branch 'master' of git://anongit.kde.org/baloo-widgets into 3_test
git checkout master arc patch D10149 --nobranch
should tell you whether the latest revision applies fine on master.
| autotests/filemetadataitemcounttest.cpp | ||
|---|---|---|
| 78–88 | Actually I don't understand why there is a lambda here. Imho we should test how many signals we emit (should be only 1, right?), and then check the number of items: QVERIFY(spy.wait()); QCOMPARE(spy.count(), 1); QList<QWidget*> items = m_widget->findChildren<QWidget*>(QString(), Qt::FindDirectChildrenOnly); QCOMPARE(items.count(), expectedItems * widgetsPerItem); | |
| autotests/filemetadatawidgettest.cpp | ||
| 70 | Why commas at the beginning of lines? This is usually done only in constructor initializer lists | |
| 172 | Same here. How many signals do we expect? Do we really need the lambda? | |
| autotests/filemetadataitemcounttest.cpp | ||
|---|---|---|
| 78–88 | I don't either. Historical reasons, maybe? | |
Almost there!
| autotests/filemetadatawidgettest.cpp | ||
|---|---|---|
| 150–151 | Can be merged in a single line. Btw this is a QLabel so I'd call it label or maybe valueLabel. Just value can be confusing. | |
| 157 | Please call it ratingWidget. That will fix the weird rating->rating() line below ;) | |
| 180 | Same, don't call it value | |
| 189 | Same as above | |
| autotests/filemetadatawidgettest.cpp | ||
|---|---|---|
| 195–196 | Oh, I forgot about this: we could use QEXPECT_FAIL here. | |
| autotests/filemetadatawidgettest.cpp | ||
|---|---|---|
| 195–196 | Too late :-) | |