Ubuntu UI Toolkit

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

ap-toolbar-open
Merge into trunk

Proposed by Tim Peeters on 2013-11-06

Status:

Merged

Approved by:

Florian Boucault on 2013-11-08

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

Critical

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	continuous-integration		Approve on 2013-11-08
Leo Arias (community)	code review, test analysis, ran tests		Approve on 2013-11-08
Ubuntu SDK team		2013-11-06	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

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-11-06:

FAILED: Continuous integration, rev:826
http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-ci/1168/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty/517/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty-touch/505/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-trusty-amd64-ci/116/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-trusty-armhf-ci/116/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/517/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/505/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Leo Arias (elopio) wrote on 2013-11-06:

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) on 2013-11-06:

review: Needs Fixing

Revision history for this message

Olivier Tilloy (osomon) wrote on 2013-11-07:

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 on 2013-11-07:

> 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 on 2013-11-07:

> > 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 on 2013-11-07:

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 on 2013-11-07:

> 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 on 2013-11-07:

> 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 on 2013-11-07:

> 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 :)

lp:~tpeeters/ubuntu-ui-toolkit/ap-toolbar-open updated on 2013-11-07

827. By Tim Peeters on 2013-11-07: remove return in close() call
828. By Tim Peeters on 2013-11-07: additional documentation for click_button
829. By Tim Peeters on 2013-11-07: use new/updated functions in test_emulators

Revision history for this message

Tim Peeters (tpeeters) wrote on 2013-11-07:

> 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

lp:~tpeeters/ubuntu-ui-toolkit/ap-toolbar-open updated on 2013-11-07

830. By Tim Peeters on 2013-11-07: require toolbar to be opened before calling click_button
831. By Tim Peeters on 2013-11-07: put test_click_unexisting_button() back in. should work with AP 1.4

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-11-07:

FAILED: Continuous integration, rev:829
http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-ci/1186/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty/587/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty-touch/572/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-trusty-amd64-ci/134/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-trusty-armhf-ci/134/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/587/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/572/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-11-07:

FAILED: Continuous integration, rev:831
http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-ci/1188/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty/592/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty-touch/577/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-trusty-amd64-ci/136/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-trusty-armhf-ci/136/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/592/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/577/console

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

review: Needs Fixing (continuous-integration)

lp:~tpeeters/ubuntu-ui-toolkit/ap-toolbar-open updated on 2013-11-08

832. By Tim Peeters on 2013-11-08: fix too long line in python
833. By Tim Peeters on 2013-11-08: revert fix, is fixed in upcoming merge
834. By Tim Peeters on 2013-11-08: Added missing tests and fixed pep257. Thanks elopio.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-11-08:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-11-08:

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

review: Needs Fixing (continuous-integration)

lp:~tpeeters/ubuntu-ui-toolkit/ap-toolbar-open updated on 2013-11-08

835. By Tim Peeters on 2013-11-08: empty

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-11-08:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Leo Arias (elopio) wrote on 2013-11-08:

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)

lp:~tpeeters/ubuntu-ui-toolkit/ap-toolbar-open updated on 2013-11-08

836. By Tim Peeters on 2013-11-08: fix indentation

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-11-08:

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

review: Needs Fixing (continuous-integration)

lp:~tpeeters/ubuntu-ui-toolkit/ap-toolbar-open updated on 2013-11-08

837. By Tim Peeters on 2013-11-08: push trunk

Revision history for this message

Tim Peeters (tpeeters) wrote on 2013-11-08:

^"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 on 2013-11-08:

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

Subscribers

People subscribed via source and target branches

to all changes:

Brian Blevins

Caco V. Topia

Jaroslavas Karmazinas

Tim Peeters

Ubuntu SDK team

to status/vote changes:

william

 === 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 @@
          :return: The toolbar.
          """
--        toolbar = self.get_toolbar()
--        toolbar.animating.wait_for(False)
--        if not toolbar.opened:
--            self._drag_to_open_toolbar()
--            toolbar.opened.wait_for(True)
--            toolbar.animating.wait_for(False)
--
--        return toolbar
--
--    def _drag_to_open_toolbar(self):
--        x, y, _, _ = self.globalRect
--        line_x = x + self.width * 0.50
--        start_y = y + self.height - 1
--        stop_y = y + self.height - self.get_toolbar().height
--
--        self.pointing_device.drag(line_x, start_y, line_x, stop_y)
++        return self.get_toolbar().open()
      def close_toolbar(self):
          """Close the toolbar if it's opened."""
--        toolbar = self.get_toolbar()
--        toolbar.animating.wait_for(False)
--        if toolbar.opened:
--            self._drag_to_close_toolbar()
--            toolbar.opened.wait_for(False)
--            toolbar.animating.wait_for(False)
--
--    def _drag_to_close_toolbar(self):
--        x, y, _, _ = self.globalRect
--        line_x = x + self.width * 0.50
--        start_y = y + self.height - self.get_toolbar().height
--        stop_y = y + self.height - 1
--
--        self.pointing_device.drag(line_x, start_y, line_x, stop_y)
++        self.get_toolbar().close()
      def get_tabs(self):
          """Return the Tabs emulator of the MainView.
@@ -244,9 +216,52 @@
  class Toolbar(UbuntuUIToolkitEmulatorBase):
      """Toolbar Autopilot emulator."""
++    def open(self):
++        """Open the toolbar if it's not already opened.
++
++        :return: The toolbar.
++
++        """
++        self.animating.wait_for(False)
++        if not self.opened:
++            self._drag_to_open()
++            self.opened.wait_for(True)
++            self.animating.wait_for(False)
++
++        return self
++
++    def _drag_to_open(self):
++        x, y, _, _ = self.globalRect
++        line_x = x + self.width * 0.50
++        start_y = y + self.height - 1
++        stop_y = y
++
++        self.pointing_device.drag(line_x, start_y, line_x, stop_y)
++
++    def close(self):
++        """Close the toolbar if it's opened."""
++        self.animating.wait_for(False)
++        if self.opened:
++            self._drag_to_close()
++            self.opened.wait_for(False)
++            self.animating.wait_for(False)
++
++    def _drag_to_close(self):
++        x, y, _, _ = self.globalRect
++        line_x = x + self.width * 0.50
++        start_y = y
++        stop_y = y + self.height - 1
++
++        self.pointing_device.drag(line_x, start_y, line_x, stop_y)
++
      def click_button(self, object_name):
          """Click a button of the toolbar.
++        The toolbar should be opened before clicking the button, or an
++        exception will be raised. If the toolbar is closed for some reason
++        (e.g., timer finishes) after moving the mouse cursor and before
++        clicking the button, it is re-opened automatically by this function.
++
          :parameter object_name: The QML objectName property of the button.
          :raise ToolkitEmulatorException: If there is no button with that object
              name.
@@ -257,6 +272,14 @@
          except dbus.StateNotFoundError:
              raise ToolkitEmulatorException(
                  'Button with objectName "{0}" not found.'.format(object_name))
++        # ensure the toolbar is open
++        if not self.opened:
++            raise ToolkitEmulatorException(
++                'Toolbar must be opened before calling click_button().')
++        self.pointing_device.move_to_object(button)
++        # ensure the toolbar is still open (may have closed due to timeout)
++        self.open()
++        # click the button
          self.pointing_device.click_object(button)
      def _get_button(self, object_name):
 === 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 @@
          toolbar = self.main_view.get_toolbar()
          self.assertIsInstance(toolbar, emulators.Toolbar)
++    def test_open_toolbar(self):
++        with mock.patch.object(emulators.Toolbar, 'open') as mock_open:
++            self.main_view.open_toolbar()
++
++        mock_open.assert_called_once_with()
++
++    def test_close_toolbar(self):
++        with mock.patch.object(emulators.Toolbar, 'close') as mock_close:
++            self.main_view.close_toolbar()
++
++        mock_close.assert_called_once_with()
++
      def test_open_toolbar_returns_the_toolbar(self):
          toolbar = self.main_view.open_toolbar()
          self.assertIsInstance(toolbar, emulators.Toolbar)
@@ -171,29 +183,29 @@
          self.assertFalse(self.toolbar.opened)
      def test_open_toolbar(self):
--        self.main_view.open_toolbar()
++        self.toolbar.open()
          self.assertTrue(self.toolbar.opened)
          self.assertFalse(self.toolbar.animating)
      def test_opened_toolbar_is_not_opened_again(self):
--        self.main_view.open_toolbar()
++        self.toolbar.open()
          with mock.patch.object(
                  self.main_view.pointing_device, 'drag') as mock_drag:
--            self.main_view.open_toolbar()
++            self.toolbar.open()
          self.assertFalse(mock_drag.called)
          self.assertTrue(self.toolbar.opened)
      def test_close_toolbar(self):
--        self.main_view.open_toolbar()
--        self.main_view.close_toolbar()
++        self.toolbar.open()
++        self.toolbar.close()
          self.assertFalse(self.toolbar.opened)
          self.assertFalse(self.toolbar.animating)
      def test_closed_toolbar_is_not_closed_again(self):
          with mock.patch.object(
                  self.main_view.pointing_device, 'drag') as mock_drag:
--            self.main_view.close_toolbar()
++            self.toolbar.close()
          self.assertFalse(mock_drag.called)
          self.assertFalse(self.toolbar.opened)
@@ -201,7 +213,7 @@
      def test_click_toolbar_button(self):
          label = self.app.select_single('Label', objectName='clicked_label')
          self.assertNotEqual(label.text, 'Button clicked.')
--        self.main_view.open_toolbar()
++        self.toolbar.open()
          self.toolbar.click_button('buttonName')
          self.assertEqual(label.text, 'Button clicked.')
@@ -213,6 +225,14 @@
          self.assertEqual(
              error.message, 'Button with objectName "unexisting" not found.')
++    def test_click_button_on_closed_toolbar(self):
++        error = self.assertRaises(
++            emulators.ToolkitEmulatorException, self.toolbar.click_button,
++            'buttonName')
++        self.assertEqual(
++            error.message,
++            'Toolbar must be opened before calling click_button().')
++
  class TabsTestCase(tests.QMLStringAppTestCase):

Ubuntu UI Toolkit

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

Commit message

Description of the change

Preview Diff

Subscribers