D17455: Write unit tests for DownloadManager

Johnny Jazeix noreply at phabricator.kde.org
Sun Dec 9 20:51:44 GMT 2018

jjazeix added a comment.

  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.


> DownloadManagerTest.cpp:8
> +
> +class TestDownloadManager : public QObject
> +{

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

> DownloadManagerTest.cpp:22
> +    }
> +    void test_downloadManagerProvider()
> +    {

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)

> DownloadManagerTest.cpp:30
> +    {
> +        QCOMPARE(subject->getVoicesResourceForLocale("en_US"), QString("data2/voices-ogg/voices-en.rcc"));
> +        QCOMPARE(subject->getVoicesResourceForLocale("en_UK"), QString("data2/voices-ogg/voices-en.rcc"));

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

> DownloadManagerTest.cpp:48
> +        QVERIFY(subject->downloadResource("algorithm.rcc"));
> +        QVERIFY(!subject->downloadResource("invalid.blabla"));
> +    }

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

> DownloadManagerTest.cpp:54
> +        QVERIFY(subject->updateResource("money.rcc"));
> +        QVERIFY(!subject->updateResource("invalid.haha"));
> +    }

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.

> DownloadManagerTest.cpp:72
> +        // FIXME always results in false (probably, because nothing was registered yet)
> +        QVERIFY(subject->isDataRegistered("words"));
> +        QVERIFY(!subject->isDataRegistered("invalid"));

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

> DownloadManagerTest.cpp:75
> +    }
> +    void test_areVoicesRegistered()
> +    {

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

  R2 GCompris


To: alexkovrigin, jjazeix
Cc: alexkovrigin, kde-edu, narvaez, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20181209/8c92879f/attachment-0001.html>

More information about the kde-edu mailing list