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 666116 - some tests provoke undefined behaviour, which is undesirable under valgrind
some tests provoke undefined behaviour, which is undesirable under valgrind
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: general
2.31.x
Other Linux
: Normal minor
: ---
Assigned To: Simon McVittie
gtkdev
Depends on:
Blocks:
 
 
Reported: 2011-12-13 19:35 UTC by Simon McVittie
Modified: 2012-01-05 16:17 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
various tests: do not provoke SIGTRAP under valgrind (28.85 KB, patch)
2011-12-13 19:35 UTC, Simon McVittie
none Details | Review
Add undefined/no-undefined mode options to GTester (7.84 KB, patch)
2011-12-14 18:13 UTC, Simon McVittie
none Details | Review
Clarify documentation of fast/slow/thorough and quiet/verbose tests (1.84 KB, patch)
2011-12-14 18:13 UTC, Simon McVittie
committed Details | Review
various tests: do not provoke SIGTRAP with -m no-undefined (27.44 KB, patch)
2011-12-14 18:13 UTC, Simon McVittie
committed Details | Review
[1/3 v2] Add undefined/no-undefined mode options to GTester (8.35 KB, patch)
2011-12-14 20:42 UTC, Simon McVittie
committed Details | Review
[1/2] Skip tests of incorrect property usage under gtester -m no-undefined (1.71 KB, patch)
2012-01-05 15:11 UTC, Simon McVittie
accepted-commit_now Details | Review
[2/2] Mention g_test_undefined() when documenting assert_failed, assert_stderr (1.57 KB, patch)
2012-01-05 15:12 UTC, Simon McVittie
accepted-commit_now Details | Review

Description Simon McVittie 2011-12-13 19:35:14 UTC
Created attachment 203384 [details] [review]
various tests: do not provoke SIGTRAP under valgrind

Some of the GLib tests deliberately provoke warnings (or even fatal
errors) in a forked child. Normally, this is fine, but under valgrind
it's somewhat undesirable. We do want to follow fork(), so we can check
for leaks in child processes that exit gracefully; but we don't want to
be told about "leaks" in processes that are crashing, because there'd
be no point in cleaning those up anyway.

With this patch, defining the environment variable G_TEST_DO_NOT_CRASH
skips the tests that would otherwise crash a child process.

---

I'm open to other suggestions; in particular, the name of the environment variable could probably be better.
Comment 1 Matthias Clasen 2011-12-14 03:16:53 UTC
I would think this would be nicer as an extension of the 'test mode' construct thats already there:

  "  -m {perf|slow|thorough|quick}  Execute tests according modes\n"

together with

#define g_test_quick()                  (g_test_config_vars->test_quick)
#define g_test_slow()                   (!g_test_config_vars->test_quick)
#define g_test_thorough()               (!g_test_config_vars->test_quick)
#define g_test_perf()                   (g_test_config_vars->test_perf)
Comment 2 Simon McVittie 2011-12-14 16:38:27 UTC
(In reply to comment #1)
> I would think this would be nicer as an extension of the 'test mode' construct
> thats already there:

Is it considered to be OK (not an ABI break) to make GTestConfig larger? I could add a g_test_undefined() similar to those, for instance.

At the moment, the -m options are nearly mutually exclusive (although you can use -m perf together with -m thorough or -m slow); do you think this would be better as -m undefined, or as a new --undefined/--no-undefined similar to --quiet and --verbose?
Comment 3 Matthias Clasen 2011-12-14 16:50:06 UTC
I must say the fact that all those structs are exposed there is really ugly.
But I can't see how enlarging the GTestConfig struct could break anything. Its not like you could subclass it or even allocate your own.

I would not read too much into the meaning of these modes; they don't seem very well thought out. I think -m undefined would be fine, and I slightly prefer to stick to -m for options that change the set of tests to run (--quiet and --verbose don't)
Comment 4 Simon McVittie 2011-12-14 18:13:16 UTC
Created attachment 203503 [details] [review]
Add undefined/no-undefined mode options to GTester
Comment 5 Simon McVittie 2011-12-14 18:13:37 UTC
Created attachment 203504 [details] [review]
Clarify documentation of fast/slow/thorough and  quiet/verbose tests

It turns out that there is no middle setting between fast and
slow/thorough, but there *is* a middle setting between quiet and verbose.
Comment 6 Simon McVittie 2011-12-14 18:13:53 UTC
Created attachment 203505 [details] [review]
various tests: do not provoke SIGTRAP with -m  no-undefined

Some of the GLib tests deliberately provoke warnings (or even fatal
errors) in a forked child. Normally, this is fine, but under valgrind
it's somewhat undesirable. We do want to follow fork(), so we can check
for leaks in child processes that exit gracefully; but we don't want to
be told about "leaks" in processes that are crashing, because there'd
be no point in cleaning those up anyway.
Comment 7 Simon McVittie 2011-12-14 20:42:30 UTC
Created attachment 203516 [details] [review]
[1/3 v2] Add undefined/no-undefined mode options to GTester

Oops, insufficient testing. Here's a patch that actually works.
Comment 8 Matthias Clasen 2011-12-17 04:54:38 UTC
Review of attachment 203504 [details] [review]:

Sure
Comment 9 Matthias Clasen 2011-12-17 04:57:31 UTC
Review of attachment 203516 [details] [review]:

Looks good
Comment 10 Matthias Clasen 2011-12-17 04:58:25 UTC
Review of attachment 203505 [details] [review]:

ok
Comment 11 Simon McVittie 2012-01-05 15:11:00 UTC
desrt added more tests of undefined behaviour before my patches were applied, so this isn't fully fixed. I'll also add some documentation improvements.
Comment 12 Simon McVittie 2012-01-05 15:11:59 UTC
Created attachment 204691 [details] [review]
[1/2] Skip tests of incorrect property usage under gtester -m  no-undefined

---

These tests were added since I wrote my first round of patches, but before they were actually applied.
Comment 13 Simon McVittie 2012-01-05 15:12:32 UTC
Created attachment 204692 [details] [review]
[2/2] Mention g_test_undefined() when documenting  assert_failed, assert_stderr
Comment 14 Colin Walters 2012-01-05 15:25:13 UTC
Review of attachment 204692 [details] [review]:

Ok.  Though I think the whole idea of forking tests is pretty dubious.
Comment 15 Colin Walters 2012-01-05 15:29:28 UTC
Review of attachment 204691 [details] [review]:

Looks fine.
Comment 16 Simon McVittie 2012-01-05 16:17:14 UTC
Thanks, fixed in 254efaf85e0bb and 2fe964eeb15 for 2.31.7.