CoreApplicationSettingsTest
ClosedPublic

Authored by himanshuvishwakarma on Mar 4 2018, 9:26 PM.

Details

Summary

The unit test is added of class ApplicationSettings

Diff Detail

Repository
R2 GCompris
Lint
Lint Skipped
Unit
Unit Tests Skipped
Restricted Application added a project: KDE Edu. · View Herald TranscriptMar 4 2018, 9:26 PM
Restricted Application added a subscriber: KDE Edu. · View Herald Transcript
himanshuvishwakarma requested review of this revision.Mar 4 2018, 9:26 PM
jjazeix added inline comments.Mar 5 2018, 6:59 AM
tests/core/ApplicationSettingsTest.cpp
93

applicationSettings

94

It would be better to override the class with a constructor that initialize these fields.
The most important part is that on this file, we are writing on the configuration. You also need to override it to not change the real configuration (https://github.com/gcompris/GCompris-qt/blob/ba315944567ada9440b95f798887dbdc408074e6/src/core/ApplicationSettings.cpp#L92)

It would be nice to also test the content of the configuration file at the end of the function to be sure all the content has been written as expected.

jjazeix edited the summary of this revision. (Show Details)Mar 5 2018, 7:00 AM
jjazeix added a reviewer: GCompris.
jjazeix requested changes to this revision.Mar 10 2018, 11:47 AM
This revision now requires changes to proceed.Mar 10 2018, 11:47 AM
himanshuvishwakarma marked an inline comment as done.
himanshuvishwakarma added inline comments.
tests/core/ApplicationSettingsTest.cpp
94

Yes, it changed the settings of the application. So As per your recommendation, I have created a DummyApplicationSettings class in the unit test and inherited from ApplicationSettings class and in the constructor of the class I override and initialized these fields with NULL,
but unable to override the real configuration because the m_config is in the private member. And NO any other method is coming to me to override real configuration (https://github.com/gcompris/GCompris-qt/blob/ba315944567ada9440b95f798887dbdc408074e6/src/core/ApplicationSettings.cpp#L92)

please give me any solution, link to tutorial or blog regarding the same.

One another thing we may introduce in our GCompris is RestoreDefaultSettings(A member function of class ApplicationSettings) in the settings section by which the user can restore all default settings.

tests/core/ApplicationSettingsTest.cpp
41

try to make a separate config file for the test which does not affect the application settings. But not get success. Any help or suggestion ??

jjazeix added inline comments.Mar 10 2018, 1:56 PM
tests/core/ApplicationSettingsTest.cpp
41

I see 2 potential solutions:
either inheritance and protected
or
friend keyword.

I let you investigate a bit more to look for them and choose :).
(I don't like the 3rd solution which would be "#define private public" in the test file)

50

you don't have to redefine a new attribute with the same name, it won't do what you expect and it does not correspond to the one in the parent class

62

this one is never freed so it leaks, don't use pointer when you don't need them :)
And don't use the same object shared between different tests, there might be side effects.

143

variable names should start with a lowercase letter

jjazeix requested changes to this revision.Mar 10 2018, 1:58 PM
This revision now requires changes to proceed.Mar 10 2018, 1:58 PM
himanshuvishwakarma marked 5 inline comments as done.

Now the unit test is not changing the default configuration of the application.

tests/core/ApplicationSettingsTest.cpp
41

Yaa, I tried both methods but both of them compelled to change in class ApplicationSettings. which we have to test. So I find a nice approach by this our default configuration is not going to change.

In this method we are first saving the default data(which is already present in the config file ), then test the unit test with our data and again reset the default data.

In this approach, we don't need to make the dummy class for the testing and not to change in the ApplicationSettings class.

now everything is working fine.

50

Okay, next time this will not happen and now NO need of this.

62

here also, I remember it for the next time.

143

here also, I remember it for the next time.

jjazeix added inline comments.Mar 11 2018, 4:16 PM
tests/core/ApplicationSettingsTest.cpp
41

What happens if for some reason the test crashes while there is a change done?
Also, you are still adding dummy things in the real configuration that has nothing to do with the real application.
You should not touch real application data when doing tests.

45

indentation should be 4 spaces on c++ files.

tests/core/CMakeLists.txt
15

can you do a macro or function to avoid duplicating these 3 lines everytime?

yes, I agree with you that if the test is crash during the running it will change the default settings of the application.

So, I think that the solution is that, I don't have to use the main settings( ~/.config/gcompris/gcompris-qt.conf ) of the application in our unit test. To achieve this, I tried with making DummyApplicationSettings class by inheriting the ApplicationSettings class and change access specifiers of m_config to protected and then tried to change the config file location but there is NO any function in the QSettings to change the location of the config file. So dummy class also changed the main settings of the application while running the test.

The main problem is NO way to change the default location of the config file in the object of QSettings class.

maybe wrong approach.
or
maybe some other method may present to do the tests. but presently I have no idea what to do next in this test, I am searching... if anything I will get, I will update the diff.

tests/core/CMakeLists.txt
15

Nice Idea ;-)
I will update in the next diff.

