Re: Gtk+ unit tests suite

[Iago Toral Quiroga <itoral igalia com>]

Hi Tim,

thank you for answering to my email, it is good to know there is
interest in the community about this topic. And thank you also for
looking at the tests in that detail. I'm adding my opinion about the
topics you commented below:

El mié, 25-10-2006 a las 17:16 +0200, Tim Janik escribió: 
[...]
> 
> Gtk+ definietely lacks a unit test frame work, so it's good to try to
> get the ball rolling on this some.
> i've recently worked on improving unit test integration in another
> project, a brief summary of that can be found in my last blog entry:
>    http://blogs.gnome.org/view/timj/2006/10/23/0 # Beast and unit testing

About this comment:

"However we did run into the problem of make check being less likely
executed before commits, because running the tests would be too slow to
bother with. That of course somewhat defeats the purpose of having a
test harness."

I definitely agree. I think this is an important topic when talking
about unit tests. Before working on these unit tests I spent some time
looking at gtkvts:
http://cvs.gnome.org/bonsai/rview.cgi?cvsroot=/cvs/gnome&dir=gtkvts
and I think they run into that problem somehow, taking too much time to
execute the entire test suite. I made a comment about this issue in the
build-brigade list:
http://mail.gnome.org/archives/build-brigade-list/2006-August/msg00006.html

> > I have set up a temporary modified version of Gtk+ here:
> >
> > cvs -d :pserver:anonymous cvs igalia com:/var/publiccvs co gtk+
> >
> > Currently, this modified version includes a small set of unit tests for
> > several widgets that would grow in ammount if the project seems
> > interesting to the Gtk+ community.
> 
> first i have to say that copying a random Gtk+ CVS version and adding tests
> to it is not the most conveninent review basis. getting a reviewable diff of
> your changes took figuring of an exact CVS version (-D "2006/09/17 11:59:00")
> and significant manual cleanups. i've attached the resulting diff for future
> reference.
> 

Yes, you are right.

In this particular case, there were 2 reasons for doing it that way:

First, I wanted to integrate the tests with buildbot, in the builbot
demo that Dape announced some days ago in planet gnome:
http://blogs.igalia.com/dape/2006/10/03
Though it is possible to do that maintaining a patch, for us it was
easier/faster to setup a temporary cvs repository.

On the other hand, for me, it is very easy to provide a patch for
current gtk+ HEAD, cause I know most of the code is a new directory
(/ut) and some minor changes for autotools files that I perfectly know.

But I understand your point and agree with you.

> second, your code is missing a license, releasing/publishing code should
> always be accompanied by *some* kind of license information. in our current
> society, there's little point in publishing something without telling what
> can legally be done and not done with the published material.

