From 3689fd9f8d3da65e7429edbc2de918e875cc27dc Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 5 Nov 2019 09:51:47 +0000 Subject: [PATCH 1/5] tests: Use g_assert_*() instead of g_assert() in bookmarkfile test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `g_assert_*()` gives more useful messages on failure, and isn’t compiled out by `G_DISABLE_ASSERT`. Signed-off-by: Philip Withnall --- glib/tests/bookmarkfile.c | 88 +++++++++++++++++++-------------------- 1 file changed, 44 insertions(+), 44 deletions(-) diff --git a/glib/tests/bookmarkfile.c b/glib/tests/bookmarkfile.c index d2ece45a4..a0d842c26 100644 --- a/glib/tests/bookmarkfile.c +++ b/glib/tests/bookmarkfile.c @@ -28,7 +28,7 @@ test_load_from_data_dirs (void) res = g_bookmark_file_load_from_data_dirs (bookmark, "no-such-bookmark-file.xbel", &path, &error); - g_assert (!res); + g_assert_false (res); g_assert_error (error, G_FILE_ERROR, G_FILE_ERROR_NOENT); g_assert_null (path); g_error_free (error); @@ -49,19 +49,19 @@ test_to_file (void) filename = g_test_get_filename (G_TEST_DIST, "bookmarks", "valid-01.xbel", NULL); res = g_bookmark_file_load_from_file (bookmark, filename, &error); - g_assert (res); + g_assert_true (res); g_assert_no_error (error); res = g_bookmark_file_to_file (bookmark, "out.xbel", &error); - g_assert (res); + g_assert_true (res); g_assert_no_error (error); res = g_file_get_contents (filename, &in, NULL, &error); - g_assert (res); + g_assert_true (res); g_assert_no_error (error); res = g_file_get_contents ("out.xbel", &out, NULL, &error); - g_assert (res); + g_assert_true (res); g_assert_no_error (error); remove ("out.xbel"); @@ -84,28 +84,28 @@ test_move_item (void) filename = g_test_get_filename (G_TEST_DIST, "bookmarks", "valid-01.xbel", NULL); res = g_bookmark_file_load_from_file (bookmark, filename, &error); - g_assert (res); + g_assert_true (res); g_assert_no_error (error); res = g_bookmark_file_move_item (bookmark, "file:///home/zefram/Documents/milan-stuttgart.ps", "file:///tmp/schedule.ps", &error); - g_assert (res); + g_assert_true (res); g_assert_no_error (error); res = g_bookmark_file_move_item (bookmark, "file:///tmp/schedule.ps", "file:///tmp/schedule.ps", &error); - g_assert (res); + g_assert_true (res); g_assert_no_error (error); res = g_bookmark_file_move_item (bookmark, "file:///no-such-file.xbel", "file:///tmp/schedule.ps", &error); - g_assert (!res); + g_assert_false (res); g_assert_error (error, G_BOOKMARK_FILE_ERROR, G_BOOKMARK_FILE_ERROR_URI_NOT_FOUND); g_clear_error (&error); @@ -113,7 +113,7 @@ test_move_item (void) "file:///tmp/schedule.ps", NULL, &error); - g_assert (res); + g_assert_true (res); g_assert_no_error (error); g_bookmark_file_free (bookmark); @@ -135,7 +135,7 @@ test_misc (void) filename = g_test_get_filename (G_TEST_DIST, "bookmarks", "valid-01.xbel", NULL); res = g_bookmark_file_load_from_file (bookmark, filename, &error); - g_assert (res); + g_assert_true (res); g_assert_no_error (error); res = g_bookmark_file_get_icon (bookmark, @@ -143,7 +143,7 @@ test_misc (void) NULL, NULL, &error); - g_assert (!res); + g_assert_false (res); g_assert_no_error (error); res = g_bookmark_file_get_icon (bookmark, @@ -151,7 +151,7 @@ test_misc (void) NULL, NULL, &error); - g_assert (!res); + g_assert_false (res); g_assert_error (error, G_BOOKMARK_FILE_ERROR, G_BOOKMARK_FILE_ERROR_URI_NOT_FOUND); g_clear_error (&error); @@ -194,7 +194,7 @@ test_misc (void) "file:///tmp/schedule2.ps", &error); g_assert_no_error (error); - g_assert (res); + g_assert_true (res); time (&now); g_bookmark_file_set_added (bookmark, @@ -204,7 +204,7 @@ test_misc (void) "file:///tmp/schedule3.ps", &error); g_assert_no_error (error); - g_assert (t == now); + g_assert_cmpint (t, ==, now); g_bookmark_file_set_modified (bookmark, "file:///tmp/schedule4.ps", @@ -213,7 +213,7 @@ test_misc (void) "file:///tmp/schedule4.ps", &error); g_assert_no_error (error); - g_assert (t == now); + g_assert_cmpint (t, ==, now); g_bookmark_file_set_visited (bookmark, "file:///tmp/schedule5.ps", @@ -222,7 +222,7 @@ test_misc (void) "file:///tmp/schedule5.ps", &error); g_assert_no_error (error); - g_assert (t == now); + g_assert_cmpint (t, ==, now); g_bookmark_file_set_icon (bookmark, "file:///tmp/schedule6.ps", @@ -234,7 +234,7 @@ test_misc (void) NULL, &error); g_assert_no_error (error); - g_assert (res); + g_assert_true (res); g_assert_cmpstr (s, ==, "application-x-postscript"); g_free (s); @@ -247,14 +247,14 @@ test_misc (void) NULL, &error); g_assert_no_error (error); - g_assert (!res); + g_assert_false (res); res = g_bookmark_file_has_application (bookmark, "file:///tmp/schedule7.ps", "foo", &error); g_assert_error (error, G_BOOKMARK_FILE_ERROR, G_BOOKMARK_FILE_ERROR_URI_NOT_FOUND); - g_assert (!res); + g_assert_false (res); g_clear_error (&error); g_bookmark_file_add_application (bookmark, @@ -266,13 +266,13 @@ test_misc (void) &exec, &count, &t, &error); g_assert_no_error (error); - g_assert (res); + g_assert_true (res); cmd = g_strconcat (g_get_prgname (), " file:///tmp/schedule7.ps", NULL); g_assert_cmpstr (exec, ==, cmd); g_free (cmd); g_free (exec); g_assert_cmpuint (count, ==, 1); - g_assert (t == now); + g_assert_cmpint (t, ==, now); g_bookmark_file_free (bookmark); } @@ -308,19 +308,19 @@ test_query (GBookmarkFile *bookmark) for (i = 0; i < uris_len; i++) { - g_assert (g_bookmark_file_has_item (bookmark, uris[i])); + g_assert_true (g_bookmark_file_has_item (bookmark, uris[i])); error = NULL; mime = g_bookmark_file_get_mime_type (bookmark, uris[i], &error); - g_assert (mime != NULL); + g_assert_nonnull (mime); g_assert_no_error (error); g_free (mime); } g_strfreev (uris); - g_assert (!g_bookmark_file_has_item (bookmark, "file:///no/such/uri")); + g_assert_false (g_bookmark_file_has_item (bookmark, "file:///no/such/uri")); error = NULL; mime = g_bookmark_file_get_mime_type (bookmark, "file:///no/such/uri", &error); - g_assert (mime == NULL); + g_assert_null (mime); g_assert_error (error, G_BOOKMARK_FILE_ERROR, G_BOOKMARK_FILE_ERROR_URI_NOT_FOUND); g_error_free (error); g_free (mime); @@ -376,18 +376,18 @@ test_modify (GBookmarkFile *bookmark) g_assert_no_error (error); g_assert_cmpstr (text, ==, "a description"); g_free (text); - g_assert (g_bookmark_file_get_is_private (bookmark, TEST_URI_0, &error)); + g_assert_true (g_bookmark_file_get_is_private (bookmark, TEST_URI_0, &error)); g_assert_no_error (error); stamp = g_bookmark_file_get_added (bookmark, TEST_URI_0, &error); g_assert_no_error (error); - g_assert (stamp == now); + g_assert_cmpint (stamp, ==, now); stamp = g_bookmark_file_get_modified (bookmark, TEST_URI_0, &error); g_assert_no_error (error); - g_assert (stamp == now); + g_assert_cmpint (stamp, ==, now); stamp = g_bookmark_file_get_visited (bookmark, TEST_URI_0, &error); g_assert_no_error (error); - g_assert (stamp == now); - g_assert (g_bookmark_file_get_icon (bookmark, TEST_URI_0, &icon, &mime, &error)); + g_assert_cmpint (stamp, ==, now); + g_assert_true (g_bookmark_file_get_icon (bookmark, TEST_URI_0, &icon, &mime, &error)); g_assert_no_error (error); g_assert_cmpstr (icon, ==, "testicon"); g_assert_cmpstr (mime, ==, "image/png"); @@ -419,21 +419,21 @@ test_modify (GBookmarkFile *bookmark) if (g_test_verbose ()) g_printerr ("\t=> check application..."); g_bookmark_file_set_mime_type (bookmark, TEST_URI_0, TEST_MIME); - g_assert (!g_bookmark_file_has_application (bookmark, TEST_URI_0, TEST_APP_NAME, NULL)); + g_assert_false (g_bookmark_file_has_application (bookmark, TEST_URI_0, TEST_APP_NAME, NULL)); g_bookmark_file_add_application (bookmark, TEST_URI_0, TEST_APP_NAME, TEST_APP_EXEC); - g_assert (g_bookmark_file_has_application (bookmark, TEST_URI_0, TEST_APP_NAME, NULL)); + g_assert_true (g_bookmark_file_has_application (bookmark, TEST_URI_0, TEST_APP_NAME, NULL)); g_bookmark_file_get_app_info (bookmark, TEST_URI_0, TEST_APP_NAME, &text, &count, &stamp, &error); g_assert_no_error (error); - g_assert (count == 1); - g_assert (stamp == g_bookmark_file_get_modified (bookmark, TEST_URI_0, NULL)); + g_assert_cmpuint (count, ==, 1); + g_assert_cmpint (stamp, ==, g_bookmark_file_get_modified (bookmark, TEST_URI_0, NULL)); g_free (text); - g_assert (g_bookmark_file_remove_application (bookmark, TEST_URI_0, TEST_APP_NAME, &error)); + g_assert_true (g_bookmark_file_remove_application (bookmark, TEST_URI_0, TEST_APP_NAME, &error)); g_assert_no_error (error); g_bookmark_file_add_application (bookmark, TEST_URI_0, TEST_APP_NAME, TEST_APP_EXEC); apps = g_bookmark_file_get_applications (bookmark, TEST_URI_0, &length, &error); @@ -455,11 +455,11 @@ test_modify (GBookmarkFile *bookmark) if (g_test_verbose ()) g_printerr ("\t=> check groups..."); - g_assert (!g_bookmark_file_has_group (bookmark, TEST_URI_1, "Test", NULL)); + g_assert_false (g_bookmark_file_has_group (bookmark, TEST_URI_1, "Test", NULL)); g_bookmark_file_add_group (bookmark, TEST_URI_1, "Test"); - g_assert (g_bookmark_file_has_group (bookmark, TEST_URI_1, "Test", NULL)); - g_assert (!g_bookmark_file_has_group (bookmark, TEST_URI_1, "Fail", NULL)); - g_assert (g_bookmark_file_remove_group (bookmark, TEST_URI_1, "Test", &error)); + g_assert_true (g_bookmark_file_has_group (bookmark, TEST_URI_1, "Test", NULL)); + g_assert_false (g_bookmark_file_has_group (bookmark, TEST_URI_1, "Fail", NULL)); + g_assert_true (g_bookmark_file_remove_group (bookmark, TEST_URI_1, "Test", &error)); g_assert_no_error (error); groups = g_bookmark_file_get_groups (bookmark, TEST_URI_1, NULL, &error); g_assert_cmpint (g_strv_length (groups), ==, 0); @@ -480,9 +480,9 @@ test_modify (GBookmarkFile *bookmark) if (g_test_verbose ()) g_printerr ("\t=> check remove..."); - g_assert (g_bookmark_file_remove_item (bookmark, TEST_URI_1, &error) == TRUE); + g_assert_true (g_bookmark_file_remove_item (bookmark, TEST_URI_1, &error)); g_assert_no_error (error); - g_assert (g_bookmark_file_remove_item (bookmark, TEST_URI_1, &error) == FALSE); + g_assert_false (g_bookmark_file_remove_item (bookmark, TEST_URI_1, &error)); g_assert_error (error, G_BOOKMARK_FILE_ERROR, G_BOOKMARK_FILE_ERROR_URI_NOT_FOUND); g_clear_error (&error); if (g_test_verbose ()) @@ -501,7 +501,7 @@ test_file (gconstpointer d) GError *error; bookmark_file = g_bookmark_file_new (); - g_assert (bookmark_file != NULL); + g_assert_nonnull (bookmark_file); success = test_load (bookmark_file, filename); @@ -519,7 +519,7 @@ test_file (gconstpointer d) g_bookmark_file_free (bookmark_file); - g_assert (success == (strstr (filename, "fail") == NULL)); + g_assert_true (success == (strstr (filename, "fail") == NULL)); } int From 4332e3b160a1fef92f86b38be3b7286712925d67 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 5 Nov 2019 10:08:45 +0000 Subject: [PATCH 2/5] gbookmarkfile: Fix a minor leak on an error path Signed-off-by: Philip Withnall --- glib/gbookmarkfile.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/glib/gbookmarkfile.c b/glib/gbookmarkfile.c index 25f1234d7..e22f794e8 100644 --- a/glib/gbookmarkfile.c +++ b/glib/gbookmarkfile.c @@ -775,13 +775,22 @@ parse_bookmark_element (GMarkupParseContext *context, item = bookmark_item_new (uri); if (added != NULL && !timestamp_from_iso8601 (added, &item->added, error)) - return; + { + bookmark_item_free (item); + return; + } if (modified != NULL && !timestamp_from_iso8601 (modified, &item->modified, error)) - return; + { + bookmark_item_free (item); + return; + } if (visited != NULL && !timestamp_from_iso8601 (visited, &item->visited, error)) - return; + { + bookmark_item_free (item); + return; + } add_error = NULL; g_bookmark_file_add_item (parse_data->bookmark_file, From a9900b774abe32815a73e00924d299b0a5d67aa1 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 5 Nov 2019 10:09:00 +0000 Subject: [PATCH 3/5] tests: Fix race conditions in bookmarkfile tests The time handling was assuming that the test would complete in the same second as it started, which was not always true. Signed-off-by: Philip Withnall Fixes: #1930 --- glib/tests/bookmarkfile.c | 43 +++++++++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 11 deletions(-) diff --git a/glib/tests/bookmarkfile.c b/glib/tests/bookmarkfile.c index a0d842c26..75daeaf22 100644 --- a/glib/tests/bookmarkfile.c +++ b/glib/tests/bookmarkfile.c @@ -127,7 +127,7 @@ test_misc (void) gboolean res; GError *error = NULL; gchar *s; - time_t now, t; + time_t before, after, t; gchar *cmd, *exec; guint count; @@ -196,7 +196,8 @@ test_misc (void) g_assert_no_error (error); g_assert_true (res); - time (&now); + time (&before); + g_bookmark_file_set_added (bookmark, "file:///tmp/schedule3.ps", (time_t)-1); @@ -204,7 +205,12 @@ test_misc (void) "file:///tmp/schedule3.ps", &error); g_assert_no_error (error); - g_assert_cmpint (t, ==, now); + + time (&after); + g_assert_cmpint (before, <=, t); + g_assert_cmpint (t, <=, after); + + time (&before); g_bookmark_file_set_modified (bookmark, "file:///tmp/schedule4.ps", @@ -213,7 +219,12 @@ test_misc (void) "file:///tmp/schedule4.ps", &error); g_assert_no_error (error); - g_assert_cmpint (t, ==, now); + + time (&after); + g_assert_cmpint (before, <=, t); + g_assert_cmpint (t, <=, after); + + time (&before); g_bookmark_file_set_visited (bookmark, "file:///tmp/schedule5.ps", @@ -222,7 +233,10 @@ test_misc (void) "file:///tmp/schedule5.ps", &error); g_assert_no_error (error); - g_assert_cmpint (t, ==, now); + + time (&after); + g_assert_cmpint (before, <=, t); + g_assert_cmpint (t, <=, after); g_bookmark_file_set_icon (bookmark, "file:///tmp/schedule6.ps", @@ -257,7 +271,9 @@ test_misc (void) g_assert_false (res); g_clear_error (&error); - g_bookmark_file_add_application (bookmark, + time (&before); + + g_bookmark_file_add_application (bookmark, "file:///tmp/schedule7.ps", NULL, NULL); res = g_bookmark_file_get_app_info (bookmark, @@ -272,7 +288,9 @@ test_misc (void) g_free (cmd); g_free (exec); g_assert_cmpuint (count, ==, 1); - g_assert_cmpint (t, ==, now); + time (&after); + g_assert_cmpint (before, <=, t); + g_assert_cmpint (t, <=, after); g_bookmark_file_free (bookmark); } @@ -364,10 +382,16 @@ test_modify (GBookmarkFile *bookmark) g_bookmark_file_set_is_private (bookmark, TEST_URI_0, TRUE); time (&now); g_bookmark_file_set_added (bookmark, TEST_URI_0, now); - g_bookmark_file_set_modified (bookmark, TEST_URI_0, now); g_bookmark_file_set_visited (bookmark, TEST_URI_0, now); g_bookmark_file_set_icon (bookmark, TEST_URI_0, "testicon", "image/png"); + /* Check the modification date by itself, as it’s updated whenever we modify + * other properties. */ + g_bookmark_file_set_modified (bookmark, TEST_URI_0, now); + stamp = g_bookmark_file_get_modified (bookmark, TEST_URI_0, &error); + g_assert_no_error (error); + g_assert_cmpint (stamp, ==, now); + text = g_bookmark_file_get_title (bookmark, TEST_URI_0, &error); g_assert_no_error (error); g_assert_cmpstr (text, ==, "a title"); @@ -381,9 +405,6 @@ test_modify (GBookmarkFile *bookmark) stamp = g_bookmark_file_get_added (bookmark, TEST_URI_0, &error); g_assert_no_error (error); g_assert_cmpint (stamp, ==, now); - stamp = g_bookmark_file_get_modified (bookmark, TEST_URI_0, &error); - g_assert_no_error (error); - g_assert_cmpint (stamp, ==, now); stamp = g_bookmark_file_get_visited (bookmark, TEST_URI_0, &error); g_assert_no_error (error); g_assert_cmpint (stamp, ==, now); From a86c994255c760b36b6ea3590a31dd45d3148d68 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 5 Nov 2019 10:19:36 +0000 Subject: [PATCH 4/5] tests: Switch some assertions around In general, we should aim to always check a `GError` before checking a boolean, since the error message from the `GError` gives us a lot more information about failure, which helps with debugging. Signed-off-by: Philip Withnall --- glib/tests/bookmarkfile.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/glib/tests/bookmarkfile.c b/glib/tests/bookmarkfile.c index 75daeaf22..738452e49 100644 --- a/glib/tests/bookmarkfile.c +++ b/glib/tests/bookmarkfile.c @@ -49,20 +49,20 @@ test_to_file (void) filename = g_test_get_filename (G_TEST_DIST, "bookmarks", "valid-01.xbel", NULL); res = g_bookmark_file_load_from_file (bookmark, filename, &error); - g_assert_true (res); g_assert_no_error (error); + g_assert_true (res); res = g_bookmark_file_to_file (bookmark, "out.xbel", &error); - g_assert_true (res); g_assert_no_error (error); + g_assert_true (res); res = g_file_get_contents (filename, &in, NULL, &error); - g_assert_true (res); g_assert_no_error (error); + g_assert_true (res); res = g_file_get_contents ("out.xbel", &out, NULL, &error); - g_assert_true (res); g_assert_no_error (error); + g_assert_true (res); remove ("out.xbel"); g_assert_cmpstr (in, ==, out); From 422feb743d4024fc87b5dc3eebfdbd9567fffae3 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 5 Nov 2019 10:20:15 +0000 Subject: [PATCH 5/5] tests: Enable G_TEST_OPTIONS_ISOLATE_DIRS for bookmarkfile test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This ensures that when running many instances of the test in parallel, they don’t collide in the same current directory, and hence spuriously fail. This can happen when writing `out.xbel`, for example. Signed-off-by: Philip Withnall Fixes: #1930 --- glib/tests/bookmarkfile.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/glib/tests/bookmarkfile.c b/glib/tests/bookmarkfile.c index 738452e49..f5622968b 100644 --- a/glib/tests/bookmarkfile.c +++ b/glib/tests/bookmarkfile.c @@ -551,7 +551,7 @@ main (int argc, char *argv[]) const gchar *name; gchar *path; - g_test_init (&argc, &argv, NULL); + g_test_init (&argc, &argv, G_TEST_OPTION_ISOLATE_DIRS, NULL); if (argc > 1) {