Details
- Reviewers
elvisangelaccio - Group Reviewers
Dolphin - Commits
- R824:9973f0a3ddf7: baloo-widgets: Add autotest for asychronously extracted data
Diff Detail
- Repository
- R824 Baloo Widgets
- Branch
- 3_test (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage
- 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 | |
167 | 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 ;) | |
214 | Same, don't call it value | |
223 | Same as above |
autotests/filemetadatawidgettest.cpp | ||
---|---|---|
229–230 | Oh, I forgot about this: we could use QEXPECT_FAIL here. |
autotests/filemetadatawidgettest.cpp | ||
---|---|---|
229–230 | Too late :-) |