[Merge] lp:~ssweeny/location-service/async-providers.15.04 into lp:location-service/15.04
Thomas Voß
thomas.voss at canonical.com
Wed Nov 25 10:57:02 UTC 2015
Review: Needs Fixing
Looking good thus far, a few niggles inline. Btw: indent with 4 spaces, please, indentation in namespaces is 0.
Diff comments:
>
> === modified file 'src/location_service/com/ubuntu/location/providers/remote/provider.cpp'
> --- src/location_service/com/ubuntu/location/providers/remote/provider.cpp 2015-04-14 18:54:00 +0000
> +++ src/location_service/com/ubuntu/location/providers/remote/provider.cpp 2015-11-24 22:39:44 +0000
> @@ -198,10 +198,16 @@
> {
> std::shared_ptr<remote::Provider::Stub> result{new remote::Provider::Stub{config}};
>
> - // This call throws if we fail to reach the remote end. With that, we make sure that
> - // we do not return a potentially invalid instance that throws later on.
> - result->ping();
> -
> + // Since we can register asynchronously now, wait for the service to come up
> + while (true) {// XXX(ssweeny): There must be a better way.
Yup, I think we want to check if someone owns the "name" of the provider on the bus, if so: go ahead. If not: monitor the name for when it is claimed on the bus and carry out required setup steps (see http://bazaar.launchpad.net/~phablet-team/dbus-cpp/15.04/view/head:/tests/service_watcher_test.cpp#L96 for an example of how to use a dbus::ServiceWatcher).
> + try {
> + result->ping();
> + break;
> + } catch (std::exception e)
Throw by value, catch by reference :)
> + {
> + continue;
> + }
> + }
> result->setup_event_connections();
> return result;
> }
>
> === added file 'tests/delayed_service_test.cpp'
> --- tests/delayed_service_test.cpp 1970-01-01 00:00:00 +0000
> +++ tests/delayed_service_test.cpp 2015-11-24 22:39:44 +0000
> @@ -0,0 +1,198 @@
> +/*
> + * Copyright © 2014 Canonical Ltd.
> + *
> + * This program is free software: you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser General Public License version 3,
> + * as published by the Free Software Foundation.
> + *
> + * This program 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 program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Authored by: Thomas Voß <thomas.voss at canonical.com>
> + */
> +
> +#include <com/ubuntu/location/service/daemon.h>
> +#include <com/ubuntu/location/service/provider_daemon.h>
> +#include <com/ubuntu/location/providers/remote/provider.h>
> +
> +#include <com/ubuntu/location/service/stub.h>
> +
> +#include <com/ubuntu/location/position.h>
> +#include <com/ubuntu/location/update.h>
> +
> +#include <core/dbus/fixture.h>
> +
> +#include <core/dbus/asio/executor.h>
> +
> +#include <core/posix/fork.h>
> +#include <core/posix/signal.h>
> +#include <core/posix/this_process.h>
> +
> +#include "did_finish_successfully.h"
> +
> +#include <gmock/gmock.h>
> +
> +namespace location = com::ubuntu::location;
> +namespace remote = com::ubuntu::location::providers::remote;
> +
> +namespace
> +{
> +struct MockEventReceiver
Hmmm, I think we are using this little helper a lot around in the tests :) Mind factoring it into a common header, into namespace testing?
> +{
> + MOCK_METHOD1(position_update_received, void(const location::Update<location::Position>&));
> + MOCK_METHOD1(heading_update_received, void(const location::Update<location::Heading>&));
> + MOCK_METHOD1(velocity_update_received, void(const location::Update<location::Velocity>&));
> +};
> +
> +struct DelayedServiceTest : public core::dbus::testing::Fixture, public ::testing::WithParamInterface<int>
> +{
> +
> +};
> +}
> +
> +TEST_P(DelayedServiceTest, AClientReceivesUpdatesFromADelayedProvider)
> +{
> + auto oopp = core::posix::fork([this]()
> + {
> + auto bus = session_bus();
> +
> + // Pass the delay value to the provider
> + char delay[50];
> + sprintf(delay, "--dummy::DelayedProvider::DelayInMs=%d", GetParam());
> +
> + const char* argv[] =
> + {
> + "--bus", "session", // 2
> + "--service-name", "com.ubuntu.location.providers.Dummy", // 4
> + "--service-path", "/com/ubuntu/location/providers/Dummy", // 6
> + "--provider", "dummy::DelayedProvider", // 8
> + delay // 9
> + };
> +
> + auto dbus_connection_factory = [bus](core::dbus::WellKnownBus)
> + {
> + return bus;
> + };
> +
> + return static_cast<core::posix::exit::Status>(
> + location::service::ProviderDaemon::main(
> + location::service::ProviderDaemon::Configuration::from_command_line_args(
> + 9, argv, dbus_connection_factory)));
> + }, core::posix::StandardStream::empty);
> +
> + std::this_thread::sleep_for(std::chrono::milliseconds{250});
> +
> + auto service = core::posix::fork([this]()
> + {
> + core::posix::this_process::env::set_or_throw(
> + "TRUST_STORE_PERMISSION_MANAGER_IS_RUNNING_UNDER_TESTING",
> + "1");
> +
> + const char* argv[] =
> + {
> + "--bus", "session", // 2
> + "--provider", "remote::Provider", // 4
> + "--remote::Provider::bus=session_with_address_from_env", // 5
> + "--remote::Provider::name=com.ubuntu.location.providers.Dummy", // 6
> + "--remote::Provider::path=/com/ubuntu/location/providers/Dummy" // 7
> + };
> +
> + // The daemon instance requires two bus instances.
> + auto dbus_connection_factory = [this](core::dbus::WellKnownBus)
> + {
> + return session_bus();
> + };
> +
> + return static_cast<core::posix::exit::Status>(
> + location::service::Daemon::main(
> + location::service::Daemon::Configuration::from_command_line_args(
> + 7, argv, dbus_connection_factory)));
> + }, core::posix::StandardStream::empty);
> +
> + // Wait to setup the client until the service is up
> + std::this_thread::sleep_for(std::chrono::milliseconds{250 + GetParam()});
> +
> + auto client = core::posix::fork([this]()
> + {
> + using namespace ::testing;
> +
> + auto trap = core::posix::trap_signals_for_all_subsequent_threads(
> + {
> + core::posix::Signal::sig_term,
> + core::posix::Signal::sig_int
> + });
> +
> + trap->signal_raised().connect([trap](const core::posix::Signal&)
> + {
> + trap->stop();
> + });
> +
> + auto bus = session_bus();
> + bus->install_executor(core::dbus::asio::make_executor(bus));
> +
> + std::thread worker
> + {
> + [bus]() { bus->run(); }
> + };
> +
> + MockEventReceiver receiver;
> + EXPECT_CALL(receiver, position_update_received(_)).Times(AtLeast(1));
> + EXPECT_CALL(receiver, heading_update_received(_)).Times(AtLeast(1));
> + EXPECT_CALL(receiver, velocity_update_received(_)).Times(AtLeast(1));
> +
> + com::ubuntu::location::service::Stub service{bus};
> +
> + auto session = service.create_session_for_criteria(location::Criteria{});
> +
> + session->updates().position.changed().connect([&receiver](location::Update<location::Position> pos)
> + {
> + receiver.position_update_received(pos);
> + });
> +
> + session->updates().heading.changed().connect([&receiver](location::Update<location::Heading> heading)
> + {
> + receiver.heading_update_received(heading);
> + });
> +
> + session->updates().velocity.changed().connect([&receiver](location::Update<location::Velocity> velocity)
> + {
> + receiver.velocity_update_received(velocity);
> + });
> +
> + session->updates().position_status = location::service::session::Interface::Updates::Status::enabled;
> + session->updates().velocity_status = location::service::session::Interface::Updates::Status::enabled;
> + session->updates().heading_status = location::service::session::Interface::Updates::Status::enabled;
> +
> + trap->run();
> +
> + bus->stop();
> +
> + if (worker.joinable())
> + worker.join();
> +
> + return ::testing::Test::HasFailure() ? core::posix::exit::Status::failure :
> + core::posix::exit::Status::success;
> + }, core::posix::StandardStream::empty);
> +
> + std::this_thread::sleep_for(std::chrono::milliseconds{4000});
> +
> + std::cout << "Shutting down client" << std::endl;
> + client.send_signal_or_throw(core::posix::Signal::sig_term);
> + EXPECT_TRUE(did_finish_successfully(client.wait_for(core::posix::wait::Flags::untraced)));
> +
> + std::cout << "Shutting down service" << std::endl;
> + service.send_signal_or_throw(core::posix::Signal::sig_term);
> + EXPECT_TRUE(did_finish_successfully(service.wait_for(core::posix::wait::Flags::untraced)));
> +
> + std::cout << "Shutting down oopp" << std::endl;
> + oopp.send_signal_or_throw(core::posix::Signal::sig_term);
> + EXPECT_TRUE(did_finish_successfully(oopp.wait_for(core::posix::wait::Flags::untraced)));
> +}
> +
> +INSTANTIATE_TEST_CASE_P(ServiceDelays, DelayedServiceTest,
> + ::testing::Values(0, 250, 500, 1000, 2000, 5000, 10000));
--
https://code.launchpad.net/~ssweeny/location-service/async-providers.15.04/+merge/278092
Your team Ubuntu Phablet Team is subscribed to branch lp:location-service/15.04.
More information about the Ubuntu-reviews
mailing list