----------------------------------------------------------- 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 > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
