GNOME Bugzilla – Bug 549783
gtester lacks framework for tests with data files
Last modified: 2013-05-29 13:04:17 UTC
you can't have tests with data files. if you try to open a file from the current directory then that will work until you try with srcdir != builddir (like make distcheck does). some sort of makefile love in the gtester makefile to set a SRCDIR environment variable or something might help...
Please try to be a little less vague. I.e. have a concrete test case/program that we could discuss about, toy around with. In general, programs called from makefiles that need support for srcdir!=builddir either support a mechanism resembling Make's VPATH, or support something along the lines of CPP include directives. Adding support logic for this kind of file loads to GTest or GLib is well possible, but a set of test cases and example programs here would definitely help to nail the scope and possible API of such a thing.
see the test_bounds() case i just added to glib/tests/strfuncs.c for an example
Created attachment 245483 [details] [review] Add g_test_build_filename() This function allows testcases to find data files in various situations of srcdir == builddir, srcdir != builddir and for installed tests.
Created attachment 245484 [details] [review] tests: use new g_test_build_filename() API Port most of the tests to the new g_test_build_filename() API.
Created attachment 245485 [details] [review] gtest: Add more path building API Add a pair of functions for returning strings that don't need to be freed. This is a bit of a hack but it will turn the 99% case of using these functions from: gchar *tmp; tmp = g_test_build_filename (...); fd = open (tmp, ...); g_free (tmp); to: fd = open (g_test_get_filename (...), ...); which is a pretty substantial win.
Created attachment 245486 [details] [review] tests: move tests to new _get_filename() API This API was introduced to save a few lines of code here and there, so let's start by removing a bunch from our own tests.
Review of attachment 245483 [details] [review]: ::: glib/gtestutils.c @@ +1048,3 @@ + g_free (test_argv0_dirname); + test_argv0_dirname = tmp; + } It'd seem cleaner to require that test runners execute the test binaries in Automake's equivalent of $(builddir). Then G_TEST_BUILDDIR could fall back to ".". Right? @@ +2968,3 @@ + + va_start (ap, first_path); + for (num_path_segments = 2; num_path_segments < 16; num_path_segments++) < G_N_ELEMENTS(pathv) @@ +2975,3 @@ + } + + g_assert (num_path_segments < 16); Ditto. ::: glib/gtestutils.h @@ +376,3 @@ +{ + G_TEST_DISTED, + G_TEST_BUILT Why not G_TEST_SOURCE/G_TEST_BUILT matching the srcdir/builddir? The "disted" nomenclature comes from Automake, but one need not use gtester with automake. Everyone should understand "source".
Review of attachment 245484 [details] [review]: This looks a lot nicer. But unless you change the Makefile.am to set G_TEST_SRCDIR and G_TEST_BUILDDIR in the generated .test files, it will break the installed tests case.
(In reply to comment #8) > Review of attachment 245484 [details] [review]: > > This looks a lot nicer. But unless you change the Makefile.am to set > G_TEST_SRCDIR and G_TEST_BUILDDIR in the generated .test files, it will break > the installed tests case. This is exactly why I do the argv0 basename hackery that you commented on above. This covers the installed case (which I tested, and found to be working). It also allows running tests in-tree without the cwd being exactly correct, which I think is a nice added flexibility.
Review of attachment 245483 [details] [review]: ::: glib/gtestutils.c @@ +2975,3 @@ + } + + g_assert (num_path_segments < 16); I used 16 directly here so that the assert output would be more clear. I agree that G_N_ELEMENTS() would still be better on balance, though -- and meanwhile I remember that we have g_assert_cmpint() that will still give me the output I want. Will fix.
Review of attachment 245485 [details] [review]: A few comments. ::: glib/gtestutils.c @@ +2928,3 @@ + pathv[1] = first_path; + + for (num_path_segments = 2; num_path_segments < 16; num_path_segments++) G_N_ELEMENTS etc. @@ +2935,3 @@ + } + + g_assert (num_path_segments < 16); Ditto. @@ -2945,3 @@ * g_test_build_filename(). - * - * Since: 2.38 Intentional deletion? Or maybe just diff movement. @@ +3068,3 @@ + do + node->next = *test_filename_free_list; + while (g_atomic_pointer_compare_and_exchange (test_filename_free_list, node->next, node)); Can you actually use g_atomic_pointer_compare_and_exchange() on pointers to pointers? The docs only say gint/guint. The GCC implementation appears to be useful for any pointer type...dunno. Do we have this "atomic list append" functionality needed elsewhere? Offhand I am not seeing a flaw, but if it really works it's probably worth splitting out into a documented utility function, even if static.
(In reply to comment #9) > This is exactly why I do the argv0 basename hackery that you commented on > above. This covers the installed case (which I tested, and found to be > working). It also allows running tests in-tree without the cwd being exactly > correct, which I think is a nice added flexibility. Ah, hmm. Historically *relying* on argv0 for this sort of thing is a bit unreliable...it can be eaten by intermediate programs. But if this patch lands, we'll be pointlessly setting G_TEST_HOME, so let's either remove it and rely on argv0, or change it to use G_TEST_SRCDIR/G_TEST_BUILDDIR?
Review of attachment 245485 [details] [review]: Thanks for the reviews. Will redo the patchset now and submit again. ::: glib/gtestutils.c @@ -2945,3 @@ * g_test_build_filename(). - * - * Since: 2.38 Legitimate oops. I forgot the Since tags when I first did the original patch and I tried to go back and fix it. Clearly I messed that up. @@ +3068,3 @@ + do + node->next = *test_filename_free_list; + while (g_atomic_pointer_compare_and_exchange (test_filename_free_list, node->next, node)); There's nothing special about a pointer to a pointer here... Normally you use g_atomic_pointer_* like so: gpointer x; g_atomic_pointer_... (&x, ...); This is just adding another variable between: gpointer x; gpointer *xptr = &x; g_atomic_pointer_... (xptr, ...); it's really exactly the same.
Created attachment 245497 [details] [review] Add g_test_build_filename() This function allows testcases to find data files in various situations of srcdir == builddir, srcdir != builddir and for installed tests.
Created attachment 245498 [details] [review] tests: use new g_test_build_filename() API Port most of the tests to the new g_test_build_filename() API.
Created attachment 245499 [details] [review] gtest: Add more path building API Add a pair of functions for returning strings that don't need to be freed. This is a bit of a hack but it will turn the 99% case of using these functions from: gchar *tmp; tmp = g_test_build_filename (...); fd = open (tmp, ...); g_free (tmp); to: fd = open (g_test_get_filename (...), ...); which is a pretty substantial win.
Created attachment 245500 [details] [review] tests: move tests to new _get_filename() API This API was introduced to save a few lines of code here and there, so let's start by removing a bunch from our own tests.
Review of attachment 245499 [details] [review]: ::: glib/gtestutils.c @@ +2935,3 @@ + } + + g_assert (num_path_segments, <, G_N_ELEMENTS (pathv)); sigh. g_assert_cmpint() obviously.
Created attachment 245511 [details] [review] gtest: Add more path building API Add a pair of functions for returning strings that don't need to be freed. This is a bit of a hack but it will turn the 99% case of using these functions from: gchar *tmp; tmp = g_test_build_filename (...); fd = open (tmp, ...); g_free (tmp); to: fd = open (g_test_get_filename (...), ...); which is a pretty substantial win.
Created attachment 245512 [details] [review] tests: move tests to new _get_filename() API This API was introduced to save a few lines of code here and there, so let's start by removing a bunch from our own tests.
Created attachment 245513 [details] [review] Test data file API: port two more testcases These ones were slightly non-trivial so they didn't get included in the previous patches. Port them now.
Created attachment 245514 [details] [review] Remove G_TEST_DATA= from installed .test files This is no longer needed with the new test data file finding stuff.
I think this patchset is ready to go (in the order attached), pending final review.
Review of attachment 245497 [details] [review]: One minor comment. ::: glib/gtestutils.c @@ +2916,3 @@ + * argument. + * + * The data file should either have been disted with the module If we're keeping "dist", maybe expand this to "distributed" here in the docs, so it's more obvious it's the expansion of DISTED?
Review of attachment 245498 [details] [review]: Looks fine in spot checks.
Review of attachment 245511 [details] [review]: Ok.
Review of attachment 245512 [details] [review]: Yeah, definitely nicer.
Review of attachment 245513 [details] [review]: Looks fine.a
Review of attachment 245514 [details] [review]: Ok, cool.
Review of attachment 245497 [details] [review]: This comment forces me to realise how utterly automake-centric this entire feature is (which is fine). But since it is, we may as well use the term that automake itself uses, which is "DIST" and explain the connection in the docs more explicitly. I'll add another patch that does just that.
Created attachment 245548 [details] [review] Rename G_TEST_DISTED to G_TEST_DIST Since this feature is so utterly automake-centric, we may as well be using the same terminology as automake itself (ie: although it's BUILT_SOURCES, it's DIST_EXTRA, not DISTED). Also add some comments to the enum explaining that these terms are really corresponding directly to the automake terms.
Attachment 245497 [details] pushed as 0c48067 - Add g_test_build_filename() Attachment 245498 [details] pushed as 58c6ca3 - tests: use new g_test_build_filename() API Attachment 245511 [details] pushed as 575a9da - gtest: Add more path building API Attachment 245512 [details] pushed as 17ded32 - tests: move tests to new _get_filename() API Attachment 245513 [details] pushed as ddd7e94 - Test data file API: port two more testcases Attachment 245514 [details] pushed as da478ac - Remove G_TEST_DATA= from installed .test files Attachment 245548 [details] pushed as 8df1bb3 - Rename G_TEST_DISTED to G_TEST_DIST