[Merge] lp:~vrruiz/gallery-app/test-share-photo into lp:gallery-app

Leo Arias leo.arias at canonical.com
Mon Jul 14 23:18:48 UTC 2014


Review: Needs Fixing code review

202	+ def post_button(self):
214	+ def do_share_photo(self):
206	+ def share_panel(self):

These shouldn't be part of the test case. They should be custom proxy objects or used in methods of the parent custom proxy object.

43	+class AccountManager(object):
159	+class FakeFriendsService(object):

Please talk with Richard and Brendan. They are working with the devs to move a fake dbus service like this one to the right project. In their case URLDispatcher, in your case the friends service. We agreed that it's ok to duplicate the code in the project like you are doing here just if we are working at the same time to put it in the right place. Maybe one of them can help you talking with your devs to see how easy would it be to get this done in the right way.

254	+ sleep(5)

You should use Eventually instead of a sleep.

249	+ def test_facebook(self):

You need a better name for this test. Something like test_share_with_facebook_must_call_friends_service.

Finally, all your FIXMEs look really weird. I don't understand how you are creating the account without starting the main loop, as it's asynchronous. It seems though that you don't need to store any credentials, just create the account. That's really nice, you can probably remove a lot of the code in Account Manager. If you can get mardy to help you with this it would be great. If not, maybe I can give you a hand later on the week.

Thanks Victor. This test is great.
-- 
https://code.launchpad.net/~vrruiz/gallery-app/test-share-photo/+merge/225947
Your team Ubuntu Phablet Team is requested to review the proposed merge of lp:~vrruiz/gallery-app/test-share-photo into lp:gallery-app.



More information about the Ubuntu-reviews mailing list