"-k" means "do not abort the program after the first test that calls g_test_fail()". (IOW, it is behaving as designed, it's just not designed to do something useful.)

The fact that g_assert() and g_assert_not_reached() always abort was also intentional, and I'm pretty sure it's because otherwise they can't be marked G_GNUC_NORETURN, and we don't want to remove that from them.

or probably not "can't be marked G_GNUC_NORETURN", but "if you mark them G_GNUC_NORETURN and then sometimes they return, there's no guarantee that the compiler will actually generate the right code".

Maybe we could have a #define to change their behavior, which could be used in test programs...

(In reply to comment #0)
> this breaks TAP suites pretty badly.

so, I think the fix for this is:

  1. --tap should imply -k

  2. if you run with --tap, and the test program does not call
     set_nonfatal_assertions(), then gtestutils should run each test
     as a separate process (reusing some of the g_test_trap_subprocess()
     infrastructure), so that the individual tests can happily crash
     without crashing the program as a whole.

This is not a compatible change though; programs that do setup/teardown stuff in main() (eg, setting up GTestDBus) will need to be changed to check g_test_subprocess() so they don't redo the setup for each child. So maybe we should add a new flag "--tap-all" for the new behavior, and update the glib-tap.mk stuff to pass that instead of --tap (so that then people won't get the new behavior until they import a new glib-tap.mk).

(In reply to comment #3)
>   2. if you run with --tap, and the test program does not call
>      set_nonfatal_assertions(), then gtestutils should run each test
>      as a separate process

Thought about this some more while playing with libsoup tests...

There's no reason to make this TAP-only, but it does need to be opt-in by the developer, either via an extra function call, or a param in g_test_init()'s varargs.

And I think this is definitely a better approach than g_test_set_nonfatal_assertions(), which sort of does the trick, but leads to really weird code like:

        foo = foo_new (x, y, z, &error);

        g_assert_no_error (error);
        g_clear_error (&error);

        g_assert_nonnull (foo);
        if (foo == NULL)
            return;

        ...

Emmanuele had talked about redefining the macros to be like:

        #define g_assert_nonnull(x) \
            if ((x) == NULL) { g_assertion_message (...); return; }

but that only works if all of your assertions are in the "top level" test functions, which isn't the case for me; I have lots of helper functions that get used by multiple tests.

Another possibility would be to turn all the assert macros into boolean expressions, so you could do:

        g_return_if_fail (g_assert_nonnull (foo));

and then make your helper functions return booleans too so you can do the same thing there.

But even if we do one of these, there's still the possibility that a bug in the library will unexpectedly cause a test to segv or divide by zero or something. So I think "run the test in a subprocess and just let it crash" is the best approach.

(In reply to comment #4)

> But even if we do one of these, there's still the possibility that a bug in the
> library will unexpectedly cause a test to segv or divide by zero or something.
> So I think "run the test in a subprocess and just let it crash" is the best
> approach.

I think this should be the default for every test suite. yes, it makes it mildly inconvenient for a test suite that does set up/tear down, but it's the only sane way to do testing with the live grenade of abort() going off.

if we want to get conservative, we can use only if the --tap argument is given on the command line and fix the GDBus test suite if needed, but I cannot stress this enough: running a test suite that aborts under TAP is utterly, utterly broken, and against the whole point of the TAP support.

Created attachment 292694 [details] [review]
gtestutils: make TAP mode always run all tests (on unix)

When using --tap mode on unix, spawn a subprocess to do the actual
test running, and if it crashes, spawn a new one to pick up where the
first one left off.

Also, drop the "# Start of %s tests" / "#End of %s tests" messages in
the TAP output; they don't add much (since the full pathname of each
test is printed as it runs), and it would be slightly complicated to
not emit them redundantly in the respawning case.

So, this doesn't make any attempt to deal with the "program does lots of setup in main() before running g_test_run()" case, because I forgot about that while I was hacking on it... So something needs to happen there I guess. But even without that, it's pretty backward-compatible; it passes "make check" in GLib without needing any changes to any test programs.

(Possible hack; if --tap is passed, run the tests in a subprocess from g_test_init() rather than g_test_run(), and exit() at the end without ever running the rest of main()...)

Created attachment 298444 [details] [review]
gtestutils: make TAP mode always run all tests (on unix)

When using --tap mode on unix, spawn a subprocess to do the actual
test running, and if it crashes, spawn a new one to pick up where the
first one left off.

Also, drop the "# Start of %s tests" / "#End of %s tests" messages in
the TAP output; they don't add much (since the full pathname of each
test is printed as it runs), and it would be slightly complicated to
not emit them redundantly in the respawning case.

I get the impression that this is going to interact pretty badly with people who do setup tasks in main()...

(In reply to Ryan Lortie (desrt) from comment #9)
> I get the impression that this is going to interact pretty badly with people
> who do setup tasks in main()...

Yes (comment 7).

I guess the easy fix would be to make g_test_run_in_subprocess() be a public function, and then you have to opt in by calling that instead of g_test_run().

But then we'd really need a better way to arrange for global setup/teardown. And ideally we'd want to run the global teardown function even if every actual test case crashed. But maybe now we're getting into "new-and-improved libgtest" territory.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/821.