Write unit tests. QTest is used. Work in progress.
Details
- Reviewers
jjazeix - Maniphest Tasks
- T9608: unit test for DownloadManager
- 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 5904 Build 5922: arc lint + arc unit
src/core/CMakeLists.txt | ||
---|---|---|
28 ↗ | (On Diff #47190) | The "core" CMakeLists.txt should not be changed. |
src/core/TestDownloadManager.cpp | ||
1 ↗ | (On Diff #47190) | the license header is missing. |
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. |
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). | |
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. | |
73 | why not using the "money.rcc" registered above? | |
76 | no need to test this one, it will be too complicated to download voices and register them (and not wanted). |
tests/core/DownloadManagerTest.cpp | ||
---|---|---|
172 | 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 | ||
---|---|---|
172 | This function is a bit tricky and not really easy to test. | |
191 | typo (Resource) + use const reference for passing arguments that won't be updated to avoid copies. |
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.