Merge lp:~mvo/unity-scope-click/lp1292645-use-libclick into lp:unity-scope-click/devel

[Michael Vogt]

Use libclick instead of spawning "click {info,list}"

This branch uses libclick to get the data of installed apps instead of doing run_process().

Please let me know how you want the tests. E.g. do you still want to test for the json parsing exception? Can gmock mock external libs and should I do this?

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:265
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~mvo/unity-scope-click/lp1292645-use-libclick/+merge/220061/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-ci/68/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-amd64-ci/43
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/42
        deb: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/42/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-i386-ci/42

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-team-unity-scope-click-devel-ci/68/rebuild

review: Needs Fixing (continuous-integration)

[dobey (dobey)]

Can you please do "bzr commit --fixes=lp:1292645" to link the bug if this branch is fixing it? Thanks.

review: Needs Fixing

[lp:~mvo/unity-scope-click/lp1292645-use-libclick]

266. By Michael Vogt on 2014-05-19

* add missing --fixes=lp:1292645

[Michael Vogt (mvo)]

Thanks, I commited the missing --fixes as the r266 to this branch.

review: Needs Resubmitting

[dobey (dobey)]

Please don't vote "resubmit" on your own MPs. It's not necessary to vote on your own MP (can you please change your vote to "Abstain" instead)? That is not how one resubmits an MP (it's a vote by a reviewer that the submitter should resubmit for whatever provided reason). There's no need to generally resubmit reviews for every change made to satisfy a review request. :)

[Michael Vogt (mvo)]

review: Abstain

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:266
http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-ci/73/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-amd64-ci/48
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/47
        deb: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/47/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-i386-ci/47

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-team-unity-scope-click-devel-ci/73/rebuild

review: Approve (continuous-integration)

[Alejandro J. Cura (alecu)]

Hi mvo, thanks for working on this.

We can drop the tests that do the specific things that are now done on libclick, but I'd like to keep at least some of the tests that apply to these functions, so eg, looking at:
http://bazaar.launchpad.net/~mvo/unity-scope-click/lp1292645/revision/223

in that branch I see that we can keep getAvailableFrameworksTwoResults and getAvailableFrameworksNoResults if we can mock the result returned by click_framework_get_frameworks.

But gmock can only mock non-static methods, so the workaround is adding yet another class, like explained here: https://code.google.com/p/googlemock/wiki/FrequentlyAskedQuestions#My_code_calls_a_static/global_function.__Can_I_mock_it?

I started preparing a branch that show how to do this:
https://code.launchpad.net/~alecu/unity-scope-click/lp1292645

That branch is missing a way to create fake Framework instances, because their constructor is private; what should happen is that a new GObject type should be created that has the same "name" property as Framework. Or, perhaps simpler, its constructor should be made public in the vala library. I'm travelling tomorrow to the sprint, so I won't be able to continue working on this for a few days; feel free to take over it, or else I can work on my return.

In any case, I agree that this is a lot of work to test these functions that look so simple, so I'm open to discussing alternative options.

review: Needs Information

[Thomas Voß (thomas-voss)]

Hey Michael,

thanks for getting this started. Looking at the code, we might want to have a click::Database abstract base-class, with simple structs Manifest and Framework summarizing all the attributes that the scope is interested in. With that, we could have:

namespace click
{
struct Manifest
{
    // Omitting details here.
};

struct Framework
{
    // Omitting details here.
};

class Database
{
public:
    // Omitting boilerplate here

    virtual void for_each_framework(std::function<void(const click::Framework&)> f) = 0;
    virtual void for_each_manifest(std::function<void(const click::Manifest&)> f) = 0;
};
}

We then could think about having a:

namespace libclick
{
class Database : public click::Database
{
};
}

and would still be able to easily create instances of Manifest and Framework without requiring an instance of click::Database.

What do you think?

[Michael Vogt (mvo)]

This is now superseeded by https://code.launchpad.net/~mvo/unity-scope-click/lp1292645-use-libclick2/+merge/220433 - please either reject or alternatively I can delete the MP (but that will loose the discussion history AIUI that was very helpful).

Unmerged revisions

266. By Michael Vogt on 2014-05-19

* add missing --fixes=lp:1292645

265. By Michael Vogt on 2014-05-19

fix typo

264. By Michael Vogt on 2014-05-19

use libclick instead of run_process("click {info,list}")

263. By Michael Vogt on 2014-05-19

use libclick instead of using list_folder() to get available frameworks

=== modified file 'debian/control'
--- debian/control	2014-05-16 13:09:36 +0000
+++ debian/control	2014-05-19 18:49:23 +0000
@@ -6,6 +6,7 @@
                dh-translations,
                google-mock,
                intltool,
+               libclick-0.4-dev,
                libglib2.0-dev (>= 2.32),
                libjsoncpp-dev,
                libubuntu-download-manager-client-dev (>= 0.3+14.10.20140430-0ubuntu1),

=== modified file 'debian/tests/control'
--- debian/tests/control	2013-12-17 17:56:49 +0000
+++ debian/tests/control	2014-05-19 18:49:23 +0000
@@ -5,6 +5,7 @@
  dh-autoreconf,
  gnome-common,
  libaccounts-glib-dev,
+ libclick-0.4-dev,
  libdee-dev (>= 1.2.5),
  libgee-dev,
  libglib2.0-dev (>= 2.32),

=== modified file 'scope/click/CMakeLists.txt'
--- scope/click/CMakeLists.txt	2014-05-13 19:32:29 +0000
+++ scope/click/CMakeLists.txt	2014-05-19 18:49:23 +0000
@@ -2,6 +2,7 @@
 SET (CMAKE_AUTOMOC ON)
 find_package (Qt5Core REQUIRED)
 pkg_check_modules(JSON_CPP REQUIRED jsoncpp)
