[Merge] lp:~iahmad/messaging-app/more_test_helpers into lp:messaging-app

Leo Arias leo.arias at canonical.com
Mon Jul 21 19:08:04 UTC 2014


Review: Needs Fixing code review

The two methods that you added are not used, so we will have no way to know if they must be updated by a design change. Every helper that you add must come with a test.

8	+ def get_recipient_from_recipients(self, contact_name):

This helper is at the wrong level of abstraction. We shouldn't return autopilot introspection objects. Instead, you should ask what's the action that the user would do with that item, and encapsulate that action in a method. In this case I'm not sure what you would like to achieve with the method. If what you would like to check is that a recipient is visible on the list, I have found that it's a lot more useful to make a helper called something like get_recipients and make it return a list of strings. Then you assertIn('test', get_recipients())

30	+ def select_contact_number_from_list(self, contact_view,
31	+ contact_name,
32	+ contact_number):

Here it seems that you are putting the helper on the MainView and passing the parent as a parameter. That's wrong. You should instead make it a method in the paretn object, and call it something like contact_view.select_contact_number()

It's hard to fully understand the purpose of your helpers. That's a by product of the tests, it's clearer why are you adding them and makes it easier to review.
-- 
https://code.launchpad.net/~iahmad/messaging-app/more_test_helpers/+merge/227533
Your team Ubuntu Phablet Team is subscribed to branch lp:messaging-app.



More information about the Ubuntu-reviews mailing list