Merge lp:~ahayzen/ubuntu-weather-app/reboot-1452497-add-location-from-home into lp:ubuntu-weather-app

Proposed by Andrew Hayzen on 2015-07-27
Status: Merged
Approved by: Victor Thompson on 2015-08-06
Approved revision: 82
Merged at revision: 80
Proposed branch: lp:~ahayzen/ubuntu-weather-app/reboot-1452497-add-location-from-home
Merge into: lp:ubuntu-weather-app
Diff against target: 416 lines (+196/-30)
7 files modified
app/ui/AddLocationPage.qml (+22/-7)
app/ui/LocationsPage.qml (+3/-0)
debian/changelog (+3/-0)
po/com.ubuntu.weather.pot (+10/-10)
tests/autopilot/ubuntu_weather_app/__init__.py (+53/-12)
tests/autopilot/ubuntu_weather_app/tests/test_add_location_page.py (+100/-0)
tests/autopilot/ubuntu_weather_app/tests/test_empty_state.py (+5/-1)
To merge this branch: bzr merge lp:~ahayzen/ubuntu-weather-app/reboot-1452497-add-location-from-home
Reviewer Review Type Date Requested Status
Victor Thompson 2015-07-27 Approve on 2015-08-06
Ubuntu Phone Apps Jenkins Bot continuous-integration Approve on 2015-08-06
Andrew Hayzen Abstain on 2015-08-02
Review via email: mp+266051@code.launchpad.net

Commit message

* Create autopilot test which adds a location via searching
* Create autopilot test which adds a location via cached results
* Fix for racy search bar causing incorrect search query when typed quickly

Description of the change

* Create autopilot test which adds a location via searching
* Create autopilot test which adds a location via cached results
* Fix for racy search bar causing incorrect search query when typed quickly

These test that you can add a location from the add locations page by either searching for a location or using the cached list.

To post a comment you must log in.
Victor Thompson (vthompson) wrote :

I have some inline comments that should be addressed. Also, of the 5 or so runs I did, one instance incorrectly resized the window rather than grabbing the tool tip. I'm not 100% this was or was not my fault, but we don't want this to be a "flaky" test...

review: Needs Fixing
Andrew Hayzen (ahayzen) wrote :

If in [0] we change to only having the bottom edge as an entry point for adding locations, then this and the other test should be moved into their own test_locations_page file and class.

0 - https://code.launchpad.net/~ahayzen/ubuntu-weather-app/reboot-fix-no-settings-button-empty-state/+merge/266044

73. By Andrew Hayzen on 2015-07-29

* Merge of trunk and test from lp:~ahayzen/ubuntu-weather-app/reboot-1452497-add-location-from-empty-state
* Move tests into test_add_location_page.py

74. By Andrew Hayzen on 2015-07-29

* Check that the location name of the added item is correct

75. By Andrew Hayzen on 2015-07-29

* Remove old test code

76. By Andrew Hayzen on 2015-07-29

* Remove unneeded comments

77. By Andrew Hayzen on 2015-07-29

* Update changelog to reference bug #

Victor Thompson (vthompson) wrote :

I thought you said this was rebased on top of your empty state fix? There are currently AP and changelog merge conflicts you'll need to resolve.

Also, I've had the search test occasionally fail for me. I think you'll want to add a delay after entering the search term or maybe have a wait for the location name to be item 0 before clicking?

review: Needs Fixing
Victor Thompson (vthompson) wrote :

One more thing, I should have had you remove "ubuntu_weather_app.tests.test_empty_state.TestEmptyState.test_add_location_button" in that last MP. Can you remove it in this mp?

78. By Andrew Hayzen on 2015-08-02

* Merge of trunk

79. By Andrew Hayzen on 2015-08-02

* Fix for double merge conflicts

Andrew Hayzen (ahayzen) wrote :

Still need to add the delay to the search, otherwise it should be good now, so failing myself until I've done that.

review: Needs Fixing
80. By Andrew Hayzen on 2015-08-02

* Add delay to search

Andrew Hayzen (ahayzen) wrote :

Added a delay of 250ms to the search, not sure if there a standard value for this? Let me know if you want it shorter/longer, 250ms feels around right to me.

