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
_______________________________________________
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel
