Merge lp:~ssweeny/location-service/cleanups into lp:location-service/trunk

[Scott Sweeny]

Small fixes around provider loading

A couple of small refactors previously discussed with tvoss:

 * provider loading loop
 * extracting undecorated provider names

Also, clear any stored daemon options before parsing new ones. They were persisting between test runs, which I believe invalidated the cli tests (happily with this fix they still pass).

[Thomas Voß (thomas-voss)]

A suggestion inline, let me know what you think.

review: Needs Information

[Scott Sweeny (ssweeny)]

> A suggestion inline, let me know what you think.

Would that work with the current async loading code?

[Konrad Zapałowicz (kzapalowicz)]

Looks good. One thing however: I do not like mixing the placement of & in functions signature, i.e.

foo(const int& a);
bar(const int &b);

we should agree on keeping it one way.

review: Approve

[Scott Sweeny (ssweeny)]

> Looks good. One thing however: I do not like mixing the placement of & in
> functions signature, i.e.
>
> foo(const int& a);
> bar(const int &b);
>
> we should agree on keeping it one way.

I think that's the result of my preferences and QtCreator's preferences not matching up. I'll fix these up. Thanks!

[Thomas Voß (thomas-voss)]

LGTM, *ideally*, we would have an interface "ProviderInstanceReceiver" that is implemented by Engine, but I will not block on that :)

review: Approve

[Scott Sweeny (ssweeny)]

Resubmitted against trunk

[Thomas Voß (thomas-voss)]

LGTM.

review: Approve

=== modified file 'include/location_service/com/ubuntu/location/provider_factory.h'
--- include/location_service/com/ubuntu/location/provider_factory.h	2015-11-19 17:56:19 +0000
+++ include/location_service/com/ubuntu/location/provider_factory.h	2016-04-05 21:09:59 +0000
@@ -59,6 +59,8 @@
 
     void enumerate(const std::function<void(const std::string&, const Factory&)>& enumerator);
 
+    static std::string extract_undecorated_name(const std::string& name);
+
   private:
     ProviderFactory() = default;
     ~ProviderFactory() = default;

=== modified file 'src/location_service/com/ubuntu/location/provider_factory.cpp'
--- src/location_service/com/ubuntu/location/provider_factory.cpp	2015-11-19 17:56:19 +0000
+++ src/location_service/com/ubuntu/location/provider_factory.cpp	2016-04-05 21:09:59 +0000
@@ -43,7 +43,7 @@
     const std::string& name, 
     const cul::ProviderFactory::Configuration& config)
 {
-    auto undecorated_name = name.substr(0, name.find("@"));
+    auto undecorated_name = extract_undecorated_name(name);
 
     std::lock_guard<std::mutex> lg(guard);
     if (factory_store.count(undecorated_name) == 0)
@@ -57,7 +57,7 @@
     const cul::ProviderFactory::Configuration& config,
     const std::function<void(Provider::Ptr)>& cb)
 {
-    auto undecorated_name = name.substr(0, name.find("@"));
+    auto undecorated_name = extract_undecorated_name(name);
 
     std::lock_guard<std::mutex> lg(guard);
     if (factory_store.count(undecorated_name) == 0)
@@ -79,4 +79,9 @@
         });
 }
 
+std::string cul::ProviderFactory::extract_undecorated_name(const std::string& name)
+{
+    return name.substr(0, name.find("@"));
+}
+
 

=== modified file 'src/location_service/com/ubuntu/location/service/daemon.cpp'
--- src/location_service/com/ubuntu/location/service/daemon.cpp	2016-03-31 10:15:53 +0000
+++ src/location_service/com/ubuntu/location/service/daemon.cpp	2016-04-05 21:09:59 +0000
@@ -109,6 +109,9 @@
 {
     location::service::Daemon::Configuration result;
 
+    // Make sure options are cleared between runs (needed for testing)
+    mutable_daemon_options().clear();
+
     if (!mutable_daemon_options().parse_from_command_line_args(argc, (const char**)argv))
         throw std::runtime_error{"Could not parse command-line, aborting..."};
 
@@ -169,6 +172,32 @@
     mutable_daemon_options().print_help(out);
 }
 