review: Abstain
Victor Thompson (vthompson) wrote :

On my first 3 runs the search test failed:

Traceback (most recent call last):
  File "/home/victor/Development/reboot-1452497-add-location-from-home/tests/autopilot/ubuntu_weather_app/tests/test_add_location_page.py", line 84, in test_add_location_via_search
    self.assertThat(self.locations_page.visible, Eventually(Equals(True)))
  File "/usr/lib/python3/dist-packages/testtools/testcase.py", line 423, in assertThat
    raise mismatch_error
testtools.matchers._impl.MismatchError: After 10.0 seconds test on LocationsPage.visible failed: True != dbus.Boolean(False, variant_level=1)

I didn't think you'd want to add a timer to the app to get a delay--I assumed you'd add a delay to the test.

81. By Andrew Hayzen on 2015-08-06

* Fix for search so that autopilot waits for it to complete

82. By Andrew Hayzen on 2015-08-06

* Include search fix in changelog

Victor Thompson (vthompson) wrote :

This lgtm. I'm still not sure the timer solution is the proper one for the "racy" search issue. I can still force the search to not populate things the way I'd assume if I type, stop typing, and start typing again. Let's see if it becomes problematic.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'app/ui/AddLocationPage.qml'
2--- app/ui/AddLocationPage.qml 2015-06-28 23:37:10 +0000
3+++ app/ui/AddLocationPage.qml 2015-08-06 00:13:15 +0000
4@@ -35,6 +35,8 @@
5 */
6 flickable: null
7
8+ property bool searching: citiesModel.loading || searchTimer.running
9+
10 state: "default"
11 states: [
12 PageHeadState {
13@@ -48,6 +50,7 @@
14 actions: [
15 Action {
16 iconName: "search"
17+ objectName: "search"
18 text: i18n.tr("City")
19 onTriggered: {
20 addLocationPage.state = "search"
21@@ -83,6 +86,23 @@
22 }
23 ]
24
25+ // Outside component so property can bind to for autopilot
26+ Timer {
27+ id: searchTimer
28+ interval: 250
29+ onTriggered: {
30+ if (searchComponentLoader.item) { // check component exists
31+ if (searchComponentLoader.item.text.trim() === "") {
32+ loadEmpty()
33+ } else {
34+ loadFromProvider(searchComponentLoader.item.text)
35+ }
36+ } else {
37+ loadEmpty()
38+ }
39+ }
40+ }
41+
42 Component {
43 id: searchComponent
44 TextField {
45@@ -93,13 +113,7 @@
46 placeholderText: i18n.tr("Search city")
47 hasClearButton: true
48
49- onTextChanged: {
50- if (text.trim() === "") {
51- loadEmpty()
52- } else {
53- loadFromProvider(text)
54- }
55- }
56+ onTextChanged: searchTimer.restart()
57 }
58 }
59
60@@ -206,6 +220,7 @@
61
62 delegate: ListItem {
63 divider.visible: false
64+ objectName: "addLocation" + index
65 Column {
66 anchors {
67 left: parent.left
68
69=== modified file 'app/ui/LocationsPage.qml'
70--- app/ui/LocationsPage.qml 2015-07-29 21:34:07 +0000
71+++ app/ui/LocationsPage.qml 2015-08-06 00:13:15 +0000
72@@ -38,6 +38,7 @@
73 actions: [
74 Action {
75 iconName: "add"
76+ objectName: "addLocation"
77 onTriggered: mainPageStack.push(Qt.resolvedUrl("AddLocationPage.qml"))
78 }
79 ]
80@@ -153,6 +154,7 @@
81
82 delegate: WeatherListItem {
83 id: locationsListItem
84+ objectName: "location" + index
85 leftSideAction: Remove {
86 onTriggered: storage.removeLocation(index)
87 }
88@@ -194,6 +196,7 @@
89
90 Label {
91 id: locationName
92+ objectName: "name"
93 elide: Text.ElideRight
94 fontSize: "medium"
95 text: name
96
97=== modified file 'debian/changelog'
98--- debian/changelog 2015-07-30 02:55:02 +0000
99+++ debian/changelog 2015-08-06 00:13:15 +0000
100@@ -19,6 +19,9 @@
101 * Remove add location button
102 * Change phrasing of manual add location
103 * Conditionally show "searching for current location" depending on the detectCurrentLocation setting
104+ * Create autopilot test which adds a location via searching (LP: #1452497)
105+ * Create autopilot test which adds a location via cached results (LP: #1452497)
106+ * Fix for racy search bar causing incorrect search query when typed quickly
107
108 -- Victor Thompson <victor.thompson@gmail.com> Mon, 01 Jun 2015 20:11:23 -0500
109
110
111=== modified file 'po/com.ubuntu.weather.pot'
112--- po/com.ubuntu.weather.pot 2015-07-29 20:34:57 +0000
113+++ po/com.ubuntu.weather.pot 2015-08-06 00:13:15 +0000
114@@ -8,7 +8,7 @@
115 msgstr ""
116 "Project-Id-Version: ubuntu-weather-app\n"
117 "Report-Msgid-Bugs-To: \n"
118-"POT-Creation-Date: 2015-07-29 21:25+0100\n"
119+"POT-Creation-Date: 2015-08-06 01:02+0100\n"
120 "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
121 "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
122 "Language-Team: LANGUAGE <LL@li.org>\n"
123@@ -82,39 +82,39 @@
124 msgid "Select a city"
125 msgstr ""
126
127-#: ../app/ui/AddLocationPage.qml:45 ../app/ui/AddLocationPage.qml:66
128+#: ../app/ui/AddLocationPage.qml:47 ../app/ui/AddLocationPage.qml:69
129 msgid "Back"
130 msgstr ""
131
132-#: ../app/ui/AddLocationPage.qml:51
133+#: ../app/ui/AddLocationPage.qml:54
134 msgid "City"
135 msgstr ""
136
137-#: ../app/ui/AddLocationPage.qml:93
138+#: ../app/ui/AddLocationPage.qml:113
139 msgid "Search city"
140 msgstr ""
141
142-#: ../app/ui/AddLocationPage.qml:278
143+#: ../app/ui/AddLocationPage.qml:293
144 msgid "No city found"
145 msgstr ""
146
147-#: ../app/ui/AddLocationPage.qml:291
148+#: ../app/ui/AddLocationPage.qml:306
149 msgid "Couldn't load weather data, please try later again!"
150 msgstr ""
151
152-#: ../app/ui/AddLocationPage.qml:301
153+#: ../app/ui/AddLocationPage.qml:316
154 msgid "Location already added."
155 msgstr ""
156
157-#: ../app/ui/AddLocationPage.qml:304
158+#: ../app/ui/AddLocationPage.qml:319
159 msgid "OK"
160 msgstr ""
161
162-#: ../app/ui/HomePage.qml:31 ../app/ui/LocationsPage.qml:30
163+#: ../app/ui/HomePage.qml:31 ../app/ui/LocationsPage.qml:31
164 msgid "Locations"
165 msgstr ""
166
167-#: ../app/ui/LocationsPage.qml:104
168+#: ../app/ui/LocationsPage.qml:106
169 msgid "Current Location"
170 msgstr ""
171
172
173=== modified file 'tests/autopilot/ubuntu_weather_app/__init__.py'
174--- tests/autopilot/ubuntu_weather_app/__init__.py 2015-07-29 21:34:07 +0000
175+++ tests/autopilot/ubuntu_weather_app/__init__.py 2015-08-06 00:13:15 +0000
176@@ -10,7 +10,6 @@
177 import logging
178 from ubuntuuitoolkit import MainView, UbuntuUIToolkitCustomProxyObjectBase
179
180-
181 logger = logging.getLogger(__name__)
182
183
184@@ -51,18 +50,27 @@
185
186 class Page(UbuntuUIToolkitCustomProxyObjectBase):
187 """Autopilot helper for Pages."""
188- def __init__(self, *args):
189- super(Page, self).__init__(*args)
190+ def __init__(self, *args, **kwargs):
191+ super(Page, self).__init__(*args, **kwargs)
192+
193+ # Use only objectName due to bug 1350532 as it is MainView12
194+ self.main_view = self.get_root_instance().select_single(
195+ objectName="weather")
196+
197+ def click_back(self):
198+ return self.main_view.get_header().click_back_button()
199
200
201 class PageWithBottomEdge(Page):
202- """Autopilot helper for PageWithBottomEdge."""
203- def __init__(self, *args):
204- super(PageWithBottomEdge, self).__init__(*args)
205+ """
206+ An emulator class that makes it easy to interact with the bottom edge
207+ swipe page
208+ """
209+ def __init__(self, *args, **kwargs):
210+ super(PageWithBottomEdge, self).__init__(*args, **kwargs)
211
212 def reveal_bottom_edge_page(self):
213 """Bring the bottom edge page to the screen"""
214-
215 self.bottomEdgePageLoaded.wait_for(True)
216
217 try:
218@@ -73,7 +81,6 @@
219 start_y = (action_item.globalRect.y +
220 (action_item.height * 0.5))
221 stop_y = start_y - (self.height * 0.7)
222-
223 self.pointing_device.drag(start_x, start_y,
224 start_x, stop_y, rate=2)
225 self.isReady.wait_for(True)
226@@ -84,14 +91,36 @@
227
228 class AddLocationPage(Page):
229 """Autopilot helper for AddLocationPage."""
230- def __init__(self, *args):
231- super(AddLocationPage, self).__init__(*args)
232+ def __init__(self, *args, **kwargs):
233+ super(AddLocationPage, self).__init__(*args, **kwargs)
234+
235+ @click_object
236+ def click_location(self, index):
237+ return self.select_single("UCListItem",
238+ objectName="addLocation" + str(index))
239+
240+ def click_search_action(self):
241+ self.main_view.get_header().click_action_button("search")
242+
243+ def get_search_field(self):
244+ header = self.main_view.get_header()
245+
246+ return header.select_single("TextField", objectName="searchField")
247+
248+ def search(self, value):
249+ self.click_search_action()
250+
251+ search_field = self.get_search_field()
252+ search_field.write(value)
253+
254+ # Wait for model to finish loading
255+ self.searching.wait_for(False)
256
257
258 class HomePage(PageWithBottomEdge):
259 """Autopilot helper for HomePage."""
260- def __init__(self, *args):
261- super(HomePage, self).__init__(*args)
262+ def __init__(self, *args, **kwargs):
263+ super(HomePage, self).__init__(*args, **kwargs)
264
265 def get_location_count(self):
266 return self.wait_select_single(
267@@ -103,6 +132,13 @@
268 def __init__(self, *args, **kwargs):
269 super(LocationsPage, self).__init__(*args, **kwargs)
270
271+ def click_add_location_action(self):
272+ self.main_view.get_header().click_action_button("addLocation")
273+
274+ def get_location(self, index):
275+ return self.select_single(WeatherListItem,
276+ objectName="location" + str(index))
277+
278
279 class MainView(MainView):
280 """Autopilot custom proxy object for the MainView."""
281@@ -111,3 +147,8 @@
282 def __init__(self, *args):
283 super(MainView, self).__init__(*args)
284 self.visible.wait_for(True)
285+
286+
287+class WeatherListItem(UbuntuUIToolkitCustomProxyObjectBase):
288+ def get_name(self):
289+ return self.select_single("Label", objectName="name").text
290
291=== added file 'tests/autopilot/ubuntu_weather_app/tests/test_add_location_page.py'
292--- tests/autopilot/ubuntu_weather_app/tests/test_add_location_page.py 1970-01-01 00:00:00 +0000
293+++ tests/autopilot/ubuntu_weather_app/tests/test_add_location_page.py 2015-08-06 00:13:15 +0000
294@@ -0,0 +1,100 @@
295+# -*- Mode: Python; coding: utf-8; indent-tabs-mode: nil; tab-width: 4 -*-
296+# Copyright 2013, 2014, 2015 Canonical
297+#
298+# This program is free software: you can redistribute it and/or modify it
299+# under the terms of the GNU General Public License version 3, as published
300+# by the Free Software Foundation.
301+
302+"""Ubuntu Weather app autopilot tests."""
303+
304+from __future__ import absolute_import
305+
306+import logging
307+from autopilot.matchers import Eventually
308+from testtools.matchers import Equals
309+
310+
311+from ubuntu_weather_app.tests import UbuntuWeatherAppTestCaseWithData
312+
313+logger = logging.getLogger(__name__)
314+
315+
316+class TestAddLocationPage(UbuntuWeatherAppTestCaseWithData):
317+
318+ """ Tests for the add locations page
319+ setUp jumps to add to locations page as all tests start from here """
320+
321+ def setUp(self):
322+ super(TestAddLocationPage, self).setUp()
323+
324+ # Get the start count of the homepage
325+ self.home_page = self.app.get_home_page()
326+ self.start_count = self.home_page.get_location_count()
327+
328+ # Open the locations page from bottom edge
329+ self.home_page.reveal_bottom_edge_page()
330+
331+ self.locations_page = self.app.get_locations_page()
332+ self.locations_page.visible.wait_for(True)
333+
334+ # Select the add header action and get the add locations page
335+ self.locations_page.click_add_location_action()
336+
337+ # Get the add locations page
338+ self.add_location_page = self.app.get_add_location_page()
339+ self.add_location_page.visible.wait_for(True)
340+
341+ def test_add_location_via_cache(self):
342+ """ tests adding a location via cached location list """
343+
344+ # Select the location
345+ self.add_location_page.click_location(0)
346+
347+ # Check locations page is now visible
348+ self.assertThat(self.locations_page.visible, Eventually(Equals(True)))
349+
350+ # Get the list item of the added location
351+ list_item = self.locations_page.get_location(self.start_count)
352+
353+ # Check that the name is correct
354+ self.assertThat(list_item.get_name(), Equals("Amsterdam"))
355+
356+ # Go back to the homepage
357+ self.locations_page.click_back()
358+
359+ # Check homepage is now visible
360+ self.assertThat(self.home_page.visible, Eventually(Equals(True)))
361+
362+ # Check that the location was added
363+ self.assertThat(self.home_page.get_location_count,
364+ Eventually(Equals(self.start_count + 1)))
365+
366+ def test_add_location_via_search(self):
367+ """ tests adding a location via searching for the location """
368+
369+ location_name = "Paris"
370+
371+ # Perform search
372+ self.add_location_page.search(location_name)
373+
374+ # Select the location
375+ self.add_location_page.click_location(0)
376+
377+ # Check locations page is now visible
378+ self.assertThat(self.locations_page.visible, Eventually(Equals(True)))
379+
380+ # Get the list item of the added location
381+ list_item = self.locations_page.get_location(self.start_count)
382+
383+ # Check that the name is correct
384+ self.assertThat(list_item.get_name(), Equals(location_name))
385+
386+ # Go back to the homepage
387+ self.locations_page.click_back()
388+
389+ # Check homepage is now visible
390+ self.assertThat(self.home_page.visible, Eventually(Equals(True)))
391+
392+ # Check that the location was added
393+ self.assertThat(self.home_page.get_location_count,
394+ Eventually(Equals(self.start_count + 1)))
395
396=== modified file 'tests/autopilot/ubuntu_weather_app/tests/test_empty_state.py'
397--- tests/autopilot/ubuntu_weather_app/tests/test_empty_state.py 2015-07-29 21:34:07 +0000
398+++ tests/autopilot/ubuntu_weather_app/tests/test_empty_state.py 2015-08-06 00:13:15 +0000
399@@ -24,12 +24,16 @@
400 def setUp(self):
401 super(TestEmptyState, self).setUp()
402
403- def test_add_location_button(self):
404+ def test_add_locations_page_from_bottom_edge(self):
405 """ tests that the add location page is shown after swiping up
406 the bottom edge"""
407
408 home_page = self.app.get_home_page()
409 home_page.visible.wait_for(True)
410+
411+ # Check that there are no locations
412+ self.assertThat(home_page.get_location_count, Eventually(Equals(0)))
413+
414 home_page.reveal_bottom_edge_page()
415
416 locations_page = self.app.get_locations_page()

Subscribers

People subscribed via source and target branches

to all changes: