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

Proposed by Michael Vogt
Status: Rejected
Rejected by: Alejandro J. Cura
Proposed branch: lp:~mvo/unity-scope-click/lp1292645-use-libclick
Merge into: lp:unity-scope-click/devel
Diff against target: 483 lines (+93/-248)
9 files modified
debian/control (+1/-0)
debian/tests/control (+1/-0)
scope/click/CMakeLists.txt (+3/-0)
scope/click/configuration.cpp (+16/-2)
scope/click/configuration.h (+0/-3)
scope/click/interface.cpp (+67/-57)
scope/click/interface.h (+0/-4)
scope/tests/test_configuration.cpp (+4/-44)
scope/tests/test_interface.cpp (+1/-138)
To merge this branch: bzr merge lp:~mvo/unity-scope-click/lp1292645-use-libclick
Reviewer Review Type Date Requested Status
Alejandro J. Cura (community) Needs Information
PS Jenkins bot (community) continuous-integration Approve
Michael Vogt Abstain
dobey (community) Needs Fixing
Review via email: mp+220061@code.launchpad.net

Commit message

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

Description of the change

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?

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
dobey (dobey) wrote :

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

review: Needs Fixing
266. By Michael Vogt

* add missing --fixes=lp:1292645

Revision history for this message
Michael Vogt (mvo) wrote :

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

review: Needs Resubmitting
Revision history for this message
dobey (dobey) wrote :

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. :)

Revision history for this message
Michael Vogt (mvo) :
review: Abstain
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Alejandro J. Cura (alecu) wrote :

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
Revision history for this message
Thomas Voß (thomas-voss) wrote :

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?

Revision history for this message
Michael Vogt (mvo) wrote :

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

* add missing --fixes=lp:1292645

265. By Michael Vogt

fix typo

264. By Michael Vogt

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