+pkg_check_modules(CLICK REQUIRED click-0.4)
 
 add_definitions(
   -DGETTEXT_PACKAGE=\"${PROJECT_NAME}\"
@@ -30,6 +31,7 @@
 include_directories(
   ${CMAKE_SOURCE_DIR}/libclickscope
   ${JSON_CPP_INCLUDE_DIRS}
+  ${CLICK_INCLUDE_DIRS}
 )
 
 qt5_use_modules (${SCOPE_LIB_UNVERSIONED} Network)
@@ -42,6 +44,7 @@
   ${UBUNTUONE_LDFLAGS}
   ${UBUNTU_DOWNLOAD_MANAGER_CLIENT_LDFLAGS}
   ${UBUNTU_DOWNLOAD_MANAGER_COMMON_LDFLAGS}
+  ${CLICK_LDFLAGS}
 )
 
 install(

=== modified file 'scope/click/configuration.cpp'
--- scope/click/configuration.cpp	2014-05-01 21:04:23 +0000
+++ scope/click/configuration.cpp	2014-05-19 18:49:23 +0000
@@ -27,6 +27,10 @@
  * files in the program, then also delete it here.
  */
 
+#include <glib.h>
+#include <click.h>
+
+
 #include <string>
 #include <vector>
 
@@ -57,10 +61,20 @@
 
 std::vector<std::string> Configuration::get_available_frameworks()
 {
+    GList *l = nullptr;
+    GList *frameworks = l = click_framework_get_frameworks();
+
     std::vector<std::string> result;
-    for (auto f: list_folder(FRAMEWORKS_FOLDER, FRAMEWORKS_PATTERN)) {
-        result.push_back(f.substr(0, f.size()-FRAMEWORKS_EXTENSION_LENGTH));
+    while (l != nullptr) {
+        gchar *framework_name = nullptr;
+        GObject *framework = (GObject *)l->data;
+        g_object_get(framework, "name", &framework_name, nullptr);
+        result.push_back(framework_name);
+        l = g_list_next(l);
+        g_free(framework_name);
     }
+    g_list_free_full(frameworks, g_object_unref);
+
     return result;
 }
 

=== modified file 'scope/click/configuration.h'
--- scope/click/configuration.h	2014-05-01 21:04:23 +0000
+++ scope/click/configuration.h	2014-05-19 18:49:23 +0000
@@ -39,9 +39,6 @@
 class Configuration
 {
 public:
-    constexpr static const char* FRAMEWORKS_FOLDER {"/usr/share/click/frameworks/"};
-    constexpr static const char* FRAMEWORKS_PATTERN {"*.framework"};
-    constexpr static const int FRAMEWORKS_EXTENSION_LENGTH = 10; // strlen(".framework")
     constexpr static const char* LANGUAGE_ENVVAR {"LANGUAGE"};
 
     virtual std::vector<std::string> get_available_frameworks();

=== modified file 'scope/click/interface.cpp'
--- scope/click/interface.cpp	2014-05-13 19:32:29 +0000
+++ scope/click/interface.cpp	2014-05-19 18:49:23 +0000
@@ -27,6 +27,8 @@
  * files in the program, then also delete it here.
  */
 
+#include <click-0.4/click.h>
+
 #include <QDebug>
 #include <QDir>
 #include <QProcess>
@@ -323,39 +325,76 @@
 
 void Interface::get_manifests(std::function<void(ManifestList, ManifestError)> callback)
 {
-    std::string command = "click list --manifest";
-    qDebug() << "Running command:" << command.c_str();
-    run_process(command, [callback](int code, const std::string& stdout_data, const std::string&) {
-        if (code == 0) {
-            try {
-                ManifestList manifests = manifest_list_from_json(stdout_data);
-                callback(manifests, ManifestError::NoError);
-            } catch (...) {
-                callback(ManifestList(), ManifestError::ParseError);
-            }
-        } else {
-            callback(ManifestList(), ManifestError::CallError);
-        }
-    });
+   ClickDB *db = click_db_new();
+   GError *error = NULL;
+   char *c_output = NULL;
+   std::string manifest_data;
+
+   c_output = click_db_get_manifests_as_string(db, false, &error);
+   g_object_unref(db);
+   if (error != NULL)
+   {
+      g_error_free(error);
+      // FIXME: pass the GError details on
+      callback(ManifestList(), ManifestError::CallError);
+   }
+   manifest_data = c_output;
+   g_free(c_output);
+
+   try {
+      ManifestList manifests = manifest_list_from_json(manifest_data);
+      callback(manifests, ManifestError::NoError);
+   } catch (...) {
+      callback(ManifestList(), ManifestError::ParseError);
+   }
 }
 
 void Interface::get_manifest_for_app(const std::string &app_id,
                                      std::function<void(Manifest, ManifestError)> callback)
 {
-    std::string command = "click info " + app_id;
-    qDebug() << "Running command:" << command.c_str();
-    run_process(command, [callback](int code, const std::string& stdout_data, const std::string&) {
-        if (code == 0) {
-            try {
-                Manifest manifest = manifest_from_json(stdout_data);
-                callback(manifest, ManifestError::NoError);
-            } catch (...) {
-                callback(Manifest(), ManifestError::ParseError);
-            }
-        } else {
-            callback(Manifest(), ManifestError::CallError);
-        }
-    });
+    GError *error = NULL;
+    ClickDB *db = NULL;
+    ClickUser *user = NULL;
+    char *c_output = NULL;
+
+    db = click_db_new();
+    click_db_read(db, NULL, &error);
+    if (error != NULL) {
+       g_error_free (error);
+       g_object_unref(db);
+       // FIXME: pass error data along
+       callback(Manifest(), ManifestError::CallError);
+    }
+
+    user = click_user_new_for_user(db, NULL, &error);
+    if (error != NULL) {
+       g_error_free (error);
+       g_object_unref(db);
+       // FIXME: pass error data along
+       callback(Manifest(), ManifestError::CallError);
+    }
+    if (click_user_has_package_name(user, app_id.c_str()) == false) {
+       g_error_free (error);
+       g_object_unref(db);
+       g_object_unref(user);
+       // FIXME: hm, not a call error as such really more a NotInstalledError ?
+       callback(Manifest(), ManifestError::CallError);
+    }
+    
+    c_output = click_user_get_manifest_as_string (user, app_id.c_str(), &error);
+    std::string json_output = c_output;
+
+    g_free(c_output);
+    g_object_unref(db);
+    g_object_unref(user);
+
+    try {
+       Manifest manifest = manifest_from_json(json_output);
+       callback(manifest, ManifestError::NoError);
+    } catch (...) {
+       callback(Manifest(), ManifestError::ParseError);
+    }
+
 }
 
 void Interface::get_dotdesktop_filename(const std::string &app_id,
@@ -380,33 +419,4 @@
     });
 }
 
-void Interface::run_process(const std::string& command,
-                            std::function<void(int code,
-                                               const std::string& stdout_data,
-                                               const std::string& stderr_data)> callback)
-{
-    QSharedPointer<QProcess> process(new QProcess());
-    typedef void(QProcess::*QProcessFinished)(int, QProcess::ExitStatus);
-    typedef void(QProcess::*QProcessError)(QProcess::ProcessError);
-    QObject::connect(process.data(),
-                     static_cast<QProcessFinished>(&QProcess::finished),
-                     [callback, process](int code, QProcess::ExitStatus /*status*/) {
-                         qDebug() << "command finished with exit code:" << code;
-                         auto data = process.data()->readAllStandardOutput().data();
-                         auto errors = process.data()->readAllStandardError().data();
-                         callback(code, data, errors);
-                     } );
-
-    QObject::connect(process.data(),
-                     static_cast<QProcessError>(&QProcess::error),
-                     [callback, process](QProcess::ProcessError error) {
-                         qCritical() << "error running command:" << error;
-                         auto data = process.data()->readAllStandardOutput().data();
-                         auto errors = process.data()->readAllStandardError().data();
-                         callback(process.data()->exitCode(), data, errors);
-                     } );
-
-    process->start(command.c_str());
-}
-
 } // namespace click

=== modified file 'scope/click/interface.h'
--- scope/click/interface.h	2014-05-06 19:17:30 +0000
+++ scope/click/interface.h	2014-05-19 18:49:23 +0000
@@ -95,10 +95,6 @@
     virtual bool is_visible_app(const unity::util::IniParser& keyFile);
     virtual bool show_desktop_apps();
 
-    virtual void run_process(const std::string& command,
-                             std::function<void(int code,
-                                                const std::string& stdout_data,
-                                                const std::string& stderr_data)> callback);
 private:
     QSharedPointer<KeyFileLocator> keyFileLocator;
 };

=== modified file 'scope/tests/test_configuration.cpp'
--- scope/tests/test_configuration.cpp	2014-05-01 21:04:23 +0000
+++ scope/tests/test_configuration.cpp	2014-05-19 18:49:23 +0000
@@ -46,50 +46,10 @@
 
 }
 
-
-TEST(Configuration, getAvailableFrameworksUsesRightFolder)
-{
-    using namespace ::testing;
-    FakeConfiguration locator;
-    EXPECT_CALL(locator, list_folder(Configuration::FRAMEWORKS_FOLDER, _))
-            .Times(1).WillOnce(Return(std::vector<std::string>()));
-    locator.get_available_frameworks();
-}
-
-TEST(Configuration, getAvailableFrameworksUsesRightPattern)
-{
-    using namespace ::testing;
-    FakeConfiguration locator;
-    EXPECT_CALL(locator, list_folder(_, Configuration::FRAMEWORKS_PATTERN))
-            .Times(1).WillOnce(Return(std::vector<std::string>()));
-    locator.get_available_frameworks();
-}
-
-TEST(Configuration, getAvailableFrameworksTwoResults)
-{
-    using namespace ::testing;
-
-    FakeConfiguration locator;
-    std::vector<std::string> response = {"abc.framework", "def.framework"};
-    EXPECT_CALL(locator, list_folder(_, _))
-            .Times(1)
-            .WillOnce(Return(response));
-    auto frameworks = locator.get_available_frameworks();
-    std::vector<std::string> expected = {"abc", "def"};
-    EXPECT_EQ(expected, frameworks);
-}
-
-TEST(Configuration, getAvailableFrameworksNoResults)
-{
-    using namespace ::testing;
-
-    FakeConfiguration locator;
-    std::vector<std::string> response = {};
-    EXPECT_CALL(locator, list_folder(_, _))
-            .Times(1)
-            .WillOnce(Return(response));
-    auto frameworks = locator.get_available_frameworks();
-    EXPECT_EQ(0, frameworks.size());
+TEST(Configuration, getAvailableFrameworksSmokeTest)
+{
+    FakeConfiguration locator;
+    std::vector<std::string> frameworks = locator.get_available_frameworks();
 }
 
 TEST(Configuration, getLanguageCorrect)

=== modified file 'scope/tests/test_interface.cpp'
--- scope/tests/test_interface.cpp	2014-05-06 19:17:30 +0000
+++ scope/tests/test_interface.cpp	2014-05-19 18:49:23 +0000
@@ -130,7 +130,6 @@
     FakeClickInterface() {}
 
     MOCK_METHOD0(show_desktop_apps, bool());
-    MOCK_METHOD2(run_process, void(const std::string&, std::function<void(int, const std::string&, const std::string&)>));
 };
 
 TEST(ClickInterface, testIsNonClickAppFalse)
@@ -320,140 +319,4 @@
     EXPECT_FALSE(iface.show_desktop_apps());
 }
 
-TEST(ClickInterface, testGetManifestForAppCorrectCommand)
-{
-    FakeClickInterface iface;
-    std::string command = "click info " + FAKE_PACKAGENAME;
-    EXPECT_CALL(iface, run_process(command, _)).
-        Times(1);
-    iface.get_manifest_for_app(FAKE_PACKAGENAME, [](Manifest, ManifestError){});
-}
-
-TEST_F(ClickInterfaceTest, testGetManifestForAppParseError)
-{
-    FakeClickInterface iface;
-    EXPECT_CALL(iface, run_process(_, _)).
-        Times(1).
-        WillOnce(Invoke([&](const std::string&,
-                            std::function<void(int, const std::string&,
-                                               const std::string&)> callback){
-                            callback(0, "INVALID JSON", "");
-                        }));
-    EXPECT_CALL(*this, manifest_callback(_, ManifestError::ParseError));
-    iface.get_manifest_for_app(FAKE_PACKAGENAME, [this](Manifest manifest,
-                                                        ManifestError error){
-                                   manifest_callback(manifest, error);
-                               });
-}
-
-TEST_F(ClickInterfaceTest, testGetManifestForAppCommandFailed)
-{
-    FakeClickInterface iface;
-    EXPECT_CALL(iface, run_process(_, _)).
-        Times(1).
-        WillOnce(Invoke([&](const std::string&,
-                            std::function<void(int, const std::string&,
-                                               const std::string&)> callback){
-                            callback(-1, "", "CRITICAL: FAIL");
-                        }));
-    EXPECT_CALL(*this, manifest_callback(_, ManifestError::CallError));
-    iface.get_manifest_for_app(FAKE_PACKAGENAME, [this](Manifest manifest,
-                                                        ManifestError error){
-                                   manifest_callback(manifest, error);
-                               });
-}
-
-TEST_F(ClickInterfaceTest, testGetManifestForAppIsRemovable)
-{
-    FakeClickInterface iface;
-    EXPECT_CALL(iface, run_process(_, _)).
-        Times(1).
-        WillOnce(Invoke([&](const std::string&,
-                            std::function<void(int, const std::string&,
-                                               const std::string&)> callback){
-                            callback(0, FAKE_JSON_MANIFEST_REMOVABLE, "");
-                        }));
-    iface.get_manifest_for_app(FAKE_PACKAGENAME, [](Manifest manifest,
-                                                    ManifestError error){
-                                   ASSERT_TRUE(error == ManifestError::NoError);
-                                   ASSERT_TRUE(manifest.removable);
-                               });
-}
-
-TEST_F(ClickInterfaceTest, testGetManifestForAppIsNotRemovable)
-{
-    FakeClickInterface iface;
-    EXPECT_CALL(iface, run_process(_, _)).
-        Times(1).
-        WillOnce(Invoke([&](const std::string&,
-                            std::function<void(int, const std::string&,
-                                               const std::string&)> callback){
-                            callback(0, FAKE_JSON_MANIFEST_NONREMOVABLE, "");
-                        }));
-    iface.get_manifest_for_app(FAKE_PACKAGENAME, [](Manifest manifest,
-                                                    ManifestError error){
-                                   ASSERT_TRUE(error == ManifestError::NoError);
-                                   ASSERT_FALSE(manifest.removable);
-                               });
-}
-
-TEST(ClickInterface, testGetManifestsCorrectCommand)
-{
-    FakeClickInterface iface;
-    std::string command = "click list --manifest";
-    EXPECT_CALL(iface, run_process(command, _)).
-        Times(1);
-    iface.get_manifests([](ManifestList, ManifestError){});
-}
-
-TEST_F(ClickInterfaceTest, testGetManifestsParseError)
-{
-    FakeClickInterface iface;
-    EXPECT_CALL(iface, run_process(_, _)).
-        Times(1).
-        WillOnce(Invoke([&](const std::string&,
-                            std::function<void(int, const std::string&,
-                                               const std::string&)> callback){
-                            callback(0, "INVALID JSON", "");
-                        }));
-    EXPECT_CALL(*this, manifests_callback(_, ManifestError::ParseError));
-    iface.get_manifests([this](ManifestList manifests, ManifestError error){
-            manifests_callback(manifests, error);
-        });
-}
-
-TEST_F(ClickInterfaceTest, testGetManifestsCommandFailed)
-{
-    FakeClickInterface iface;
-    EXPECT_CALL(iface, run_process(_, _)).
-        Times(1).
-        WillOnce(Invoke([&](const std::string&,
-                            std::function<void(int, const std::string&,
-                                               const std::string&)> callback){
-                            callback(-1, "", "CRITICAL: FAIL");
-                        }));
-    EXPECT_CALL(*this, manifests_callback(_, ManifestError::CallError));
-    iface.get_manifests([this](ManifestList manifests, ManifestError error){
-            manifests_callback(manifests, error);
-        });
-}
-
-TEST_F(ClickInterfaceTest, testGetManifestsParsed)
-{
-    FakeClickInterface iface;
-    std::string expected_str = "[" + FAKE_JSON_MANIFEST_NONREMOVABLE + "," +
-        FAKE_JSON_MANIFEST_REMOVABLE + "]";
-    ManifestList expected = manifest_list_from_json(expected_str);
-
-    EXPECT_CALL(iface, run_process(_, _)).
-        Times(1).
-        WillOnce(Invoke([&](const std::string&,
-                            std::function<void(int, const std::string&,
-                                               const std::string&)> callback){
-                            callback(0, expected_str, "");
-                        }));
-    iface.get_manifests([expected](ManifestList manifests, ManifestError error){
-            ASSERT_TRUE(error == ManifestError::NoError);
-            ASSERT_TRUE(manifests.size() == expected.size());
-        });
-}
+// FIXME: re-add tests by mocking libclick