D17455: Write unit tests for DownloadManager

[Johnny Jazeix]

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.

INLINE COMMENTS

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

REPOSITORY
  R2 GCompris

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

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>