Merge lp:~tpeeters/ubuntu-ui-toolkit/ap-toolbar-open into lp:ubuntu-ui-toolkit

Proposed by Tim Peeters
Status: Merged
Approved by: Florian Boucault
Approved revision: 837
Merged at revision: 826
Proposed branch: lp:~tpeeters/ubuntu-ui-toolkit/ap-toolbar-open
Merge into: lp:ubuntu-ui-toolkit
Diff against target: 195 lines (+80/-37)
2 files modified
tests/autopilot/ubuntuuitoolkit/emulators.py (+53/-30)
tests/autopilot/ubuntuuitoolkit/tests/test_emulators.py (+27/-7)
To merge this branch: bzr merge lp:~tpeeters/ubuntu-ui-toolkit/ap-toolbar-open
Reviewer Review Type Date Requested Status
PS Jenkins bot continuous-integration Approve
Leo Arias (community) code review, test analysis, ran tests Approve
Ubuntu SDK team Pending
Review via email: mp+194122@code.launchpad.net

Description of the change

Ensure the toolbar is opened when clicking a toolbar in autopilot tests

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Leo Arias (elopio) wrote :

42 + return self.get_toolbar().close()

the close method doesn't return anything, so you can remove the 'return' from there.

50 + def open(self):
72 + def close(self):

Now we are missing tests for those two new public methods. It would be basically to copy the existing tests, but calling get_toolbar(). With the existing tests for the open and close methods of the main view, you can leave them as they are, or you could use a mock just to check that the new open and close methods were called.

95 + # ensure the toolbar is open
96 + self.open()

I find it weird that the click_button method opens the toolbar if it's not open. I think I would prefer to raise an exception if when called click_button the toolbar is not opened.

98 + # ensure the toolbar is still open (may have closed due to timeout)
99 + self.open()

I do like this other open. I think it's just how a real use would act.

119 +# def test_click_unexisting_button(self):

This test is failing for you probably because you are not using autopilot 1.4.

With this branch, we would be missing also a test that checks that the button is clicked even if the toolbar was closed during the method call. I think you can reproduce that condition changing self.pointing_device.move_to_object(button) for a mock that just sleeps until the toolbar is closed.

Thank you for working on this!

Revision history for this message
Leo Arias (elopio) :
review: Needs Fixing
Revision history for this message
Olivier Tilloy (osomon) wrote :

Copying my comments from bug #1248487:

 « I’m not proposing that open_toolbar close it first, I’m proposing that if it’s already open the emulator does some sort of interaction with it (details to be figured out) to ensure it won’t autohide.

   Your proposal works for click_button(), but what if I want to interact differently with the toolbar. E.g., what if the toolbar contains a TextField, or some other widget? I will still want to ensure that when I call open_toolbar(), it remains open. »

Revision history for this message
Tim Peeters (tpeeters) wrote :

> Copying my comments from bug #1248487:
>
> « I’m not proposing that open_toolbar close it first, I’m proposing that if
> it’s already open the emulator does some sort of interaction with it (details
> to be figured out) to ensure it won’t autohide.
>
> Your proposal works for click_button(), but what if I want to interact
> differently with the toolbar. E.g., what if the toolbar contains a TextField,
> or some other widget? I will still want to ensure that when I call
> open_toolbar(), it remains open. »

The current design specs say that if it is not interacted with for 5 seconds it should close. So I don't know how to incorporate that with your requirement.

Do you currently have a use case?

Revision history for this message
Olivier Tilloy (osomon) wrote :

> > Copying my comments from bug #1248487:
> >
> > « I’m not proposing that open_toolbar close it first, I’m proposing that if
> > it’s already open the emulator does some sort of interaction with it
> (details
> > to be figured out) to ensure it won’t autohide.
> >
> > Your proposal works for click_button(), but what if I want to interact
> > differently with the toolbar. E.g., what if the toolbar contains a
> TextField,
> > or some other widget? I will still want to ensure that when I call
> > open_toolbar(), it remains open. »
>
> The current design specs say that if it is not interacted with for 5 seconds
> it should close. So I don't know how to incorporate that with your
> requirement.

So even if it was manually revealed, it will auto-hide after 5 seconds of inactivity?

Revision history for this message
Tim Peeters (tpeeters) wrote :

Yes, it will always hide after 5 seconds.

For most apps this is not a problem, because clicking a button takes virtually no time. But I realize that for the browser, the user may be entering a URL in the address bar for >5s. So for that case, I would have to detect when the user starts and stops interacting with items inside the toolbar, and stop the timer during interaction (and reset afterwards).

Revision history for this message
Olivier Tilloy (osomon) wrote :

> Yes, it will always hide after 5 seconds.

Ok. I hadn’t understood this. I thought it would autohide only when automatically shown.
So I guess the only solution for autopilot tests is to always ensure the toolbar is first hidden by interacting with the application’s contents, then manually reveal it and interact with it (e.g. click a button).
Otherwise we’ll always run into race conditions where the toolbar auto-hides while the cursor is moving towards one of its buttons.
Either that, or your proposal of ensuring the toolbar is shown in click_button(…) before actually clicking the button.

> For most apps this is not a problem, because clicking a button takes virtually
> no time. But I realize that for the browser, the user may be entering a URL in
> the address bar for >5s. So for that case, I would have to detect when the
> user starts and stops interacting with items inside the toolbar, and stop the
> timer during interaction (and reset afterwards).

No, this is not a problem in the browser, as it uses a custom toolbar, not the default one.

Revision history for this message
Tim Peeters (tpeeters) wrote :

> 95 + # ensure the toolbar is open
> 96 + self.open()
>
> I find it weird that the click_button method opens the toolbar if it's not
> open. I think I would prefer to raise an exception if when called click_button
> the toolbar is not opened.

You cannot really know whether the toolbar is open when clicking.
When you call toolbar_open(), the toolbar opens, but things can happen between that and the actual click on the button so that the toolbar closes again. For example, a timeout can occur. If the toolbar was opened initially, it will close after 5s. But in a test it is possible that only after 4.5s toolbar.open() is called (which does nothing because it was open already), and then it takes 1s to move the cursor to the button to click it, but in the meantime the toolbar closed already.

I think it is a matter of choice whether we want click_button() to open the toolbar for you, or if we want always toolbar_open() to be called before click_button(). Note that you would *always* call toolbar_open() (or toolbar.open()) before click_button, so why not integrate it in that function?

Revision history for this message
Tim Peeters (tpeeters) wrote :

> 119 +# def test_click_unexisting_button(self):
>
> This test is failing for you probably because you are not using autopilot 1.4.

I know. Before merging this, I will put the test back in. I just uncommented it now to make it easier to test on my laptop.

> Thank you for working on this!

No problem :) thanks for assisting me :)

827. By Tim Peeters

remove return in close() call

828. By Tim Peeters

additional documentation for click_button

829. By Tim Peeters

use new/updated functions in test_emulators

Revision history for this message
Tim Peeters (tpeeters) wrote :

> Yes, it will always hide after 5 seconds.
>
> For most apps this is not a problem, because clicking a button takes virtually
> no time. But I realize that for the browser, the user may be entering a URL in
> the address bar for >5s. So for that case, I would have to detect when the
> user starts and stops interacting with items inside the toolbar, and stop the
> timer during interaction (and reset afterwards).

This is no problem for the browser, because it uses its own custom toolbar.
However, if applications use the toolbar as in the browser, this can cause problems.

I reported this bug to fix it: https://bugs.launchpad.net/ubuntu-ui-toolkit/+bug/1249031

830. By Tim Peeters

require toolbar to be opened before calling click_button

831. By Tim Peeters

