Merge lp:~charlesk/indicator-display/adbd-client-test-failure into lp:indicator-display
- adbd-client-test-failure
- Merge into trunk.16.10
| Status: | Merged |
|---|---|
| Approved by: | dobey |
| Approved revision: | 40 |
| Merged at revision: | 21 |
| Proposed branch: | lp:~charlesk/indicator-display/adbd-client-test-failure |
| Merge into: | lp:indicator-display |
| Diff against target: |
1012 lines (+474/-170) 14 files modified
src/adbd-client.cpp (+16/-9) src/greeter.cpp (+132/-82) src/greeter.h (+8/-2) src/usb-manager.cpp (+65/-48) src/usb-snap.cpp (+1/-0) tests/integration/usb-manager-test.cpp (+11/-5) tests/unit/CMakeLists.txt (+5/-0) tests/unit/adbd-client-test.cpp (+3/-2) tests/unit/greeter-test.cpp (+159/-0) tests/unit/usb-snap-test.cpp (+4/-9) tests/utils/adbd-server.h (+9/-11) tests/utils/gtest-print-helpers.h (+18/-0) tests/utils/mock-greeter.h (+2/-2) tests/utils/mock-unity-greeter.py (+41/-0) |
| To merge this branch: | bzr merge lp:~charlesk/indicator-display/adbd-client-test-failure |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| dobey (community) | Approve | ||
| unity-api-1-bot | continuous-integration | Approve | |
|
Review via email:
|
|||
Commit message
Fix test error in adbd-client-test
Description of the change
1. port Greeter communication bugfix from 15.04 branch
2. adbd-client thread improvements:
- use a std::atomic for m_pkresponse_ready
- check for thread.joinable() before calling thread.join()
- remove pessimization of getting mutex lock before calling condition variable.
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
- 21. By Charles Kerr
-
in PKIdleData, take the std::string argument as a const& to make cppcheck happy on yakkety
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
PASSED: Continuous integration, rev:
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
- 22. By Charles Kerr
-
in adbd-client's dtor, check the worker thread's joinable() state before calling join() on it
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
FAILED: Continuous integration, rev:21
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
Click here to trigger a rebuild:
https:/
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
PASSED: Continuous integration, rev:22
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
- 23. By Charles Kerr
-
adbd-client's std::condition_
variable: :wait() call seems to be the system_error culprit, so wrapping in try-catch to be cautious
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
PASSED: Continuous integration, rev:23
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
- 24. By Charles Kerr
-
add a plethora of log statements to help figure out what's causing the silo test failures
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
PASSED: Continuous integration, rev:24
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
- 25. By Charles Kerr
-
GAdbdClient:
:Impl:: on_public_ key_response( ) doesn't need a condition variable wait, so use std::lock_guard rather than std::unique_lock
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
PASSED: Continuous integration, rev:25
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
- 26. By Charles Kerr
-
fix r25 copypaste error, lock on the correct mutex
- 27. By Charles Kerr
-
The notifying thread does not need to hold the lock on the same mutex as the one held by the waiting thread(s); in fact doing so is a pessimization.
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
PASSED: Continuous integration, rev:27
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
- 28. By Charles Kerr
-
more g_debug() tracers
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
PASSED: Continuous integration, rev:28
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
- 29. By Charles Kerr
-
in AdbdClientDest, explicitly disconnect the core::Signals at the end of the test
- 30. By Charles Kerr
-
in AdbdClientTest, raise the test timeout interval from 2s to 5s
- 31. By Charles Kerr
-
in UsbSnapTest, explicitly disconnect the core::Signals at the end of the test
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
PASSED: Continuous integration, rev:31
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
- 32. By Charles Kerr
-
more g_debug() tracers
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
PASSED: Continuous integration, rev:32
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
- 33. By Charles Kerr
-
tweak the debug tracers
- 34. By Charles Kerr
-
keep piling 'em on
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
PASSED: Continuous integration, rev:34
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
- 35. By Charles Kerr
-
sync with lp:~charlesk/indicator-display/lp-1572545-prompt-in-lockscreen
- 36. By Charles Kerr
-
fix tyop
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
FAILED: Continuous integration, rev:35
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
Click here to trigger a rebuild:
https:/
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
PASSED: Continuous integration, rev:36
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
- 37. By Charles Kerr
-
use a std::atomic for adbd-client's m_pkresponse_ready
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
PASSED: Continuous integration, rev:37
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
- 38. By Charles Kerr
-
remove the temporary tracers from r34 and r35
- 39. By Charles Kerr
-
remove dead code
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
PASSED: Continuous integration, rev:39
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
| dobey (dobey) wrote : | # |
Generally looks OK to me, but I noticed many uses of if or if/else without curly braces around a single result statement. I think it'd be better to use braces or convert these statements to the ternary operator.
- 40. By Charles Kerr
-
add block braces as suggested by dobey
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
PASSED: Continuous integration, rev:40
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
Preview Diff
| 1 | === modified file 'src/adbd-client.cpp' |
| 2 | --- src/adbd-client.cpp 2016-03-24 16:01:16 +0000 |
| 3 | +++ src/adbd-client.cpp 2016-10-21 11:40:57 +0000 |
| 4 | @@ -23,6 +23,7 @@ |
| 5 | #include <gio/gunixsocketaddress.h> |
| 6 | |
| 7 | #include <algorithm> |
| 8 | +#include <atomic> |
| 9 | #include <cctype> |
| 10 | #include <cstring> |
| 11 | #include <chrono> |
| 12 | @@ -48,7 +49,9 @@ |
| 13 | g_cancellable_cancel(m_cancellable); |
| 14 | m_pkresponse_cv.notify_one(); |
| 15 | m_sleep_cv.notify_one(); |
| 16 | - m_worker_thread.join(); |
| 17 | + if (m_worker_thread.joinable()) { |
| 18 | + m_worker_thread.join(); |
| 19 | + } |
| 20 | g_clear_object(&m_cancellable); |
| 21 | } |
| 22 | |
| 23 | @@ -66,7 +69,7 @@ |
| 24 | GCancellable* cancellable = nullptr; |
| 25 | const std::string public_key; |
| 26 | |
| 27 | - PKIdleData(Impl* self_, GCancellable* cancellable_, std::string public_key_): |
| 28 | + PKIdleData(Impl* self_, GCancellable* cancellable_, const std::string& public_key_): |
| 29 | self(self_), |
| 30 | cancellable(G_CANCELLABLE(g_object_ref(cancellable_))), |
| 31 | public_key(public_key_) {} |
| 32 | @@ -104,8 +107,9 @@ |
| 33 | |
| 34 | void on_public_key_response(PKResponse response) |
| 35 | { |
| 36 | + g_debug("%s thread %p got response %d", G_STRLOC, g_thread_self(), int(response)); |
| 37 | + |
| 38 | // set m_pkresponse and wake up the waiting worker thread |
| 39 | - std::unique_lock<std::mutex> lk(m_pkresponse_mutex); |
| 40 | m_pkresponse = response; |
| 41 | m_pkresponse_ready = true; |
| 42 | m_pkresponse_cv.notify_one(); |
| 43 | @@ -121,11 +125,11 @@ |
| 44 | |
| 45 | while (!g_cancellable_is_cancelled(m_cancellable)) |
| 46 | { |
| 47 | - g_debug("%s creating a client socket to '%s'", G_STRLOC, socket_path.c_str()); |
| 48 | + g_debug("%s thread %p creating a client socket to '%s'", G_STRLOC, g_thread_self(), socket_path.c_str()); |
| 49 | auto socket = create_client_socket(socket_path); |
| 50 | bool got_valid_req = false; |
| 51 | |
| 52 | - g_debug("%s calling read_request", G_STRLOC); |
| 53 | + g_debug("%s thread %p calling read_request", g_thread_self(), G_STRLOC); |
| 54 | std::string reqstr; |
| 55 | if (socket != nullptr) |
| 56 | reqstr = read_request(socket); |
| 57 | @@ -135,22 +139,25 @@ |
| 58 | if (reqstr.substr(0,2) == "PK") { |
| 59 | PKResponse response = PKResponse::DENY; |
| 60 | const auto public_key = reqstr.substr(2); |
| 61 | - g_debug("%s got pk [%s]", G_STRLOC, public_key.c_str()); |
| 62 | + g_debug("%s thread %p got pk [%s]", G_STRLOC, g_thread_self(), public_key.c_str()); |
| 63 | if (!public_key.empty()) { |
| 64 | got_valid_req = true; |
| 65 | std::unique_lock<std::mutex> lk(m_pkresponse_mutex); |
| 66 | m_pkresponse_ready = false; |
| 67 | + m_pkresponse = AdbdClient::PKResponse::DENY; |
| 68 | pass_public_key_to_main_thread(public_key); |
| 69 | m_pkresponse_cv.wait(lk, [this](){ |
| 70 | return m_pkresponse_ready || g_cancellable_is_cancelled(m_cancellable); |
| 71 | }); |
| 72 | response = m_pkresponse; |
| 73 | - g_debug("%s got response '%d', is-cancelled %d", G_STRLOC, |
| 74 | + g_debug("%s thread %p got response '%d', is-cancelled %d", G_STRLOC, |
| 75 | + g_thread_self(), |
| 76 | int(response), |
| 77 | int(g_cancellable_is_cancelled(m_cancellable))); |
| 78 | } |
| 79 | - if (!g_cancellable_is_cancelled(m_cancellable)) |
| 80 | + if (!g_cancellable_is_cancelled(m_cancellable)) { |
| 81 | send_pk_response(socket, response); |
| 82 | + } |
| 83 | } else if (!reqstr.empty()) { |
| 84 | g_warning("Invalid ADB request: [%s]", reqstr.c_str()); |
| 85 | } |
| 86 | @@ -270,7 +277,7 @@ |
| 87 | |
| 88 | std::mutex m_pkresponse_mutex; |
| 89 | std::condition_variable m_pkresponse_cv; |
| 90 | - bool m_pkresponse_ready = false; |
| 91 | + std::atomic<bool> m_pkresponse_ready {false}; |
| 92 | PKResponse m_pkresponse = PKResponse::DENY; |
| 93 | }; |
| 94 | |
| 95 | |
| 96 | === modified file 'src/greeter.cpp' |
| 97 | --- src/greeter.cpp 2016-03-22 13:15:38 +0000 |
| 98 | +++ src/greeter.cpp 2016-10-21 11:40:57 +0000 |
| 99 | @@ -26,117 +26,167 @@ |
| 100 | { |
| 101 | public: |
| 102 | |
| 103 | - Impl(): |
| 104 | - m_cancellable{g_cancellable_new()} |
| 105 | - { |
| 106 | - g_bus_get(G_BUS_TYPE_SESSION, m_cancellable, on_bus_ready_static, this); |
| 107 | - } |
| 108 | - |
| 109 | - ~Impl() |
| 110 | - { |
| 111 | - g_cancellable_cancel(m_cancellable); |
| 112 | - g_clear_object(&m_cancellable); |
| 113 | - |
| 114 | - if (m_subscription_id != 0) |
| 115 | - g_dbus_connection_signal_unsubscribe (m_bus, m_subscription_id); |
| 116 | - |
| 117 | - g_clear_object(&m_bus); |
| 118 | - } |
| 119 | - |
| 120 | - core::Property<bool>& is_active() |
| 121 | - { |
| 122 | - return m_is_active; |
| 123 | + Impl() |
| 124 | + { |
| 125 | + m_cancellable.reset( |
| 126 | + g_cancellable_new(), |
| 127 | + [](GCancellable* o){ |
| 128 | + g_cancellable_cancel(o); |
| 129 | + g_clear_object(&o); |
| 130 | + } |
| 131 | + ); |
| 132 | + |
| 133 | + g_bus_get(G_BUS_TYPE_SESSION, m_cancellable.get(), on_bus_ready, this); |
| 134 | + } |
| 135 | + |
| 136 | + ~Impl() =default; |
| 137 | + |
| 138 | + core::Property<State>& state() |
| 139 | + { |
| 140 | + return m_state; |
| 141 | } |
| 142 | |
| 143 | private: |
| 144 | |
| 145 | - static void on_bus_ready_static(GObject* /*source*/, GAsyncResult* res, gpointer gself) |
| 146 | + void set_state(const State& state) |
| 147 | + { |
| 148 | + m_state.set(state); |
| 149 | + } |
| 150 | + |
| 151 | + static void on_bus_ready( |
| 152 | + GObject* /*source*/, |
| 153 | + GAsyncResult* res, |
| 154 | + gpointer gself) |
| 155 | { |
| 156 | GError* error {}; |
| 157 | - auto bus = g_bus_get_finish (res, &error); |
| 158 | - if (error != nullptr) { |
| 159 | + auto bus = g_bus_get_finish(res, &error); |
| 160 | + if (error != nullptr) |
| 161 | + { |
| 162 | if (!g_error_matches(error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) |
| 163 | - g_warning("UsbSnap: Error getting session bus: %s", error->message); |
| 164 | + g_warning("Greeter: Error getting bus: %s", error->message); |
| 165 | g_clear_error(&error); |
| 166 | - } else { |
| 167 | - static_cast<Impl*>(gself)->on_bus_ready(bus); |
| 168 | - } |
| 169 | - g_clear_object(&bus); |
| 170 | - } |
| 171 | - |
| 172 | - void on_bus_ready(GDBusConnection* bus) |
| 173 | - { |
| 174 | - m_bus = G_DBUS_CONNECTION(g_object_ref(G_OBJECT(bus))); |
| 175 | - |
| 176 | - g_dbus_connection_call(m_bus, |
| 177 | - DBusNames::UnityGreeter::NAME, |
| 178 | - DBusNames::UnityGreeter::PATH, |
| 179 | - DBusNames::Properties::INTERFACE, |
| 180 | - "Get", |
| 181 | - g_variant_new("(ss)", DBusNames::UnityGreeter::INTERFACE, "IsActive"), |
| 182 | - G_VARIANT_TYPE("(v)"), |
| 183 | - G_DBUS_CALL_FLAGS_NONE, |
| 184 | - -1, |
| 185 | - m_cancellable, |
| 186 | - on_get_is_active_ready, |
| 187 | - this); |
| 188 | - |
| 189 | - m_subscription_id = g_dbus_connection_signal_subscribe(m_bus, |
| 190 | - DBusNames::UnityGreeter::NAME, |
| 191 | - DBusNames::Properties::INTERFACE, |
| 192 | - DBusNames::Properties::PropertiesChanged::NAME, |
| 193 | - DBusNames::UnityGreeter::PATH, |
| 194 | - DBusNames::UnityGreeter::INTERFACE, |
| 195 | - G_DBUS_SIGNAL_FLAGS_NONE, |
| 196 | - on_properties_changed_signal, |
| 197 | - this, |
| 198 | - nullptr); |
| 199 | - } |
| 200 | - |
| 201 | - static void on_get_is_active_ready(GObject* source, GAsyncResult* res, gpointer gself) |
| 202 | + } |
| 203 | + else |
| 204 | + { |
| 205 | + auto self = static_cast<Impl*>(gself); |
| 206 | + |
| 207 | + const auto watcher_id = g_bus_watch_name_on_connection( |
| 208 | + bus, |
| 209 | + DBusNames::UnityGreeter::NAME, |
| 210 | + G_BUS_NAME_WATCHER_FLAGS_AUTO_START, |
| 211 | + on_greeter_appeared, |
| 212 | + on_greeter_vanished, |
| 213 | + gself, |
| 214 | + nullptr); |
| 215 | + |
| 216 | + const auto subscription_id = g_dbus_connection_signal_subscribe( |
| 217 | + bus, |
| 218 | + DBusNames::UnityGreeter::NAME, |
| 219 | + DBusNames::Properties::INTERFACE, |
| 220 | + DBusNames::Properties::PropertiesChanged::NAME, |
| 221 | + DBusNames::UnityGreeter::PATH, |
| 222 | + DBusNames::UnityGreeter::INTERFACE, |
| 223 | + G_DBUS_SIGNAL_FLAGS_NONE, |
| 224 | + on_properties_changed_signal, |
| 225 | + gself, |
| 226 | + nullptr); |
| 227 | + |
| 228 | + self->m_bus.reset( |
| 229 | + bus, |
| 230 | + [watcher_id, subscription_id](GDBusConnection* o){ |
| 231 | + g_bus_unwatch_name(watcher_id); |
| 232 | + g_dbus_connection_signal_unsubscribe(o, subscription_id); |
| 233 | + g_clear_object(&o); |
| 234 | + } |
| 235 | + ); |
| 236 | + } |
| 237 | + } |
| 238 | + |
| 239 | + static void on_greeter_appeared( |
| 240 | + GDBusConnection* bus, |
| 241 | + const char* /*name*/, |
| 242 | + const char* name_owner, |
| 243 | + gpointer gself) |
| 244 | + { |
| 245 | + auto self = static_cast<Impl*>(gself); |
| 246 | + |
| 247 | + self->m_owner = name_owner; |
| 248 | + |
| 249 | + g_dbus_connection_call( |
| 250 | + bus, |
| 251 | + DBusNames::UnityGreeter::NAME, |
| 252 | + DBusNames::UnityGreeter::PATH, |
| 253 | + DBusNames::Properties::INTERFACE, |
| 254 | + "Get", |
| 255 | + g_variant_new("(ss)", DBusNames::UnityGreeter::INTERFACE, "IsActive"), |
| 256 | + G_VARIANT_TYPE("(v)"), |
| 257 | + G_DBUS_CALL_FLAGS_NONE, |
| 258 | + -1, |
| 259 | + self->m_cancellable.get(), |
| 260 | + on_get_is_active_ready, |
| 261 | + gself); |
| 262 | + } |
| 263 | + |
| 264 | + static void on_greeter_vanished( |
| 265 | + GDBusConnection* /*bus*/, |
| 266 | + const char* /*name*/, |
| 267 | + gpointer gself) |
| 268 | + { |
| 269 | + auto self = static_cast<Impl*>(gself); |
| 270 | + |
| 271 | + self->m_owner.clear(); |
| 272 | + self->set_state(State::UNAVAILABLE); |
| 273 | + } |
| 274 | + |
| 275 | + static void on_get_is_active_ready( |
| 276 | + GObject* source, |
| 277 | + GAsyncResult* res, |
| 278 | + gpointer gself) |
| 279 | { |
| 280 | GError* error {}; |
| 281 | auto v = g_dbus_connection_call_finish(G_DBUS_CONNECTION(source), res, &error); |
| 282 | if (error != nullptr) { |
| 283 | - if (!g_error_matches(error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) |
| 284 | - g_warning("UsbSnap: Error getting session bus: %s", error->message); |
| 285 | + if (!g_error_matches(error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { |
| 286 | + g_warning("Greeter: Error getting IsActive property: %s", error->message); |
| 287 | + } |
| 288 | g_clear_error(&error); |
| 289 | } else { |
| 290 | GVariant* is_active {}; |
| 291 | g_variant_get_child(v, 0, "v", &is_active); |
| 292 | - static_cast<Impl*>(gself)->m_is_active.set(g_variant_get_boolean(is_active)); |
| 293 | + static_cast<Impl*>(gself)->set_state(g_variant_get_boolean(is_active) ? State::ACTIVE : State::INACTIVE); |
| 294 | g_clear_pointer(&is_active, g_variant_unref); |
| 295 | } |
| 296 | g_clear_pointer(&v, g_variant_unref); |
| 297 | } |
| 298 | |
| 299 | - static void on_properties_changed_signal(GDBusConnection* /*connection*/, |
| 300 | - const gchar* /*sender_name*/, |
| 301 | - const gchar* object_path, |
| 302 | - const gchar* interface_name, |
| 303 | - const gchar* signal_name, |
| 304 | - GVariant* parameters, |
| 305 | - gpointer gself) |
| 306 | + static void on_properties_changed_signal( |
| 307 | + GDBusConnection* /*bus*/, |
| 308 | + const gchar* sender_name, |
| 309 | + const gchar* object_path, |
| 310 | + const gchar* interface_name, |
| 311 | + const gchar* signal_name, |
| 312 | + GVariant* parameters, |
| 313 | + gpointer gself) |
| 314 | { |
| 315 | + auto self = static_cast<Impl*>(gself); |
| 316 | + |
| 317 | + g_return_if_fail(!g_strcmp0(sender_name, self->m_owner.c_str())); |
| 318 | g_return_if_fail(!g_strcmp0(object_path, DBusNames::UnityGreeter::PATH)); |
| 319 | g_return_if_fail(!g_strcmp0(interface_name, DBusNames::Properties::INTERFACE)); |
| 320 | g_return_if_fail(!g_strcmp0(signal_name, DBusNames::Properties::PropertiesChanged::NAME)); |
| 321 | g_return_if_fail(g_variant_is_of_type(parameters, G_VARIANT_TYPE(DBusNames::Properties::PropertiesChanged::ARGS_VARIANT_TYPE))); |
| 322 | |
| 323 | - auto v = g_variant_get_child_value (parameters, 1); |
| 324 | + auto v = g_variant_get_child_value(parameters, 1); |
| 325 | gboolean is_active {}; |
| 326 | if (g_variant_lookup(v, "IsActive", "b", &is_active)) |
| 327 | - { |
| 328 | - g_debug("%s is_active changed to %d", G_STRLOC, int(is_active)); |
| 329 | - static_cast<Impl*>(gself)->m_is_active.set(is_active); |
| 330 | - } |
| 331 | + self->set_state(is_active ? State::ACTIVE : State::INACTIVE); |
| 332 | g_clear_pointer(&v, g_variant_unref); |
| 333 | } |
| 334 | |
| 335 | - core::Property<bool> m_is_active; |
| 336 | - GCancellable* m_cancellable {}; |
| 337 | - GDBusConnection* m_bus {}; |
| 338 | - unsigned int m_subscription_id {}; |
| 339 | + core::Property<State> m_state {State::UNAVAILABLE}; |
| 340 | + std::shared_ptr<GCancellable> m_cancellable; |
| 341 | + std::shared_ptr<GDBusConnection> m_bus; |
| 342 | + std::string m_owner; |
| 343 | }; |
| 344 | |
| 345 | /*** |
| 346 | @@ -154,8 +204,8 @@ |
| 347 | |
| 348 | UnityGreeter::~UnityGreeter() =default; |
| 349 | |
| 350 | -core::Property<bool>& |
| 351 | -UnityGreeter::is_active() |
| 352 | +core::Property<Greeter::State>& |
| 353 | +UnityGreeter::state() |
| 354 | { |
| 355 | - return impl->is_active(); |
| 356 | + return impl->state(); |
| 357 | } |
| 358 | |
| 359 | === modified file 'src/greeter.h' |
| 360 | --- src/greeter.h 2016-03-21 23:15:05 +0000 |
| 361 | +++ src/greeter.h 2016-10-21 11:40:57 +0000 |
| 362 | @@ -29,7 +29,13 @@ |
| 363 | public: |
| 364 | Greeter(); |
| 365 | virtual ~Greeter(); |
| 366 | - virtual core::Property<bool>& is_active() =0; |
| 367 | + |
| 368 | + enum class State { UNAVAILABLE, INACTIVE, ACTIVE }; |
| 369 | +static inline const char* state_str(const State& state) { |
| 370 | + static constexpr char const * state_str[] = { "Unavailable", "Inactive", "Active" }; |
| 371 | + return state_str[int(state)]; |
| 372 | +} |
| 373 | + virtual core::Property<State>& state() =0; |
| 374 | }; |
| 375 | |
| 376 | |
| 377 | @@ -38,7 +44,7 @@ |
| 378 | public: |
| 379 | UnityGreeter(); |
| 380 | virtual ~UnityGreeter(); |
| 381 | - core::Property<bool>& is_active() override; |
| 382 | + core::Property<State>& state() override; |
| 383 | |
| 384 | protected: |
| 385 | class Impl; |
| 386 | |
| 387 | === modified file 'src/usb-manager.cpp' |
| 388 | --- src/usb-manager.cpp 2016-03-23 19:44:23 +0000 |
| 389 | +++ src/usb-manager.cpp 2016-10-21 11:40:57 +0000 |
| 390 | @@ -46,72 +46,89 @@ |
| 391 | m_greeter{greeter} |
| 392 | { |
| 393 | m_usb_monitor->on_usb_disconnected().connect([this](const std::string& /*usb_name*/) { |
| 394 | - restart(); |
| 395 | - }); |
| 396 | - |
| 397 | - m_greeter->is_active().changed().connect([this](bool /*is_active*/) { |
| 398 | - maybe_snap(); |
| 399 | - }); |
| 400 | - |
| 401 | - restart(); |
| 402 | + m_req.reset(); |
| 403 | + }); |
| 404 | + |
| 405 | + m_greeter->state().changed().connect([this](const Greeter::State& state) { |
| 406 | + if (state == Greeter::State::INACTIVE) { |
| 407 | + maybe_snap(); |
| 408 | + } else { |
| 409 | + stop_snap(); |
| 410 | + } |
| 411 | + }); |
| 412 | + |
| 413 | + // create a new adbd client |
| 414 | + m_adbd_client.reset(new GAdbdClient{m_socket_path}); |
| 415 | + m_adbd_client->on_pk_request().connect( |
| 416 | + [this](const AdbdClient::PKRequest& req) { |
| 417 | + g_debug("%s got pk request: %s, calling maybe_snap()", G_STRLOC, req.fingerprint.c_str()); |
| 418 | + |
| 419 | + m_response = AdbdClient::PKResponse::DENY; // set the fallback response |
| 420 | + m_req.reset( |
| 421 | + new AdbdClient::PKRequest(req), |
| 422 | + [this](AdbdClient::PKRequest* r) { |
| 423 | + stop_snap(); |
| 424 | + r->respond(m_response); |
| 425 | + delete r; |
| 426 | + } |
| 427 | + ); |
| 428 | + maybe_snap(); |
| 429 | + } |
| 430 | + ); |
| 431 | } |
| 432 | |
| 433 | ~Impl() |
| 434 | { |
| 435 | - if (m_restart_idle_tag) |
| 436 | - g_source_remove(m_restart_idle_tag); |
| 437 | - |
| 438 | - clear(); |
| 439 | + if (m_request_complete_idle_tag) { |
| 440 | + g_source_remove(m_request_complete_idle_tag); |
| 441 | + } |
| 442 | } |
| 443 | |
| 444 | private: |
| 445 | |
| 446 | - void clear() |
| 447 | + void stop_snap() |
| 448 | { |
| 449 | - // clear out old state |
| 450 | m_snap_connections.clear(); |
| 451 | m_snap.reset(); |
| 452 | - m_req = decltype(m_req)(); |
| 453 | - m_adbd_client.reset(); |
| 454 | - } |
| 455 | - |
| 456 | - void restart() |
| 457 | - { |
| 458 | - clear(); |
| 459 | - |
| 460 | - // set a new client |
| 461 | - m_adbd_client.reset(new GAdbdClient{m_socket_path}); |
| 462 | - m_adbd_client->on_pk_request().connect( |
| 463 | - [this](const AdbdClient::PKRequest& req) { |
| 464 | - g_debug("%s got pk request: %s", G_STRLOC, req.fingerprint.c_str()); |
| 465 | - m_req = req; |
| 466 | - maybe_snap(); |
| 467 | - } |
| 468 | - ); |
| 469 | } |
| 470 | |
| 471 | void maybe_snap() |
| 472 | { |
| 473 | - // don't prompt in the greeter! |
| 474 | - if (!m_req.public_key.empty() && !m_greeter->is_active().get()) |
| 475 | - snap(); |
| 476 | + // only prompt if there's something to prompt about |
| 477 | + if (!m_req) { |
| 478 | + return; |
| 479 | + } |
| 480 | + |
| 481 | + // only prompt in an unlocked session |
| 482 | + if (m_greeter->state().get() != Greeter::State::INACTIVE) { |
| 483 | + return; |
| 484 | + } |
| 485 | + |
| 486 | + snap(); |
| 487 | } |
| 488 | |
| 489 | void snap() |
| 490 | { |
| 491 | - m_snap = std::make_shared<UsbSnap>(m_req.fingerprint); |
| 492 | + m_snap = std::make_shared<UsbSnap>(m_req->fingerprint); |
| 493 | m_snap_connections.insert((*m_snap).on_user_response().connect( |
| 494 | [this](AdbdClient::PKResponse response, bool remember_choice){ |
| 495 | - g_debug("%s user responded! response %d, remember %d", G_STRLOC, int(response), int(remember_choice)); |
| 496 | - m_req.respond(response); |
| 497 | - if (remember_choice && (response == AdbdClient::PKResponse::ALLOW)) |
| 498 | - write_public_key(m_req.public_key); |
| 499 | - m_restart_idle_tag = g_idle_add([](gpointer gself){ |
| 500 | - auto self = static_cast<Impl*>(gself); |
| 501 | - self->m_restart_idle_tag = 0; |
| 502 | - self->restart(); |
| 503 | - return G_SOURCE_REMOVE; |
| 504 | - }, this); |
| 505 | + |
| 506 | + if (remember_choice && (response == AdbdClient::PKResponse::ALLOW)) { |
| 507 | + write_public_key(m_req->public_key); |
| 508 | + } |
| 509 | + |
| 510 | + m_response = response; |
| 511 | + |
| 512 | + // defer finishing the request into an idle func because |
| 513 | + // ScopedConnections can't be destroyed inside their callbacks |
| 514 | + if (m_request_complete_idle_tag == 0) { |
| 515 | + m_request_complete_idle_tag = g_idle_add([](gpointer gself){ |
| 516 | + auto self = static_cast<Impl*>(gself); |
| 517 | + self->m_request_complete_idle_tag = 0; |
| 518 | + self->m_req.reset(); |
| 519 | + return G_SOURCE_REMOVE; |
| 520 | + }, this); |
| 521 | + } |
| 522 | } |
| 523 | )); |
| 524 | } |
| 525 | @@ -152,12 +169,13 @@ |
| 526 | const std::shared_ptr<UsbMonitor> m_usb_monitor; |
| 527 | const std::shared_ptr<Greeter> m_greeter; |
| 528 | |
| 529 | - unsigned int m_restart_idle_tag {}; |
| 530 | + unsigned int m_request_complete_idle_tag {}; |
| 531 | |
| 532 | std::shared_ptr<GAdbdClient> m_adbd_client; |
| 533 | - AdbdClient::PKRequest m_req; |
| 534 | std::shared_ptr<UsbSnap> m_snap; |
| 535 | std::set<core::ScopedConnection> m_snap_connections; |
| 536 | + AdbdClient::PKResponse m_response {AdbdClient::PKResponse::DENY}; |
| 537 | + std::shared_ptr<AdbdClient::PKRequest> m_req; |
| 538 | }; |
| 539 | |
| 540 | /*** |
| 541 | @@ -177,4 +195,3 @@ |
| 542 | UsbManager::~UsbManager() |
| 543 | { |
| 544 | } |
| 545 | - |
| 546 | |
| 547 | === modified file 'src/usb-snap.cpp' |
| 548 | --- src/usb-snap.cpp 2016-03-23 17:16:06 +0000 |
| 549 | +++ src/usb-snap.cpp 2016-10-21 11:40:57 +0000 |
| 550 | @@ -208,6 +208,7 @@ |
| 551 | const bool remember_this_choice = response == AdbdClient::PKResponse::ALLOW; |
| 552 | |
| 553 | m_on_user_response(response, remember_this_choice); |
| 554 | + m_notification_id = 0; |
| 555 | } |
| 556 | |
| 557 | void on_notification_closed(uint32_t close_reason) |
| 558 | |
| 559 | === modified file 'tests/integration/usb-manager-test.cpp' |
| 560 | --- tests/integration/usb-manager-test.cpp 2016-03-23 17:16:06 +0000 |
| 561 | +++ tests/integration/usb-manager-test.cpp 2016-10-21 11:40:57 +0000 |
| 562 | @@ -143,7 +143,7 @@ |
| 563 | ); |
| 564 | |
| 565 | // confirm that the AdbdServer got the right response |
| 566 | - wait_for([adbd_server](){return !adbd_server->m_responses.empty();}, 2000); |
| 567 | + wait_for([adbd_server](){return !adbd_server->m_responses.empty();}, 5000); |
| 568 | ASSERT_EQ(1, adbd_server->m_responses.size()); |
| 569 | EXPECT_EQ("OK", adbd_server->m_responses.front()); |
| 570 | |
| 571 | @@ -163,13 +163,16 @@ |
| 572 | const std::shared_ptr<std::string> public_keys_path {new std::string{*m_tmpdir+"/adb_keys"}, file_deleter}; |
| 573 | |
| 574 | // start a mock AdbdServer ready to submit a request |
| 575 | + const size_t N_TESTS {3}; |
| 576 | const std::string public_key {"public_key"}; |
| 577 | - auto adbd_server = std::make_shared<GAdbdServer>(*socket_path, std::vector<std::string>{"PK"+public_key}); |
| 578 | + const std::vector<std::string> requests(N_TESTS, "PK"+public_key); |
| 579 | + const std::vector<std::string> expected_responses(N_TESTS, "NO"); |
| 580 | + auto adbd_server = std::make_shared<GAdbdServer>(*socket_path, requests); |
| 581 | |
| 582 | // set up a UsbManager to process the request |
| 583 | auto usb_manager = std::make_shared<UsbManager>(*socket_path, *public_keys_path, m_usb_monitor, m_greeter); |
| 584 | |
| 585 | - for (int i=0; i<3; i++) |
| 586 | + for (std::remove_const<decltype(N_TESTS)>::type i=0; i<N_TESTS; ++i) |
| 587 | { |
| 588 | // add a signal spy to listen to the notification daemon |
| 589 | QSignalSpy notificationsSpy( |
| 590 | @@ -194,6 +197,9 @@ |
| 591 | EXPECT_EQ("CloseNotification", notificationsSpy.at(0).at(0)); |
| 592 | notificationsSpy.clear(); |
| 593 | } |
| 594 | + |
| 595 | + EXPECT_TRUE(wait_for([adbd_server, N_TESTS](){return adbd_server->m_responses.size() == N_TESTS;}, 5000)); |
| 596 | + EXPECT_EQ(expected_responses, adbd_server->m_responses); |
| 597 | } |
| 598 | |
| 599 | TEST_F(UsbManagerFixture, Greeter) |
| 600 | @@ -206,7 +212,7 @@ |
| 601 | auto adbd_server = std::make_shared<GAdbdServer>(*socket_path, std::vector<std::string>{"PK"+public_key}); |
| 602 | |
| 603 | // set up a UsbManager to process the request |
| 604 | - m_greeter->m_is_active.set(true); |
| 605 | + m_greeter->m_state.set(Greeter::State::ACTIVE); |
| 606 | auto usb_manager = std::make_shared<UsbManager>(*socket_path, *public_keys_path, m_usb_monitor, m_greeter); |
| 607 | |
| 608 | // add a signal spy to listen to the notification daemon |
| 609 | @@ -219,7 +225,7 @@ |
| 610 | EXPECT_FALSE(notificationsSpy.wait(2000)); |
| 611 | |
| 612 | // disable the greeter, the notification should appear |
| 613 | - m_greeter->m_is_active.set(false); |
| 614 | + m_greeter->m_state.set(Greeter::State::INACTIVE); |
| 615 | wait_for_signals(notificationsSpy, 1); |
| 616 | EXPECT_EQ("Notify", notificationsSpy.at(0).at(0)); |
| 617 | notificationsSpy.clear(); |
| 618 | |
| 619 | === modified file 'tests/unit/CMakeLists.txt' |
| 620 | --- tests/unit/CMakeLists.txt 2016-03-16 00:25:33 +0000 |
| 621 | +++ tests/unit/CMakeLists.txt 2016-10-21 11:40:57 +0000 |
| 622 | @@ -14,6 +14,10 @@ |
| 623 | ${GMOCK_LIBRARIES} |
| 624 | ) |
| 625 | |
| 626 | +add_definitions( |
| 627 | + -DGREETER_TEMPLATE="${CMAKE_SOURCE_DIR}/tests/utils/mock-unity-greeter.py" |
| 628 | +) |
| 629 | + |
| 630 | function(add_test_by_name name) |
| 631 | set(TEST_NAME ${name}) |
| 632 | add_executable (${TEST_NAME} ${TEST_NAME}.cpp) |
| 633 | @@ -31,4 +35,5 @@ |
| 634 | set_property(TEST ${TEST_NAME} APPEND PROPERTY ENVIRONMENT ${CTEST_ENVIRONMENT}) |
| 635 | target_link_libraries(${TEST_NAME} ${SERVICE_LINK_LIBRARIES} ${QT_LINK_LIBRARIES} ${TEST_LINK_LIBRARIES} ${THREAD_LINK_LIBRARIES}) |
| 636 | endfunction() |
| 637 | +add_qt_test_by_name(greeter-test) |
| 638 | add_qt_test_by_name(usb-snap-test) |
| 639 | |
| 640 | === modified file 'tests/unit/adbd-client-test.cpp' |
| 641 | --- tests/unit/adbd-client-test.cpp 2016-03-17 15:24:41 +0000 |
| 642 | +++ tests/unit/adbd-client-test.cpp 2016-10-21 11:40:57 +0000 |
| 643 | @@ -73,7 +73,7 @@ |
| 644 | // start an AdbdClient that listens for PKRequests |
| 645 | std::string pk; |
| 646 | auto adbd_client = std::make_shared<GAdbdClient>(socket_path); |
| 647 | - adbd_client->on_pk_request().connect([&pk, main_thread, test](const AdbdClient::PKRequest& req){ |
| 648 | + auto connection = adbd_client->on_pk_request().connect([&pk, main_thread, test](const AdbdClient::PKRequest& req){ |
| 649 | EXPECT_EQ(main_thread, g_thread_self()); |
| 650 | g_message("in on_pk_request with %s", req.public_key.c_str()); |
| 651 | pk = req.public_key; |
| 652 | @@ -82,12 +82,13 @@ |
| 653 | |
| 654 | // start a mock AdbdServer with to fire test key requests and wait for a response |
| 655 | auto adbd_server = std::make_shared<GAdbdServer>(socket_path, std::vector<std::string>{test.request}); |
| 656 | - wait_for([adbd_server](){return !adbd_server->m_responses.empty();}, 2000); |
| 657 | + wait_for([adbd_server](){return !adbd_server->m_responses.empty();}, 5000); |
| 658 | EXPECT_EQ(test.expected_pk, pk); |
| 659 | ASSERT_EQ(1, adbd_server->m_responses.size()); |
| 660 | EXPECT_EQ(test.expected_response, adbd_server->m_responses.front()); |
| 661 | |
| 662 | // cleanup |
| 663 | + connection.disconnect(); |
| 664 | adbd_client.reset(); |
| 665 | adbd_server.reset(); |
| 666 | g_unlink(socket_path.c_str()); |
| 667 | |
| 668 | === added file 'tests/unit/greeter-test.cpp' |
| 669 | --- tests/unit/greeter-test.cpp 1970-01-01 00:00:00 +0000 |
| 670 | +++ tests/unit/greeter-test.cpp 2016-10-21 11:40:57 +0000 |
| 671 | @@ -0,0 +1,159 @@ |
| 672 | +/* |
| 673 | + * Copyright 2016 Canonical Ltd. |
| 674 | + * |
| 675 | + * This program is free software: you can redistribute it and/or modify it |
| 676 | + * under the terms of the GNU General Public License version 3, as published |
| 677 | + * by the Free Software Foundation. |
| 678 | + * |
| 679 | + * This program is distributed in the hope that it will be useful, but |
| 680 | + * WITHOUT ANY WARRANTY; without even the implied warranties of |
| 681 | + * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR |
| 682 | + * PURPOSE. See the GNU General Public License for more details. |
| 683 | + * |
| 684 | + * You should have received a copy of the GNU General Public License along |
| 685 | + * with this program. If not, see <http://www.gnu.org/licenses/>. |
| 686 | + * |
| 687 | + * Authors: |
| 688 | + * Charles Kerr <charles.kerr@canonical.com> |
| 689 | + */ |
| 690 | + |
| 691 | +#include <tests/utils/qt-fixture.h> |
| 692 | +#include <tests/utils/gtest-print-helpers.h> |
| 693 | + |
| 694 | +#include <src/dbus-names.h> |
| 695 | +#include <src/greeter.h> |
| 696 | + |
| 697 | +#include <libqtdbustest/DBusTestRunner.h> |
| 698 | +#include <libqtdbustest/QProcessDBusService.h> |
| 699 | +#include <libqtdbusmock/DBusMock.h> |
| 700 | + |
| 701 | +class GreeterFixture: public QtFixture |
| 702 | +{ |
| 703 | +private: |
| 704 | + |
| 705 | + using super = QtFixture; |
| 706 | + |
| 707 | +public: |
| 708 | + |
| 709 | + GreeterFixture() =default; |
| 710 | + ~GreeterFixture() =default; |
| 711 | + |
| 712 | +protected: |
| 713 | + |
| 714 | + std::shared_ptr<QtDBusTest::DBusTestRunner> m_dbus_runner; |
| 715 | + std::shared_ptr<QtDBusMock::DBusMock> m_dbus_mock; |
| 716 | + GDBusConnection* m_bus {}; |
| 717 | + |
| 718 | + void SetUp() override |
| 719 | + { |
| 720 | + super::SetUp(); |
| 721 | + |
| 722 | + // use a fresh bus for each test run |
| 723 | + m_dbus_runner.reset(new QtDBusTest::DBusTestRunner()); |
| 724 | + m_dbus_mock.reset(new QtDBusMock::DBusMock(*m_dbus_runner.get())); |
| 725 | + |
| 726 | + GError* error {}; |
| 727 | + m_bus = g_bus_get_sync (G_BUS_TYPE_SESSION, nullptr, &error); |
| 728 | + g_assert_no_error(error); |
| 729 | + g_dbus_connection_set_exit_on_close(m_bus, FALSE); |
| 730 | + } |
| 731 | + |
| 732 | + void TearDown() override |
| 733 | + { |
| 734 | + g_clear_object(&m_bus); |
| 735 | + |
| 736 | + super::TearDown(); |
| 737 | + } |
| 738 | + |
| 739 | + void start_greeter_service(bool is_active) |
| 740 | + { |
| 741 | + // set a watcher to look for our mock greeter to appear |
| 742 | + bool owned {}; |
| 743 | + QDBusServiceWatcher watcher( |
| 744 | + DBusNames::UnityGreeter::NAME, |
| 745 | + m_dbus_runner->sessionConnection() |
| 746 | + ); |
| 747 | + QObject::connect( |
| 748 | + &watcher, |
| 749 | + &QDBusServiceWatcher::serviceRegistered, |
| 750 | + [&owned](const QString&){owned = true;} |
| 751 | + ); |
| 752 | + |
| 753 | + // start the mock greeter |
| 754 | + QVariantMap parameters; |
| 755 | + parameters["IsActive"] = QVariant(is_active); |
| 756 | + m_dbus_mock->registerTemplate( |
| 757 | + DBusNames::UnityGreeter::NAME, |
| 758 | + GREETER_TEMPLATE, |
| 759 | + parameters, |
| 760 | + QDBusConnection::SessionBus |
| 761 | + ); |
| 762 | + m_dbus_runner->startServices(); |
| 763 | + |
| 764 | + // wait for the watcher |
| 765 | + ASSERT_TRUE(wait_for([&owned]{return owned;})); |
| 766 | + } |
| 767 | +}; |
| 768 | + |
| 769 | +#define ASSERT_PROPERTY_EQ_EVENTUALLY(expected_in, property_in) \ |
| 770 | + do { \ |
| 771 | + const auto& e = expected_in; \ |
| 772 | + const auto& p = property_in; \ |
| 773 | + ASSERT_TRUE(wait_for([e, &p](){return e == p.get();})) \ |
| 774 | + << "expected " << e << " but got " << p.get(); \ |
| 775 | + } while(0) |
| 776 | + |
| 777 | +/** |
| 778 | + * Test startup timing by looking at four different cases: |
| 779 | + * [unity greeter shows up on bus (before, after) we start listening] |
| 780 | + * x [unity greeter is (active, inactive)] |
| 781 | + */ |
| 782 | + |
| 783 | +TEST_F(GreeterFixture, ActiveServiceStartsBeforeWatcher) |
| 784 | +{ |
| 785 | + constexpr bool is_active {true}; |
| 786 | + constexpr Greeter::State expected {Greeter::State::ACTIVE}; |
| 787 | + |
| 788 | + start_greeter_service(is_active); |
| 789 | + |
| 790 | + UnityGreeter greeter; |
| 791 | + |
| 792 | + ASSERT_PROPERTY_EQ_EVENTUALLY(expected, greeter.state()); |
| 793 | +} |
| 794 | + |
| 795 | +TEST_F(GreeterFixture, WatcherStartsBeforeActiveService) |
| 796 | +{ |
| 797 | + constexpr bool is_active {true}; |
| 798 | + constexpr Greeter::State expected {Greeter::State::ACTIVE}; |
| 799 | + |
| 800 | + UnityGreeter greeter; |
| 801 | + |
| 802 | + start_greeter_service(is_active); |
| 803 | + |
| 804 | + ASSERT_PROPERTY_EQ_EVENTUALLY(expected, greeter.state()); |
| 805 | +} |
| 806 | + |
| 807 | +TEST_F(GreeterFixture, InactiveServiceStartsBeforeWatcher) |
| 808 | +{ |
| 809 | + constexpr bool is_active {false}; |
| 810 | + constexpr Greeter::State expected {Greeter::State::INACTIVE}; |
| 811 | + |
| 812 | + start_greeter_service(is_active); |
| 813 | + |
| 814 | + UnityGreeter greeter; |
| 815 | + |
| 816 | + ASSERT_PROPERTY_EQ_EVENTUALLY(expected, greeter.state()); |
| 817 | +} |
| 818 | + |
| 819 | +TEST_F(GreeterFixture, WatcherStartsBeforeInactiveService) |
| 820 | +{ |
| 821 | + constexpr bool is_active {false}; |
| 822 | + constexpr Greeter::State expected {Greeter::State::INACTIVE}; |
| 823 | + |
| 824 | + UnityGreeter greeter; |
| 825 | + |
| 826 | + start_greeter_service(is_active); |
| 827 | + |
| 828 | + ASSERT_PROPERTY_EQ_EVENTUALLY(expected, greeter.state()); |
| 829 | +} |
| 830 | + |
| 831 | |
| 832 | === modified file 'tests/unit/usb-snap-test.cpp' |
| 833 | --- tests/unit/usb-snap-test.cpp 2016-03-23 17:16:06 +0000 |
| 834 | +++ tests/unit/usb-snap-test.cpp 2016-10-21 11:40:57 +0000 |
| 835 | @@ -88,7 +88,7 @@ |
| 836 | auto snap = std::make_shared<UsbSnap>(test.fingerprint); |
| 837 | AdbdClient::PKResponse user_response {}; |
| 838 | bool user_response_set = false; |
| 839 | - snap->on_user_response().connect([&user_response,&user_response_set](AdbdClient::PKResponse response, bool /*remember*/){ |
| 840 | + auto connection = snap->on_user_response().connect([&user_response,&user_response_set](AdbdClient::PKResponse response, bool /*remember*/){ |
| 841 | user_response = response; |
| 842 | user_response_set = true; |
| 843 | }); |
| 844 | @@ -130,14 +130,9 @@ |
| 845 | EXPECT_TRUE(user_response_set); |
| 846 | ASSERT_EQ(test.expected_response, user_response); |
| 847 | |
| 848 | - // confirm that the snap dtor cleans up the notification |
| 849 | + // confirm that the snap dtor doesn't try to close |
| 850 | + // the notification that's already been closed by user choice |
| 851 | snap.reset(); |
| 852 | - wait_for_signals(notificationsSpy, 1); |
| 853 | - { |
| 854 | - QVariantList const& call(notificationsSpy.at(0)); |
| 855 | - EXPECT_EQ("CloseNotification", call.at(0)); |
| 856 | - QVariantList const& args(call.at(1).toList()); |
| 857 | - EXPECT_EQ(id, args.at(0)); |
| 858 | - } |
| 859 | + EXPECT_FALSE(notificationsSpy.wait(1000)); |
| 860 | } |
| 861 | } |
| 862 | |
| 863 | === modified file 'tests/utils/adbd-server.h' |
| 864 | --- tests/utils/adbd-server.h 2016-03-24 16:01:16 +0000 |
| 865 | +++ tests/utils/adbd-server.h 2016-10-21 11:40:57 +0000 |
| 866 | @@ -38,7 +38,7 @@ |
| 867 | GAdbdServer(const std::string& socket_path, |
| 868 | const std::vector<std::string>& requests): |
| 869 | m_requests{requests}, |
| 870 | - m_socket_path{socket_path}, |
| 871 | + m_server_socket{create_server_socket(socket_path)}, |
| 872 | m_cancellable{g_cancellable_new()}, |
| 873 | m_worker_thread{&GAdbdServer::worker_func, this} |
| 874 | { |
| 875 | @@ -50,6 +50,7 @@ |
| 876 | g_cancellable_cancel(m_cancellable); |
| 877 | m_worker_thread.join(); |
| 878 | g_clear_object(&m_cancellable); |
| 879 | + g_clear_object(&m_server_socket); |
| 880 | } |
| 881 | |
| 882 | const std::vector<std::string> m_requests; |
| 883 | @@ -59,18 +60,14 @@ |
| 884 | |
| 885 | void worker_func() // runs in worker thread |
| 886 | { |
| 887 | - auto server_socket = create_server_socket(m_socket_path); |
| 888 | auto requests = m_requests; |
| 889 | |
| 890 | - GError* error {}; |
| 891 | - g_socket_listen (server_socket, &error); |
| 892 | - g_assert_no_error (error); |
| 893 | - |
| 894 | while (!g_cancellable_is_cancelled(m_cancellable) && !requests.empty()) |
| 895 | { |
| 896 | // wait for a client connection |
| 897 | g_message("GAdbdServer::Impl::worker_func() calling g_socket_accept()"); |
| 898 | - auto client_socket = g_socket_accept(server_socket, m_cancellable, &error); |
| 899 | + GError* error {}; |
| 900 | + auto client_socket = g_socket_accept(m_server_socket, m_cancellable, &error); |
| 901 | if (error != nullptr) { |
| 902 | if (!g_error_matches(error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) |
| 903 | g_message("GAdbdServer: Error accepting socket connection: %s", error->message); |
| 904 | @@ -121,8 +118,6 @@ |
| 905 | // cleanup |
| 906 | g_clear_object(&client_socket); |
| 907 | } |
| 908 | - |
| 909 | - g_clear_object(&server_socket); |
| 910 | } |
| 911 | |
| 912 | // bind to a local domain socket |
| 913 | @@ -139,11 +134,14 @@ |
| 914 | g_assert_no_error (error); |
| 915 | g_clear_object (&address); |
| 916 | |
| 917 | + g_socket_listen (socket, &error); |
| 918 | + g_assert_no_error (error); |
| 919 | + |
| 920 | return socket; |
| 921 | } |
| 922 | |
| 923 | - const std::string m_socket_path; |
| 924 | - GCancellable* m_cancellable = nullptr; |
| 925 | + GSocket* m_server_socket {}; |
| 926 | + GCancellable* m_cancellable {}; |
| 927 | std::thread m_worker_thread; |
| 928 | }; |
| 929 | |
| 930 | |
| 931 | === added file 'tests/utils/gtest-print-helpers.h' |
| 932 | --- tests/utils/gtest-print-helpers.h 1970-01-01 00:00:00 +0000 |
| 933 | +++ tests/utils/gtest-print-helpers.h 2016-10-21 11:40:57 +0000 |
| 934 | @@ -0,0 +1,18 @@ |
| 935 | + |
| 936 | +#pragma once |
| 937 | + |
| 938 | +#include <src/greeter.h> |
| 939 | + |
| 940 | +inline void PrintTo(const Greeter::State& state, std::ostream* os) { |
| 941 | + switch(state) { |
| 942 | + case Greeter::State::ACTIVE: *os << "Active"; break; |
| 943 | + case Greeter::State::INACTIVE: *os << "Inactive"; break; |
| 944 | + case Greeter::State::UNAVAILABLE: *os << "Unavailable"; break; |
| 945 | + } |
| 946 | +} |
| 947 | + |
| 948 | +inline std::ostream& operator<<(std::ostream& os, const Greeter::State& state) { |
| 949 | + PrintTo(state, &os); |
| 950 | + return os; |
| 951 | +} |
| 952 | + |
| 953 | |
| 954 | === modified file 'tests/utils/mock-greeter.h' |
| 955 | --- tests/utils/mock-greeter.h 2016-03-22 20:39:36 +0000 |
| 956 | +++ tests/utils/mock-greeter.h 2016-10-21 11:40:57 +0000 |
| 957 | @@ -26,7 +26,7 @@ |
| 958 | public: |
| 959 | MockGreeter() =default; |
| 960 | virtual ~MockGreeter() =default; |
| 961 | - core::Property<bool>& is_active() override {return m_is_active;} |
| 962 | - core::Property<bool> m_is_active {false}; |
| 963 | + core::Property<Greeter::State>& state() override {return m_state;} |
| 964 | + core::Property<Greeter::State> m_state {State::INACTIVE}; |
| 965 | }; |
| 966 | |
| 967 | |
| 968 | === added file 'tests/utils/mock-unity-greeter.py' |
| 969 | --- tests/utils/mock-unity-greeter.py 1970-01-01 00:00:00 +0000 |
| 970 | +++ tests/utils/mock-unity-greeter.py 2016-10-21 11:40:57 +0000 |
| 971 | @@ -0,0 +1,41 @@ |
| 972 | +'''unity greeter mock template |
| 973 | + |
| 974 | +Very basic template that just mocks the greeter is-active flag |
| 975 | +''' |
| 976 | + |
| 977 | +# This program is free software; you can redistribute it and/or modify it under |
| 978 | +# the terms of the GNU Lesser General Public License as published by the Free |
| 979 | +# Software Foundation; either version 3 of the License, or (at your option) any |
| 980 | +# later version. See http://www.gnu.org/copyleft/lgpl.html for the full text |
| 981 | +# of the license. |
| 982 | + |
| 983 | +__author__ = 'Charles Kerr' |
| 984 | +__email__ = 'charles.kerr@canonical.com' |
| 985 | +__copyright__ = '(c) 2016 Canonical Ltd.' |
| 986 | +__license__ = 'LGPL 3+' |
| 987 | + |
| 988 | +import dbus |
| 989 | +import os |
| 990 | + |
| 991 | +from dbusmock import MOCK_IFACE, mockobject |
| 992 | + |
| 993 | +BUS_NAME = 'com.canonical.UnityGreeter' |
| 994 | +MAIN_OBJ = '/' |
| 995 | +MAIN_IFACE = 'com.canonical.UnityGreeter' |
| 996 | +SYSTEM_BUS = False |
| 997 | + |
| 998 | + |
| 999 | +def load(mock, parameters): |
| 1000 | + mock.AddMethods( |
| 1001 | + MAIN_IFACE, [ |
| 1002 | + ('HideGreeter', '', '', 'self.Set("com.canonical.UnityGreeter", "IsActive", False)'), |
| 1003 | + ('ShowGreeter', '', '', 'self.Set("com.canonical.UnityGreeter", "IsActive", True)') |
| 1004 | + ] |
| 1005 | + ) |
| 1006 | + mock.AddProperties( |
| 1007 | + MAIN_IFACE, |
| 1008 | + dbus.Dictionary({ |
| 1009 | + 'IsActive': parameters.get('IsActive', False), |
| 1010 | + }, signature='sv') |
| 1011 | + ) |
| 1012 | + |
FAILED: Continuous integration, rev:20
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
Click here to trigger a rebuild:
https:/