D11034: CoreApplicationSettingsTest

[Johnny Jazeix]

jjazeix added inline comments.

INLINE COMMENTS

> CMakeLists.txt:47
>  
> -  option(BUILD_TESTING "Build and enable unit tests" OFF)
> +  option(BUILD_TESTING "Build and enable unit tests" ON)
>    include(ECMCoverageOption)

it was done on purpose to not build the tests by default

> ApplicationSettings.h:471
> +protected:
> +    explicit ApplicationSettings(QObject *parent, QString path);
> +

you can delegate constructors in c++11.
Use const reference for attributes.
The previous code was good, only one constructor with default values:
explicit ApplicationSettings(const QString &path = "...", QObject *parent = 0);

> ApplicationSettingsTest.cpp:37
> +
> +class DummyApplicationSettings : public ApplicationSettings
> +{

can you create new files (cpp, h) for this mock (ApplicationSettingsMock), it can be used on other tests?
There are other files that uses the singleton ApplicationSettings

REPOSITORY
  R2 GCompris

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

To: himanshuvishwakarma, jjazeix, dmadaan, rudranilbasu, timotheegiet, #gcompris
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/20180321/40ecf17a/attachment.html>