[Bug 774657] add proxy control binding

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=774657

--- Comment #9 from Stefan Sauer (gstreamer, gtkdoc dev) <ensonic at sonicpulse.de> ---
Review of attachment 340488:
 --> (https://bugzilla.gnome.org/review?bug=774657&attachment=340488)

This is really about test maintenance. I did not invent these best practices
and I recently sent a mail to the list about it ("better unit tests" -
http://gstreamer-devel.966125.n4.nabble.com/better-unit-tests-td4680069.html)

::: tests/check/libs/controller.c
@@ +1545,3 @@
+  cb = gst_proxy_control_binding_new (GST_OBJECT (elem), "int",
+      GST_OBJECT (elem2), "int");
+  GValue gval1 = G_VALUE_INIT, gval2 = G_VALUE_INIT;

personally I don't like asserts that are out side of the scope of the test. We
already have a test that verifies that we can add_control_bindings, right?

@@ +1551,3 @@
+  fail_unless (val1 == NULL);
+  fail_if (gst_control_binding_get_value_array (cb, 0, 0, 1, &int1));
+  elem = gst_element_factory_make ("testobj", NULL);

here the first test ends. It could be called 'controller_proxy_does_nothing'

@@ +1558,3 @@
+
+  cb2 = gst_direct_control_binding_new (GST_OBJECT (elem2), "int", cs);
+  /* proxy control binding from elem to elem2 */

see above. In general a test should ideally have a single assert (or
fail_unless/if). This single check is what the tests verifies.

@@ +1589,3 @@
+  g_free (val2);
+  g_value_unset (&gval1);
+  fail_unless (gst_object_add_control_binding (GST_OBJECT (elem2), cb2));

Here you verify 3 things. You tests that 3 different functions are proxied:
get_value, get_value_array and get_g_value_array. This could be 3 tests. If you
feel it would repeat code, use helpers.

@@ +1615,3 @@
+  g_free (val2);
+  g_value_unset (&gval1);
+      g_value_get_int (val1));

This repeats the above code. This can be done more efficiently using a 'loop'
test:
https://libcheck.github.io/check/doc/check_html/check_4.html#Looping-Tests

@@ +1624,3 @@
+  time = 1 * GST_SECOND;
+  gst_object_sync_values (GST_OBJECT (elem2), time);
+  fail_unless (gst_control_binding_get_value_array (cb2, time, 0, 1, &int2));

Another test: e.g. 'controller_proxy_sync_original_still_work', should also be
a loop test.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.