Code review comment for lp:~azzar1/unity/devices-cleanup-and-test

> > > Forgive me for the long diff. Refactoring code to make it testable is hard
> > without breaking legacy code.
> >
> > There is no need to apologize for this - a large diff that gets code under
> > test and makes it cleaner is better than a short diff that does not include
> > test cases.
> >
> > 235 +class DeviceNotificationShower : private boost::noncopyable
> > 236 +{
> > 237 +public:
> > 238 + typedef std::shared_ptr<DeviceNotificationShower> Ptr;
> > 239 +
> >
> > This class can probably be renamed to DeviceNotificationDisplay - the word
> > "Shower" has two meanings in the english langauge and although it was silly
> of
> > me to confuse one for the other, I did so anyways.
>
> Will do!
>
> >
> > 235 +class DeviceNotificationShower : private boost::noncopyable
> > 236 +{
> > 237 +public:
> > 238 + typedef std::shared_ptr<DeviceNotificationShower> Ptr;
> > 239 +
> >
> > I'm also not entirely certain about making the interface noncopyable.
> > Generally speaking the object should be noncopyable.
> >
>
> We really don't need to make that object copyable. Also making in un-copyable
> force us to use it through a pointer.

Right, my point was that you've made the interface itself noncopyable. However there probably isn't much
of a way to prevent copyablility except if we were to do it like that, so that seems okay.

>
> > > IconLoader::GetDefault().
> >
>
> IconLoader is used in dash/hud/launcher/switcher code. I think it's not a good
> idea to do it in this branch.

Sure, just food for thought.

>
> > Can we remove that singleton?
> >
> > 3019 + g_mock_volume_set_can_eject(gvolume_, TRUE);
> > 3020 + EXPECT_TRUE(volume_->CanBeEjected());
> >
> > Generally speaking I prefer to operate mocked objects through their
> > interfaces, eg
> >
> > g_volume_set_can_eject (...); (I assume that you're testing the effect of
> > external GIO code calling set_can_eject through the interface ...).
>
> Here I'm testing the VolumeImp mocking GVolumeIface.

Is VolumeImp providing a C++ wrapper around GVolumeIface ?

I feel like its a bit strange then, if the contents of the underlying GVolume can never be altered except by VolumeImp, that they are being altered in the tests to check the behaviour of VolumeImp.

Wouldn't it be better to alter GVolume through VolumeImp ?

Unless it is that it wraps an /existing/ GVolume, which has can_eject () already set. In this case, it probably makes more ense to implement GMockVolume in terms of google mock.

>
> >
> > Also it might be useful for you to hook up your mock implementation of
> GDrive
> > to a Google Mock directly. That way you can expect that your
> VolumeLauncherImp
> > is calling the correct function and also determine what the return value is
> > directly within the test code without having to add new api to GMockDrive to
> > set them.
> >
>
> Can we mock a GObject using Google Mock?

Yes you can, by making the interface delegate to a google mock.

Have a look at this:

http://bazaar.launchpad.net/~compiz-team/compiz/compiz.gtk-window-decorator-gsettings/view/head:/gtk/window-decorator/tests/compiz_gwd_mock_settings_writable.cpp
>
> > 3068 + EXPECT_CALL(*file_manager_opener_,
> > Open("file:///some/directory/testfile"))
> > 3069 + .Times(1);
> > 3070 +
> > 3071 + volume_->MountAndOpenInFileManager();
> > 3072 + EXPECT_TRUE(volume_->IsMounted());
> >
> > It was a bit confusing as to where: "file:///some/directory/testfile" came
> > from - better make it a constant.
>
> Will fix it.