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

[Matěj Laitl]

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



tests/core/collections/TestCollection.h
<http://git.reviewboard.kde.org/r/105166/#comment11730>

    Strange indentation. Don't you mix tabs and spaces, jasneet?



tests/core/collections/TestCollection.h
<http://git.reviewboard.kde.org/r/105166/#comment11732>

    Hmm, I wonder why CollectionBase exists at all, it is just MetaCapability reimplemented. This is not a problem with the test, but with actual implementation.



tests/core/collections/TestCollection.h
<http://git.reviewboard.kde.org/r/105166/#comment11734>

    abstract (implemented for convenience) method, should not be tested.



tests/core/collections/TestCollection.h
<http://git.reviewboard.kde.org/r/105166/#comment11735>

    abstract (implemented for convenience) method, should not be tested.



tests/core/collections/TestCollection.h
<http://git.reviewboard.kde.org/r/105166/#comment11736>

    abstract (implemented for convenience) method, should not be tested.



tests/core/collections/TestCollection.h
<http://git.reviewboard.kde.org/r/105166/#comment11737>

    abstract (implemented for convenience) method, should not be tested.



tests/core/collections/TestCollection.h
<http://git.reviewboard.kde.org/r/105166/#comment11738>

    abstract (implemented for convenience) method, should not be tested.



tests/core/collections/TestCollection.cpp
<http://git.reviewboard.kde.org/r/105166/#comment11733>

    Strange indentation again. Hmm?



tests/core/collections/TestCollection.cpp
<http://git.reviewboard.kde.org/r/105166/#comment11739>

    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.



tests/core/collections/TestCollection.cpp
<http://git.reviewboard.kde.org/r/105166/#comment11742>

    While not strictly needed in tests, it is good habit to write code without memory leaks. Here you would "delete collection;", because it doesn't delete itself unless prepareSomething() is called on it.



tests/core/collections/TestCollection.cpp
<http://git.reviewboard.kde.org/r/105166/#comment11740>

    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.



tests/core/collections/TestCollection.cpp
<http://git.reviewboard.kde.org/r/105166/#comment11741>

    Same as comment for testIsWritable()


- Matěj Laitl


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/3f9f3e7b/attachment-0001.html>