Merge lp:~carla-sella/ubuntu-calendar-app/calendar-app into lp:ubuntu-calendar-app

[Carla Sella]

Calendar app autopilot new event test.
Re-submitted merge as there were problems with the motion fields, I modified the code for these fields a part the time picker that is a bit more complex.
Launching pep8 on the test gives me this error:
tests/autopilot/calendar_app/tests/test_calendar.py:29:32: E712 comparison to True should be 'if cond is not True:' or 'if not cond:'
but if I change what I put in the code the test does not work anymore :P.

[Nicholas Skaggs (nskaggs)]

Going to give this a whirl on a device -- if anyone can try it on a galaxy nexus or nexus 4, that would be great. I'll try on the nexus 10.

[Olivier Tilloy (osomon)]

101 +## @unittest.skip("Adding a new event is broken, needs fixing. "
102 +## "See http://pad.lv/1206048.")

Instead of commented out, the skip decorator should be removed altogether, as bug #1206048 is now fixed.

[Olivier Tilloy (osomon)]

30 + objectName: "OKButton"

That’s a very generic name, probably too generic. If in the future the app gets more popups, there will probably be several "OK buttons".

[Olivier Tilloy (osomon)]

84 +from time import sleep

This import is unused, can you please remove it?

[Olivier Tilloy (osomon)]

79 +import math

Same for this import, it seems it is unused.

[Olivier Tilloy (osomon)]

148 + self.pointing_device.drag(x_Hscroller+(width_Hscroller/4),
149 + (y_Hscroller+((height_Hscroller/4)*3)),
150 + x_Hscroller+(width_Hscroller/4),
151 + (y_Hscroller+((height_Hscroller/4)*2)))

You should ensure you always pass integers as parameters to .drag(…), so you need to surround each parameter with int(…).

review: Needs Fixing

[Olivier Tilloy (osomon)]

182 + #---> TODO input end time

So this MR is not complete yet, is it? If not complete, please set its status to "Work in progress", and set it back to "Needs review" once it is actually ready. Thanks!

[Carla Sella (carla-sella)]

The test is complete for now even if it can be improved, but for the moment we want to run it as it is.

[Carla Sella (carla-sella)]

review: Needs Resubmitting

[Nicholas Skaggs (nskaggs)]

A minor nitpick; I would add eventually to these lines:

self.assertThat(title_label, NotEquals(None))

and

self.assertThat(save_button, NotEquals(None))

[Carla Sella (carla-sella)]

If I add eventually to the two lines you suggested, I get this error: TypeError: Eventually is only usable with attributes that have a wait_for function or callable objects.

[Olivier Tilloy (osomon)]

172 + save_button = self.main_window.get_event_save_button()
173 + self.assertThat(lambda: save_button, Eventually(NotEquals(None)))

179 + title_label = self.main_window.get_title_label(eventTitle)
180 + self.assertThat(lambda: title_label, Eventually(NotEquals(None)))

This is not doing what you think it does: because 'save_button' and 'title_label' are assigned before defining the lambda function, they contain static values, so the lambda is useless. For it to be useful, you need to use it like that:

    lambda: self.main_window.get_event_save_button()
    lambda: self.main_window.get_title_label(eventTitle)

And since you’re comparing to None, instead of using Equals (or NotEquals), you should use the identity matcher, Is, like that:

    self.assertThat(…, Eventually(Not(Is(None))))

review: Needs Fixing

[Carla Sella (carla-sella)]

Thanks Olivier, I fixed the two asserts, hope they are ok now.

review: Needs Resubmitting

[Olivier Tilloy (osomon)]

42 - def get_month_view(self):
43 - return self.app.select_single("MonthView")

Removing this breaks all the other existing tests. Please revert this change.

review: Needs Fixing

[Olivier Tilloy (osomon)]

92 - @unittest.skip("Adding a new event is broken, needs fixing. "
93 - "See http://pad.lv/1206048.")

Now that you removed this decorator, the "import unittest" line should be removed from tests/autopilot/calendar_app/tests/test_calendar.py.

review: Needs Fixing

[Olivier Tilloy (osomon)]

pyflakes reports 4 warnings, all of them easily fixable by just removing unused code:

tests/autopilot/calendar_app/tests/test_calendar.py:17: 'unittest' imported but unused
tests/autopilot/calendar_app/tests/test_calendar.py:47: local variable 'end_time_field' is assigned to but never used
tests/autopilot/calendar_app/tests/test_calendar.py:82: local variable 'minutesBeforeChange' is assigned to but never used
tests/autopilot/calendar_app/tests/test_calendar.py:120: local variable 'title_label' is assigned to but never used

[Carla Sella (carla-sella)]

Hope everything's fine now :p.
Thanks Olilvier.

review: Needs Resubmitting

