After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 549783 - gtester lacks framework for tests with data files
gtester lacks framework for tests with data files
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2008-08-29 03:02 UTC by Allison Karlitskaya (desrt)
Modified: 2013-05-29 13:04 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add g_test_build_filename() (7.19 KB, patch)
2013-05-28 19:39 UTC, Allison Karlitskaya (desrt)
reviewed Details | Review
tests: use new g_test_build_filename() API (32.00 KB, patch)
2013-05-28 19:39 UTC, Allison Karlitskaya (desrt)
reviewed Details | Review
gtest: Add more path building API (7.29 KB, patch)
2013-05-28 19:39 UTC, Allison Karlitskaya (desrt)
reviewed Details | Review
tests: move tests to new _get_filename() API (27.07 KB, patch)
2013-05-28 19:39 UTC, Allison Karlitskaya (desrt)
none Details | Review
Add g_test_build_filename() (7.23 KB, patch)
2013-05-28 21:43 UTC, Allison Karlitskaya (desrt)
committed Details | Review
tests: use new g_test_build_filename() API (32.00 KB, patch)
2013-05-28 21:43 UTC, Allison Karlitskaya (desrt)
committed Details | Review
gtest: Add more path building API (7.25 KB, patch)
2013-05-28 21:43 UTC, Allison Karlitskaya (desrt)
none Details | Review
tests: move tests to new _get_filename() API (25.73 KB, patch)
2013-05-28 21:44 UTC, Allison Karlitskaya (desrt)
none Details | Review
gtest: Add more path building API (7.26 KB, patch)
2013-05-29 03:42 UTC, Allison Karlitskaya (desrt)
committed Details | Review
tests: move tests to new _get_filename() API (25.73 KB, patch)
2013-05-29 03:42 UTC, Allison Karlitskaya (desrt)
committed Details | Review
Test data file API: port two more testcases (10.89 KB, patch)
2013-05-29 03:42 UTC, Allison Karlitskaya (desrt)
committed Details | Review
Remove G_TEST_DATA= from installed .test files (2.05 KB, patch)
2013-05-29 03:43 UTC, Allison Karlitskaya (desrt)
committed Details | Review
Rename G_TEST_DISTED to G_TEST_DIST (26.88 KB, patch)
2013-05-29 13:00 UTC, Allison Karlitskaya (desrt)
committed Details | Review

Description Allison Karlitskaya (desrt) 2008-08-29 03:02:05 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...
Comment 1 Tim Janik 2008-08-29 10:09:05 UTC
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.
Comment 2 Allison Karlitskaya (desrt) 2008-08-29 18:01:22 UTC
see the test_bounds() case i just added to glib/tests/strfuncs.c for an example
Comment 3 Allison Karlitskaya (desrt) 2013-05-28 19:39:43 UTC
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.
Comment 4 Allison Karlitskaya (desrt) 2013-05-28 19:39:47 UTC
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.
Comment 5 Allison Karlitskaya (desrt) 2013-05-28 19:39:50 UTC
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.
Comment 6 Allison Karlitskaya (desrt) 2013-05-28 19:39:53 UTC
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.
Comment 7 Colin Walters 2013-05-28 20:27:33 UTC
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".
Comment 8 Colin Walters 2013-05-28 20:28:57 UTC
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.
Comment 9 Allison Karlitskaya (desrt) 2013-05-28 20:35:18 UTC
(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.
Comment 10 Allison Karlitskaya (desrt) 2013-05-28 20:36:57 UTC
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.
Comment 11 Colin Walters 2013-05-28 21:06:28 UTC
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.
Comment 12 Colin Walters 2013-05-28 21:09:27 UTC
(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?
Comment 13 Allison Karlitskaya (desrt) 2013-05-28 21:34:21 UTC
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.
Comment 14 Allison Karlitskaya (desrt) 2013-05-28 21:43:42 UTC
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.
Comment 15 Allison Karlitskaya (desrt) 2013-05-28 21:43:49 UTC
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.
Comment 16 Allison Karlitskaya (desrt) 2013-05-28 21:43:57 UTC
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.
Comment 17 Allison Karlitskaya (desrt) 2013-05-28 21:44:02 UTC
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.
Comment 18 Allison Karlitskaya (desrt) 2013-05-28 21:47:38 UTC
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.
Comment 19 Allison Karlitskaya (desrt) 2013-05-29 03:42:27 UTC
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.
Comment 20 Allison Karlitskaya (desrt) 2013-05-29 03:42:49 UTC
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.
Comment 21 Allison Karlitskaya (desrt) 2013-05-29 03:42:57 UTC
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.
Comment 22 Allison Karlitskaya (desrt) 2013-05-29 03:43:05 UTC
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.
Comment 23 Allison Karlitskaya (desrt) 2013-05-29 03:43:59 UTC
I think this patchset is ready to go (in the order attached), pending final review.
Comment 24 Colin Walters 2013-05-29 12:33:37 UTC
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?
Comment 25 Colin Walters 2013-05-29 12:36:18 UTC
Review of attachment 245498 [details] [review]:

Looks fine in spot checks.
Comment 26 Colin Walters 2013-05-29 12:38:20 UTC
Review of attachment 245511 [details] [review]:

Ok.
Comment 27 Colin Walters 2013-05-29 12:39:03 UTC
Review of attachment 245512 [details] [review]:

Yeah, definitely nicer.
Comment 28 Colin Walters 2013-05-29 12:42:10 UTC
Review of attachment 245513 [details] [review]:

Looks fine.a
Comment 29 Colin Walters 2013-05-29 12:42:33 UTC
Review of attachment 245514 [details] [review]:

Ok, cool.
Comment 30 Allison Karlitskaya (desrt) 2013-05-29 12:59:47 UTC
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.
Comment 31 Allison Karlitskaya (desrt) 2013-05-29 13:00:28 UTC
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.
Comment 32 Allison Karlitskaya (desrt) 2013-05-29 13:03:55 UTC
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