+void location::service::Daemon::load_providers(const Configuration& config, std::shared_ptr<Engine> engine)
+{
+    for (const std::string& provider : config.providers)
+    {
+        std::cout << "Instantiating and configuring: " << provider << std::endl;
+
+        try
+        {
+            auto result = std::async(std::launch::async, [provider, config, engine] {
+                return location::ProviderFactory::instance().create_provider_for_name_with_config(
+                    provider,
+                    config.provider_options.count(provider) > 0 ?
+                        config.provider_options.at(provider) : location::Configuration {},
+                    [engine](Provider::Ptr provider)
+                    {
+                        engine->add_provider(provider);
+                    }
+                );
+            });
+        } catch(const std::runtime_error& e)
+        {
+            std::cerr << "Issue instantiating provider: " << e.what() << std::endl;
+        }
+    }
+}
+
 int location::service::Daemon::main(const location::service::Daemon::Configuration& config)
 {
     // Ensure that log files dating back to before the fix
@@ -200,28 +229,7 @@
 
     location::service::DefaultConfiguration dc;
     auto engine = dc.the_engine(std::set<location::Provider::Ptr>{}, dc.the_provider_selection_policy(), config.settings);
-    for (const std::string& provider : config.providers)
-    {
-        std::cout << "Instantiating and configuring: " << provider << std::endl;
-
-        try
-        {
-            auto result = std::async(std::launch::async, [provider, config, engine] {
-                return location::ProviderFactory::instance().create_provider_for_name_with_config(
-                    provider,
-                    config.provider_options.count(provider) > 0 ?
-                        config.provider_options.at(provider) : location::Configuration {},
-                    [engine](Provider::Ptr provider)
-                    {
-                        engine->add_provider(provider);
-                    }
-                );
-            });
-        } catch(const std::runtime_error& e)
-        {
-            std::cerr << "Issue instantiating provider: " << e.what() << std::endl;
-        }
-    }
+    load_providers(config, engine);
 
     config.incoming->install_executor(dbus::asio::make_executor(config.incoming, runtime->service()));
     config.outgoing->install_executor(dbus::asio::make_executor(config.outgoing, runtime->service()));

=== modified file 'src/location_service/com/ubuntu/location/service/daemon.h'
--- src/location_service/com/ubuntu/location/service/daemon.h	2015-01-25 12:45:30 +0000
+++ src/location_service/com/ubuntu/location/service/daemon.h	2016-04-05 21:09:59 +0000
@@ -19,6 +19,7 @@
 #define LOCATION_SERVICE_COM_UBUNTU_LOCATION_SERVICE_DAEMON_H_
 
 #include <com/ubuntu/location/configuration.h>
+#include <com/ubuntu/location/engine.h>
 #include <com/ubuntu/location/settings.h>
 
 #include <com/ubuntu/location/service/dbus_connection_factory.h>
@@ -163,6 +164,9 @@
     /** @brief Pretty-prints the CLI's help text to the given output stream. */
     static void print_help(std::ostream& out);
 
+    /** @brief Instantiates and configures each provider selected in the config */
+    static void load_providers(const Configuration& config, std::shared_ptr<location::Engine> engine);
+
     /**
      * @brief Executes the daemon with the given configuration.
      * @return EXIT_SUCCESS or EXIT_FAILURE.

=== modified file 'src/location_service/com/ubuntu/location/service/program_options.h'
--- src/location_service/com/ubuntu/location/service/program_options.h	2014-06-20 07:40:34 +0000
+++ src/location_service/com/ubuntu/location/service/program_options.h	2016-04-05 21:09:59 +0000
@@ -157,6 +157,11 @@
             enumerator(s);
     }
 
+    void clear()
+    {
+        vm.clear();
+    }
+
     void print(std::ostream& out) const
     {
         for (const auto& pair : vm)

=== modified file 'tests/daemon_and_cli_tests.cpp'
--- tests/daemon_and_cli_tests.cpp	2014-12-17 09:26:11 +0000
+++ tests/daemon_and_cli_tests.cpp	2016-04-05 21:09:59 +0000
@@ -17,6 +17,7 @@
  */
 
 #include <com/ubuntu/location/service/daemon.h>
+#include <com/ubuntu/location/service/default_configuration.h>
 
 #include <com/ubuntu/location/boost_ptree_settings.h>
 #include <com/ubuntu/location/space_vehicle.h>
@@ -28,8 +29,11 @@
 #include <core/testing/cross_process_sync.h>
 #include <core/testing/fork_and_run.h>
 
+#include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
+#include "mock_engine.h"
+
 #include <ctime>
 
 #include <thread>
@@ -263,3 +267,39 @@
     EXPECT_EQ("test2", config.provider_options.at(config.providers[0]).get<std::string>("option2"));
     EXPECT_EQ("test3", config.provider_options.at(config.providers[0]).get<std::string>("option3"));
 }
