From 51ce8d204c62bdc67f7d6f5816b3192f43ac6175 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 11 Jul 2018 17:29:49 +0200 Subject: [PATCH 1/2] gtestutils: Document difference between g_assert() and g_assert_*() g_assert() must not be used in tests. g_assert_*() must not be used in production code. Signed-off-by: Philip Withnall https://gitlab.gnome.org/GNOME/glib/issues/976 --- glib/gtestutils.c | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/glib/gtestutils.c b/glib/gtestutils.c index 93f2ba095..6c3d998d5 100644 --- a/glib/gtestutils.c +++ b/glib/gtestutils.c @@ -87,14 +87,18 @@ * creates a test suite called "misc" with a single test case named * "assertions", which consists of running the test_assertions function. * - * In addition to the traditional g_assert(), the test framework provides + * In addition to the traditional g_assert_true(), the test framework provides * an extended set of assertions for comparisons: g_assert_cmpfloat(), * g_assert_cmpfloat_with_epsilon(), g_assert_cmpint(), g_assert_cmpuint(), * g_assert_cmphex(), g_assert_cmpstr(), and g_assert_cmpmem(). The - * advantage of these variants over plain g_assert() is that the assertion + * advantage of these variants over plain g_assert_true() is that the assertion * messages can be more elaborate, and include the values of the compared * entities. * + * Note that g_assert() should not be used in unit tests, since it is a no-op + * when compiling with `G_DISABLE_ASSERT`. Use g_assert() in production code, + * and g_assert_true() in unit tests. + * * A full example of creating a test suite with two tests using fixtures: * |[ * #include @@ -473,7 +477,10 @@ * * The macro can be turned off in final releases of code by defining * `G_DISABLE_ASSERT` when compiling the application, so code must - * not depend on any side effects from @expr. + * not depend on any side effects from @expr. Similarly, it must not be used + * in unit tests, otherwise the unit tests will be ineffective if compiled with + * `G_DISABLE_ASSERT`. Use g_assert_true() and related macros in unit tests + * instead. */ /** @@ -484,7 +491,8 @@ * application is terminated. * * The macro can be turned off in final releases of code by defining - * `G_DISABLE_ASSERT` when compiling the application. + * `G_DISABLE_ASSERT` when compiling the application. Hence, it should not be + * used in unit tests, where assertions should always be effective. */ /** @@ -497,6 +505,10 @@ * an error message is logged and the application is either * terminated or the testcase marked as failed. * + * Note that unlike g_assert(), this macro is unaffected by whether + * `G_DISABLE_ASSERT` is defined. Hence it should only be used in tests and, + * conversely, g_assert() should not be used in tests. + * * See g_test_set_nonfatal_assertions(). * * Since: 2.38 @@ -512,6 +524,10 @@ * an error message is logged and the application is either * terminated or the testcase marked as failed. * + * Note that unlike g_assert(), this macro is unaffected by whether + * `G_DISABLE_ASSERT` is defined. Hence it should only be used in tests and, + * conversely, g_assert() should not be used in tests. + * * See g_test_set_nonfatal_assertions(). * * Since: 2.38 @@ -527,6 +543,10 @@ * an error message is logged and the application is either * terminated or the testcase marked as failed. * + * Note that unlike g_assert(), this macro is unaffected by whether + * `G_DISABLE_ASSERT` is defined. Hence it should only be used in tests and, + * conversely, g_assert() should not be used in tests. + * * See g_test_set_nonfatal_assertions(). * * Since: 2.38 @@ -542,6 +562,10 @@ * an error message is logged and the application is either * terminated or the testcase marked as failed. * + * Note that unlike g_assert(), this macro is unaffected by whether + * `G_DISABLE_ASSERT` is defined. Hence it should only be used in tests and, + * conversely, g_assert() should not be used in tests. + * * See g_test_set_nonfatal_assertions(). * * Since: 2.40 From ca23acdb24b568ed98257f1c462eb83ca527a205 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 11 Jul 2018 17:30:24 +0200 Subject: [PATCH 2/2] gtestutils: Bail out of g_test_init() if G_DISABLE_ASSERT is defined MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If G_DISABLE_ASSERT is defined, g_assert() is a no-op. Despite it now being standard practice to *not* use g_assert() in unit tests (use g_assert_*() instead), a lot of existing unit tests still use it. Compiling those tests with G_DISABLE_ASSERT would make them silently no-ops. Avoid that by warning the user loudly. Note that it’s pretty rare for people to compile with G_DISABLE_ASSERT, so it’s not expected that this will be hit often. Signed-off-by: Philip Withnall https://gitlab.gnome.org/GNOME/glib/issues/976 --- glib/gtestutils.c | 6 +++--- glib/gtestutils.h | 23 +++++++++++++++++++++++ 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/glib/gtestutils.c b/glib/gtestutils.c index 6c3d998d5..da6c7338c 100644 --- a/glib/gtestutils.c +++ b/glib/gtestutils.c @@ -1274,9 +1274,9 @@ parse_args (gint *argc_p, * Since: 2.16 */ void -g_test_init (int *argc, - char ***argv, - ...) +(g_test_init) (int *argc, + char ***argv, + ...) { static char seedstr[4 + 4 * 8 + 1]; va_list args; diff --git a/glib/gtestutils.h b/glib/gtestutils.h index b247f2f3a..550e2414e 100644 --- a/glib/gtestutils.h +++ b/glib/gtestutils.h @@ -147,6 +147,29 @@ GLIB_AVAILABLE_IN_ALL void g_test_init (int *argc, char ***argv, ...) G_GNUC_NULL_TERMINATED; + +/* While we discourage its use, g_assert() is often used in unit tests + * (especially in legacy code). g_assert_*() should really be used instead. + * g_assert() can be disabled at client program compile time, which can render + * tests useless. Highlight that to the user. */ +#ifdef G_DISABLE_ASSERT +#if defined(G_HAVE_ISO_VARARGS) +#define g_test_init(argc, argv, ...) \ + G_STMT_START { \ + g_printerr ("Tests were compiled with G_DISABLE_ASSERT and are likely no-ops. Aborting.\n"); \ + exit (1); \ + } G_STMT_END +#elif defined(G_HAVE_GNUC_VARARGS) +#define g_test_init(argc, argv...) \ + G_STMT_START { \ + g_printerr ("Tests were compiled with G_DISABLE_ASSERT and are likely no-ops. Aborting.\n"); \ + exit (1); \ + } G_STMT_END +#else /* no varargs */ + /* do nothing */ +#endif /* varargs support */ +#endif /* G_DISABLE_ASSERT */ + /* query testing framework config */ #define g_test_initialized() (g_test_config_vars->test_initialized) #define g_test_quick() (g_test_config_vars->test_quick)