Review Request: SoK - Unit Test : core/collections/QueryMaker

[Matěj Laitl]

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


The test is generally OK, but please address/answer following concerns.


tests/core/collections/MockQueryMaker.h
<http://git.reviewboard.kde.org/r/105172/#comment11884>

    I gues you're going to use MockQueryMaker in multiple tests?



tests/core/collections/MockQueryMaker.h
<http://git.reviewboard.kde.org/r/105172/#comment11880>

    Code style: pointer is next to variable, not next to type:
    virtual QueryMaker *setQueryType( ... )



tests/core/collections/MockQueryMaker.h
<http://git.reviewboard.kde.org/r/105172/#comment11881>

    In tests, it is ethical to have:
    virtual QueryMaker *setQueryType( QueryType )
    if you don't use the type so that Q_UNUSED() is not needed. This makes the mock shorter.



tests/core/collections/MockQueryMaker.h
<http://git.reviewboard.kde.org/r/105172/#comment11882>

    I would name this "emitQueryDone()"



tests/core/collections/MockQueryMaker.cpp
<http://git.reviewboard.kde.org/r/105172/#comment11885>

    If you don't want to put MockQueryMaker implementation into a .cpp file (understandable), this .cpp is not needed at all.



tests/core/collections/MockQueryMaker.cpp
<http://git.reviewboard.kde.org/r/105172/#comment11883>

    No need to include moc. if the Q_OBJECT is in .h file.



tests/core/collections/TestQueryMaker.cpp
<http://git.reviewboard.kde.org/r/105172/#comment11886>

    Code style: include order:
    1. own header
    2. amaork headers
    3. kde headers (qtest_kde.h)
    4. Qt headers
    5. possibly other headers.



tests/core/collections/TestQueryMaker.cpp
<http://git.reviewboard.kde.org/r/105172/#comment11887>

    Oh oh oh oh! Why the assignment? Why the dynamic_cast? Do don't need to assing to m_moctQueryMaker at all!


- Matěj Laitl


On June 23, 2012, 7:05 p.m., Jasneet Bhatti wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105172/
> -----------------------------------------------------------
> 
> (Updated June 23, 2012, 7:05 p.m.)
> 
> 
> Review request for Amarok, Matěj Laitl and Sven Krohlas.
> 
> 
> Description
> -------
> 
> Implements unit test for the QueryMaker class in core/collections/QueryMaker
> 
> 
> Diffs
> -----
> 
>   tests/core/collections/CMakeLists.txt b01b655 
>   tests/core/collections/MockQueryMaker.h PRE-CREATION 
>   tests/core/collections/MockQueryMaker.cpp PRE-CREATION 
>   tests/core/collections/TestQueryMaker.h PRE-CREATION 
>   tests/core/collections/TestQueryMaker.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/105172/diff/
> 
> 
> Testing
> -------
> 
> Test builds and runs fine
> 
> 
> Thanks,
> 
> Jasneet Bhatti
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20120626/0c7463f3/attachment.html>