Sounds potentially nice; the fork-but-don't-exec is really pretty problematic

The one think I like about the existing setup is that I can do tests of situations where I have a huge list of 'correct' steps that I'm supposed to take, but at each step I want to check what happens if I do the wrong thing...


  if (fork ()) { wrong_1 (); }
  check_failed ();

  correct_1 ();

  if (fork ()) { wrong_2 (); }
  check_failed ();

  correct_2 ();

  etc...

This is convenient, but I think the ills of forking may not be worth it...

Created attachment 222494 [details] [review]
gtestutils: fix "-p" logic

If you had two tests "/foo/bar" and "/foo/bar/baz", and ran the test
program with "-p /foo/bar/baz", it would run "/foo/bar" too. Fix that.

Created attachment 222495 [details] [review]
gtestutils: add g_test_skip()

Add a way to mark a test as skipped by default, eg, for tests that are
too slow to run normally, or that are known to fail but that you still
want to keep around anyway. They can still be run manually by using
the "-p" command-line option.

Created attachment 222496 [details] [review]
gtestutils: add g_test_trap_subprocess(), deprecate g_test_trap_fork()

g_test_trap_fork() doesn't work on Windows and is potentially flaky on
unix anyway given the fork-but-don't-exec. Replace it with
g_test_trap_subprocess(), which re-spawns the same program with
arguments telling it to run a specific (otherwise-ignored) test case.

Port some users of g_test_trap_fork() to g_test_trap_subprocess().
(FIXME: incomplete)

proof of concept / work in progress.

I realized that "tests that are registered but don't normally get run" could have more applications than just for this, so I implemented a generic test-skipping mechanism. Having a separate function call to mark the subprocess test as not-ordinarily-run also lets you use g_test_add_data_func() rather than g_test_add_func() for the test if you want, which you wouldn't be able to do if we just had a single "g_test_add_subprocess_func()".

That said, I'm not totally sold on the idea that register-then-call-g_test_skip() is the nicest idiom for this... I also thought about maybe having a specific naming convention that indicates that it's a child test (eg "/foo/bar" could have child tests "/foo/bar:apple" and "/foo/bar:banana"), and then they'd get skipped automatically.

I ported some of the tests. It's definitely messier, but the addition of g_test_expect_message() got rid of a lot of old uses of g_test_trap_fork(), so there's less mess than there would have been... And in some cases you can merge some of the children together (eg, in test-printf I just had a single subprocess print 10 lines rather than 10 subprocesses each printing 1 line).

The fact that you can use "./testname -p /path/to/child/test" to run the child test directly is a nice bonus feature; it makes it easier to debug tests that aren't doing what you want...

Oh, the reason trap_subprocess() implements timeouts differently from trap_fork() is because there's no way to interrupt g_spawn_sync(), and implementing it in terms of g_spawn_async_with_pipes() would basically involving copying the entire definition of both the unix and windows versions of g_spawn_sync() into gtestutils.c...

Created attachment 229959 [details] [review]
win32: suppress fatal error dialog box when running tests

When running a test program (ie, if g_test_init() has been called),
don't pop up a dialog box when a fatal error occurs. Just print the
message to stderr and exit.

Created attachment 229960 [details] [review]
gtestutils: add g_test_trap_subprocess(), deprecate g_test_trap_fork()

g_test_trap_fork() doesn't work on Windows and is potentially flaky on
unix anyway given the fork-but-don't-exec. Replace it with
g_test_trap_subprocess(), which re-spawns the same program with
arguments telling it to run a specific (otherwise-ignored) test case.

Make the existing g_test_trap_fork() unit tests be unix-only (they
never passed on Windows anyway), and add a parallel set of
g_test_trap_subprocess() tests.

Created attachment 229961 [details] [review]
tests: port from g_test_trap_subprocess() to g_test_trap_fork()

(or, in a few cases, to g_test_expect_message())

Created attachment 229962 [details] [review]
gtestutils: fix "-p" logic

If you had two tests "/foo/bar" and "/foo/bar/baz", and ran the test
program with "-p /foo/bar/baz", it would run "/foo/bar" too. Fix that.
And add a test to tests/testing for it.

