Write unit tests for DownloadManager
ClosedPublic

Authored by alexkovrigin on Dec 9 2018, 5:01 PM.

Details

Summary

Write unit tests. QTest is used. Work in progress.

Test Plan
  • Build and run the app.
  • See the command line output with test results.

Diff Detail

Repository
R2 GCompris
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 6033
Build 6051: arc lint + arc unit
alexkovrigin created this revision.Dec 9 2018, 5:01 PM
Restricted Application added a project: KDE Edu. · View Herald TranscriptDec 9 2018, 5:01 PM
Restricted Application added a subscriber: kde-edu. · View Herald Transcript
alexkovrigin requested review of this revision.Dec 9 2018, 5:01 PM
jjazeix added inline comments.Dec 9 2018, 5:39 PM
src/core/CMakeLists.txt
28 ↗(On Diff #47190)

The "core" CMakeLists.txt should not be changed.
The tests have their own folder.

src/core/TestDownloadManager.cpp
1 ↗(On Diff #47190)

the license header is missing.
I think when this file will be in the tests/core folder, there won't be any problem including QTest

src/core/main.cpp
38 ↗(On Diff #47190)

this should not be in the src/core/main.cpp file which is used to run the application.
We don't want to "pollute" the real play with unit tests ;)

Move unit tests to a better place

Thank you for the tests.
Note that you can also use data driven tests to avoid duplicating the calls on each test: https://doc.qt.io/qt-5.11/qttestlib-tutorial2-example.html

This class is quite complicated to test as it is related to network. Also, it is conflicting with our actual installation (because if you download something during the test, it will be installed where it is when we run GCompris).
It's not intended and we need to avoid these side effects.
I think we can workaround this issue by creating a mock of the ApplicationSettings class (that stores all the settings of GCompris) to use a local one instead of the real configuration: https://github.com/gcompris/GCompris-qt/blob/master/tests/core/ApplicationSettingsMock.h. In the dummy_application_settings.conf, you can put settings that will be local to the test folder.

tests/core/DownloadManagerTest.cpp
9

it would be better to name it DownloadManagerTest to keep consistency with other tests classes

23

don't bother testing this function, I've tried to find better but didn't (for example, it would have been nice to have a function accessing the registered objects but I didn't find one)

31

this works because on Linux, we use ogg. But it will fail on macOS (as COMPRESSED_AUDIO will be aac) or Windows (mp3 is used).
Can you replace the QString("data2/voices-ogg/voices-en.rcc") with QString("data2/voices-%1/voices-en.rcc").arg(COMPRESSED_AUDIO)?

49

yes, according to the code, to return false, it would mean that either a download for the same file already started.

55

The easier way to have it return false, would be to do checkDownloadRestriction return true.
To do so, using the ApplicationSettingsMock (not the real one, it needs to be created at first before the DownloadManager object), and setting ApplicationSettings::getInstance()->isAutomaticDownloadsEnabled() to false.

73

why not using the "money.rcc" registered above?
Or better, register it on this function directly (to avoid dependencies between the order of the tests running).

76

no need to test this one, it will be too complicated to download voices and register them (and not wanted).

  • Modernize unit tests to data driven testing
alexkovrigin marked 6 inline comments as done.Dec 10 2018, 7:50 PM
alexkovrigin added inline comments.Dec 10 2018, 7:52 PM
tests/core/DownloadManagerTest.cpp
173

I've been debugging and trying to solve this issue for some time, but I couldn't find the solution.

subject->isDataRegistered(resource)

Always returns false.

Thank you for the unit test.
It may be easier to read if you have the data() then the corresponding test() function instead of having all the data() and at the end the test() functions (when you read one test, you'll just have to scroll above to know which data is tested)

tests/core/DownloadManagerTest.cpp
173

This function is a bit tricky and not really easy to test.
Basically, when we register an activity, we use the following path: "qrc:/gcompris/src/activities/{activity_name}".
The function isDataRegistered looks for data in "qrc:/gcompris/data" which is filled in only if we have registered a specific data file "words.rcc".
But, this file is not compiled and needs to be downloaded (~80Mo). We don't want this (because it will be downloaded at every commit).
If we want to test this function, I think we can try to mock the registerResource to "save" a fake data in the ":/gcompris/data" resource.
Not sure if it would work, but having a local qrc file containing one file in ":/gcompris/data/" and doing INIT_REGISTER_RESOURCE on the qrc. More info can be found in https://doc.qt.io/qt-5/resources.html (first and last paragraphs).

192

typo (Resource) + use const reference for passing arguments that won't be updated to avoid copies.

  • All the tests work now

Sorry for being late for one day...

tests/core/DownloadManagerTest.cpp
173

I think, I did it. Don't think it's the nicest solution, but it does its job

192

I removed those functions into the caller one.

jjazeix accepted this revision.Dec 16 2018, 1:08 PM

Thank you, it's commited: https://commits.kde.org/gcompris/41b4d43d5885f9350ada3b6c1b12549495fb828a
We are now above 50% of lines tested in GCompris (you can find the result in https://build.kde.org/job/Extragear/job/gcompris/job/kf5-qt5%20SUSEQt5.10/35/cobertura/!
I removed the 2 cases with register of test_isDataRegistered, as it did not work as expected.

This revision is now accepted and ready to land.Dec 16 2018, 1:08 PM
jjazeix requested changes to this revision.Dec 16 2018, 1:08 PM
This revision now requires changes to proceed.Dec 16 2018, 1:08 PM
jjazeix accepted this revision.Dec 16 2018, 1:08 PM
This revision is now accepted and ready to land.Dec 16 2018, 1:08 PM
jjazeix closed this revision.Dec 16 2018, 1:08 PM