[Olivier Tilloy (osomon)]

> 42 - def get_month_view(self):
> 43 - return self.app.select_single("MonthView")
>
> Removing this breaks all the other existing tests. Please revert this change.

This one still needs to be fixed, all the other autopilot tests are currently failing because of this removal.

[Carla Sella (carla-sella)]

review: Needs Resubmitting

[Olivier Tilloy (osomon)]

Looks good, thanks Carla!

review: Approve

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

FAILED: Autolanding.
More details in the following jenkins job:
http://91.189.93.70:8080/job/ubuntu-calendar-app-autolanding/23/
Executed test runs:
    FAILURE: http://91.189.93.70:8080/job/generic-mediumtests/348/console
    FAILURE: http://91.189.93.70:8080/job/ubuntu-calendar-app-precise-amd64-autolanding/18/console
    FAILURE: http://91.189.93.70:8080/job/ubuntu-calendar-app-quantal-amd64-autolanding/22/console
    FAILURE: http://91.189.93.70:8080/job/ubuntu-calendar-app-raring-amd64-autolanding/22/console
    FAILURE: http://91.189.93.70:8080/job/ubuntu-calendar-app-saucy-amd64-autolanding/18/console

review: Needs Fixing (continuous-integration)

[Olivier Tilloy (osomon)]

Apparently the builder is now considering pep8 warnings as errors…
I’ll fix all the warnings in trunk in a separate branch and we’ll get it merged first, so that you have to address only those that are related to your changes in this branch.

[Olivier Tilloy (osomon)]

> Apparently the builder is now considering pep8 warnings as errors…
> I’ll fix all the warnings in trunk in a separate branch and we’ll get it
> merged first, so that you have to address only those that are related to your
> changes in this branch.

In fact it appears all the pep8 warnings were introduced by your changes, the trunk is "clean".
To get a list of all the warnings, simply install 'pep8' on your machine, and from within your branch, run `pep8 tests`.
This will print out a list of warnings, usually self-explanatory on how to fix them.

[Carla Sella (carla-sella)]

review: Needs Resubmitting

[Olivier Tilloy (osomon)]

I’m still getting 20 pep8 warnings:

$ pep8 tests
tests/autopilot/calendar_app/tests/test_calendar.py:78:50: E225 missing whitespace around operator
tests/autopilot/calendar_app/tests/test_calendar.py:78:67: E225 missing whitespace around operator
tests/autopilot/calendar_app/tests/test_calendar.py:79:51: E225 missing whitespace around operator
tests/autopilot/calendar_app/tests/test_calendar.py:79:70: E225 missing whitespace around operator
tests/autopilot/calendar_app/tests/test_calendar.py:79:73: E225 missing whitespace around operator
tests/autopilot/calendar_app/tests/test_calendar.py:80:50: E225 missing whitespace around operator
tests/autopilot/calendar_app/tests/test_calendar.py:80:67: E225 missing whitespace around operator
tests/autopilot/calendar_app/tests/test_calendar.py:81:51: E225 missing whitespace around operator
tests/autopilot/calendar_app/tests/test_calendar.py:81:70: E225 missing whitespace around operator
tests/autopilot/calendar_app/tests/test_calendar.py:81:73: E225 missing whitespace around operator
tests/autopilot/calendar_app/tests/test_calendar.py:94:50: E225 missing whitespace around operator
tests/autopilot/calendar_app/tests/test_calendar.py:94:67: E225 missing whitespace around operator
tests/autopilot/calendar_app/tests/test_calendar.py:95:51: E225 missing whitespace around operator
tests/autopilot/calendar_app/tests/test_calendar.py:95:70: E225 missing whitespace around operator
tests/autopilot/calendar_app/tests/test_calendar.py:95:73: E225 missing whitespace around operator
tests/autopilot/calendar_app/tests/test_calendar.py:96:50: E225 missing whitespace around operator
tests/autopilot/calendar_app/tests/test_calendar.py:96:67: E225 missing whitespace around operator
tests/autopilot/calendar_app/tests/test_calendar.py:97:51: E225 missing whitespace around operator
tests/autopilot/calendar_app/tests/test_calendar.py:97:70: E225 missing whitespace around operator
tests/autopilot/calendar_app/tests/test_calendar.py:97:73: E225 missing whitespace around operator

If you add whitespaces around the "+" operator, this should fix it.

[Carla Sella (carla-sella)]

review: Needs Resubmitting

[Olivier Tilloy (osomon)]

Yes, looks like all the warnings are fixed now! Thanks for your patience Carla :)

review: Approve

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

review: Approve (continuous-integration)

[Carla Sella (carla-sella)]

Re-submitted for approval as there were problems with motion fields that I tried to fix (a part the time picker).

review: Needs Resubmitting