jjazeix added inline comments.Mar 12 2018, 5:58 PM
tests/core/ApplicationSettingsTest.cpp
2

ApplicationSettingsTest.cpp

tests/core/CMakeLists.txt
15

You can take a look at: https://api.kde.org/ecm/module/ECMAddTests.html, it may be interesting

yes, I agree with you that if the test is crash during the running it will change the default settings of the application.

So, I think that the solution is that, I don't have to use the main settings( ~/.config/gcompris/gcompris-qt.conf ) of the application in our unit test. To achieve this, I tried with making DummyApplicationSettings class by inheriting the ApplicationSettings class and change access specifiers of m_config to protected and then tried to change the config file location but there is NO any function in the QSettings to change the location of the config file. So dummy class also changed the main settings of the application while running the test.

The main problem is NO way to change the default location of the config file in the object of QSettings class.

maybe wrong approach.
or
maybe some other method may present to do the tests. but presently I have no idea what to do next in this test, I am searching... if anything I will get, I will update the diff.

If you need to add some protected methods to change the settings, you can

Now, I removed the part which is changing the default settings of the GCompris

yes, I agree with you that if the test is crash during the running it will change the default settings of the application.

So, I think that the solution is that, I don't have to use the main settings( ~/.config/gcompris/gcompris-qt.conf ) of the application in our unit test. To achieve this, I tried with making DummyApplicationSettings class by inheriting the ApplicationSettings class and change access specifiers of m_config to protected and then tried to change the config file location but there is NO any function in the QSettings to change the location of the config file. So dummy class also changed the main settings of the application while running the test.

The main problem is NO way to change the default location of the config file in the object of QSettings class.

maybe wrong approach.
or
maybe some other method may present to do the tests. but presently I have no idea what to do next in this test, I am searching... if anything I will get, I will update the diff.

If you need to add some protected methods to change the settings, you can

I tried many times, but not getting any positive results.

In the derived class(DummyApplicationSettings) inherits the object of the QSettings (m_config) after that I used this function to change the path of the config file location: http://doc.qt.io/qt-5/qsettings.html#setPath
But this function doesn't affect existing QSettings objects.

Therefore I removed that part of the unit test which is affecting the settings of GCompris because aqpplication settings are more important than a unit test.

yes, I agree with you that if the test is crash during the running it will change the default settings of the application.

So, I think that the solution is that, I don't have to use the main settings( ~/.config/gcompris/gcompris-qt.conf ) of the application in our unit test. To achieve this, I tried with making DummyApplicationSettings class by inheriting the ApplicationSettings class and change access specifiers of m_config to protected and then tried to change the config file location but there is NO any function in the QSettings to change the location of the config file. So dummy class also changed the main settings of the application while running the test.

The main problem is NO way to change the default location of the config file in the object of QSettings class.

maybe wrong approach.
or
maybe some other method may present to do the tests. but presently I have no idea what to do next in this test, I am searching... if anything I will get, I will update the diff.

If you need to add some protected methods to change the settings, you can

I tried many times, but not getting any positive results.

In the derived class(DummyApplicationSettings) inherits the object of the QSettings (m_config) after that I used this function to change the path of the config file location: http://doc.qt.io/qt-5/qsettings.html#setPath
But this function doesn't affect existing QSettings objects.

Therefore I removed that part of the unit test which is affecting the settings of GCompris because aqpplication settings are more important than a unit test.

So your solution for unit testing this class would be to test nothing (the remaining tests you do also change the configuration file) :)?
If you add a protected constructor taking the path as parameter and use it in to instantiate the inherited class, would it work?

Is this possible solution can be acceptable ??

I tried your solution i.e. add a protected constructor taking the path as parameter and use it in to instantiate the inherited class
But adding a constructor give the ambiguity error in this line https://github.com/gcompris/GCompris-qt/blob/master/src/core/ApplicationSettings.h#L263

I am doing the mistake in the adding a protected constructor so, it gives the ambiguity error.

Now, I add a protected constructor taking the path as parameter and use it in to instantiate the inherited class and it working fine.

I also add a function: getConstructor() in private to the class ApplicationSettings because the same code is repeating again and again in the constructor.

