Merge lp:~carla-sella/ubuntu-calendar-app/calendar-app into lp:ubuntu-calendar-app
- calendar-app
- Merge into trunk
Status: | Rejected |
---|---|
Rejected by: | Olivier Tilloy |
Proposed branch: | lp:~carla-sella/ubuntu-calendar-app/calendar-app |
Merge into: | lp:ubuntu-calendar-app |
Diff against target: |
71 lines (+28/-12) 1 file modified
tests/autopilot/calendar_app/tests/test_calendar.py (+28/-12) |
To merge this branch: | bzr merge lp:~carla-sella/ubuntu-calendar-app/calendar-app |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Olivier Tilloy (community) | Disapprove | ||
Carla Sella (community) | Needs Resubmitting | ||
Ubuntu Calendar Developers | Pending | ||
Ubuntu Phone Apps Jenkins Bot | continuous-integration | Pending | |
Review via email:
|
This proposal supersedes a proposal from 2013-08-12.
Commit message
Description of the change
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
but if I change what I put in the code the test does not work anymore :P.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Nicholas Skaggs (nskaggs) wrote : Posted in a previous version of this proposal | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Olivier Tilloy (osomon) wrote : Posted in a previous version of this proposal | # |
101 +## @unittest.
102 +## "See http://
Instead of commented out, the skip decorator should be removed altogether, as bug #1206048 is now fixed.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Olivier Tilloy (osomon) wrote : Posted in a previous version of this proposal | # |
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".
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Olivier Tilloy (osomon) wrote : Posted in a previous version of this proposal | # |
84 +from time import sleep
This import is unused, can you please remove it?
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Olivier Tilloy (osomon) wrote : Posted in a previous version of this proposal | # |
79 +import math
Same for this import, it seems it is unused.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Olivier Tilloy (osomon) wrote : Posted in a previous version of this proposal | # |
148 + self.pointing_
149 + (y_Hscroller+
150 + x_Hscroller+
151 + (y_Hscroller+
You should ensure you always pass integers as parameters to .drag(…), so you need to surround each parameter with int(…).
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Olivier Tilloy (osomon) wrote : Posted in a previous version of this proposal | # |
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!
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Carla Sella (carla-sella) wrote : Posted in a previous version of this proposal | # |
The test is complete for now even if it can be improved, but for the moment we want to run it as it is.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Carla Sella (carla-sella) : Posted in a previous version of this proposal | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Nicholas Skaggs (nskaggs) wrote : Posted in a previous version of this proposal | # |
A minor nitpick; I would add eventually to these lines:
self.assertThat
and
self.assertThat
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Carla Sella (carla-sella) wrote : Posted in a previous version of this proposal | # |
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.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Olivier Tilloy (osomon) wrote : Posted in a previous version of this proposal | # |
172 + save_button = self.main_
173 + self.assertThat
179 + title_label = self.main_
180 + self.assertThat
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_
lambda: self.main_
And since you’re comparing to None, instead of using Equals (or NotEquals), you should use the identity matcher, Is, like that:
self.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Carla Sella (carla-sella) wrote : Posted in a previous version of this proposal | # |
Thanks Olivier, I fixed the two asserts, hope they are ok now.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Olivier Tilloy (osomon) wrote : Posted in a previous version of this proposal | # |
42 - def get_month_
43 - return self.app.
Removing this breaks all the other existing tests. Please revert this change.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Olivier Tilloy (osomon) wrote : Posted in a previous version of this proposal | # |
92 - @unittest.
93 - "See http://
Now that you removed this decorator, the "import unittest" line should be removed from tests/autopilot
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Olivier Tilloy (osomon) wrote : Posted in a previous version of this proposal | # |
pyflakes reports 4 warnings, all of them easily fixable by just removing unused code:
tests/autopilot
tests/autopilot
tests/autopilot
tests/autopilot
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Carla Sella (carla-sella) wrote : Posted in a previous version of this proposal | # |
Hope everything's fine now :p.
Thanks Olilvier.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Olivier Tilloy (osomon) wrote : Posted in a previous version of this proposal | # |
> 42 - def get_month_
> 43 - return self.app.
>
> 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.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Carla Sella (carla-sella) : Posted in a previous version of this proposal | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Olivier Tilloy (osomon) wrote : Posted in a previous version of this proposal | # |
Looks good, thanks Carla!
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote : Posted in a previous version of this proposal | # |
FAILED: Autolanding.
More details in the following jenkins job:
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Olivier Tilloy (osomon) wrote : Posted in a previous version of this proposal | # |
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.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Olivier Tilloy (osomon) wrote : Posted in a previous version of this proposal | # |
> 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.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Carla Sella (carla-sella) : Posted in a previous version of this proposal | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Olivier Tilloy (osomon) wrote : Posted in a previous version of this proposal | # |
I’m still getting 20 pep8 warnings:
$ pep8 tests
tests/autopilot
tests/autopilot
tests/autopilot
tests/autopilot
tests/autopilot
tests/autopilot
tests/autopilot
tests/autopilot
tests/autopilot
tests/autopilot
tests/autopilot
tests/autopilot
tests/autopilot
tests/autopilot
tests/autopilot
tests/autopilot
tests/autopilot
tests/autopilot
tests/autopilot
tests/autopilot
If you add whitespaces around the "+" operator, this should fix it.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Carla Sella (carla-sella) : Posted in a previous version of this proposal | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Olivier Tilloy (osomon) wrote : Posted in a previous version of this proposal | # |
Yes, looks like all the warnings are fixed now! Thanks for your patience Carla :)
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) : Posted in a previous version of this proposal | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Carla Sella (carla-sella) wrote : | # |
Re-submitted for approval as there were problems with motion fields that I tried to fix (a part the time picker).
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Olivier Tilloy (osomon) wrote : | # |
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.
field.
self.
self.
Also note that I’m addressing similar issues in this test case (it’s failing when run on devices) there: https:/
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Olivier Tilloy (osomon) wrote : | # |
By the way, regarding the pep8 warning that you’re seeing, it would go away if you replaced "inputField.focus != True" by "!inputField.
And I suggest reading http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Carla Sella (carla-sella) wrote : | # |
Hello Olivier, sorry for the inconvenience, I will do what you suggest.
Thanks.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Olivier Tilloy (osomon) wrote : | # |
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://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Carla Sella (carla-sella) wrote : | # |
Ok, sure, thanks.
Unmerged revisions
- 92. By Carla Sella
-
Fixed code on animation fields, except the time picker.
- 91. By Carla Sella
-
Work in progress for fixing new event test.
- 90. By Carla Sella
-
Added a couple of minor fixes.
- 89. By Carla Sella
-
Added asserts for error on location field in new event test.
Preview Diff
1 | === modified file 'tests/autopilot/calendar_app/tests/test_calendar.py' | |||
2 | --- tests/autopilot/calendar_app/tests/test_calendar.py 2013-08-15 12:36:28 +0000 | |||
3 | +++ tests/autopilot/calendar_app/tests/test_calendar.py 2013-08-19 19:10:08 +0000 | |||
4 | @@ -15,11 +15,25 @@ | |||
5 | 15 | 15 | ||
6 | 16 | import time | 16 | import time |
7 | 17 | 17 | ||
8 | 18 | from time import sleep | ||
9 | 18 | from calendar_app.tests import CalendarTestCase | 19 | from calendar_app.tests import CalendarTestCase |
10 | 19 | 20 | ||
11 | 20 | 21 | ||
12 | 21 | class TestMainWindow(CalendarTestCase): | 22 | class TestMainWindow(CalendarTestCase): |
13 | 22 | 23 | ||
14 | 24 | def _input_string_in_field(self, inputField, inputString): | ||
15 | 25 | #poll clicking for focus for lack of better method, | ||
16 | 26 | #due to screen switching animation | ||
17 | 27 | #is there a property we can use instead? | ||
18 | 28 | timeout = 0 | ||
19 | 29 | while inputField.focus != True and timeout < 10: | ||
20 | 30 | self.pointing_device.click_object(inputField) | ||
21 | 31 | sleep(1) | ||
22 | 32 | timeout += 1 | ||
23 | 33 | self.keyboard.type(inputString) | ||
24 | 34 | self.assertThat(inputField.text, Eventually(Equals(inputString))) | ||
25 | 35 | self.keyboard.press_and_release('Enter') | ||
26 | 36 | |||
27 | 23 | def test_timeline_view_shows(self): | 37 | def test_timeline_view_shows(self): |
28 | 24 | """test timeline view""" | 38 | """test timeline view""" |
29 | 25 | 39 | ||
30 | @@ -50,12 +64,12 @@ | |||
31 | 50 | 64 | ||
32 | 51 | #input a new event name | 65 | #input a new event name |
33 | 52 | self.assertThat( | 66 | self.assertThat( |
40 | 53 | self.main_window.get_new_event().visible, Eventually(Equals(True))) | 67 | lambda: self.main_window.get_new_event().visible, |
41 | 54 | eventTitle = "Test event" + str(time.time()) | 68 | Eventually(Equals(True))) |
42 | 55 | 69 | inputString = "Test event" + str(time.time()) | |
43 | 56 | self.pointing_device.click_object(event_name_field) | 70 | eventTitle = inputString |
44 | 57 | self.keyboard.type(eventTitle) | 71 | self._input_string_in_field(event_name_field, inputString) |
45 | 58 | self.assertThat(event_name_field.text, Eventually(Equals(eventTitle))) | 72 | self.assertThat(event_name_field.text, Eventually(Equals(inputString))) |
46 | 59 | 73 | ||
47 | 60 | #input start time | 74 | #input start time |
48 | 61 | self.assertThat( | 75 | self.assertThat( |
49 | @@ -110,14 +124,16 @@ | |||
50 | 110 | Eventually(Equals(True))) | 124 | Eventually(Equals(True))) |
51 | 111 | 125 | ||
52 | 112 | #input location | 126 | #input location |
56 | 113 | self.pointing_device.click_object(location_field) | 127 | self.assertThat( |
57 | 114 | self.keyboard.type("My location") | 128 | lambda: location_field.visible, Eventually(Equals(True))) |
58 | 115 | self.assertThat(location_field.text, Eventually(Equals("My location"))) | 129 | inputString = "My location" |
59 | 130 | self._input_string_in_field(location_field, inputString) | ||
60 | 116 | 131 | ||
61 | 117 | #input people | 132 | #input people |
65 | 118 | self.pointing_device.click_object(people_field) | 133 | self.assertThat( |
66 | 119 | self.keyboard.type("Me") | 134 | lambda: people_field.visible, Eventually(Equals(True))) |
67 | 120 | self.assertThat(people_field.text, Eventually(Equals("Me"))) | 135 | inputString = "Me" |
68 | 136 | self._input_string_in_field(people_field, inputString) | ||
69 | 121 | 137 | ||
70 | 122 | #click save button | 138 | #click save button |
71 | 123 | save_button = self.main_window.get_event_save_button() | 139 | save_button = self.main_window.get_event_save_button() |
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.