Merge lp://staging/~ted/dbus-test-runner/dbus-mock into lp://staging/dbus-test-runner/14.04

FAILED: Continuous integration, rev:149
http://jenkins.qa.ubuntu.com/job/dbus-test-runner-ci/12/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-quantal-amd64-ci/12/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-quantal-armhf-ci/12/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-quantal-i386-ci/2/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-trusty-amd64-ci/2/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-trusty-armhf-ci/2/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-trusty-i386-ci/2/console

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/dbus-test-runner-ci/12/rebuild

FAILED: Continuous integration, rev:150
http://jenkins.qa.ubuntu.com/job/dbus-test-runner-ci/13/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-quantal-amd64-ci/13/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-quantal-armhf-ci/13/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-quantal-i386-ci/3/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-trusty-amd64-ci/3
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-trusty-armhf-ci/3
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-trusty-i386-ci/3

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/dbus-test-runner-ci/13/rebuild

FAILED: Continuous integration, rev:152
http://jenkins.qa.ubuntu.com/job/dbus-test-runner-ci/14/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-quantal-amd64-ci/14/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-quantal-armhf-ci/14/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-quantal-i386-ci/4/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-trusty-amd64-ci/4
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-trusty-armhf-ci/4
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-trusty-i386-ci/4

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/dbus-test-runner-ci/14/rebuild

PASSED: Continuous integration, rev:152
http://jenkins.qa.ubuntu.com/job/dbus-test-runner-ci/15/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-trusty-amd64-ci/5
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-trusty-armhf-ci/5
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-trusty-i386-ci/5

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/dbus-test-runner-ci/15/rebuild

PASSED: Continuous integration, rev:155
http://jenkins.qa.ubuntu.com/job/dbus-test-runner-ci/16/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-trusty-amd64-ci/6
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-trusty-armhf-ci/6
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-trusty-i386-ci/6

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/dbus-test-runner-ci/16/rebuild

FAILED: Continuous integration, rev:156
http://jenkins.qa.ubuntu.com/job/dbus-test-runner-ci/17/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-trusty-amd64-ci/7/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-trusty-armhf-ci/7
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-trusty-i386-ci/7

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/dbus-test-runner-ci/17/rebuild

PASSED: Continuous integration, rev:159
http://jenkins.qa.ubuntu.com/job/dbus-test-runner-ci/18/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-trusty-amd64-ci/8
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-trusty-armhf-ci/8
    SUCCESS: http://jenkins.qa.ubuntu.com/job/dbus-test-runner-trusty-i386-ci/8

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/dbus-test-runner-ci/18/rebuild

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

On Mon, 2013-11-18 at 17:22 +0000, Charles Kerr wrote:

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

Because I wanted it to be NULL terminated, which GPtrArray doesn't do.
That makes data usable as a GStrv.

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

To avoid allocating the structures separately. Basically all memory
management moves with the array then.

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

Yeah, it creates another, larger variant with g_variant_new().

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

Ah, always forget about that function. Fixed r160.

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

No, because one of the tests purposefully make a critical error to make
sure it's handled:

libdbustest-CRITICAL **: Property 'prop1' is not of same value in
dbus_test_dbus_mock_object_update_property()

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

Fixed r161.

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

Fixed r162.

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

Explained r163.

> * 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().

Fixed r164.

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

Hmm, I think this is done already. Do you see it in another place 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

Wow. Fixed r165.

> * 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");

Sadly, you can't. The macro requires it to be a local variable. I did
that to make it seem a little less insane, not sure if I accomplished
that or not.

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

Fixed r166.

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

Actually it was the tuple getting leaked :-) Fixed r167.

Nice!