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 709615 - Cannot use g_test_expect_message with g_error
Cannot use g_test_expect_message with g_error
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: general
2.36.x
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2013-10-08 09:13 UTC by Nicolas CANIART
Modified: 2013-10-21 18:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Don't g_test_assert_expected_messages for g_error (2.11 KB, patch)
2013-10-19 00:42 UTC, Allison Karlitskaya (desrt)
committed Details | Review

Description Nicolas CANIART 2013-10-08 09:13:22 UTC
Dear GLib Developers,


  In #514920 a "for(;;) ;" was added to the g_error() macro because it was assumed that it should never return. Yet is one uses g_test_expect_message() to try catch the precise error that occured, the g_error() macro will return has the underlying g_logv function does.

  This makes the test case enter in an infinite loop.

This problem was discovered on a Debian Jessie/SID running GLib 2.36.4, but having a quick look a the git repository show that the harmful loop is still present ([1], line 203 in HEAD).


Regards,
Nicolas CANIART.

[1] https://git.gnome.org/browse/glib/tree/glib/gmessages.h#n195
Comment 1 Dan Winship 2013-10-08 13:04:14 UTC
from the g_test_expect_message() docs:

"Note that you cannot use this to test g_error() messages, since g_error() intentionally never returns even if the program doesn't abort; use g_test_trap_subprocess() in this case."


Alternatively though, now that we have G_VERSION_MIN_REQUIRED / G_VERSION_MAX_ALLOWED, we could take one of the various other options in bug 514920 without causing ABI issues, and then make it possible to use g_test_expect_messages() with g_error() as well.
Comment 2 Nicolas CANIART 2013-10-08 16:36:01 UTC
Hi,

  I missed that in the documentation. Yet there is something wrong anyways since g_error() indeed returns, in that case. Either it should not or g_test_expect_message() should reject any G_LOG_LEVEL_ERROR as an input, as it is exceptionnally invalid with respect to the values of the GLogLevelFlags enumeration.

  I personnaly like testing error conditions, as well normal conditions. The goal being, providing the user with as much as possible useful, and *predictible* feedback on what went wrong. Thus the test(s) I wrote that made me stumble on this. And all in all, I think removing "special cases" is generally a good idea in general.

But I am not sure I grasp the whole picture with respect to the 'noreturn' attribute and the "for(;;);" GCC trick. So if there are any technical reasons that would make this a bad idea, I could live with it (feel free to provide quick explanation or pointers. I tryed to look into this but what I found so far is fairly vague).

Thanks,
Nicolas.
Comment 3 Dan Winship 2013-10-08 17:08:17 UTC
Note that you can still use g_test_trap_subprocess() and g_test_trap_assert_stderr() to test g_error()s. It's less convenient, but it's doable.
Comment 4 Nicolas CANIART 2013-10-10 09:01:26 UTC
Hi,

This works only with glib >= 2.38.0, which has not landed in Debian unstable yet. I'll try that as soon as it does. Looking at the doc, it seems simple enough.

I would still have g_test_expect_message() fail if given G_LOG_LEVEL_ERROR as input. You know, for guys who only take a glimpse at a function prototype in the doc, and think they can figure out the rest, without reading the most of the text below. (Yeah, who does that? Right ? ;-)

Then I guess the bug can be closed.
Comment 5 Allison Karlitskaya (desrt) 2013-10-19 00:42:19 UTC
Created attachment 257678 [details] [review]
Don't g_test_assert_expected_messages for g_error

Don't allow the user to assert for expected g_error().  They need to use
subprocess for this.
Comment 6 Colin Walters 2013-10-19 13:14:34 UTC
Review of attachment 257678 [details] [review]:

Makes sense to me.
Comment 7 Allison Karlitskaya (desrt) 2013-10-21 18:33:38 UTC
Pushed.  Thanks.