From d7bb4664e7f61a53657f851220aec818c0fb5fbb Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 27 Sep 2024 17:06:47 +0100 Subject: [PATCH 1/3] tests: Add a way to get the mock session bus address in tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is better than using `g_getenv ("DBUS_SESSION_BUS_ADDRESS")` as it will fail more explicitly if the mock bus somehow isn’t running. This will be used in an upcoming commit. Signed-off-by: Philip Withnall --- gio/tests/gdbus-sessionbus.c | 7 +++++++ gio/tests/gdbus-sessionbus.h | 1 + 2 files changed, 8 insertions(+) diff --git a/gio/tests/gdbus-sessionbus.c b/gio/tests/gdbus-sessionbus.c index 09d163969..f4dbb4ff9 100644 --- a/gio/tests/gdbus-sessionbus.c +++ b/gio/tests/gdbus-sessionbus.c @@ -71,3 +71,10 @@ session_bus_run (void) return ret; } + +const char * +session_bus_get_address (void) +{ + g_assert (singleton != NULL); + return g_test_dbus_get_bus_address (singleton); +} diff --git a/gio/tests/gdbus-sessionbus.h b/gio/tests/gdbus-sessionbus.h index bd6f2a2ad..9fa80336f 100644 --- a/gio/tests/gdbus-sessionbus.h +++ b/gio/tests/gdbus-sessionbus.h @@ -31,6 +31,7 @@ void session_bus_up (void); void session_bus_stop (void); void session_bus_down (void); gint session_bus_run (void); +const char *session_bus_get_address (void); G_END_DECLS From d7e368f2060f7c7a5358cc1cf9bb8dfb739940f9 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 27 Sep 2024 17:06:40 +0100 Subject: [PATCH 2/3] tests: Move fake-document-portal subprocess inside dbus-appinfo test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For the reasons given in https://gitlab.gnome.org/GNOME/glib/-/merge_requests/4176#note_2233317, it’s best to not rely on subprocesses when writing tests. Spawning a subprocess can go wrong, getting feedback and assertion data from a subprocess is a pain, and making sure the subprocess is killed properly at the end of the test is hard to get right. For tests where we are trying to mock a D-Bus service, it’s much more reliable to run that service in-process (either in the main thread or in a separate thread). So, do that for the `fake-document-portal` former subprocess in the `dbus-appinfo` test: move it to a worker thread. This speeds the test up, simplifies the build slightly, and should make the test run more reliable. In particular, it provides a pattern for future `fake-*-portal` tests to be built off. This is particularly useful for more complex portals, where data needs to be relayed back from the mock portal service to the unit test to check that the code under test has behaved properly. That’s a pain to do from a subprocess. Delete the `org.freedesktop.portal.Documents.service` file because we no longer need to rely on D-Bus service activation within the test, as we’re setting up the mock service thread explicitly now. Signed-off-by: Philip Withnall --- gio/tests/dbus-appinfo.c | 15 ++ gio/tests/fake-document-portal.c | 225 ++++++++++++++---- gio/tests/fake-document-portal.h | 37 +++ gio/tests/meson.build | 20 +- gio/tests/services/meson.build | 1 - ...rg.freedesktop.portal.Documents.service.in | 3 - 6 files changed, 238 insertions(+), 63 deletions(-) create mode 100644 gio/tests/fake-document-portal.h delete mode 100644 gio/tests/services/org.freedesktop.portal.Documents.service.in diff --git a/gio/tests/dbus-appinfo.c b/gio/tests/dbus-appinfo.c index d84de7de9..99b4e24b7 100644 --- a/gio/tests/dbus-appinfo.c +++ b/gio/tests/dbus-appinfo.c @@ -23,6 +23,7 @@ #include #include "gdbus-sessionbus.h" +#include "fake-document-portal.h" static GDesktopAppInfo *appinfo; static int current_state; @@ -348,9 +349,14 @@ test_flatpak_doc_export (void) GDesktopAppInfo *flatpak_appinfo; GApplication *app; int status; + GFakeDocumentPortalThread *thread = NULL; g_test_summary ("Test that files launched via Flatpak apps are made available via the document portal."); + /* Run a fake-document-portal */ + thread = g_fake_document_portal_thread_new (session_bus_get_address ()); + g_fake_document_portal_thread_run (thread); + desktop_file = g_test_build_filename (G_TEST_DIST, "org.gtk.test.dbusappinfo.flatpak.desktop", NULL); @@ -369,6 +375,8 @@ test_flatpak_doc_export (void) g_object_unref (app); g_object_unref (flatpak_appinfo); + g_fake_document_portal_thread_stop (thread); + g_clear_object (&thread); } static void @@ -426,9 +434,14 @@ test_flatpak_missing_doc_export (void) GDesktopAppInfo *flatpak_appinfo; GApplication *app; int status; + GFakeDocumentPortalThread *thread = NULL; g_test_summary ("Test that files launched via Flatpak apps are made available via the document portal."); + /* Run a fake-document-portal */ + thread = g_fake_document_portal_thread_new (session_bus_get_address ()); + g_fake_document_portal_thread_run (thread); + desktop_file = g_test_build_filename (G_TEST_DIST, "org.gtk.test.dbusappinfo.flatpak.desktop", NULL); @@ -447,6 +460,8 @@ test_flatpak_missing_doc_export (void) g_object_unref (app); g_object_unref (flatpak_appinfo); g_free (desktop_file); + g_fake_document_portal_thread_stop (thread); + g_clear_object (&thread); } int diff --git a/gio/tests/fake-document-portal.c b/gio/tests/fake-document-portal.c index 585dee58b..291e374d8 100644 --- a/gio/tests/fake-document-portal.c +++ b/gio/tests/fake-document-portal.c @@ -26,8 +26,56 @@ #include #include +#include "fake-document-portal.h" #include "fake-document-portal-generated.h" +struct _GFakeDocumentPortalThread +{ + GObject parent_instance; + + char *address; /* (not nullable) */ + GCancellable *cancellable; /* (not nullable) (owned) */ + GThread *thread; /* (not nullable) (owned) */ + GCond cond; /* (mutex mutex) */ + GMutex mutex; + gboolean ready; /* (mutex mutex) */ +}; + +G_DEFINE_FINAL_TYPE (GFakeDocumentPortalThread, g_fake_document_portal_thread, G_TYPE_OBJECT) + +static void g_fake_document_portal_thread_finalize (GObject *object); + +static void +g_fake_document_portal_thread_class_init (GFakeDocumentPortalThreadClass *klass) +{ + GObjectClass *gobject_class = G_OBJECT_CLASS (klass); + + gobject_class->finalize = g_fake_document_portal_thread_finalize; +} + +static void +g_fake_document_portal_thread_init (GFakeDocumentPortalThread *self) +{ + self->cancellable = g_cancellable_new (); + g_cond_init (&self->cond); + g_mutex_init (&self->mutex); +} + +static void +g_fake_document_portal_thread_finalize (GObject *object) +{ + GFakeDocumentPortalThread *self = G_FAKE_DOCUMENT_PORTAL_THREAD (object); + + g_assert (self->thread == NULL); /* should already have been joined */ + + g_mutex_clear (&self->mutex); + g_cond_clear (&self->cond); + g_clear_object (&self->cancellable); + g_clear_pointer (&self->address, g_free); + + G_OBJECT_CLASS (g_fake_document_portal_thread_parent_class)->finalize (object); +} + static gboolean on_handle_get_mount_point (FakeDocuments *object, GDBusMethodInvocation *invocation, @@ -76,13 +124,66 @@ on_handle_add_full (FakeDocuments *object, } static void -on_bus_acquired (GDBusConnection *connection, - const gchar *name, - gpointer user_data) +on_name_acquired (GDBusConnection *connection, + const gchar *name, + gpointer user_data) { - FakeDocuments *interface; - GError *error = NULL; + GFakeDocumentPortalThread *self = G_FAKE_DOCUMENT_PORTAL_THREAD (user_data); + g_test_message ("Acquired the name %s", name); + + g_mutex_lock (&self->mutex); + self->ready = TRUE; + g_cond_signal (&self->cond); + g_mutex_unlock (&self->mutex); +} + +static void +on_name_lost (GDBusConnection *connection, + const gchar *name, + gpointer user_data) +{ + g_test_message ("Lost the name %s", name); +} + +static gboolean +cancelled_cb (GCancellable *cancellable, + gpointer user_data) +{ + g_test_message ("fake-document-portal cancelled"); + return G_SOURCE_CONTINUE; +} + +static gpointer +fake_document_portal_thread_cb (gpointer user_data) +{ + GFakeDocumentPortalThread *self = G_FAKE_DOCUMENT_PORTAL_THREAD (user_data); + GMainContext *context = NULL; + GDBusConnection *connection = NULL; + GSource *source = NULL; + guint id; + FakeDocuments *interface = NULL; + GError *local_error = NULL; + + context = g_main_context_new (); + g_main_context_push_thread_default (context); + + connection = g_dbus_connection_new_for_address_sync (self->address, + G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_CLIENT | + G_DBUS_CONNECTION_FLAGS_MESSAGE_BUS_CONNECTION, + NULL, + self->cancellable, + &local_error); + g_assert_no_error (local_error); + + /* Listen for cancellation. The source will wake up the context iteration + * which can then re-check its exit condition below. */ + source = g_cancellable_source_new (self->cancellable); + g_source_set_callback (source, G_SOURCE_FUNC (cancelled_cb), NULL, NULL); + g_source_attach (source, context); + g_source_unref (source); + + /* Set up the interface */ g_test_message ("Acquired a message bus connection"); interface = fake_documents_skeleton_new (); @@ -98,51 +199,81 @@ on_bus_acquired (GDBusConnection *connection, g_dbus_interface_skeleton_export (G_DBUS_INTERFACE_SKELETON (interface), connection, "/org/freedesktop/portal/documents", - &error); - g_assert_no_error (error); -} + &local_error); + g_assert_no_error (local_error); -static void -on_name_acquired (GDBusConnection *connection, - const gchar *name, - gpointer user_data) -{ - g_test_message ("Acquired the name %s", name); -} + /* Own the portal name */ + id = g_bus_own_name_on_connection (connection, + "org.freedesktop.portal.Documents", + G_BUS_NAME_OWNER_FLAGS_ALLOW_REPLACEMENT | + G_BUS_NAME_OWNER_FLAGS_REPLACE, + on_name_acquired, + on_name_lost, + self, + NULL); -static void -on_name_lost (GDBusConnection *connection, - const gchar *name, - gpointer user_data) -{ - g_test_message ("Lost the name %s", name); -} - - -gint -main (gint argc, gchar *argv[]) -{ - GMainLoop *loop; - guint id; - - g_log_writer_default_set_use_stderr (TRUE); - - loop = g_main_loop_new (NULL, FALSE); - - id = g_bus_own_name (G_BUS_TYPE_SESSION, - "org.freedesktop.portal.Documents", - G_BUS_NAME_OWNER_FLAGS_ALLOW_REPLACEMENT | - G_BUS_NAME_OWNER_FLAGS_REPLACE, - on_bus_acquired, - on_name_acquired, - on_name_lost, - loop, - NULL); - - g_main_loop_run (loop); + while (!g_cancellable_is_cancelled (self->cancellable)) + g_main_context_iteration (context, TRUE); g_bus_unown_name (id); - g_main_loop_unref (loop); + g_dbus_interface_skeleton_unexport (G_DBUS_INTERFACE_SKELETON (interface)); + g_clear_object (&interface); + g_clear_object (&connection); + g_main_context_pop_thread_default (context); + g_clear_pointer (&context, g_main_context_unref); - return 0; + return NULL; +} + +/* + * Create a new #GFakeDocumentPortalThread. The thread isn’t started until + * g_fake_document_portal_thread_run() is called on it. + * + * Returns: (transfer full): the new fake document portal wrapper + */ +GFakeDocumentPortalThread * +g_fake_document_portal_thread_new (const char *address) +{ + GFakeDocumentPortalThread *self = g_object_new (G_TYPE_FAKE_DOCUMENT_PORTAL_THREAD, NULL); + self->address = g_strdup (address); + return g_steal_pointer (&self); +} + +/* + * Start a worker thread which will run a fake + * `org.freedesktop.portal.Documents` portal on the bus at @address. This is + * intended to be used with #GTestDBus to mock up a portal from within a unit + * test process, rather than relying on D-Bus activation of a mock portal + * subprocess. + * + * It blocks until the thread has owned its D-Bus name and is ready to handle + * requests. + */ +void +g_fake_document_portal_thread_run (GFakeDocumentPortalThread *self) +{ + g_return_if_fail (G_IS_FAKE_DOCUMENT_PORTAL_THREAD (self)); + g_return_if_fail (self->thread == NULL); + + self->thread = g_thread_new ("fake-document-portal", fake_document_portal_thread_cb, self); + + /* Block until the thread is ready. */ + g_mutex_lock (&self->mutex); + while (!self->ready) + g_cond_wait (&self->cond, &self->mutex); + g_mutex_unlock (&self->mutex); +} + +/* Stop and join a worker thread started with fake_document_portal_thread_run(). + * Blocks until the thread has stopped and joined. + * + * Once this has been called, it’s safe to drop the final reference on @self. */ +void +g_fake_document_portal_thread_stop (GFakeDocumentPortalThread *self) +{ + g_return_if_fail (G_IS_FAKE_DOCUMENT_PORTAL_THREAD (self)); + g_return_if_fail (self->thread != NULL); + + g_cancellable_cancel (self->cancellable); + g_thread_join (g_steal_pointer (&self->thread)); } diff --git a/gio/tests/fake-document-portal.h b/gio/tests/fake-document-portal.h new file mode 100644 index 000000000..f1845b563 --- /dev/null +++ b/gio/tests/fake-document-portal.h @@ -0,0 +1,37 @@ +/* + * Copyright 2024 GNOME Foundation + * + * SPDX-License-Identifier: LGPL-2.1-or-later + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General + * Public License along with this library; if not, see . + */ + +#ifndef __FAKE_DOCUMENT_PORTAL_H__ +#define __FAKE_DOCUMENT_PORTAL_H__ + +#include +#include + +G_BEGIN_DECLS + +#define G_TYPE_FAKE_DOCUMENT_PORTAL_THREAD (g_fake_document_portal_thread_get_type ()) +G_DECLARE_FINAL_TYPE (GFakeDocumentPortalThread, g_fake_document_portal_thread, G, FAKE_DOCUMENT_PORTAL_THREAD, GObject) + +GFakeDocumentPortalThread *g_fake_document_portal_thread_new (const char *address); +void g_fake_document_portal_thread_run (GFakeDocumentPortalThread *self); +void g_fake_document_portal_thread_stop (GFakeDocumentPortalThread *self); + +G_END_DECLS + +#endif /* __FAKE_DOCUMENT_PORTAL_H__ */ diff --git a/gio/tests/meson.build b/gio/tests/meson.build index 2fd1a2a7b..3e4e1e847 100644 --- a/gio/tests/meson.build +++ b/gio/tests/meson.build @@ -544,15 +544,6 @@ if host_machine.system() != 'windows' }, } - if not glib_have_cocoa - gio_tests += { - 'dbus-appinfo' : { - 'extra_sources' : extra_sources, - 'extra_programs' : ['fake-document-portal'], - }, - } - endif - fake_document_portal_generated = custom_target('fake-document-portal-generated', input : ['../org.freedesktop.portal.Documents.xml'], output : ['fake-document-portal-generated.h', @@ -566,10 +557,15 @@ if host_machine.system() != 'windows' '--c-namespace', 'Fake', '@INPUT@']) + if not glib_have_cocoa + gio_tests += { + 'dbus-appinfo' : { + 'extra_sources' : [extra_sources, 'fake-document-portal.c', fake_document_portal_generated], + }, + } + endif + test_extra_programs += { - 'fake-document-portal' : { - 'extra_sources': fake_document_portal_generated, - }, 'fake-service-name' : {} } endif # have_dbus_daemon diff --git a/gio/tests/services/meson.build b/gio/tests/services/meson.build index b6a901bc5..27e8948f4 100644 --- a/gio/tests/services/meson.build +++ b/gio/tests/services/meson.build @@ -1,5 +1,4 @@ dbus_service_files = [ - 'org.freedesktop.portal.Documents.service', 'org.gtk.GDBus.FakeService.service' ] diff --git a/gio/tests/services/org.freedesktop.portal.Documents.service.in b/gio/tests/services/org.freedesktop.portal.Documents.service.in deleted file mode 100644 index 2769ff7b6..000000000 --- a/gio/tests/services/org.freedesktop.portal.Documents.service.in +++ /dev/null @@ -1,3 +0,0 @@ -[D-BUS Service] -Name=org.freedesktop.portal.Documents -Exec=@installed_tests_dir@/fake-document-portal From 6b2cc430e0d8250e4f3e097998ed218b9bf12672 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 27 Sep 2024 17:12:52 +0100 Subject: [PATCH 3/3] tests: Run dbus-appinfo tests with G_TEST_OPTION_ISOLATE_DIRS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eliminate any possibility of them accidentally using the user’s actual home directory. Signed-off-by: Philip Withnall --- gio/tests/dbus-appinfo.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gio/tests/dbus-appinfo.c b/gio/tests/dbus-appinfo.c index 99b4e24b7..80861fdd1 100644 --- a/gio/tests/dbus-appinfo.c +++ b/gio/tests/dbus-appinfo.c @@ -467,7 +467,7 @@ test_flatpak_missing_doc_export (void) int main (int argc, char **argv) { - g_test_init (&argc, &argv, NULL); + g_test_init (&argc, &argv, G_TEST_OPTION_ISOLATE_DIRS, NULL); g_test_add_func ("/appinfo/dbusappinfo", test_dbus_appinfo); g_test_add_func ("/appinfo/flatpak-doc-export", test_flatpak_doc_export);