[amarok] src: Fix DynamicModel test

[Matěj Laitl]

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.

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

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

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

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

Keep up the good work fixing our tests, Ralf, I appreciate it, I just thing we 
should take another direction when fixing them.

Cheers,
			Matěj