Re: [PATCH] tests/unit/test-char.c: Fix error handling issues

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 8 Jun 2021 at 20:51, Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
>>
>> Hi
>>
>> On Tue, Jun 8, 2021 at 9:06 PM Peter Maydell <peter.maydell@linaro.org> 
>> wrote:
>>> I think that improving the quality of the failure reporting
>>> in 'make check' is useful, and that we should probably turn
>>> on g_test_set_nonfatal_assertions() everywhere. (The worst that
>>> can happen is that instead of crashing on the assert we proceed
>>> and crash a bit later, I think.) Awkwardly we don't have a single
>>> place where we could put that call, so I guess it's a coccinelle
>>> script to add it to every test's main() function.
>>>
>>
>> I don't have any strong opinion on this. But I don't see much sense in
>> having extra code for things that should never happen.
>
> The point is that I want to make them happen, though...

I'd prefer not to.

Writing tests is tedious enough as it is.  Replacing

    assert COND in one of the many ways GLib provides

by

    assert COND in one of the many ways GLib provides
    if (!COND) {
        bail out
    }

makes it worse.

Readability suffers, too.

>> I would teach coverity instead that those asserts are always fatal.
>
> If you want an assert that's always fatal, that's g_assert().
> These ones are documented as not always fatal.

You'd sacrifice the additional output from g_assert_cmpint() & friends,
which can sometimes save a trip through the debugger.  I don't care all
that much myself, but I know others do.

>> Fwiw, none of the tests in glib or gtk seem to use
>> g_test_set_nonfatal_assertions(), probably for similar considerations.
>
> That's interesting. I did wonder about these APIs, and if glib
> themselves aren't using them that seems like a reason why they're
> so awkward.

Plain assert()'s behavior is configurable at compile time: assertion
checking on / off.  This sets a trap for the unwary: side effects in the
argument.  We avoid the trap by gluing the compile-time switch to "on".

GLib's optionally non-fatal assertions add new traps, with much less
excuse.  Without recovery code, non-fatal assertions make little sense.
But when you have to add recovery code anyway, you could easily switch
to a new set of check functions, too.  Overloading the existing
assertion functions was in bad taste.