Code review comment for lp:~nik90/ubuntu-clock-app/fix-alarm-tests

Revision history for this message
Leo Arias (elopio) wrote :

25 + containerHeight: itemHeight * 7 // Temporary fix for autopilot

Please, file a bug where you explain the problem, and put a link to it next to the comment in the code.

Remember updating the copyright. You are missing it here:
tests/autopilot/ubuntu_clock_app/emulators.py

209 + tx = x + (w / 2)
210 + ty = y + (h / 2.5)
211 + self.pointing_device.drag(tx, ty - (h / 4), tx - (w / 2), ty + (h / 2))
217 + tx = x + (w / 2)
218 + ty = y + (h / 2.5)
219 + self.pointing_device.drag(tx, ty - (h / 4), tx + (w / 2), ty + (h / 2))

I've seen some code by barry where he changes the / to //. // returns the floor division, so it sounds safer to use when we need ints, like in this case. And with it, we will get the same behaviour on python 2 and 3.
http://www.python.org/dev/peps/pep-0238/

266 + days_object = day_option_selector.select_single("StyledItem",
267 + objectName="listContainer")
266 + days_object = day_option_selector.select_single("StyledItem",
267 + objectName="listContainer")
296 + day_array.append(self.wait_select_single('CheckBox',
297 + objectName='repeatDaysSwitch' + repr(i)))
314 + done_button = self.wait_select_single('Button',
315 + objectName='confirmRepeatDaysButton')
367 + alarm = self.wait_select_single('CheckBox',
368 + objectName='listAlarmStatus{}'.format(index))

This is not properly aligned, and pep8 will complain.
I prefer:
days_object = day_option_selector.select_single(
    "StyledItem", objectName="listContainer")

But it's totally valid to break after the first argument and some people prefer it. You just have to make sure all arguments start on the same column.

278 + day_index = i

You can add a break after you found the element, so you stop the loop and don't do more unnecessary checks.
I find the for loops prettier, but that's a matter of preference:

for index, day_element in enumerate(daylist):
    if(day_element.text == day):
        day_index = index
        break

292 + day_array = []

Why do you need this date array?

for index in range(7):
    day_element = self.wait_select_single(
        toolkit_emulators.CheckBox, objectName='repeatDaysSwitch{}'.format(index))
        if index % 2 == 0:
            day_element.check()
        else:
            day_element.uncheck()

That looks a little cleaner for me ^

779 + """ Test to add a single type alarm and check if it is properly
780 + added to the alarm list.
781 + """

When the comments are of more than one line, please split them into a one line summary, and then add extra information on following paragraphs. That's from http://www.python.org/dev/peps/pep-0257/
In this case, I'd just remove the comment because you have given the test a perfect name that explains it all.

I like this very much. You have hidden all the low level details behind the public methods add_alarm, delete_alarm and toggle_alarm, and you named them using the user language. I couldn't have asked for more.

Well, I always can ask for more because I'm a PITA. Please add the logging decorator to those three public methods, so the autopilot logs show a trace of what we were doing.

Thank you!

« Back to merge proposal