[amarok] src: Fix DynamicModel test

[Ralf Engels]

Hi,

the problem with our test running on Jenkins are that we have still less 
stuff that we are currently assuming.
For example phonon might not really work as expected.

Also not we get a big hit from our habit of creating the singleton 
objects in the Amarok object.
The comments for e.g. Amarok::Components::logger should state the fact 
that you are only allowed to
call this function after the initialization of an Amarok::Amarok object.

Also during the last versions we have added another couple of 
KMessageBox calls to indicate severe problems. However on Jenkins a 
stray creation of a QWidget is currently blocking five test cases.

On 07/12/2012 12:44 PM, ext Matěj Laitl wrote:
> On 11. 7. 2012 Ralf Engels wrote:
>> Git commit 0066525f8d299a22509818e3814eafd08202945d by Ralf Engels.
>> Committed on 10/07/2012 at 11:03.
>> Pushed by rengels into branch 'master'.
>>
>> Fix DynamicModel test
>>
>> Don't rely on engine controller to be there.
>> Don't create actions for a tty application (they crashed the test)
>> Don't rely on the logger to be there (which migth not be in a test case)
> I object this change and I'd rather see it reverted (or reworked), see below.
>
>> M  +6    -2    src/EngineController.cpp
>> M  +4    -3    src/core-impl/meta/stream/Stream_p.h
>> M  +23   -7
>> src/core-impl/playlists/providers/user/UserPlaylistProvider.cpp
>>
>> http://commits.kde.org/amarok/0066525f8d299a22509818e3814eafd08202945d
>>
>> @@ -293,7 +293,9 @@ EngineController::supportedMimeTypes() //static
>>       {
>>           if ( !installDistroCodec() )
>>           {
>> -            Amarok::Components::logger()->longMessage(
>> +            Amarok::Logger* logger = Amarok::Components::logger(); // test
>> cases might not have a logger
>> +            if( logger )
> I don't think we should change the (correct) implementation to make the tests
> pass. In fact, you should add
> Amarok::Components::setLogger( new MockLogger() ) to
> TestDynamicModel::initTestCase() (and destroy it in dual method)
>
> In fact, we should even test that it calls logger->longMessage( ... ) in non-
> existing EngineController test if instrallDistroCodec() fails (which itself
> can be mocked).
>
> Otherwise this will create place for hidden bugs when we change init order and
> logger becomes null even in normal operation.

If we consider "logger" a singleton then in my opinion the call to 
Amarok::Components::logger should always work.
Since it obviously can return a null pointer we need to check for that.
Why do we need the logger anyway? Isn't the debug output enough?

>
>> @@ -40,9 +40,10 @@ class MetaStream::Track::Private : public QObject
>>
>>               // force a direct connection or slot might not be called
>> because of thread // affinity. (see BUG 300334)
>> -            connect( engine, SIGNAL(currentMetadataChanged( QVariantMap) ),
>> -                     this, SLOT(currentMetadataChanged( QVariantMap )), -
>>                     Qt::DirectConnection );
>> +            if( engine ) // test cases might not have an engine
>> +                connect( engine, SIGNAL(currentMetadataChanged(
> Again, place for hard-to-find bugs in future. You should
> Amarok::Compontents::setEngineController( new EngineControllerMock() ) in
> test's initTestCase() instead.
Again a singleton that is not working as expected.
We need to comment the functions correctly or make sure that they cannot 
return a null pointer.

>
>> @@ -30,7 +31,13 @@ Q_DECLARE_METATYPE( PlaylistTrackMap )
>>
>>   Playlists::UserPlaylistProvider::UserPlaylistProvider( QObject *parent )
>>
>>       : PlaylistProvider( parent )
>>
>> +    , m_deleteAction( 0 )
>> +    , m_renameAction( 0 )
>> +    , m_removeTrackAction( 0 )
>>   {
>> +    if (QApplication::type() == QApplication::Tty ) // for test cases we
>> don't need the actions (and it crashes)
>> +        return;
> Ugly. Why don't you just declare the test as requiring GUI? Half of our tests
> require it, no harm done.
That was a tricky one.
I have now idea why the QActions caused a crash. This is a border line 
case and if GUI solves the problem then I am fine.
>>   Playlists::UserPlaylistProvider::~UserPlaylistProvider()
>>   {
>> +    delete m_deleteAction;
>> +    delete m_renameAction;
>> +    delete m_removeTrackAction;
>>   }
> All these QObjects have Playlists::UserPlaylistProvider set as their parent,
> you should not delete them explicitly.
They have? I haven't noticed that.

>
>> @@ -69,13 +79,19 @@ Playlists::UserPlaylistProvider::playlistActions(
>> Playlists::PlaylistPtr playlis {
>>       QList<QAction *>  actions;
>>
>> -    Playlists::PlaylistList actionList =
>> m_deleteAction->data().value<Playlists::PlaylistList>(); -    actionList<<
>> playlist;
>> -    m_deleteAction->setData( QVariant::fromValue( actionList ) );
>> -    actions<<  m_deleteAction;
>> +    if( m_deleteAction )
>> +    {
>> +        Playlists::PlaylistList actionList =
>> m_deleteAction->data().value<Playlists::PlaylistList>(); +
>> actionList<<  playlist;
>> +        m_deleteAction->setData( QVariant::fromValue( actionList ) );
>> +        actions<<  m_deleteAction;
>> +    }
>>
>> -    m_renameAction->setData( QVariant::fromValue( playlist ) );
>> -    actions<<  m_renameAction;
>> +    if( m_renameAction )
>> +    {
>> +        m_renameAction->setData( QVariant::fromValue( playlist ) );
>> +        actions<<  m_renameAction;
>> +    }
> Ugly, ugly, ugly. I strongly disagree with convoluting our code just for the
> test cases. In order for tests to be meaningful, they must test the actual
> code (not extra paths created for them), even if it is harder to do so.

I agree, and if you look sharply then you will notice that I don't even 
check for the third action.
However one thing came up in my mind while fighting with the stuff.

Could you maybe next time write some more comments and run the tests?

If you can get the DynamicModel test to run with some more sensible 
changes, then I don't have any complaints.

Cheers,
Ralf