[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