D10950: GComprisActivityInfoTest

[Johnny Jazeix]

jjazeix added a comment.


  Thank you for your contribution, it's a really nice patch (most of the remarks are minor ones).
  
  c++ files are compiled twice (one for the lib, one for the executable). I remembered I had issues (crashes on android) when I tried to link the executable to the library (https://github.com/gcompris/GCompris-qt/blob/networkClient/src/core/CMakeLists.txt#L66 on this branch).
  
  Do you think it can be applied to activities too (as the code is mostly qml and js, it may be more difficult to unit tests them, making automatic tests more viable and interesting to avoid regressions)?

INLINE COMMENTS

> CMakeLists.txt:462
> +# Tell CMake to run moc when necessary:
> +set(CMAKE_AUTOMOC ON)
> +

if you use automoc, can you remove the qt5_wrap_cpp in the src/core folder and check if it still works?
You may need to move this line higher on the file to also apply to src subdirectory

> CMakeLists.txt:466
> +# to always look for includes there:
> +set(CMAKE_INCLUDE_CURRENT_DIR ON)
> +add_subdirectory(tests)

same here, we used include_directories() in the src/core, it may not be needed anymore with this.

> CMakeLists.txt:80
> +
> +add_library(GCompris SHARED ${gcompris_SRCS} ${gcompris_MOC} ${gcompris_RES})
> +target_link_libraries(GCompris

I would name it "gcompris_core" instead of "GCompris" and I would not add "gcompris_RES" to it as they are the resource files and so not linked to the library itself but the executable

> CMakeLists.txt:3
> +
> +include_directories(
> +    ${CMAKE_SOURCE_DIR}/src/core

it would be preferable to not include the folders and use #include "src/core/ActivityInfo.h" instead (so we know where to look for when we look for the file).

Also, you probably don't need the resource include

> CMakeLists.txt:14
> +
> +add_executable(GComprisActivityInfoTest activityinfotest.cpp)
> +target_link_libraries(GComprisActivityInfoTest ${CORE_TEST_LIBRARIES})

it would be better to prefix it with Core instead of GCompris. Basically, all the tests will be for GCompris so no need to add it but we may have different modules (activities, core...).

> activityinfotest.cpp:1
> +#include <QtTest>
> +#include <QObject>

missing licence header

> activityinfotest.cpp:2
> +#include <QtTest>
> +#include <QObject>
> +

filename should be ActivityInfoTest.cpp

> activityinfotest.cpp:6
> +
> +Q_DECLARE_METATYPE(unsigned int);
> +

why do you need this?

> activityinfotest.cpp:41
> +{
> +	ActivityInfo* activityinfo = new ActivityInfo();
> +	QCOMPARE(activityinfo->name(), QStringLiteral(""));

do you need a pointer here?

> activityinfotest.cpp:73
> +
> +	QSignalSpy spyName(activityinfo, &ActivityInfo::nameChanged);
> +	QVERIFY(spyName.count() == 0);

I think you can create a macro that will can simplify the tests:

  #define TEST_ATTRIBUTE(attributeName, accessorName, attributeType) \
  { \
    QFETCH(attributeType, attributeName); \
    QSignalSpy spy(activityinfo, &ActivityInfo::attributeName ## Changed); \
    QVERIFY(spy.count() == 0); \
    activityinfo->set ## accessorName(attributeName); \
    QVERIFY(spy.count() == 1); \
    QCOMPARE(activityinfo->##accessorName(), attributeName); \
  }

and be used for all attributes?

REPOSITORY
  R2 GCompris

REVISION DETAIL
  https://phabricator.kde.org/D10950

To: himanshuvishwakarma, #gcompris, jjazeix, dmadaan, rudranilbasu, timotheegiet
Cc: #kde_edu, #gcompris, harrymecwan, ganeshredcobra, nityanandkumar, echarruau, rahulyadav, narvaez, scagarwal, apol, timotheegiet, hkaelberer, jjazeix, bcoudoin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20180301/a0950c2a/attachment-0001.html>