263. By Michael Vogt

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/control'
2--- debian/control 2014-05-16 13:09:36 +0000
3+++ debian/control 2014-05-19 18:49:23 +0000
4@@ -6,6 +6,7 @@
5 dh-translations,
6 google-mock,
7 intltool,
8+ libclick-0.4-dev,
9 libglib2.0-dev (>= 2.32),
10 libjsoncpp-dev,
11 libubuntu-download-manager-client-dev (>= 0.3+14.10.20140430-0ubuntu1),
12
13=== modified file 'debian/tests/control'
14--- debian/tests/control 2013-12-17 17:56:49 +0000
15+++ debian/tests/control 2014-05-19 18:49:23 +0000
16@@ -5,6 +5,7 @@
17 dh-autoreconf,
18 gnome-common,
19 libaccounts-glib-dev,
20+ libclick-0.4-dev,
21 libdee-dev (>= 1.2.5),
22 libgee-dev,
23 libglib2.0-dev (>= 2.32),
24
25=== modified file 'scope/click/CMakeLists.txt'
26--- scope/click/CMakeLists.txt 2014-05-13 19:32:29 +0000
27+++ scope/click/CMakeLists.txt 2014-05-19 18:49:23 +0000
28@@ -2,6 +2,7 @@
29 SET (CMAKE_AUTOMOC ON)
30 find_package (Qt5Core REQUIRED)
31 pkg_check_modules(JSON_CPP REQUIRED jsoncpp)
32+pkg_check_modules(CLICK REQUIRED click-0.4)
33
34 add_definitions(
35 -DGETTEXT_PACKAGE=\"${PROJECT_NAME}\"
36@@ -30,6 +31,7 @@
37 include_directories(
38 ${CMAKE_SOURCE_DIR}/libclickscope
39 ${JSON_CPP_INCLUDE_DIRS}
40+ ${CLICK_INCLUDE_DIRS}
41 )
42
43 qt5_use_modules (${SCOPE_LIB_UNVERSIONED} Network)
44@@ -42,6 +44,7 @@
45 ${UBUNTUONE_LDFLAGS}
46 ${UBUNTU_DOWNLOAD_MANAGER_CLIENT_LDFLAGS}
47 ${UBUNTU_DOWNLOAD_MANAGER_COMMON_LDFLAGS}
48+ ${CLICK_LDFLAGS}
49 )
50
51 install(
52
53=== modified file 'scope/click/configuration.cpp'
54--- scope/click/configuration.cpp 2014-05-01 21:04:23 +0000
55+++ scope/click/configuration.cpp 2014-05-19 18:49:23 +0000
56@@ -27,6 +27,10 @@
57 * files in the program, then also delete it here.
58 */
59
60+#include <glib.h>
61+#include <click.h>
62+
63+
64 #include <string>
65 #include <vector>
66
67@@ -57,10 +61,20 @@
68
69 std::vector<std::string> Configuration::get_available_frameworks()
70 {
71+ GList *l = nullptr;
72+ GList *frameworks = l = click_framework_get_frameworks();
73+
74 std::vector<std::string> result;
75- for (auto f: list_folder(FRAMEWORKS_FOLDER, FRAMEWORKS_PATTERN)) {
76- result.push_back(f.substr(0, f.size()-FRAMEWORKS_EXTENSION_LENGTH));
77+ while (l != nullptr) {
78+ gchar *framework_name = nullptr;
79+ GObject *framework = (GObject *)l->data;
80+ g_object_get(framework, "name", &framework_name, nullptr);
81+ result.push_back(framework_name);
82+ l = g_list_next(l);
83+ g_free(framework_name);
84 }
85+ g_list_free_full(frameworks, g_object_unref);
86+
87 return result;
88 }
89
90
91=== modified file 'scope/click/configuration.h'
92--- scope/click/configuration.h 2014-05-01 21:04:23 +0000
93+++ scope/click/configuration.h 2014-05-19 18:49:23 +0000
94@@ -39,9 +39,6 @@
95 class Configuration
96 {
97 public:
98- constexpr static const char* FRAMEWORKS_FOLDER {"/usr/share/click/frameworks/"};
99- constexpr static const char* FRAMEWORKS_PATTERN {"*.framework"};
100- constexpr static const int FRAMEWORKS_EXTENSION_LENGTH = 10; // strlen(".framework")
101 constexpr static const char* LANGUAGE_ENVVAR {"LANGUAGE"};
102
103 virtual std::vector<std::string> get_available_frameworks();
104
105=== modified file 'scope/click/interface.cpp'
106--- scope/click/interface.cpp 2014-05-13 19:32:29 +0000
107+++ scope/click/interface.cpp 2014-05-19 18:49:23 +0000
108@@ -27,6 +27,8 @@
109 * files in the program, then also delete it here.
110 */
111
112+#include <click-0.4/click.h>
113+
114 #include <QDebug>
115 #include <QDir>
116 #include <QProcess>
117@@ -323,39 +325,76 @@
118
119 void Interface::get_manifests(std::function<void(ManifestList, ManifestError)> callback)
120 {
121- std::string command = "click list --manifest";
122- qDebug() << "Running command:" << command.c_str();
123- run_process(command, [callback](int code, const std::string& stdout_data, const std::string&) {
124- if (code == 0) {
125- try {
126- ManifestList manifests = manifest_list_from_json(stdout_data);
127- callback(manifests, ManifestError::NoError);
128- } catch (...) {
129- callback(ManifestList(), ManifestError::ParseError);
130- }
131- } else {
132- callback(ManifestList(), ManifestError::CallError);
133- }
134- });
135+ ClickDB *db = click_db_new();
136+ GError *error = NULL;
137+ char *c_output = NULL;
138+ std::string manifest_data;
139+
140+ c_output = click_db_get_manifests_as_string(db, false, &error);
141+ g_object_unref(db);
142+ if (error != NULL)
143+ {
144+ g_error_free(error);
145+ // FIXME: pass the GError details on
146+ callback(ManifestList(), ManifestError::CallError);
147+ }
148+ manifest_data = c_output;
149+ g_free(c_output);
150+
151+ try {
152+ ManifestList manifests = manifest_list_from_json(manifest_data);
153+ callback(manifests, ManifestError::NoError);
154+ } catch (...) {
155+ callback(ManifestList(), ManifestError::ParseError);
156+ }
157 }
158
159 void Interface::get_manifest_for_app(const std::string &app_id,
160 std::function<void(Manifest, ManifestError)> callback)
161 {
162- std::string command = "click info " + app_id;
163- qDebug() << "Running command:" << command.c_str();
164- run_process(command, [callback](int code, const std::string& stdout_data, const std::string&) {
165- if (code == 0) {
166- try {
167- Manifest manifest = manifest_from_json(stdout_data);
168- callback(manifest, ManifestError::NoError);
169- } catch (...) {
170- callback(Manifest(), ManifestError::ParseError);
171- }
172- } else {
173- callback(Manifest(), ManifestError::CallError);
174- }
175- });
176+ GError *error = NULL;
177+ ClickDB *db = NULL;
178+ ClickUser *user = NULL;
179+ char *c_output = NULL;
180+
181+ db = click_db_new();
182+ click_db_read(db, NULL, &error);
183+ if (error != NULL) {
184+ g_error_free (error);
185+ g_object_unref(db);
186+ // FIXME: pass error data along
187+ callback(Manifest(), ManifestError::CallError);
188+ }
189+
190+ user = click_user_new_for_user(db, NULL, &error);
191+ if (error != NULL) {
192+ g_error_free (error);
193+ g_object_unref(db);
194+ // FIXME: pass error data along
195+ callback(Manifest(), ManifestError::CallError);
196+ }
197+ if (click_user_has_package_name(user, app_id.c_str()) == false) {
198+ g_error_free (error);
199+ g_object_unref(db);
200+ g_object_unref(user);
201+ // FIXME: hm, not a call error as such really more a NotInstalledError ?
202+ callback(Manifest(), ManifestError::CallError);
203+ }
204+
205+ c_output = click_user_get_manifest_as_string (user, app_id.c_str(), &error);
206+ std::string json_output = c_output;
207+
208+ g_free(c_output);
209+ g_object_unref(db);
210+ g_object_unref(user);
211+
212+ try {
213+ Manifest manifest = manifest_from_json(json_output);
214+ callback(manifest, ManifestError::NoError);
215+ } catch (...) {
216+ callback(Manifest(), ManifestError::ParseError);
217+ }
218+
219 }
220
221 void Interface::get_dotdesktop_filename(const std::string &app_id,
222@@ -380,33 +419,4 @@
223 });
224 }
225
226-void Interface::run_process(const std::string& command,
227- std::function<void(int code,
228- const std::string& stdout_data,
229- const std::string& stderr_data)> callback)
230-{
231- QSharedPointer<QProcess> process(new QProcess());
232- typedef void(QProcess::*QProcessFinished)(int, QProcess::ExitStatus);
233- typedef void(QProcess::*QProcessError)(QProcess::ProcessError);
234- QObject::connect(process.data(),
235- static_cast<QProcessFinished>(&QProcess::finished),
236- [callback, process](int code, QProcess::ExitStatus /*status*/) {
237- qDebug() << "command finished with exit code:" << code;
238- auto data = process.data()->readAllStandardOutput().data();
239- auto errors = process.data()->readAllStandardError().data();
240- callback(code, data, errors);
241- } );
242-
243- QObject::connect(process.data(),
244- static_cast<QProcessError>(&QProcess::error),
245- [callback, process](QProcess::ProcessError error) {
246- qCritical() << "error running command:" << error;
247- auto data = process.data()->readAllStandardOutput().data();
248- auto errors = process.data()->readAllStandardError().data();
249- callback(process.data()->exitCode(), data, errors);
250- } );
251-
252- process->start(command.c_str());
253-}
254-
255 } // namespace click
256
257=== modified file 'scope/click/interface.h'
258--- scope/click/interface.h 2014-05-06 19:17:30 +0000
259+++ scope/click/interface.h 2014-05-19 18:49:23 +0000
260@@ -95,10 +95,6 @@
261 virtual bool is_visible_app(const unity::util::IniParser& keyFile);
262 virtual bool show_desktop_apps();
263
264- virtual void run_process(const std::string& command,
265- std::function<void(int code,
266- const std::string& stdout_data,
267- const std::string& stderr_data)> callback);
268 private:
269 QSharedPointer<KeyFileLocator> keyFileLocator;
270 };
271
272=== modified file 'scope/tests/test_configuration.cpp'
273--- scope/tests/test_configuration.cpp 2014-05-01 21:04:23 +0000
274+++ scope/tests/test_configuration.cpp 2014-05-19 18:49:23 +0000
275@@ -46,50 +46,10 @@
276
277 }
278
279-
280-TEST(Configuration, getAvailableFrameworksUsesRightFolder)
281-{
282- using namespace ::testing;
283- FakeConfiguration locator;
284- EXPECT_CALL(locator, list_folder(Configuration::FRAMEWORKS_FOLDER, _))
285- .Times(1).WillOnce(Return(std::vector<std::string>()));
286- locator.get_available_frameworks();
287-}
288-
289-TEST(Configuration, getAvailableFrameworksUsesRightPattern)
290-{
291- using namespace ::testing;
292- FakeConfiguration locator;
293- EXPECT_CALL(locator, list_folder(_, Configuration::FRAMEWORKS_PATTERN))
294- .Times(1).WillOnce(Return(std::vector<std::string>()));
295- locator.get_available_frameworks();
296-}
297-
298-TEST(Configuration, getAvailableFrameworksTwoResults)
299-{
300- using namespace ::testing;
301-
302- FakeConfiguration locator;
303- std::vector<std::string> response = {"abc.framework", "def.framework"};
304- EXPECT_CALL(locator, list_folder(_, _))
305- .Times(1)
306- .WillOnce(Return(response));
307- auto frameworks = locator.get_available_frameworks();
308- std::vector<std::string> expected = {"abc", "def"};
309- EXPECT_EQ(expected, frameworks);
310-}
311-
312-TEST(Configuration, getAvailableFrameworksNoResults)
313-{
314- using namespace ::testing;
315-
316- FakeConfiguration locator;
317- std::vector<std::string> response = {};
318- EXPECT_CALL(locator, list_folder(_, _))
319- .Times(1)
320- .WillOnce(Return(response));
321- auto frameworks = locator.get_available_frameworks();
322- EXPECT_EQ(0, frameworks.size());
323+TEST(Configuration, getAvailableFrameworksSmokeTest)
324+{
325+ FakeConfiguration locator;
326+ std::vector<std::string> frameworks = locator.get_available_frameworks();
327 }
328
329 TEST(Configuration, getLanguageCorrect)
330
331=== modified file 'scope/tests/test_interface.cpp'
332--- scope/tests/test_interface.cpp 2014-05-06 19:17:30 +0000
333+++ scope/tests/test_interface.cpp 2014-05-19 18:49:23 +0000
334@@ -130,7 +130,6 @@
335 FakeClickInterface() {}
336
337 MOCK_METHOD0(show_desktop_apps, bool());
338- MOCK_METHOD2(run_process, void(const std::string&, std::function<void(int, const std::string&, const std::string&)>));
339 };
340
341 TEST(ClickInterface, testIsNonClickAppFalse)
342@@ -320,140 +319,4 @@
343 EXPECT_FALSE(iface.show_desktop_apps());
344 }
345
346-TEST(ClickInterface, testGetManifestForAppCorrectCommand)
347-{
348- FakeClickInterface iface;
349- std::string command = "click info " + FAKE_PACKAGENAME;
350- EXPECT_CALL(iface, run_process(command, _)).
351- Times(1);
352- iface.get_manifest_for_app(FAKE_PACKAGENAME, [](Manifest, ManifestError){});
353-}
354-
355-TEST_F(ClickInterfaceTest, testGetManifestForAppParseError)
356-{
357- FakeClickInterface iface;
358- EXPECT_CALL(iface, run_process(_, _)).
359- Times(1).
360- WillOnce(Invoke([&](const std::string&,
361- std::function<void(int, const std::string&,
362- const std::string&)> callback){
363- callback(0, "INVALID JSON", "");
364- }));
365- EXPECT_CALL(*this, manifest_callback(_, ManifestError::ParseError));
366- iface.get_manifest_for_app(FAKE_PACKAGENAME, [this](Manifest manifest,
367- ManifestError error){
368- manifest_callback(manifest, error);
369- });
370-}
371-
372-TEST_F(ClickInterfaceTest, testGetManifestForAppCommandFailed)
373-{
374- FakeClickInterface iface;
375- EXPECT_CALL(iface, run_process(_, _)).
376- Times(1).
377- WillOnce(Invoke([&](const std::string&,
378- std::function<void(int, const std::string&,
379- const std::string&)> callback){
380- callback(-1, "", "CRITICAL: FAIL");
381- }));
382- EXPECT_CALL(*this, manifest_callback(_, ManifestError::CallError));
383- iface.get_manifest_for_app(FAKE_PACKAGENAME, [this](Manifest manifest,
384- ManifestError error){
385- manifest_callback(manifest, error);
386- });
387-}
388-
389-TEST_F(ClickInterfaceTest, testGetManifestForAppIsRemovable)
390-{
391- FakeClickInterface iface;
392- EXPECT_CALL(iface, run_process(_, _)).
393- Times(1).
394- WillOnce(Invoke([&](const std::string&,
395- std::function<void(int, const std::string&,
396- const std::string&)> callback){
397- callback(0, FAKE_JSON_MANIFEST_REMOVABLE, "");
398- }));
399- iface.get_manifest_for_app(FAKE_PACKAGENAME, [](Manifest manifest,
400- ManifestError error){
401- ASSERT_TRUE(error == ManifestError::NoError);
402- ASSERT_TRUE(manifest.removable);
403- });
404-}
405-
406-TEST_F(ClickInterfaceTest, testGetManifestForAppIsNotRemovable)
407-{
408- FakeClickInterface iface;
409- EXPECT_CALL(iface, run_process(_, _)).
410- Times(1).
411- WillOnce(Invoke([&](const std::string&,
412- std::function<void(int, const std::string&,
413- const std::string&)> callback){
414- callback(0, FAKE_JSON_MANIFEST_NONREMOVABLE, "");
415- }));
416- iface.get_manifest_for_app(FAKE_PACKAGENAME, [](Manifest manifest,
417- ManifestError error){
418- ASSERT_TRUE(error == ManifestError::NoError);
419- ASSERT_FALSE(manifest.removable);
420- });
421-}
422-
423-TEST(ClickInterface, testGetManifestsCorrectCommand)
424-{
425- FakeClickInterface iface;
426- std::string command = "click list --manifest";
427- EXPECT_CALL(iface, run_process(command, _)).
428- Times(1);
429- iface.get_manifests([](ManifestList, ManifestError){});
430-}
431-
432-TEST_F(ClickInterfaceTest, testGetManifestsParseError)
433-{
434- FakeClickInterface iface;
435- EXPECT_CALL(iface, run_process(_, _)).
436- Times(1).
437- WillOnce(Invoke([&](const std::string&,
438- std::function<void(int, const std::string&,
439- const std::string&)> callback){
440- callback(0, "INVALID JSON", "");
441- }));
442- EXPECT_CALL(*this, manifests_callback(_, ManifestError::ParseError));
443- iface.get_manifests([this](ManifestList manifests, ManifestError error){
444- manifests_callback(manifests, error);
445- });
446-}
447-
448-TEST_F(ClickInterfaceTest, testGetManifestsCommandFailed)
449-{
450- FakeClickInterface iface;
451- EXPECT_CALL(iface, run_process(_, _)).
452- Times(1).
453- WillOnce(Invoke([&](const std::string&,
454- std::function<void(int, const std::string&,
455- const std::string&)> callback){
456- callback(-1, "", "CRITICAL: FAIL");
457- }));
458- EXPECT_CALL(*this, manifests_callback(_, ManifestError::CallError));
459- iface.get_manifests([this](ManifestList manifests, ManifestError error){
460- manifests_callback(manifests, error);
461- });
462-}
463-
464-TEST_F(ClickInterfaceTest, testGetManifestsParsed)
465-{
466- FakeClickInterface iface;
467- std::string expected_str = "[" + FAKE_JSON_MANIFEST_NONREMOVABLE + "," +
468- FAKE_JSON_MANIFEST_REMOVABLE + "]";
469- ManifestList expected = manifest_list_from_json(expected_str);
470-
471- EXPECT_CALL(iface, run_process(_, _)).
472- Times(1).
473- WillOnce(Invoke([&](const std::string&,
474- std::function<void(int, const std::string&,
475- const std::string&)> callback){
476- callback(0, expected_str, "");
477- }));
478- iface.get_manifests([expected](ManifestList manifests, ManifestError error){
479- ASSERT_TRUE(error == ManifestError::NoError);
480- ASSERT_TRUE(manifests.size() == expected.size());
481- });
482-}
483+// FIXME: re-add tests by mocking libclick

Subscribers

People subscribed via source and target branches

to all changes: