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 710991 - test: g_debug messages shouldn't affect g_assert_expected_messages
test: g_debug messages shouldn't affect g_assert_expected_messages
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2013-10-28 09:21 UTC by Stef Walter
Modified: 2013-10-28 20:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
test: g_debug messages shouldn't affect g_assert_expected_messages (3.11 KB, patch)
2013-10-28 09:21 UTC, Stef Walter
accepted-commit_now Details | Review
test: g_debug messages shouldn't affect g_assert_expected_messages (3.78 KB, patch)
2013-10-28 19:47 UTC, Stef Walter
reviewed Details | Review

Description Stef Walter 2013-10-28 09:21:02 UTC
We should be ignoring g_debug messages in the g_assert_expected_messages
code. g_debug messages are often not predictable (contain line numbers
for instance), racy order (in the case of async) and are about how the process 
is implemented rather than testable messages.

Without this patch g_assert_expected_messages() is pretty useless for
projects that use debug messages.
Comment 1 Stef Walter 2013-10-28 09:21:05 UTC
Created attachment 258274 [details] [review]
test: g_debug messages shouldn't affect g_assert_expected_messages

Debug messages are meant to give insight into how a process is
proceeding, and are unpredictable in nature. They also often have
line numbers in them.

This patch ignores debug messages in g_test_expected_messages().
Comment 2 Dan Winship 2013-10-28 14:00:28 UTC
Comment on attachment 258274 [details] [review]
test: g_debug messages shouldn't affect g_assert_expected_messages

Yes, I had the same thought at one point. You want to be able to change the debugging messages without having to update test programs every time.

One possible problem with this patch is that some people may already have tests that are setting expectations for debug messages, which would start failing now because of the new return-if-fail... but let's try it and see.

(A more complicated possibility would be to require them to match if you expicitly expect() them, but to ignore them when they are unexpected.)

>This patch ignores debug messages in g_test_expected_messages().
                                             ^assert_

>+ * Trapping g_debug() messages (ie: messages with G_LOG_LEVEL_DEBUG)
>+ * is also not possible with this function.

Need a "%" on "G_LOG_LEVEL_DEBUG". Also maybe clarify here that g_test_assert_expected_messages() ignores them too?
Comment 3 Stef Walter 2013-10-28 19:47:57 UTC
Created attachment 258338 [details] [review]
test: g_debug messages shouldn't affect g_assert_expected_messages

Debug messages are meant to give insight into how a process is
proceeding, and are unpredictable in nature. They also often have
line numbers in them.

This patch ignores debug messages in g_test_expected_messages().
Comment 4 Stef Walter 2013-10-28 19:49:40 UTC
Implemented your suggestion to match for g_debug() messages if explicitly expected. Added % to docs.
Comment 5 Dan Winship 2013-10-28 20:13:08 UTC
Comment on attachment 258338 [details] [review]
test: g_debug messages shouldn't affect g_assert_expected_messages

>This patch ignores debug messages in g_test_expected_messages().

You're missing "assert" in the function name there.

>+      else if ((log_level & G_LOG_LEVEL_DEBUG) != G_LOG_LEVEL_DEBUG &&
>+               (expected->log_level & G_LOG_LEVEL_DEBUG) != G_LOG_LEVEL_DEBUG)

I don't think you want the second part of that. If the caller expected a DEBUG message, but got a non-DEBUG message, then something went wrong and we should abort. So we don't care about expected->log_level here.

(OK to commit with that fix, assuming you agree.)
Comment 6 Stef Walter 2013-10-28 20:35:44 UTC
Thanks. Pushed.