[amarok] src: Fix DynamicModel test

[Matěj Laitl]

On 12. 7. 2012 Ralf Engels wrote:
> 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.
> > > -            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.

I don't consider it a singleton. Amarok::Components is not a singleton pattern 
IMO. It is rather some kind of ApplicationController pattern where whole 
components should declare their dependencies and ApplicationController (or the 
programmer) then determines order of their initialization.

> Since it obviously can return a null pointer we need to check for that.

It should be guaranteed by some kind of app controller that dependent 
components are created before the depending ones.

> Why do we need the logger anyway? Isn't the debug output enough?

The "logger" is the thing that pops up that pretty message in lower right 
amarok corner.

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

I don't want nor consider EngineController a singleton, rather a component. 
See above.

> We need to comment the functions correctly or make sure that they cannot
> return a null pointer.

You're right, we should comment Amarok::Components much better. Well, it was 
Bart who introduced it AFAIK.

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

Yes, my fault, I probably haven't run the tests when changing 
UserPlaylistProvider. Comments to what?

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

Will try, let's see.

	Matěj