Yes, big mistake on my side :(. I thought I already added it, will fix
that asap. Thanks for warning me.

[...]
> ok, running your tests and reading through some of your code, i collected
> a few comments:
> 
> 
> i'm not very fond of the introduction of a new library/package dependency
> for Gtk+ (or GLib) just for writing unit tests. 

So you would prefer avoiding a test tool, and doing all from scratch to
avoid that dependency... Actually, I do not agree on this, I think these
tools are there to be used in these situations. 

Regarding the dependency, in the temporary gtk+ repository I set up,
Check is needed to run unit tests, but it is not required to build Gtk+,
if Check is not installed, unit tests are disabled. I find it logical
that you need a test tool in order to execute unit tests.

> Check in particular has a
> few quirks that i find particularly undesirable, such as the prefix/postfix
> magic macros around each test function which also tend to break C syntax
> parsers, see:
>    http://bugzilla.gnome.org/show_bug.cgi?id=364672
> for more details. other oddities involve e.g. its automake/configure.in
> integration.
> i'll get into more details about this in another email.
> 

Ok, although that problem is already in Glib and Gtk+ as I read in your
bug report. I guess that is something that could bother some people,
yes. I guess, we could always try asking Check developers to provide a
fix for that.

> the way you're using fail_if(assert_expr,error_string) tends to state too
> much the obvious, e.g.:
> +  button = GTK_BUTTON (gtk_button_new_with_label (set_label));
> + 
> +  fail_if (!GTK_IS_BUTTON (button),
> +           "gtk_button_new_with_label fails");
> this can really be shortened to:
>     button = gtk_button_new_with_label (label);
>     ASSERT (GTK_IS_BUTTON (button));

Well, actually, that is not exactly the same. In your ASSERT statement
you know something wrong happened with a button, but you lose what is
wrong: the button creation has failed in a gtk_button_new_with_label
call. However, I guess the assert information should be enough to locate
the error properly, so maybe you are right.

> other things are arguably not worth checking:
> +  get_label = gtk_button_get_label (button);
> + 
> +  fail_if (strcmp (set_label, get_label) != 0,
> +           "Retrieved label is not the same as the one used at construction: (%s,%s)",
> +           set_label, get_label); 
> i.e. i'd not be surprised if a future or tailored version of Gtk+
> returned a translated or maybe space-stripped string here.
> 

Mmm... but in that case you are modifying the way GTK+ behaves, so you
are breaking compatibility with apps that rely on the current behavior.
I see tests as small user applications, they tell you what they expect
from your API. If you change the behavior of the API, then it should
break the tests so you realize that change can damage other
applications.

Besides, it's a problem when the API contract is not documented, or
poorly documented. These kind of tests would help with understanding
that contract.

> and then there's a number of tests which are plain wrong (edited for brevity):
> +  GtkWidget *widget = NULL;
> +  widget = gtk_hbox_new (0, FALSE);
> +  /* Test 3 */
> +  get_text_buffer = gtk_text_view_get_buffer (GTK_TEXT_VIEW (widget));
> +  fail_if (get_text_buffer != NULL,
> +           "gtk_text_view_get_buffer does not return NULL when "
> +           "applied to an object that is not a GtkTextView");
> the g_return_val_if_fail() statement in gtk_text_view_get_buffer() is NOT part
> of the API contract, it may even be optimized away in some configurations.

Yes, you are right.

> the point in having unit tests is to check that the use cases meant to be
> covered by the code base are working correctly. not that things which weren't
> accounted for in the design and implementation do not work. (if that was the 
> case, the number of tests we had to write would be endless.)
> that's simply because the libraries get used for what they CAN do, not what
> they CANNOT do ;)

Sure, I think we all agree on that :). If you say this because of the
last example (testing g_return_val_if_fail), I'm trying to test how the
library deals with invalid inputs, because I think it is also
interesting.

> 
> you should call gtk_init (&argc, &argv); with real argc/argv from main().
> using:
> +  int argc = 0;
> +  gtk_init (&argc, NULL);
> all over the place is not the best idea (maybe this is due to using Check in
> fork mode, but it's still a bad idea), because this elimintaes the ability
> to (manually) try out test cases in different modes (--sync, --display,
>   --test-slow, etc...)

Yes, I completely agree. And yes, that is because of the fork mode.

> about using fork mode, there really is no need to fork at all for a test,
> except when checking tests that are supposed to fail (which is rarely the
> case), e.g. when you need to assert proper abort()-like behavior of e.g.
> g_error().
> 

Actually, the problem here is not testing abort-like operations. It is
testing and handling segmentation faults during tests execution properly
(not breaking the complete suite execution). Of course, if you only want
to know if all tests succeded or there was one test failing, it would
not be necessary.

> 
> in general, i think it's great that you started on writing unit tests for
> Gtk+, however, i'd say that what Gtk+ needs differs significantly from
> your approach. 
> i'll provide more detailed thoughts on this in a follow up.

Thanks again for your interest Tim, I really appreciate your time and
your suggestions. I'll check your follow up too!

Iago.