Differential D8630

Created unit test for PlacesItemModel
ClosedPublic
Actions

Authored by renatoo on Nov 2 2017, 8:03 PM.

Details

Reviewers
ervin
elvisangelaccio
mwolff
Commits
R318:89e4e8dd0a58: Created unit test for PlacesItemModel
Summary

Make sure that PlacesItemModel is covered by unit test

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
renatoo created this revision.Nov 2 2017, 8:03 PM
Restricted Application added a subscriber: Dolphin. · View Herald TranscriptNov 2 2017, 8:03 PM
renatoo updated this revision to Diff 21804.Nov 2 2017, 8:10 PM

Update unit test

renatoo updated this revision to Diff 21805.Nov 2 2017, 8:13 PM

rebase from origin

renatoo updated this revision to Diff 22481.Nov 16 2017, 6:51 PM

Created more unit tests

renatoo updated this revision to Diff 22484.Nov 16 2017, 8:29 PM

Removed commented code

renatoo updated this revision to Diff 22485.Nov 16 2017, 8:39 PM
  • Use Kio::KPlacesModel as source model for PlacesItemModel
renatoo updated this revision to Diff 22486.Nov 16 2017, 8:40 PM

Updated unit test

elvisangelaccio requested changes to this revision.Nov 17 2017, 12:47 PM
elvisangelaccio added a subscriber: elvisangelaccio.
elvisangelaccio added inline comments.
src/tests/placesitemmodeltest.cpp
115

nitpick: space after for

116–117

nitpick: missing braces

158

nullptr

171

Wrap it in a QVERIFY

204

Please call it systemRoot.

218

const

221–228

.at() instead of [] ?

271

QUrl::fromLocalFile ?

275

Please call it ejectAction

278

Please call it action or teardownAction

280

What if the action text is translated?

336

Please call it properties or viewProperties

401

space after for

This revision now requires changes to proceed.Nov 17 2017, 12:47 PM
renatoo updated this revision to Diff 22512.Nov 17 2017, 1:13 PM
renatoo marked 13 inline comments as done.

Fixed code style

src/tests/placesitemmodeltest.cpp
280

Since to safety check for the action label we will need to setup the translate system, I choose to remove this check. Since in the line above we already check if the action exists.

mwolff requested changes to this revision.Nov 20 2017, 11:35 AM
mwolff added a subscriber: mwolff.

lgtm, some minor nits only from my side

src/tests/placesitemmodeltest.cpp
45

why not QDir rootPath with toNativeSeparators?

152

can you instead use a signal spy and wait for some signal? or use QTRY_COMPARE below?

479

extract this into a helper function to simplify the code, it's repeated in most test functions

This revision now requires changes to proceed.Nov 20 2017, 11:35 AM
renatoo marked 2 inline comments as done.Nov 20 2017, 1:04 PM
renatoo added inline comments.
src/tests/placesitemmodeltest.cpp
45

This is based on KIO unit test: https://github.com/KDE/kio/blob/master/autotests/kfileplacesmodeltest.cpp#L34

Not sure the reason for that. But I can change it to use QDir as requested if you think that will not cause any problem.

152

I tried all models signals, and none is fired.
The problem is on placesitemmodel.cpp:101, that loads the bookmark data while on idle and we must wait for that to finish before start any test.

renatoo updated this revision to Diff 22645.Nov 20 2017, 1:13 PM
renatoo marked an inline comment as done.

Use a new function to avoid duplicated code

renatoo updated this revision to Diff 22823.Nov 23 2017, 1:26 PM

Rebased with upstream

mwolff added inline comments.Nov 23 2017, 1:30 PM
src/tests/placesitemmodeltest.cpp
45

leave it as is then, but I don't see any reason for this code

152

ok, but we could still use QTRY_COMPARE then, no?

renatoo marked 2 inline comments as done.Nov 23 2017, 2:04 PM
renatoo added inline comments.
src/tests/placesitemmodeltest.cpp
152

QTRY_COMPARE is not enough the model already has the correct size but the bookmark objects could be changed on the function executed while in idle.

In any case this is fixed on D8332

mwolff accepted this revision.Nov 23 2017, 3:16 PM

lgtm from my side

renatoo marked an inline comment as done.Nov 23 2017, 3:19 PM

@elvisangelaccio any comments?

ervin accepted this revision.Nov 27 2017, 7:44 AM

lgtm too

elvisangelaccio added a comment.Nov 27 2017, 10:43 AM

I'm getting this failure when running the test:

********* Start testing of PlacesItemModelTest *********
Config: Using QtTest library 5.10.0, Qt 5.10.0 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 7.2.0)
WARNING: PlacesItemModelTest::initTestCase() testdata fakecomputer.xml could not be located!
   Loc: [../src/tests/placesitemmodeltest.cpp(177)]
FAIL!  : PlacesItemModelTest::initTestCase() '!fakeHw.isEmpty()' returned FALSE. ()
   Loc: [../src/tests/placesitemmodeltest.cpp(178)]
PASS   : PlacesItemModelTest::cleanupTestCase()
Totals: 1 passed, 1 failed, 0 skipped, 0 blacklisted, 0ms

Am I missing something?

src/tests/placesitemmodeltest.cpp
184

QFileInfo::exists would be faster

renatoo updated this revision to Diff 23001.Nov 27 2017, 11:55 AM

Added missing file, necessary for unit test

renatoo marked an inline comment as done.Nov 27 2017, 11:56 AM
In D8630#172367, @elvisangelaccio wrote:

I'm getting this failure when running the test:

********* Start testing of PlacesItemModelTest *********
Config: Using QtTest library 5.10.0, Qt 5.10.0 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 7.2.0)
WARNING: PlacesItemModelTest::initTestCase() testdata fakecomputer.xml could not be located!
   Loc: [../src/tests/placesitemmodeltest.cpp(177)]
FAIL!  : PlacesItemModelTest::initTestCase() '!fakeHw.isEmpty()' returned FALSE. ()
   Loc: [../src/tests/placesitemmodeltest.cpp(178)]
PASS   : PlacesItemModelTest::cleanupTestCase()
Totals: 1 passed, 1 failed, 0 skipped, 0 blacklisted, 0ms

Am I missing something?

Sorry I forgot to upload xml file necessary for unit test.

elvisangelaccio added inline comments.Nov 27 2017, 1:36 PM
src/tests/placesitemmodeltest.cpp
178

Can you please move the xml file into a data/ subfolder and use QFINDTESTDATA("data/fakecomputer.xml") here?

Other than that, looks good to me now.

renatoo updated this revision to Diff 23029.Nov 27 2017, 2:45 PM

Move test data file

renatoo marked an inline comment as done.Nov 27 2017, 2:48 PM
This revision is now accepted and ready to land.Nov 27 2017, 3:04 PM
Closed by commit R318:89e4e8dd0a58: Created unit test for PlacesItemModel (authored by Renato Araujo Oliveira Filho <renato.araujo@kdab.com>, committed by ngraham). · Explain WhyNov 27 2017, 4:37 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 23038

View Options

src/tests/CMakeLists.txt

Loading...
View Options

src/tests/data/fakecomputer.xml

Loading...
View Options

src/tests/placesitemmodeltest.cpp

Loading...