Code review comment for lp:~ted/indicator-messages/sanity-testing

On Tue, 2015-03-10 at 18:42 +0000, Charles Kerr wrote:

> > + -DSCHEMA_DIR="\"$(abs_builddir)/gsettings-schemas-compiled/\"" \
>
> Might make sense to define this directory as its own variable, so that it can be used below in schemas-compiled.stamp?
>
> Also, we're using $(abs_builddir)/gsettings-schema-compiled here but $(builddir)/gsettings-schema-compiled below... I think they're the same in this MP but it's probably better to use abs_builddir everywhere?

Makes sense. r460

> > $(top_builddir)/common/indicator-messages-service.h \
> > $(top_srcdir)/src/gactionmuxer.c \
> > - $(top_srcdir)/src/gactionmuxer.h
> > + $(top_srcdir)/src/gactionmuxer.h \
>
> How did this work before?
>
> > $(top_srcdir)/src/dbus-data.h

I think because it was header files and you don't *actually* need to
include them in the source list unless you are doing a make dist to
build your tarball. If you always use the Bazaar branch you'll never
notice them being missing.

> > + operator DbusTestDbusMock* () {
> > + return mock;
> > + }
>
> overloading operator() like this seems excessive.

I was trying to make the tests read nicely in that otherwise you end up
having to do all the GObject casts. The reality is that this one isn't
too useful, but the DbusTestTask gets passed to libdbustest all the
time. Not as much with this fixture, but in the other i-sound tests it
is used like that more (mock comes from i-sound)

> > + std::shared_ptr<GMenuModel> _menu;
> > + std::shared_ptr<GActionGroup> _actions;
> > + DbusTestService * _session_service;
> > + DbusTestService * _system_service;
> > + DbusTestTask * _test_indicator;
> > + DbusTestTask * _test_dummy;
> > + GDBusConnection * _session;
> > + GDBusConnection * _system;
> > +
>
> http://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-underscore-in-a-c-identifier

Eh, sure, one underscore seems like it doesn't break the rules. Not
perfect for sure.

> > + std::for_each(mocks.begin(), mocks.end(), [this](std::shared_ptr<DbusTestTask> task) {
>
> "for (auto task : mocks) {" would be clearer
>
> > + if (dbus_test_task_get_bus(task.get()) == DBUS_TEST_SERVICE_BUS_SYSTEM) {
> > + dbus_test_service_add_task(_system_service, task.get());
> > + } else {
> > + dbus_test_service_add_task(_session_service, task.get());
> > + }
> > + });

Good point. r461

> > + virtual ~PerRunData (void) {
>
> dtor doesn't need a void argument?

The compiler at least seems to want the parameter list. The void is just
making it look less like line noise :-)

> > +static void
> > +messageReplyActivate (GObject * obj, gchar * name, GVariant * value, gpointer user_data) {
> > + std::string * res = reinterpret_cast<std::string *>(user_data);
>
> "auto res =" would avoid redundancy
>
> > + *res = g_variant_get_string(value, nullptr);
> > +}

You just want me to give into your auto everywhere world! r462

> > + EXPECT_EVENTUALLY_ACTION_ENABLED("remove-all", false);
> > +}
>
> EXPECT_ is used as the default in these tests, but most of the time followup tests don't make sense if the ones that preceded them failed & should use ASSERT_ instead

So, yes. But I felt really stupid making an "ASSERT_EVENTUALLY" macro,
it seems like you either assert or your wait for things, not both. I
think we should bike shed this at the sprint and figure out what we want
here. I get your point but I'm not sure if we're diluting things too
much by going down that path.