Created attachment 229963 [details] [review]
tests/option-context: fix under --verbose

We need to always pass G_TEST_TRAP_SILENCE_STDERR/STDOUT, or else
we can't check that they contained the right text later.

Created attachment 229964 [details] [review]
tests/protocol: redo a bit

Rather than overloading --verbose, just skip the tests that aren't
supposed to be run in the parent process (so that if you do run the
toplevel test with --verbose, it doesn't immediately error out).

Created attachment 229965 [details] [review]
tests/spawn-*.c: fix on Windows

Need to append ".exe" to the spawned binary name on Windows

hacked on this over Thanksgiving...

This is now tested on Windows (well, MinGW/Wine) as well.

I got rid of g_test_skip() and instead added the rule that tests with ':' in their name are not run unless you request them explicitly. (I had mentioned other possible use cases for g_test_skip(), but "too slow to run normally" is already covered by -m quick / -m slow.)

I didn't think of this before, but I guess this might silently end up skipping tests in some packages if they already had tests with ':' in the name... I guess we could have g_test_run() error out if you have any test with ':' in the name, but you don't ever call g_test_trap_subprocess()?

The biggest problem with this patchset right now though is that it very frequently hits the gspawn race condition discussed in bug 687061, where the glib worker thread reaps the zombie before g_spawn_sync() can set up its child watch, causing it to then hang forever. So, we need a fix for that before this can go in...

Review of attachment 229959 [details] [review]:

Makes sense, one comment:

::: glib/gtestutils.c
@@ +111,3 @@
+ * Returns %TRUE if g_test_init() has been called.
+ *
+ * Returns: %TRUE if g_test_init() has been called.

Duplicate docs

Review of attachment 229960 [details] [review]:

This makes a lot of sense...I like the approach.

Some minor nits...feel free to commit after addressing or justifying.

::: glib/gtestutils.c
@@ +1755,3 @@
+      if (!found)
+        {
+          if (g_test_verbose())

Missing space between identifier and paren

@@ +1901,3 @@
+      GThread *thread;
+
+      thread = g_thread_new ("GTestTimeout", test_timeout_thread, NULL);

Could use GLIB_PRIVATE_CALL (g_get_worker_context) () too...

::: glib/gtestutils.h
@@ +171,3 @@
+                                         GTestTrapFlags       test_trap_flags);
+
+#define  g_test_trap_subprocess(test_path, usec_timeout, test_trap_flags) \

Use do { ... } while (0) wrapper for all the regular reasons?

Review of attachment 229960 [details] [review]:

Oh one more bit on this patch - let's save the deprecation of g_test_trap_fork() until after the patch where we've ported everything.

I think it's generally cleaner/better to do the new api and deprecation of old as separate patches, because the question of whether or not to deprecate the old immediately is different from adding the new functionality.

Review of attachment 229961 [details] [review]:

I did some spot checks here...looks pretty clean, and really almost as good as the fork() version, but without the downsides.

Review of attachment 229962 [details] [review]:

Hmmm...are you sure that's not intentional?  The docs kind of imply it is...

Review of attachment 229963 [details] [review]:

Ok.

Review of attachment 229964 [details] [review]:

Will trust you on this one...

Review of attachment 229965 [details] [review]:

Ah, yes.

(In reply to comment #15)
> + * Returns %TRUE if g_test_init() has been called.
> + *
> + * Returns: %TRUE if g_test_init() has been called.
> 
> Duplicate docs

yeah, the other g_test_xxx() predicates are written that way too.

(In reply to comment #19)
> Review of attachment 229962 [details] [review]:
> 
> Hmmm...are you sure that's not intentional?  The docs kind of imply it is...

The docs say that -p runs tests "matching TESTPATH". I guess it depends on how you define "matching". The code currently does "TESTPATH and all of its descendants and all of its ancestors". With the patch, it just does "TESTPATH and all of its descendants".

Attachment 229960 [details] pushed as e3a2918 - gtestutils: add g_test_trap_subprocess(), deprecate g_test_trap_fork()
Attachment 229961 [details] pushed as ea06ec8 - tests: port from g_test_trap_subprocess() to g_test_trap_fork()
Attachment 229962 [details] pushed as 723a8f5 - gtestutils: fix "-p" logic
Attachment 229963 [details] pushed as 80253cd - tests/option-context: fix under --verbose
Attachment 229964 [details] pushed as 8d9969f - tests/protocol: redo a bit
Attachment 229965 [details] pushed as 602714a - tests/spawn-*.c: fix on Windows

oops, I was leaving these uncommitted because I knew they still had problems, but I guess that's unkosher...

Matthias, I fixed the hanging-test bug I mentioned on IRC, but there's another issue that still has to be dealt with too, so how about you just push the revert.

patches have been reverted, updating status

(In reply to comment #14)
> I didn't think of this before, but I guess this might silently end up skipping
> tests in some packages if they already had tests with ':' in the name... I
> guess we could have g_test_run() error out if you have any test with ':' in the
> name, but you don't ever call g_test_trap_subprocess()?

After writing that patch, I discovered that in fact, the earlier patch was causing us to skip over existing tests in glib (like /sequence/random/seed:%u). So it's likely other people have test names with ':' in them too, so maybe we should pick something else...

My next thought was to use dots, like unix pathnames... so "/foo/bar/.baz" would be skipped by default. Kinda ugly, but also kinda intuitive, and very unlikely to conflict with anyone else's existing test names. Other suggestions welcome.

> The biggest problem with this patchset right now though is that it very
> frequently hits the gspawn race condition discussed in bug 687061

I was confused, 687061's bug is caused by its own patches and obviously doesn't apply here. The bug turned out to be that g_test_trap_fork() blocks SIGCHLD and then doesn't unblock it, causing all future g_test_trap_subprocess() calls to potentially fail. I couldn't find any reason why g_test_trap_fork() blocked SIGCHLD (it seems to work fine without doing that), so I just removed that line.

Created attachment 234733 [details] [review]
g_test_trap_fork: don't blow away the SIGCHLD handler

Not sure why it was doing this, but it's not necessary (all of glib's
tests pass fine without it), and it breaks tests that try to use
g_spawn_sync() or GChildWatchSource after doing a g_test_trap_fork().

Created attachment 234734 [details] [review]
gtestutils: add g_test_trap_subprocess()

g_test_trap_fork() doesn't work on Windows and is potentially flaky on
unix anyway given the fork-but-don't-exec. Replace it with
g_test_trap_subprocess(), which re-spawns the same program with
arguments telling it to run a specific (otherwise-ignored) test case.

Make the existing g_test_trap_fork() unit tests be unix-only (they
never passed on Windows anyway), and add a parallel set of
g_test_trap_subprocess() tests.

Also fix the logic of gtestutils's "-p" argument (which is used by the
subprocess tests); previously if you had tests "/foo/bar" and
"/foo/bar/baz", and ran the test program with "-p /foo/bar/baz", it
would run "/foo/bar" too. Fix that and add tests.

Created attachment 234735 [details] [review]
tests/protocol: redo a bit

Rather than overloading --verbose, just skip the tests that aren't
supposed to be run in the parent process (so that if you do run the
toplevel test with --verbose, it doesn't immediately error out).

Created attachment 234736 [details] [review]
tests: port from g_test_trap_subprocess() to g_test_trap_fork()

Created attachment 234737 [details] [review]
gtestutils: deprecate g_test_trap_fork()

Created attachment 234738 [details] [review]
tests/spawn-*.c: fix on Windows

Need to append ".exe" to the spawned binary name on Windows

Comment on attachment 229959 [details] [review]
win32: suppress fatal error dialog box when running tests

this was already committed

Comment on attachment 229963 [details] [review]
tests/option-context: fix under --verbose

this was actually caused by g_test_trap_subprocess() behaving differently from g_test_trap_fork() with the trap flags

Have you seen https://bugzilla.gnome.org/show_bug.cgi?id=692125 ?  Is there any intersection there?  (Running individual tests in subprocesses vs running subprocesses from tests)

Er, obviously you've seen it, but the question stands =)

(In reply to comment #27)
> My next thought was to use dots, like unix pathnames... so "/foo/bar/.baz"
> would be skipped by default. Kinda ugly, but also kinda intuitive, and very
> unlikely to conflict with anyone else's existing test names. Other suggestions
> welcome.

The latest patches implement this, and everything works now, so this can land if people are happy with that.

Although, one last thing to think about before finalizing the API: almost every single fork/subprocess test uses G_TEST_TRAP_SILENCE_STDOUT | G_TEST_TRAP_SILENCE_STDERR. In the cases where SILENCE_STDOUT isn't used, it's generally because there isn't going to be any STDOUT output anyway. So maybe we should flip the sense of the flags for g_test_trap_subprocess()? Have it default to silencing stdout and stderr by default, add new G_TEST_TRAP_INHERIT_STDOUT and G_TEST_TRAP_INHERIT_STDERR flags to get the opposite behavior, deprecate the _SILENCE_ flags, and warn_if_fail() if someone accidentally passes them to g_test_trap_subprocess().

Additionally, it seems like it would make sense that if g_test_trap_assert_passed() fails, that it should automatically print out the stderr (and stdout?) of the last fork/subprocess test, if it had been silenced. ?

(In reply to comment #36)
> Have you seen https://bugzilla.gnome.org/show_bug.cgi?id=692125 ?  Is there any
> intersection there?  (Running individual tests in subprocesses vs running
> subprocesses from tests)

Lots of merge conflicts for whoever commits last I'm sure. :)

I think these patches would be useful as infrastructure for running every test in its own subprocess. Eg, g_test_run() would do something like:

  if (g_test_subprocess ())
    g_test_case_run (tc)
  else
    {
      g_test_trap_subprocess (tc->path, ...);
      g_test_trap_assert_passed ();
    }

But it might make sense to do some things differently too...

I think we'd still want to have the g_test_trap_subprocess() API there anyway though, even if all tests are being run as subprocesses, because you want to be able to do g_test_trap_assert_stderr() and stuff after running the child test, so you can't just have g_test_add_expect_fail() or whatever.

Created attachment 243921 [details] [review]
gtestutils: add g_test_trap_subprocess()

g_test_trap_fork() doesn't work on Windows and is potentially flaky on
unix anyway given the fork-but-don't-exec. Replace it with
g_test_trap_subprocess(), which re-spawns the same program with
arguments telling it to run a specific (otherwise-ignored) test case.

Make the existing g_test_trap_fork() unit tests be unix-only (they
never passed on Windows anyway), and add a parallel set of
g_test_trap_subprocess() tests.

Also fix the logic of gtestutils's "-p" argument (which is used by the
subprocess tests); previously if you had tests "/foo/bar" and
"/foo/bar/baz", and ran the test program with "-p /foo/bar/baz", it
would run "/foo/bar" too. Fix that and add tests.

Created attachment 243922 [details] [review]
tests/protocol: redo a bit

Rather than overloading --verbose, just skip the tests that aren't
supposed to be run in the parent process (so that if you do run the
toplevel test with --verbose, it doesn't immediately error out).

Created attachment 243923 [details] [review]
tests: port from g_test_trap_subprocess() to g_test_trap_fork()

Created attachment 243924 [details] [review]
gtestutils: deprecate g_test_trap_fork()

(In reply to comment #38)
> (In reply to comment #27)
> > My next thought was to use dots, like unix pathnames... so "/foo/bar/.baz"
> > would be skipped by default. Kinda ugly, but also kinda intuitive, and very
> > unlikely to conflict with anyone else's existing test names. Other suggestions
> > welcome.
> 
> The latest patches implement this, and everything works now, so this can land
> if people are happy with that.

I never really grew to like this, so the latest version goes with "/foo/bar/subprocess/baz" instead.

> Although, one last thing to think about before finalizing the API: almost every
> single fork/subprocess test uses G_TEST_TRAP_SILENCE_STDOUT |
> G_TEST_TRAP_SILENCE_STDERR. In the cases where SILENCE_STDOUT isn't used, it's
> generally because there isn't going to be any STDOUT output anyway. So maybe we
> should flip the sense of the flags for g_test_trap_subprocess()?

I did this; g_test_trap_subprocess() now takes GTestSubprocessFlags instead of GTestTrapFlags, and it is silent by default, with flags if you don't want that.

> Additionally, it seems like it would make sense that if
> g_test_trap_assert_passed() fails, that it should automatically print out the
> stderr (and stdout?) of the last fork/subprocess test, if it had been silenced.

Oh, I forgot about this idea, so I didn't implement that, but we still could. (Could do it later too though.)


One big change in this patch relative to the earlier one is that I realized that g_test_trap_subprocess's implementation of !SILENCE_STDOUT / !SILENCE_STDERR would break g_test_trap_assert_stdout(), assert_stdout_unmatched(), etc; you could either see the output, or test against it. Fixing this required using g_spawn_async_with_pipes() rather than g_spawn_sync(), which added a bunch more code. But it seems to work...

Review of attachment 234733 [details] [review]:

I can't think why that would be necessary either; looks good.

Review of attachment 234738 [details] [review]:

Sure.

Review of attachment 243921 [details] [review]:

::: glib/gtestutils.c
@@ -2073,3 @@
-    wr = waitpid (pid, status, 0);
-  while (wr < 0 && errno == EINTR);
-  return wr;

Wow...this is some ugly code you're deleting.  Nice!

@@ +2153,3 @@
   test_trap_last_pid = 0;
+  g_free (test_trap_last_subprocess);
+  test_trap_last_subprocess = NULL;

I know you're just fitting in with old code, but g_clear_pointer(), we can convert the other bits later?

@@ +2277,3 @@
+            nwrote = write (echo_fd, buf + total, nread - total);
+          while (nwrote == -1 && errno == EINTR);
+          g_assert (nwrote != -1);

I'm not sure we should core dump if the user pipes output of the test program to "less" for example, and presses "q" before we're done writing output (i.e. we get EPIPE).

How about just _exit (1) ?

@@ +2278,3 @@
+          while (nwrote == -1 && errno == EINTR);
+          g_assert (nwrote != -1);
+          total += nwrote;

Also we should really have a private _writeall() somewhere...

@@ -2303,3 @@
-                  break;
-                }
-            }

And lots more ugly code deleted!  \o/

@@ +2498,3 @@
+ *     g_test_add_func ("/myobject/create_large_object",
+ *                      test_create_large_object);
+ *     /&ast; Because of the '.' at the start of the name, this test will

Because of the /subprocess in the name, ...

@@ +2527,3 @@
+    g_error ("g_test_trap_subprocess: test does not exist: %s", test_path);
+
+  if (g_test_verbose())

Spaces between identifer and paren.

Review of attachment 243922 [details] [review]:

This sounds right.

Review of attachment 243923 [details] [review]:

Spot checks look fine.

::: glib/tests/slice.c
@@ +3,2 @@
 /* We test deprecated functionality here */
+G_GNUC_BEGIN_IGNORE_DEPRECATIONS

Could be a separate patch.

Review of attachment 243924 [details] [review]:

Looks right.

(In reply to comment #47)
> I know you're just fitting in with old code, but g_clear_pointer(), we can
> convert the other bits later?

I've left it as it was, and then added another commit fixing all three of them.

> +          g_assert (nwrote != -1);
> 
> I'm not sure we should core dump if the user pipes output of the test program
> to "less" for example, and presses "q" before we're done writing output (i.e.
> we get EPIPE).

OK, changed to g_error().

(In reply to comment #49)
> Could be a separate patch.

done.


OK to commit with those changes and the others you pointed out?

(In reply to comment #51)
> (In reply to comment #47)

> OK to commit with those changes and the others you pointed out?

Yep!  Great work here.

wooo

Attachment 234733 [details] pushed as 38859e8 - g_test_trap_fork: don't blow away the SIGCHLD handler
Attachment 234738 [details] pushed as 4c35644 - tests/spawn-*.c: fix on Windows
Attachment 243921 [details] pushed as 960f550 - gtestutils: add g_test_trap_subprocess()
Attachment 243922 [details] pushed as 467f9ea - tests/protocol: redo a bit
Attachment 243923 [details] pushed as e3d1869 - tests: port from g_test_trap_subprocess() to g_test_trap_fork()
Attachment 243924 [details] pushed as e9284ed - gtestutils: deprecate g_test_trap_fork()