Review Request: SoK Unit test : core/collections/Collection

[Jasneet Bhatti]

> On June 21, 2012, 2:08 p.m., Matěj Laitl wrote:
> >
> 
> Matěj Laitl wrote:
>     Collection has really just 3 implemented methods. CollectionBase shouldn't exist, because it really is Meta::MetaCapability. This should be changed by some Amarok dev as soon as 2.6 is released. Then the change is done, CollectionBase methods will be already tested in MetaCapability, so you don't have to test them here. So I suggest you remove the tests for is<>() and create<>(), too.

Ok. Will remove the unnecessary ones.


> On June 21, 2012, 2:08 p.m., Matěj Laitl wrote:
> > tests/core/collections/TestCollection.h, line 27
> > <http://git.reviewboard.kde.org/r/105166/diff/2/?file=70024#file70024line27>
> >
> >     Strange indentation. Don't you mix tabs and spaces, jasneet?

Actually, I had some code from the previous version of this test and some new, so consistency problems with the indentation. Will correct these.


> On June 21, 2012, 2:08 p.m., Matěj Laitl wrote:
> > tests/core/collections/TestCollection.cpp, line 34
> > <http://git.reviewboard.kde.org/r/105166/diff/2/?file=70025#file70025line34>
> >
> >     Strange indentation again. Hmm?

Don't understand this one.
There are two levels of indentation and the variable definition is according to intro_and_style.
Please explain what is the fault.


> On June 21, 2012, 2:08 p.m., Matěj Laitl wrote:
> > tests/core/collections/TestCollection.cpp, line 203
> > <http://git.reviewboard.kde.org/r/105166/diff/2/?file=70025#file70025line203>
> >
> >     While entire code shouldn't be here, I wonder why you just dont
> >     QCOMPARE( collection->usedCapacity(), 0.0 )? You really want to pretend static_cast doesn't exist and use it only in at a least resort.

0.0 is a double and usedCapacity returns a float, so there was a warning on compilation. Used the cast to eliminated the warning.

http://qt-project.org/doc/qt-4.8/qtest.html#QCOMPARE
"In the case of comparing floats and doubles, qFuzzyCompare() is used for comparing. This means that comparing to 0 will likely fail. One solution to this is to compare to 1, and add 1 to the produced output."

I'm not sure if the above applies for 0.0 too, so added 1 to not take a chance.


> On June 21, 2012, 2:08 p.m., Matěj Laitl wrote:
> > tests/core/collections/TestCollection.cpp, line 228
> > <http://git.reviewboard.kde.org/r/105166/diff/2/?file=70025#file70025line228>
> >
> >     No no no, you should actually test that isWritable() returns what location()->isWritable() returns. To do this you can subclass your CollectionMock to return CollectionLocationMock where you'll cause different isWritable() values to be returned and verified.
> >     
> >     You can use http://qt-project.org/doc/qt-4.8/qtestlib-tutorial2.html + implement TestCollection::testIsWritable_data(). Then you can use QFETCH(bool, isWritable); in CollectionLocationMock.

Working on it now.


> On June 21, 2012, 2:08 p.m., Matěj Laitl wrote:
> > tests/core/collections/TestCollection.cpp, line 237
> > <http://git.reviewboard.kde.org/r/105166/diff/2/?file=70025#file70025line237>
> >
> >     Same as comment for testIsWritable()

Working on it now.


- Jasneet


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105166/#review14948
-----------------------------------------------------------


On June 21, 2012, 1:38 p.m., Jasneet Bhatti wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105166/
> -----------------------------------------------------------
> 
> (Updated June 21, 2012, 1:38 p.m.)
> 
> 
> Review request for Amarok and Sven Krohlas.
> 
> 
> Description
> -------
> 
> This patch implements a unit test for core/collections/Collection
> 
> There are abstract classes to be tested as well, which can only be done when subclasses define the pure virtual functions. So tests for those will be done along with the subclasses.
> 
> 
> Diffs
> -----
> 
>   tests/core/collections/CMakeLists.txt 2efd1fe 
>   tests/core/collections/TestCollection.h PRE-CREATION 
>   tests/core/collections/TestCollection.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/105166/diff/
> 
> 
> Testing
> -------
> 
> Test passes on my repository
> 
> 
> Thanks,
> 
> Jasneet Bhatti
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20120621/735ac1f6/attachment-0001.html>