CoreActivityInfoTest
ClosedPublic

Authored by himanshuvishwakarma on Mar 1 2018, 4:34 PM.

Details

Summary

This is the first unit test of the GCompris. The unit test is testing the main class ActivityInfo in the core which is testing the most of the function of the class.

Diff Detail

Repository
R2 GCompris
Lint
Lint Skipped
Unit
Unit Tests Skipped
Restricted Application added a project: KDE Edu. · View Herald TranscriptMar 1 2018, 4:34 PM
Restricted Application added a subscriber: KDE Edu. · View Herald Transcript
himanshuvishwakarma requested review of this revision.Mar 1 2018, 4:34 PM

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)?

CMakeLists.txt
462

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

466

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

src/core/CMakeLists.txt
80

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

tests/core/CMakeLists.txt
4

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

15

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...).

tests/core/activityinfotest.cpp
1 ↗(On Diff #28340)

missing licence header

2 ↗(On Diff #28340)

filename should be ActivityInfoTest.cpp

6 ↗(On Diff #28340)

why do you need this?

41 ↗(On Diff #28340)

do you need a pointer here?

73 ↗(On Diff #28340)

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?

himanshuvishwakarma retitled this revision from GComprisActivityInfoTest to CoreActivityInfoTest.

make the change in the autotest

himanshuvishwakarma marked 4 inline comments as done.

Pointer is removed

himanshuvishwakarma marked 2 inline comments as done.Mar 2 2018, 7:09 PM
himanshuvishwakarma added inline comments.
CMakeLists.txt
462

During the writing the test, I don't want to touch the core part of the application. Today I tried to remove the qt5_warp_cpp but it makes complex to handle both the test and application. Maybe I am doing wrong way also. I will try more to make compiled the c++ code once.

tests/core/activityinfotest.cpp
6 ↗(On Diff #28340)

I just added by mistake

41 ↗(On Diff #28340)

No, Really no needed of the pointer. It is in my practice. Now pointer is removed

jjazeix accepted this revision.Mar 3 2018, 10:48 AM

There are some issues when compiling for android (missing libs). I'm fixing them and I'll commit

This revision is now accepted and ready to land.Mar 3 2018, 10:48 AM

Thanks for considering my patch.