[Olivier Tilloy (osomon)]

Please do not re-use a merge request that was already merged. It’s ok to re-use the same branch (although most of the time it makes sense to start from a new branch), but then submit a new MR.

This is not the right approach. If you need to ensure that a text field has active focus before inputting text in it, do something like that:

    self.pointing_device.click_object(field)
    field.activeFocus.wait_for(True)
    self.keyboard.type(text)
    self.assertThat(field.text, Eventually(Equals(text)))

Also note that I’m addressing similar issues in this test case (it’s failing when run on devices) there: https://code.launchpad.net/~osomon/ubuntu-calendar-app/fix-ap-tests-new-event/+merge/180847. I’d appreciate if you could review it. Thanks!

review: Disapprove

[Olivier Tilloy (osomon)]

By the way, regarding the pep8 warning that you’re seeing, it would go away if you replaced "inputField.focus != True" by "!inputField.focus".

And I suggest reading http://unity.ubuntu.com/autopilot/tutorial/good_tests.html?highlight=wait_for#prefer-wait-for-and-eventually-to-sleep for tips on how to write efficient autopilot tests.

[Carla Sella (carla-sella)]

Hello Olivier, sorry for the inconvenience, I will do what you suggest.
Thanks.

[Olivier Tilloy (osomon)]

No worries! Autopilot tests are tricky to get right because of timing issues, especially when they have to be run across different devices and form factors.

Note that my MR was merged yesterday, and today the tests are green in the dashboard (see http://reports.qa.ubuntu.com/smokeng/saucy/image/3672/calendar-app-autopilot/), so it looks like we’re in good shape to start writing more tests, if you’re interested.

[Carla Sella (carla-sella)]

Ok, sure, thanks.

Unmerged revisions

92. By Carla Sella on 2013-08-19

Fixed code on animation fields, except the time picker.

91. By Carla Sella on 2013-08-19

Work in progress for fixing new event test.

90. By Carla Sella on 2013-08-16

Added a couple of minor fixes.

89. By Carla Sella on 2013-08-16

Added asserts for error on location field in new event test.

=== modified file 'tests/autopilot/calendar_app/tests/test_calendar.py'
--- tests/autopilot/calendar_app/tests/test_calendar.py	2013-08-15 12:36:28 +0000
+++ tests/autopilot/calendar_app/tests/test_calendar.py	2013-08-19 19:10:08 +0000
@@ -15,11 +15,25 @@
 
 import time
 
+from time import sleep
 from calendar_app.tests import CalendarTestCase
 
 
 class TestMainWindow(CalendarTestCase):
 
+    def _input_string_in_field(self, inputField, inputString):
+        #poll clicking for focus for lack of better method,
+        #due to screen switching animation
+        #is there a property we can use instead?
+        timeout = 0
+        while inputField.focus != True and timeout < 10:
+            self.pointing_device.click_object(inputField)
+            sleep(1)
+            timeout += 1
+        self.keyboard.type(inputString)
+        self.assertThat(inputField.text, Eventually(Equals(inputString)))
+        self.keyboard.press_and_release('Enter')
+
     def test_timeline_view_shows(self):
         """test timeline view"""
 
@@ -50,12 +64,12 @@
 
         #input a new event name
         self.assertThat(
-            self.main_window.get_new_event().visible, Eventually(Equals(True)))
-        eventTitle = "Test event" + str(time.time())
-
-        self.pointing_device.click_object(event_name_field)
-        self.keyboard.type(eventTitle)
-        self.assertThat(event_name_field.text, Eventually(Equals(eventTitle)))
+            lambda: self.main_window.get_new_event().visible,
+            Eventually(Equals(True)))
+        inputString = "Test event" + str(time.time())
+        eventTitle = inputString
+        self._input_string_in_field(event_name_field, inputString)
+        self.assertThat(event_name_field.text, Eventually(Equals(inputString)))
 
         #input start time
         self.assertThat(
@@ -110,14 +124,16 @@
             Eventually(Equals(True)))
 
         #input location
-        self.pointing_device.click_object(location_field)
-        self.keyboard.type("My location")
-        self.assertThat(location_field.text, Eventually(Equals("My location")))
+        self.assertThat(
+            lambda: location_field.visible, Eventually(Equals(True)))
+        inputString = "My location"
+        self._input_string_in_field(location_field, inputString)
 
         #input people
-        self.pointing_device.click_object(people_field)
-        self.keyboard.type("Me")
-        self.assertThat(people_field.text, Eventually(Equals("Me")))
+        self.assertThat(
+            lambda: people_field.visible, Eventually(Equals(True)))
+        inputString = "Me"
+        self._input_string_in_field(people_field, inputString)
 
        #click save button
         save_button = self.main_window.get_event_save_button()