Code review comment for lp://staging/~ted/dbus-test-runner/dbus-mock

Revision history for this message
Charles Kerr (charlesk) wrote :

Questions:

 * Why is DbusTestProcess.parameters a GArray instead of a GPtrArray?

 * Why are DbusTestDbusMock.properties and .methods GArrays instead of GPtrArrays?

 * in dbus-mock.c's install_object(), do the local "properties" and "methods"
   GVariants need to be unreffed, or does ..._add_object_sync() always consume them,
   even on failure?

 * in dbus-mock.c's dbus_test_dbus_mock_object_update_property(), maybe use
   g_propagate_error() instead of direct assignmnt "*error = local_error"?

 * in test-libdbustest-mock.c's main(), should G_LOG_LEVEL_CRITICAL be fatal
   in the tests?

Must Fix:

 * in process.c's dbus_test_process_get_pid(), need to use g_return_val_if_fail()
   instead of g_return_if_fail()

 * in dbus-mock.c's dbus_test_mock_dispose(), need to test self->priv->cancel for
   NULL before passing it to g_cancellable_cancel()

 * in dbus-mock.c's method_params_to_variant(), please add a comment above the
   "if (params == NULL)" explaining what DBusMock is expecting when this happens?
   I don't understand this block.

 * in dbus-mock.c's method_params_to_variant(), need to test len>0 before using
   peek[0]. Should probably also test peek!=NULL before using strlen().

 * in dbus-mock.c's install_object(), dead assign of NULL to the local "properties"
   and "method" GVariants when declaring them. Better to leave them uninitialized so
   gcc can yell at tell us if they get used without being initialized later in
   the code.

 * in process.c's dbus_test_process_append_param(), ignore the return value of
   g_array_append_val(). This is a leftover from switching from GList -> GArray

Should Fix:

 * There are still these super-strange "return;" lines everywhere...

 * in dbus-mock.c's constructed(), multiple commands on single lines.
   Might be more readable to remove the paramval variable altogether and use
   g_array_append_val (params, "-m");
   g_array_append_val (params, "dbusmock");

 * in test-lidbustest-mock.c, inconsistent use of "static" prefix for functions:
   signal_emitted() is private, but timeout_quit_func(), process_mainloop(), etc
   are public

 * in test-libdbustest-mock.c, lots of leaked "testvar"s in multiple functions

review: Needs Fixing

« Back to merge proposal