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:[EMAIL PROTECTED]:/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. _______________________________________________ gtk-devel-list mailing list gtk-devel-list@gnome.org http://mail.gnome.org/mailman/listinfo/gtk-devel-list