⚙ D8630 Created unit test for PlacesItemModel

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 added a reviewer: ervin.Nov 2 2017, 8:04 PM

Update unit test

rebase from origin

Created more unit tests

ngraham added a subscriber: ngraham.Nov 16 2017, 8:26 PM

Removed commented code

Use Kio::KPlacesModel as source model for PlacesItemModel

Updated unit test

renatoo added a dependent revision: D8855: Use Kio::KPlacesModel as source model for PlacesItemModel.Nov 16 2017, 8:43 PM

renatoo edited the summary of this revision. (Show Details)

renatoo added a dependent revision: D8862: Extend KFilePlacesModel API.Nov 17 2017, 12:22 PM

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

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.

lgtm, some minor nits only from my side

src/tests/placesitemmodeltest.cpp
44 ↗	(On Diff #22512)	why not QDir rootPath with toNativeSeparators?
151 ↗	(On Diff #22512)	can you instead use a signal spy and wait for some signal? or use QTRY_COMPARE below?
478 ↗	(On Diff #22512)	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
44 ↗	(On Diff #22512)	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.
151 ↗	(On Diff #22512)	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.

Use a new function to avoid duplicated code

Rebased with upstream

mwolff added inline comments.Nov 23 2017, 1:30 PM

src/tests/placesitemmodeltest.cpp
44 ↗	(On Diff #22512)	leave it as is then, but I don't see any reason for this code
151 ↗	(On Diff #22512)	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
151 ↗	(On Diff #22512)	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

lgtm from my side

@elvisangelaccio any comments?

@elvisangelaccio is this good now?

lgtm too

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 ↗	(On Diff #22512)	QFileInfo::exists would be faster

Added missing file, necessary for unit test

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 ↗	(On Diff #22512)	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.

Move test data file

renatoo marked an inline comment as done.Nov 27 2017, 2:48 PM

Thanks!

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.

			Path	Packages
M			src/tests/CMakeLists.txt (6 lines)
A	M		src/tests/placesitemmodeltest.cpp (532 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	21803	76698ff		Nov 2 2017, 8:03 PM	★	★
Diff 2	21804	76698ff	Update unit test	Nov 2 2017, 8:10 PM	★	★
Diff 3	21805	d1acb1d	rebase from origin	Nov 2 2017, 8:13 PM	★	★
Diff 4	22481	d1acb1d	Created more unit tests	Nov 16 2017, 6:51 PM	★	★
Diff 5	22484	d1acb1d	Removed commented code	Nov 16 2017, 8:29 PM	★	★
Diff 6	22485	d1acb1d	- Use Kio::KPlacesModel as source model for PlacesItemModel	Nov 16 2017, 8:38 PM	★	★
Diff 7	22486	d1acb1d	Updated unit test	Nov 16 2017, 8:40 PM	★	★
Diff 8	22512	d1acb1d	Fixed code style	Nov 17 2017, 1:13 PM	★	★
Diff 9	22645	d1acb1d	Use a new function to avoid duplicated code	Nov 20 2017, 1:13 PM	★	★
Diff 10	22823	a993485	Rebased with upstream	Nov 23 2017, 1:26 PM	★	★
Diff 11	23001	a993485	Added missing file, necessary for unit test	Nov 27 2017, 11:54 AM	★	★
Diff 12	23029	a993485	Move test data file	Nov 27 2017, 2:45 PM	★	★
Diff 13	23038	11e2b18	R318:89e4e8dd0a589c99e451e9f0aa4855e5720652ad	Nov 27 2017, 4:37 PM	★	★

Created unit test for PlacesItemModel
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 22645

src/tests/CMakeLists.txt

src/tests/placesitemmodeltest.cpp

Created unit test for PlacesItemModelClosedPublicActions