| Summary: | Simplify call/wait pattern in unit tests | ||
|---|---|---|---|
| Product: | dbus | Reporter: | Simon McVittie <smcv> |
| Component: | core | Assignee: | Simon McVittie <smcv> |
| Status: | RESOLVED FIXED | QA Contact: | D-Bus Maintainers <dbus> |
| Severity: | enhancement | ||
| Priority: | medium | Keywords: | patch |
| Version: | git master | ||
| Hardware: | Other | ||
| OS: | All | ||
| URL: | https://github.com/smcv/dbus/commits/call-wait-103600 | ||
| Whiteboard: | review+ | ||
| i915 platform: | i915 features: | ||
| Attachments: |
[1/8] test_peer_ping: Don't leak one method call per invocation
[2/8] test/dbus-daemon: Don't leak method call messages if we skip tests [3/8] tests: Don't leak pending calls [4/8] test/sd-activation: Make more use of dbus_clear_message() [5/8] tests: Don't use the same variable for call and reply [6/8] test-utils: Use TAP syntax to die with a fatal error [7/8] test_main_context_call_and_wait: Add [8/8] tests: Use test_main_context_call_and_wait |
||
Created attachment 135626 [details] [review] [1/8] test_peer_ping: Don't leak one method call per invocation Previously, we allocated m both during initialization, and after deciding not to skip this test. Created attachment 135627 [details] [review] [2/8] test/dbus-daemon: Don't leak method call messages if we skip tests Created attachment 135628 [details] [review] [3/8] tests: Don't leak pending calls Created attachment 135629 [details] [review] [4/8] test/sd-activation: Make more use of dbus_clear_message() Created attachment 135630 [details] [review] [5/8] tests: Don't use the same variable for call and reply Created attachment 135631 [details] [review] [6/8] test-utils: Use TAP syntax to die with a fatal error --- Not particularly related, but I happened to notice it while I was there. Created attachment 135632 [details] [review] [7/8] test_main_context_call_and_wait: Add Created attachment 135633 [details] [review] [8/8] tests: Use test_main_context_call_and_wait Also use test_oom() where the relevant lines are changing anyway. Comment on attachment 135626 [details] [review] [1/8] test_peer_ping: Don't leak one method call per invocation Review of attachment 135626 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 135627 [details] [review] [2/8] test/dbus-daemon: Don't leak method call messages if we skip tests Review of attachment 135627 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 135628 [details] [review] [3/8] tests: Don't leak pending calls Review of attachment 135628 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 135629 [details] [review] [4/8] test/sd-activation: Make more use of dbus_clear_message() Review of attachment 135629 [details] [review]: ----------------------------------------------------------------- ++, very clear. Comment on attachment 135630 [details] [review] [5/8] tests: Don't use the same variable for call and reply Review of attachment 135630 [details] [review]: ----------------------------------------------------------------- ++, I’d call that good. Comment on attachment 135631 [details] [review] [6/8] test-utils: Use TAP syntax to die with a fatal error Review of attachment 135631 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 135632 [details] [review] [7/8] test_main_context_call_and_wait: Add Review of attachment 135632 [details] [review]: ----------------------------------------------------------------- Looks good. Comment on attachment 135633 [details] [review] [8/8] tests: Use test_main_context_call_and_wait Review of attachment 135633 [details] [review]: ----------------------------------------------------------------- ++ Thanks, fixed in git for 1.13.0. None of this seemed sufficiently high-impact to be worthwhile in 1.12.x. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.
This is a frequent pattern in the unit tests: DBusMessage *m; DBusPendingCall *pc; if (!dbus_connection_send_with_reply (f->left_conn, m, &pc, DBUS_TIMEOUT_USE_DEFAULT) || pc == NULL) g_error ("OOM"); dbus_clear_message (&m); if (dbus_pending_call_get_completed (pc)) test_pending_call_store_reply (pc, &m); else if (!dbus_pending_call_set_notify (pc, test_pending_call_store_reply, &m, NULL)) g_error ("OOM"); while (m == NULL) test_main_context_iterate (f->ctx, TRUE); or sometimes with a separate variable for the reply: DBusMessage *m, *reply; DBusPendingCall *pc; if (!dbus_connection_send_with_reply (f->left_conn, m, &pc, DBUS_TIMEOUT_USE_DEFAULT) || pc == NULL) g_error ("OOM"); dbus_clear_message (&m); if (dbus_pending_call_get_completed (pc)) test_pending_call_store_reply (pc, &reply); else if (!dbus_pending_call_set_notify (pc, test_pending_call_store_reply, &reply, NULL)) g_error ("OOM"); while (reply == NULL) test_main_context_iterate (f->ctx, TRUE); We should factor out a helper to do this. (Requested by Philip Withnall on Bug #101354.)