messaging-app

Code review comment for lp:~canonical-platform-qa/messaging-app/test_helper_to_send_sms_from_suggested_contact_list

test_helper_to_send_sms_from_suggested_contact_list
Merge into trunk

Revision history for this message

Omer Akram (om26er) wrote on 2014-07-26:

> Omer, it would be nice if you try to improve all the code you are touching,
> even if you didn't write it originally. That's the only way we can get the
> tests to a better shape.
>
> 34 + to select it by tapping from the suggeted list.
>
> You have a typo there ^. It's suggeSted.

Fixed.

>
>
> > > 19 text_entry = self.get_newmessage_textarea()
> > > 20 - self.pointing_device.click_object(text_entry)
> > > 21 - text_entry.focus.wait_for(True)
> > > 22 - time.sleep(.3)
> > > 23 + self._focus_field(text_entry)
> > > 24 self.keyboard.type(str(message), delay=0.2)
> > >
> > Hi! I didn't know TextField emulator existed, thanks for that. The component
> I
> > am trying to interact with, is not a TextField, its a custom component used
> > only in messaging-app I believe.
>
> It's a text area, not a text field.
> We have an open bug to make a helper for text area. https://bugs.launchpad.net
> /ubuntu-ui-toolkit/+bug/1327354
> It's really simple, as it should just inherit from TextField. Now that you
> need it, it will be the perfect moment for you to contribute to the toolkit :D
>

Ok, proposed a branch for uitoolkit, please review: https://code.launchpad.net/~canonical-platform-qa/ubuntu-ui-toolkit/add_helpers_for_TextArea/+merge/228385

> > I am not sure about the sleep, I did not add it, it was already there and is
> > in a few other places as well, I will not remove since there was probably a
> > reason for that.
>
> Try removing the sleep and if all the tests pass, that's good enough. If we
> need to add it in the future, then we will understand the reason for it and
> put a comment. If you get new errors because of the removed sleep, you will be
> able to find the reason.
> Any thing that's not clear is broken.

Done, no random sleep.

>
> > It was already there. Now that I poked through the app, the only way to lose
> > focus in that field is by focusing the 'message' box, there is no other
> button
> > in this page. We can go with that but I am not sure.
>
> It sounds like type_contact_phone_num should leave the focus on that
> textfield. The next helper will click the other text area, so I think it's not
> needed to press enter.

Yeah, didn't think of that. Removed the Enter hack.

> Omer, it would be nice if you try to improve all the code you are touching,
> even if you didn't write it originally. That's the only way we can get the
> tests to a better shape.
> 
> 34      + to select it by tapping from the suggeted list.
> 
> You have a typo there ^. It's suggeSted.

Fixed.

> 
> 
> > > 19      text_entry = self.get_newmessage_textarea()
> > > 20      - self.pointing_device.click_object(text_entry)
> > > 21      - text_entry.focus.wait_for(True)
> > > 22      - time.sleep(.3)
> > > 23      + self._focus_field(text_entry)
> > > 24      self.keyboard.type(str(message), delay=0.2)
> > >
> > Hi! I didn't know TextField emulator existed, thanks for that. The component
> I
> > am trying to interact with, is not a TextField, its a custom component used
> > only in messaging-app I believe.
> 
> It's a text area, not a text field.
> We have an open bug to make a helper for text area. https://bugs.launchpad.net
> /ubuntu-ui-toolkit/+bug/1327354
> It's really simple, as it should just inherit from TextField. Now that you
> need it, it will be the perfect moment for you to contribute to the toolkit :D
>

Ok, proposed a branch for uitoolkit, please review: https://code.launchpad.net/~canonical-platform-qa/ubuntu-ui-toolkit/add_helpers_for_TextArea/+merge/228385
 
> > I am not sure about the sleep, I did not add it, it was already there and is
> > in a few other places as well, I will not remove since there was probably a
> > reason for that.
> 
> Try removing the sleep and if all the tests pass, that's good enough. If we
> need to add it in the future, then we will understand the reason for it and
> put a comment. If you get new errors because of the removed sleep, you will be
> able to find the reason.
> Any thing that's not clear is broken.

Done, no random sleep.

> 
> > It was already there. Now that I poked through the app, the only way to lose
> > focus in that field is by focusing the 'message' box, there is no other
> button
> > in this page. We can go with that but I am not sure.
> 
> It sounds like type_contact_phone_num should leave the focus on that
> textfield. The next helper will click the other text area, so I think it's not
> needed to press enter.

Yeah, didn't think of that. Removed the Enter hack.

« Back to merge proposal