jjazeix added inline comments.Mar 21 2018, 8:00 PM
CMakeLists.txt
47 ↗(On Diff #30148)

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

src/core/ApplicationSettings.h
471 ↗(On Diff #30148)

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

tests/core/ApplicationSettingsTest.cpp
38

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

the created new header file (ApplicationSettingsMock)

removed the protected constructor, added a additional path parameter in the constructor.

Can you check that if you instantiate a ApplicationSettingsMock object, then use ApplicationSettings::getInstance(), it returns the mock instead of creating a new ApplicationSettings object?
As we use it this way on other classes (like ApplicationInfo), it would be better that, for tests, the getInstance() returns the mock instead of the real one (else we'll have the same issue as we had here, writing on the real configuration).

Except this point (and the small one regarding parameters ordering), it seems good to me, I'll take a closer look this week-end to integrate it

src/core/ApplicationSettings.h
13 ↗(On Diff #30179)

let's keep the same format as Qt and set the parent object as last input parameter of the constructor

257 ↗(On Diff #30179)

let's keep the same format as Qt and set the parent object as last input parameter of the constructor

Change in the order of the parameters, Make the QObject paramete in last.

himanshuvishwakarma added a comment.EditedMar 23 2018, 10:10 AM

Can you check that if you instantiate a ApplicationSettingsMock object, then use ApplicationSettings::getInstance(), it returns the mock instead of creating a new ApplicationSettings object?
As we use it this way on other classes (like ApplicationInfo), it would be better that, for tests, the getInstance() returns the mock instead of the real one (else we'll have the same issue as we had here, writing on the real configuration).

Except this point (and the small one regarding parameters ordering), it seems good to me, I'll take a closer look this week-end to integrate it

Hi,

I have checked that it the when we use the ApplicationSettings::getInstance( ), after the instantiate object of ApplicationSettingsMock. It will not disturb the main config file anyway. It will disturb only when the object of ApplicationSettingsMock is not destroyed, that is the object of ApplicationSettingsMock is existing and then we call the function ApplicationSettings::getInstance( )

I have checked it by this methods

  1. Change in the file ApplicationSettingsTest.cpp like this: https://paste.kde.org/pkfzgqgtx

Run the test.
Result: Function ApplcationSettingsTest will reflect the change in the file build/test/core/DummyApplicationSettingsTest.conf && Function ActivitySettingsTest will change in the file ~/.config/gcompris/gcompris-qt.conf

  1. Change in the file ApplicationSettingsTest.cpp like this: https://paste.kde.org/ps9i5vfdg

Run the test
Result: Now the result is reverse as expected i.e. Function ApplcationSettingsTest will reflect the change in the file: ~/.config/gcompris/gcompris-qt.conf && Function ActivitySettingsTest will change in the file: build/test/core/DummyApplicationSettingsTest.conf

Correct me if I am wrong...

jjazeix accepted this revision.Mar 24 2018, 6:38 PM
src/core/ApplicationSettings.cpp
89 ↗(On Diff #30290)

const reference

src/core/ApplicationSettings.h
34 ↗(On Diff #30290)

you should not redefine it here but use the one on config.h
It may be different than gcompris-qt on some OS

tests/core/ApplicationSettingsMock.h
37 ↗(On Diff #30290)

In file included from tests/core/ApplicationSettingsTest.cpp:26:0:
tests/core/ApplicationSettingsMock.h: In constructor 'ApplicationSettingsMock::ApplicationSettingsMock()':
tests/core/ApplicationSettingsMock.h:38:31: warning: passing NULL to non-pointer argument 1 of 'void ApplicationSettings::setPreviousHeight(qint32)' [-Wconversion-null]

setPreviousHeight(NULL);
                      ^

tests/core/ApplicationSettingsMock.h:39:30: warning: passing NULL to non-pointer argument 1 of 'void ApplicationSettings::setPreviousWidth(qint32)' [-Wconversion-null]

setPreviousWidth(NULL);
                     ^

tests/core/ApplicationSettingsMock.h:40:29: warning: passing NULL to non-pointer argument 1 of 'void ApplicationSettings::setBaseFontSize(int)' [-Wconversion-null]

setBaseFontSize(NULL);
This revision is now accepted and ready to land.Mar 24 2018, 6:38 PM
jjazeix closed this revision.Mar 24 2018, 6:41 PM

Can you check that if you instantiate a ApplicationSettingsMock object, then use ApplicationSettings::getInstance(), it returns the mock instead of creating a new ApplicationSettings object?
As we use it this way on other classes (like ApplicationInfo), it would be better that, for tests, the getInstance() returns the mock instead of the real one (else we'll have the same issue as we had here, writing on the real configuration).

Except this point (and the small one regarding parameters ordering), it seems good to me, I'll take a closer look this week-end to integrate it

Hi,

I have checked that it the when we use the ApplicationSettings::getInstance( ), after the instantiate object of ApplicationSettingsMock. It will not disturb the main config file anyway.

Yes, it does, the getInstance by default will get the static object m_instance. Even if the mock is created before, the m_instance is still null when calling getInstance and thus it is instanciated to a simple ApplicationSettings which will write on the main config file instead of the dummy one.
I added a getInstance in the mock to create the m_instance as a Mock.

It will disturb only when the object of ApplicationSettingsMock is not destroyed, that is the object of ApplicationSettingsMock is existing and then we call the function ApplicationSettings::getInstance( )

I have checked it by this methods

  1. Change in the file ApplicationSettingsTest.cpp like this: https://paste.kde.org/pkfzgqgtx Run the test. Result: Function ApplcationSettingsTest will reflect the change in the file build/test/core/DummyApplicationSettingsTest.conf && Function ActivitySettingsTest will change in the file ~/.config/gcompris/gcompris-qt.conf
  2. Change in the file ApplicationSettingsTest.cpp like this: https://paste.kde.org/ps9i5vfdg Run the test Result: Now the result is reverse as expected i.e. Function ApplcationSettingsTest will reflect the change in the file: ~/.config/gcompris/gcompris-qt.conf && Function ActivitySettingsTest will change in the file: build/test/core/DummyApplicationSettingsTest.conf

    Correct me if I am wrong...