+
+TEST(Daemon, ProviderLoadingWorks)
+{
+    const char* args[] =
+    {
+        "--bus", "session",
+        "--provider", "dummy::Provider",
+    };
+
+    auto config = location::service::Daemon::Configuration::from_command_line_args(4, args, null_dbus_connection_factory);
+    location::service::DefaultConfiguration dc;
+    auto engine = std::make_shared<MockEngine>(dc.the_provider_selection_policy(), config.settings);
+
+    EXPECT_CALL(*engine, add_provider(::testing::_));
+
+    location::service::Daemon::load_providers(config, engine);
+}
+
+TEST(Daemon, MultipleProviderLoadingWorks)
+{
+    const char* args[] =
+    {
+        "--bus", "session",
+        "--provider", "dummy::Provider",
+        "--provider", "dummy::DelayedProvider",
+        "--dummy::DelayedProvider::DelayInMs=250"
+    };
+
+    auto config = location::service::Daemon::Configuration::from_command_line_args(7, args, null_dbus_connection_factory);
+    location::service::DefaultConfiguration dc;
+    auto engine = std::make_shared<MockEngine>(dc.the_provider_selection_policy(), config.settings);
+
+    EXPECT_CALL(*engine, add_provider(::testing::_)).Times(2);
+
+    location::service::Daemon::load_providers(config, engine);
+}

=== added file 'tests/mock_engine.h'
--- tests/mock_engine.h	1970-01-01 00:00:00 +0000
+++ tests/mock_engine.h	2016-04-05 21:09:59 +0000
@@ -0,0 +1,41 @@
+/*
+ * Copyright © 2016 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: Scott Sweeny <[e-mail]
+ */
+#ifndef MOCK_ENGINE_H
+#define MOCK_ENGINE_H
+
+#include <com/ubuntu/location/engine.h>
+#include <com/ubuntu/location/provider.h>
+
+#include <gmock/gmock.h>
+
+struct MockEngine : public com::ubuntu::location::Engine
+{
+    MockEngine(
+            const com::ubuntu::location::ProviderSelectionPolicy::Ptr& provider_selection_policy,
+            const com::ubuntu::location::Settings::Ptr& settings)
+            : com::ubuntu::location::Engine(provider_selection_policy, settings)
+    {
+    }
+
+    // has_provider and remove_provider cannot be mocked because they are marked noexcept
+    MOCK_METHOD1(determine_provider_selection_for_criteria, com::ubuntu::location::ProviderSelection(const com::ubuntu::location::Criteria&));
+    MOCK_METHOD1(add_provider, void(const com::ubuntu::location::Provider::Ptr&));
+    MOCK_METHOD1(for_each_provider, void(const std::function<void(const com::ubuntu::location::Provider::Ptr&)>& enumerator));
+};
+
+#endif // MOCK_ENGINE_H

=== modified file 'tests/provider_factory_test.cpp'
--- tests/provider_factory_test.cpp	2013-12-10 09:42:54 +0000
+++ tests/provider_factory_test.cpp	2016-04-05 21:09:59 +0000
@@ -53,3 +53,9 @@
                   "AnUnknownProvider", 
                   cul::ProviderFactory::Configuration{}));
 }
+
+TEST(ProviderFactory, extracting_undecorated_provider_name_works)
+{
+    EXPECT_EQ("remote::Provider", cul::ProviderFactory::extract_undecorated_name("remote::Provider@gps"));
+    EXPECT_EQ("remote::Provider", cul::ProviderFactory::extract_undecorated_name("remote::Provider@espoo"));
+}