GNOME Bugzilla – Bug 666116
some tests provoke undefined behaviour, which is undesirable under valgrind
Last modified: 2012-01-05 16:17: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.
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)
(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?
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)
Created attachment 203503 [details] [review] Add undefined/no-undefined mode options to GTester
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.
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.
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.
Review of attachment 203504 [details] [review]: Sure
Review of attachment 203516 [details] [review]: Looks good
Review of attachment 203505 [details] [review]: ok
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.
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.
Created attachment 204692 [details] [review] [2/2] Mention g_test_undefined() when documenting assert_failed, assert_stderr
Review of attachment 204692 [details] [review]: Ok. Though I think the whole idea of forking tests is pretty dubious.
Review of attachment 204691 [details] [review]: Looks fine.
Thanks, fixed in 254efaf85e0bb and 2fe964eeb15 for 2.31.7.