Code review comment for lp:~allanlesage/address-book-app/abook_navigation_favorites

55 +# Borrowed the following helper class from
56 +# lp:address-book-service/examples/contacts.py

Wouldn't it be better if this is packaged and made available as address-book-services test helpers? That way the code is on the same project that provides the service and we don't have to duplicate it. Is that possible?
This is not a blocker for this branch, just part of my ranting about developers not writing anything for testability, and we covering for their faults.

=== added file 'tests/autopilot/address_book_app/emulators/address_book.py'

I'd say this file needs unit test. With the query method we have all we need to check that the other methods are working correctly.

258 + # TODO select_many and raise if > 1?

wait_select_single will raise an exception if there's more than one item matching, so I think that's already covered. Unless I didn't get the comment.

299 + try:
300 + buttons = self.select_many("Button",
301 + objectName=objectname)
302 + for button in buttons:
303 + if button.visible:
304 + self.pointing_device.click_object(button)
305 + except StateNotFoundError:
306 + LOGGER.error(
307 + 'Button with objectName "{0}" not found.'.format(objectname)
308 + )
309 + raise

Please note that you are not breaking out of the for loop, so if there are many visible buttons, all of them will be clicked. That doesn't sound like the goal of this method.

I find this weird, as the purpose of the object name is to identify things uniquely. So if there are many buttons with the same name, we need to change the name.
This code could be simplified like this:
self.select_single('Button', objectName=object_name, visible=True)

That will raise an exception if there's no button with that name, if the button with that name is not visible, and if there are many buttons matching the query.

332 + def star_is_illuminated_for_phone_number(self, phone_number):

I think we should keep a higher language, closer to the design user stories. From a user perspective, a illuminated starts means that the phone number is favorite, so I would do this:

def is_phone_favorite(self, phone_number):
    self._is_star_illuminated_for_phone_number(phone_number)

That way we keep as private the implementation detail, and on the tests we call the public higher level method that is more likely to remain stable. If in the future design decides to change the widget of a favorite number from a start to a drop down list, we don't have to change any tests, just the method we call inside is_phone_favorite.

576 + contact_list_page.click_contact_by_name('Fulano de Tal')
577 + contact_view = self.main_window.get_contact_view_page()

Here again, clicking a contact is actually an implementation details and it would be better as a private method in the emulator.
I prefer when the test reads like this:

contact_view = contact_list_page.open_contact_details(name='Fulano de Tal')

There's a nice selenium pattern called the page object: https://code.google.com/p/selenium/wiki/PageObjects
You might find it interesting to read. Not everything in there applies to the desktop, but some things do and I always try to follow them.

I find this as the most concise guide to avoid future pains with user acceptance testing:
    The public methods represent the services that the page offers
    Try not to expose the internals of the page
    Generally don't make assertions
    Methods return other PageObjects
    Need not represent an entire page
    Different results for the same action are modelled as different methods

Please add the log_action decorators to all the public methods of the emulators that represent a user action, like cancel and delete. This will leave a trace of what the "user" is doing, that will look like a specification for manual testing. Really useful when trying to reproduce a failure manually. Here's a nice example of how we are using it:
http://bazaar.launchpad.net/~ubuntu-sdk-team/ubuntu-ui-toolkit/trunk/view/head:/tests/autopilot/ubuntuuitoolkit/emulators.py

And one small pita detail, please be consistent on how you use 'strings' or "strings". I prefer the single quote, but as long as it's consistent, anyone is ok.