<table><tr><td style="">jjazeix added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D17455">View Revision</a></tr></table><br /><div><div><p>Thank you for the tests.<br />
Note that you can also use data driven tests to avoid duplicating the calls on each test: <a href="https://doc.qt.io/qt-5.11/qttestlib-tutorial2-example.html" class="remarkup-link" target="_blank" rel="noreferrer">https://doc.qt.io/qt-5.11/qttestlib-tutorial2-example.html</a></p>
<p>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).<br />
It's not intended and we need to avoid these side effects.<br />
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: <a href="https://github.com/gcompris/GCompris-qt/blob/master/tests/core/ApplicationSettingsMock.h" class="remarkup-link" target="_blank" rel="noreferrer">https://github.com/gcompris/GCompris-qt/blob/master/tests/core/ApplicationSettingsMock.h</a>. In the dummy_application_settings.conf, you can put settings that will be local to the test folder.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D17455#inline-95778">View Inline</a><span style="color: #4b4d51; font-weight: bold;">DownloadManagerTest.cpp:8</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #aa4000">class</span> <span style="color: #00702a">TestDownloadManager</span> <span style="color: #aa2211">:</span> <span style="color: #aa4000">public</span> <span class="n">QObject</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">it would be better to name it DownloadManagerTest to keep consistency with other tests classes</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D17455#inline-95779">View Inline</a><span style="color: #4b4d51; font-weight: bold;">DownloadManagerTest.cpp:22</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">}</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">void</span> <span class="n">test_downloadManagerProvider</span><span class="p">()</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D17455#inline-95780">View Inline</a><span style="color: #4b4d51; font-weight: bold;">DownloadManagerTest.cpp:30</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QCOMPARE</span><span class="p">(</span><span class="n">subject</span><span style="color: #aa2211">-></span><span class="n">getVoicesResourceForLocale</span><span class="p">(</span><span style="color: #766510">"en_US"</span><span class="p">),</span> <span class="n">QString</span><span class="p">(</span><span style="color: #766510">"data2/voices-ogg/voices-en.rcc"</span><span class="p">));</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QCOMPARE</span><span class="p">(</span><span class="n">subject</span><span style="color: #aa2211">-></span><span class="n">getVoicesResourceForLocale</span><span class="p">(</span><span style="color: #766510">"en_UK"</span><span class="p">),</span> <span class="n">QString</span><span class="p">(</span><span style="color: #766510">"data2/voices-ogg/voices-en.rcc"</span><span class="p">));</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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).<br />
Can you replace the QString("data2/voices-ogg/voices-en.rcc") with QString("data2/voices-%1/voices-en.rcc").arg(COMPRESSED_AUDIO)?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D17455#inline-95784">View Inline</a><span style="color: #4b4d51; font-weight: bold;">DownloadManagerTest.cpp:48</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QVERIFY</span><span class="p">(</span><span class="n">subject</span><span style="color: #aa2211">-></span><span class="n">downloadResource</span><span class="p">(</span><span style="color: #766510">"algorithm.rcc"</span><span class="p">));</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QVERIFY</span><span class="p">(</span><span style="color: #aa2211">!</span><span class="n">subject</span><span style="color: #aa2211">-></span><span class="n">downloadResource</span><span class="p">(</span><span style="color: #766510">"invalid.blabla"</span><span class="p">));</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">yes, according to the code, to return false, it would mean that either a download for the same file already started.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D17455#inline-95785">View Inline</a><span style="color: #4b4d51; font-weight: bold;">DownloadManagerTest.cpp:54</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QVERIFY</span><span class="p">(</span><span class="n">subject</span><span style="color: #aa2211">-></span><span class="n">updateResource</span><span class="p">(</span><span style="color: #766510">"money.rcc"</span><span class="p">));</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QVERIFY</span><span class="p">(</span><span style="color: #aa2211">!</span><span class="n">subject</span><span style="color: #aa2211">-></span><span class="n">updateResource</span><span class="p">(</span><span style="color: #766510">"invalid.haha"</span><span class="p">));</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">The easier way to have it return false, would be to do checkDownloadRestriction return true.<br />
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.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D17455#inline-95786">View Inline</a><span style="color: #4b4d51; font-weight: bold;">DownloadManagerTest.cpp:72</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #74777d">// FIXME always results in false (probably, because nothing was registered yet)</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QVERIFY</span><span class="p">(</span><span class="n">subject</span><span style="color: #aa2211">-></span><span class="n">isDataRegistered</span><span class="p">(</span><span style="color: #766510">"words"</span><span class="p">));</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QVERIFY</span><span class="p">(</span><span style="color: #aa2211">!</span><span class="n">subject</span><span style="color: #aa2211">-></span><span class="n">isDataRegistered</span><span class="p">(</span><span style="color: #766510">"invalid"</span><span class="p">));</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">why not using the "money.rcc" registered above?<br />
Or better, register it on this function directly (to avoid dependencies between the order of the tests running).</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D17455#inline-95787">View Inline</a><span style="color: #4b4d51; font-weight: bold;">DownloadManagerTest.cpp:75</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">}</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">void</span> <span class="n">test_areVoicesRegistered</span><span class="p">()</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">no need to test this one, it will be too complicated to download voices and register them (and not wanted).</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R2 GCompris</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D17455">https://phabricator.kde.org/D17455</a></div></div><br /><div><strong>To: </strong>alexkovrigin, jjazeix<br /><strong>Cc: </strong>alexkovrigin, kde-edu, narvaez, apol<br /></div>