put test_click_unexisting_button() back in. should work with AP 1.4

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
832. By Tim Peeters

fix too long line in python

833. By Tim Peeters

revert fix, is fixed in upcoming merge

834. By Tim Peeters

Added missing tests and fixed pep257. Thanks elopio.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
835. By Tim Peeters

empty

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Leo Arias (elopio) wrote :

62 + return self

The failure is because that line is overindented. You should return the toolbar even if it is hidden.

Thank you for merging my changes.

review: Approve (code review, test analysis, ran tests)
836. By Tim Peeters

fix indentation

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:836
http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-ci/1199/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty/643
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty-touch/628/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-trusty-amd64-ci/147
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-trusty-armhf-ci/147
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-trusty-armhf-ci/147/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-trusty/584
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/643
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/643/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/628
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/628/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-maguro/3047
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-mako/3261/console
    SUCCESS: http://10.97.0.26:8080/job/touch-flash-device/1311
    SUCCESS: http://10.97.0.26:8080/job/touch-flash-device/1312

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/ubuntu-ui-toolkit-ci/1199/rebuild

review: Needs Fixing (continuous-integration)
837. By Tim Peeters

push trunk

Revision history for this message
Tim Peeters (tpeeters) wrote :

^"push trunk" should be "merge trunk" (just to clarify that nothing was pushed to trunk from here)

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:837
http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-ci/1201/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty/647
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty-touch/632
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-trusty-amd64-ci/149
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-trusty-armhf-ci/149
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-trusty-armhf-ci/149/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-trusty/588
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/647
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/647/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/632
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/632/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-maguro/3051
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-mako/3265
    SUCCESS: http://10.97.0.26:8080/job/touch-flash-device/1320
    SUCCESS: http://10.97.0.26:8080/job/touch-flash-device/1319

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/ubuntu-ui-toolkit-ci/1201/rebuild

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'tests/autopilot/ubuntuuitoolkit/emulators.py'
--- tests/autopilot/ubuntuuitoolkit/emulators.py 2013-11-06 16:38:23 +0000
+++ tests/autopilot/ubuntuuitoolkit/emulators.py 2013-11-08 17:15:25 +0000
@@ -88,39 +88,11 @@
88 :return: The toolbar.88 :return: The toolbar.
8989
90 """90 """
91 toolbar = self.get_toolbar()91 return self.get_toolbar().open()
92 toolbar.animating.wait_for(False)
93 if not toolbar.opened:
94 self._drag_to_open_toolbar()
95 toolbar.opened.wait_for(True)
96 toolbar.animating.wait_for(False)
97
98 return toolbar
99
100 def _drag_to_open_toolbar(self):
101 x, y, _, _ = self.globalRect
102 line_x = x + self.width * 0.50
103 start_y = y + self.height - 1
104 stop_y = y + self.height - self.get_toolbar().height
105
106 self.pointing_device.drag(line_x, start_y, line_x, stop_y)
10792
108 def close_toolbar(self):93 def close_toolbar(self):
109 """Close the toolbar if it's opened."""94 """Close the toolbar if it's opened."""
110 toolbar = self.get_toolbar()95 self.get_toolbar().close()
111 toolbar.animating.wait_for(False)
112 if toolbar.opened:
113 self._drag_to_close_toolbar()
114 toolbar.opened.wait_for(False)
115 toolbar.animating.wait_for(False)
116
117 def _drag_to_close_toolbar(self):
118 x, y, _, _ = self.globalRect
119 line_x = x + self.width * 0.50
120 start_y = y + self.height - self.get_toolbar().height
121 stop_y = y + self.height - 1
122
123 self.pointing_device.drag(line_x, start_y, line_x, stop_y)
12496
125 def get_tabs(self):97 def get_tabs(self):
126 """Return the Tabs emulator of the MainView.98 """Return the Tabs emulator of the MainView.
@@ -244,9 +216,52 @@
244class Toolbar(UbuntuUIToolkitEmulatorBase):216class Toolbar(UbuntuUIToolkitEmulatorBase):
245 """Toolbar Autopilot emulator."""217 """Toolbar Autopilot emulator."""
246218
219 def open(self):
220 """Open the toolbar if it's not already opened.
221
222 :return: The toolbar.
223
224 """
225 self.animating.wait_for(False)
226 if not self.opened:
227 self._drag_to_open()
228 self.opened.wait_for(True)
229 self.animating.wait_for(False)
230
231 return self
232
233 def _drag_to_open(self):
234 x, y, _, _ = self.globalRect
235 line_x = x + self.width * 0.50
236 start_y = y + self.height - 1
237 stop_y = y
238
239 self.pointing_device.drag(line_x, start_y, line_x, stop_y)
240
241 def close(self):
242 """Close the toolbar if it's opened."""
243 self.animating.wait_for(False)
244 if self.opened:
245 self._drag_to_close()
246 self.opened.wait_for(False)
247 self.animating.wait_for(False)
248
249 def _drag_to_close(self):
250 x, y, _, _ = self.globalRect
251 line_x = x + self.width * 0.50
252 start_y = y
253 stop_y = y + self.height - 1
254
255 self.pointing_device.drag(line_x, start_y, line_x, stop_y)
256
247 def click_button(self, object_name):257 def click_button(self, object_name):
248 """Click a button of the toolbar.258 """Click a button of the toolbar.
249259
260 The toolbar should be opened before clicking the button, or an
261 exception will be raised. If the toolbar is closed for some reason
262 (e.g., timer finishes) after moving the mouse cursor and before
263 clicking the button, it is re-opened automatically by this function.
264
250 :parameter object_name: The QML objectName property of the button.265 :parameter object_name: The QML objectName property of the button.
251 :raise ToolkitEmulatorException: If there is no button with that object266 :raise ToolkitEmulatorException: If there is no button with that object
252 name.267 name.
@@ -257,6 +272,14 @@
257 except dbus.StateNotFoundError:272 except dbus.StateNotFoundError:
258 raise ToolkitEmulatorException(273 raise ToolkitEmulatorException(
259 'Button with objectName "{0}" not found.'.format(object_name))274 'Button with objectName "{0}" not found.'.format(object_name))
275 # ensure the toolbar is open
276 if not self.opened:
277 raise ToolkitEmulatorException(
278 'Toolbar must be opened before calling click_button().')
279 self.pointing_device.move_to_object(button)
280 # ensure the toolbar is still open (may have closed due to timeout)
281 self.open()
282 # click the button
260 self.pointing_device.click_object(button)283 self.pointing_device.click_object(button)
261284
262 def _get_button(self, object_name):285 def _get_button(self, object_name):
263286
=== modified file 'tests/autopilot/ubuntuuitoolkit/tests/test_emulators.py'
--- tests/autopilot/ubuntuuitoolkit/tests/test_emulators.py 2013-11-06 16:45:12 +0000
+++ tests/autopilot/ubuntuuitoolkit/tests/test_emulators.py 2013-11-08 17:15:25 +0000
@@ -87,6 +87,18 @@
87 toolbar = self.main_view.get_toolbar()87 toolbar = self.main_view.get_toolbar()
88 self.assertIsInstance(toolbar, emulators.Toolbar)88 self.assertIsInstance(toolbar, emulators.Toolbar)
8989
90 def test_open_toolbar(self):
91 with mock.patch.object(emulators.Toolbar, 'open') as mock_open:
92 self.main_view.open_toolbar()
93
94 mock_open.assert_called_once_with()
95
96 def test_close_toolbar(self):
97 with mock.patch.object(emulators.Toolbar, 'close') as mock_close:
98 self.main_view.close_toolbar()
99
100 mock_close.assert_called_once_with()
101
90 def test_open_toolbar_returns_the_toolbar(self):102 def test_open_toolbar_returns_the_toolbar(self):
91 toolbar = self.main_view.open_toolbar()103 toolbar = self.main_view.open_toolbar()
92 self.assertIsInstance(toolbar, emulators.Toolbar)104 self.assertIsInstance(toolbar, emulators.Toolbar)
@@ -171,29 +183,29 @@
171 self.assertFalse(self.toolbar.opened)183 self.assertFalse(self.toolbar.opened)
172184
173 def test_open_toolbar(self):185 def test_open_toolbar(self):
174 self.main_view.open_toolbar()186 self.toolbar.open()
175 self.assertTrue(self.toolbar.opened)187 self.assertTrue(self.toolbar.opened)
176 self.assertFalse(self.toolbar.animating)188 self.assertFalse(self.toolbar.animating)
177189
178 def test_opened_toolbar_is_not_opened_again(self):190 def test_opened_toolbar_is_not_opened_again(self):
179 self.main_view.open_toolbar()191 self.toolbar.open()
180 with mock.patch.object(192 with mock.patch.object(
181 self.main_view.pointing_device, 'drag') as mock_drag:193 self.main_view.pointing_device, 'drag') as mock_drag:
182 self.main_view.open_toolbar()194 self.toolbar.open()
183195
184 self.assertFalse(mock_drag.called)196 self.assertFalse(mock_drag.called)
185 self.assertTrue(self.toolbar.opened)197 self.assertTrue(self.toolbar.opened)
186198
187 def test_close_toolbar(self):199 def test_close_toolbar(self):
188 self.main_view.open_toolbar()200 self.toolbar.open()
189 self.main_view.close_toolbar()201 self.toolbar.close()
190 self.assertFalse(self.toolbar.opened)202 self.assertFalse(self.toolbar.opened)
191 self.assertFalse(self.toolbar.animating)203 self.assertFalse(self.toolbar.animating)
192204
193 def test_closed_toolbar_is_not_closed_again(self):205 def test_closed_toolbar_is_not_closed_again(self):
194 with mock.patch.object(206 with mock.patch.object(
195 self.main_view.pointing_device, 'drag') as mock_drag:207 self.main_view.pointing_device, 'drag') as mock_drag:
196 self.main_view.close_toolbar()208 self.toolbar.close()
197209
198 self.assertFalse(mock_drag.called)210 self.assertFalse(mock_drag.called)
199 self.assertFalse(self.toolbar.opened)211 self.assertFalse(self.toolbar.opened)
@@ -201,7 +213,7 @@
201 def test_click_toolbar_button(self):213 def test_click_toolbar_button(self):
202 label = self.app.select_single('Label', objectName='clicked_label')214 label = self.app.select_single('Label', objectName='clicked_label')
203 self.assertNotEqual(label.text, 'Button clicked.')215 self.assertNotEqual(label.text, 'Button clicked.')
204 self.main_view.open_toolbar()216 self.toolbar.open()
205 self.toolbar.click_button('buttonName')217 self.toolbar.click_button('buttonName')
206 self.assertEqual(label.text, 'Button clicked.')218 self.assertEqual(label.text, 'Button clicked.')
207219
@@ -213,6 +225,14 @@
213 self.assertEqual(225 self.assertEqual(
214 error.message, 'Button with objectName "unexisting" not found.')226 error.message, 'Button with objectName "unexisting" not found.')
215227
228 def test_click_button_on_closed_toolbar(self):
229 error = self.assertRaises(
230 emulators.ToolkitEmulatorException, self.toolbar.click_button,
231 'buttonName')
232 self.assertEqual(
233 error.message,
234 'Toolbar must be opened before calling click_button().')
235
216236
217class TabsTestCase(tests.QMLStringAppTestCase):237class TabsTestCase(tests.QMLStringAppTestCase):
218238

Subscribers

People subscribed via source and target branches

to status/vote changes: