Merge lp:~elopio/webbrowser-app/test_add_bookmark into lp:webbrowser-app

Proposed by Leo Arias
Status: Rejected
Rejected by: Olivier Tilloy
Proposed branch: lp:~elopio/webbrowser-app/test_add_bookmark
Merge into: lp:webbrowser-app
Prerequisite: lp:~elopio/webbrowser-app/copyright
Diff against target: 265 lines (+178/-4)
4 files modified
src/app/webbrowser/ActivityView.qml (+4/-2)
src/app/webbrowser/PageDelegate.qml (+2/-1)
tests/autopilot/webbrowser_app/emulators/browser.py (+132/-1)
tests/autopilot/webbrowser_app/tests/test_bookmarks.py (+40/-0)
To merge this branch: bzr merge lp:~elopio/webbrowser-app/test_add_bookmark
Reviewer Review Type Date Requested Status
Olivier Tilloy Disapprove
Leo Arias (community) Disapprove
PS Jenkins bot continuous-integration Needs Fixing
Javier Collado (community) Approve
Richard Huddie Pending
Víctor R. Ruiz Pending
Review via email: mp+205080@code.launchpad.net

Commit message

Added a test to add a bookmark.

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
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Javier Collado (javier.collado) wrote :

I've been able to run the test case successfully in my laptop. However, there
are a few minor problems that should be fixed:

- Indentation in the ActivityView class

$ flake8 browser.py
browser.py:146:6: E111 indentation is not a multiple of four
browser.py:147:10: E111 indentation is not a multiple of four
browser.py:150:10: E111 indentation is not a multiple of four
browser.py:152:6: E111 indentation is not a multiple of four
browser.py:153:10: E111 indentation is not a multiple of four
browser.py:154:10: E111 indentation is not a multiple of four
browser.py:156:6: E111 indentation is not a multiple of four
browser.py:157:10: E111 indentation is not a multiple of four
browser.py:158:10: E111 indentation is not a multiple of four

- URL formatting in exceptions:

%r should be replaced with {!r} to use .format (instead %), that is this:

raise BrowserEmulatorException(
    'The page with URL %r is not open.'.format(url))

shoud be written like this:

raise BrowserEmulatorException(
    'The page with URL {!r} is not open.'.format(url))

I still need to better understand the code, but these are the issues I've seen
so far.

review: Needs Fixing
Revision history for this message
Javier Collado (javier.collado) wrote :

I'm curious about the sorting of the PageDelegate objects. Why is it a good
choice to order them by the x coordinate?

In the test case being added by this request there's only one bookmark, so only
one PageDelegate, but I guess this is relevant for some other tests.

Finally, regading the sort itself, you might want to consider using the
attrgetter function as explained here:
https://wiki.python.org/moin/HowTo/Sorting#Operator_Module_Functions

That would turn this:
pages.sort(key=lambda page: page.x)

into this:
from operator import attrgetter
...
pages.sort(key=attrgetter('x'))

Not a big time saver for just one attribute, but is good to know if multiple
attributes are needed.

Revision history for this message
Leo Arias (elopio) wrote :

Thanks Javier!

> - Indentation in the ActivityView class
>
> $ flake8 browser.py

Fixed.

> - URL formatting in exceptions:

Fixed.

Revision history for this message
Leo Arias (elopio) wrote :

> I'm curious about the sorting of the PageDelegate objects. Why is it a good
> choice to order them by the x coordinate?
>
> In the test case being added by this request there's only one bookmark, so
> only
> one PageDelegate, but I guess this is relevant for some other tests.

Here I just copied an existing method to a better place. After this branch is reviewed, I'll propose another one that refactors the tests that use it, so I can remove the duplication. I forgot to mention about it, sorry.

There are some tests that use this method after adding an extra tab. So there are two pages, and we would like to know which one was added last. As they are in an horizontal list but we can't trust on the order of the objects returned by autopilot, the x coordinate tells us the order.

This needs to be implemented in the toolkit. We need a list iterator that returns the elements in the order they are displayed. But I'm blocked with 3 branches on the toolkit and 4 on autopilot, so that will have to wait a little.

> pages.sort(key=attrgetter('x'))

I didn't know about that. I made the change.

Thank you Javier!

Revision history for this message
Javier Collado (javier.collado) wrote :

Thanks for the updates.

I followed the code execution path and looked good to me. The only thing that
looks odd to me is:
self.main_window.open_activity() returns an ActivityView object
activity.open_bookmarks_tab() returns a BookmarksView object

but:
activity.open_activity_tab() returns a TimelineView object

I guess this would be less confusing if open_activity_tab method was renamed to
open_timeline_tab.

Aside from that, I ran this multiple times and the cleanup method worked fine,
so I'm fine with these changes.

review: Approve
Revision history for this message
Leo Arias (elopio) wrote :

Yes, I didn't find a good way to solve that. I'm trying to keep the user language, so all the methods are called as things the user can do with the UI. On the UI, there's nothing that says timeline. The timeline is the Activity tab inside the Activity page. So a user opens the activity page, and then can go either to the activity tab or to the browser tab.

I think the fact that's confusing while reading the tests means that it's confusing for an user.
Wouldn't it be better if we change activity tab label to say time line? I guess we need to ask designers about this.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:453
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/600/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty/2980
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty-touch/2712/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-trusty-amd64-ci/102
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-trusty-armhf-ci/102
        deb: http://jenkins.qa.ubuntu.com/job/webbrowser-app-trusty-armhf-ci/102/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-trusty-i386-ci/102
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-trusty/2619
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/2982
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/2982/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/2713
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/2713/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-mako/5136/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/3714

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/600/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Olivier Tilloy (osomon) wrote :

Is there a reason for this branch to depend on lp:~elopio/webbrowser-app/copyright?

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

34 + Tabs {

Tabs inside Tabs? That doesn’t look right…

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

> 34 + Tabs {
>
> Tabs inside Tabs? That doesn’t look right…

It's Tab elements inside a Tabs element. That's how they do it on the toolkit example:
http://bazaar.launchpad.net/~ubuntu-sdk-team/ubuntu-ui-toolkit/trunk/view/head:/modules/Ubuntu/Components/Tabs.qml

But more importantly, the Tabs element has a property that's the currently selected tab index. We need that in order to select a tab from autopilot.

Revision history for this message
Leo Arias (elopio) wrote :

> Is there a reason for this branch to depend on lp:~elopio/webbrowser-
> app/copyright?

Huh, I just wanted to avoid a possible conflict with myself while changing the copyright year on this branch. Then I forgot to update the headers.
Now I've done it, and it seems the branches will not conflict. But still, it doesn't hurt to keep the prerequisite, right?

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Olivier Tilloy (osomon) wrote :

> > 34 + Tabs {
> >
> > Tabs inside Tabs? That doesn’t look right…
>
> It's Tab elements inside a Tabs element. That's how they do it on the toolkit
> example:
> http://bazaar.launchpad.net/~ubuntu-sdk-team/ubuntu-ui-
> toolkit/trunk/view/head:/modules/Ubuntu/Components/Tabs.qml
>
> But more importantly, the Tabs element has a property that's the currently
> selected tab index. We need that in order to select a tab from autopilot.

What I meant is that the top-level item of ActivityView is already a Tabs, and you added another Tabs as a child, I don’t think this is correct (nor needed).

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

> > Is there a reason for this branch to depend on lp:~elopio/webbrowser-
> > app/copyright?
>
> Huh, I just wanted to avoid a possible conflict with myself while changing the
> copyright year on this branch. Then I forgot to update the headers.
> Now I've done it, and it seems the branches will not conflict. But still, it
> doesn't hurt to keep the prerequisite, right?

No it doesn’t hurt indeed, it was simply curious about it.

Revision history for this message
Leo Arias (elopio) wrote :

> What I meant is that the top-level item of ActivityView is already a Tabs, and
> you added another Tabs as a child, I don’t think this is correct (nor needed).

You are right! I must have been drunk.
Back to work in progress to fix it tomorrow.

Thanks for the review.

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

FAILED: Continuous integration, rev:456
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/612/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty/3676
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty-touch/3275/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-trusty-amd64-ci/114
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-trusty-armhf-ci/114
        deb: http://jenkins.qa.ubuntu.com/job/webbrowser-app-trusty-armhf-ci/114/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-trusty-i386-ci/114
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-trusty/3229
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/3681
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/3681/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/3277
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/3277/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-mako/5648/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4488

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/612/rebuild

review: Needs Fixing (continuous-integration)
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)
Revision history for this message
Leo Arias (elopio) wrote :

This is failing because of a bad timing coincidence.
I'm proposing a fix here:
https://code.launchpad.net/~elopio/ubuntu-ui-toolkit/tab_selection_timing/+merge/210219

This branch shouldn't land until that one is released. Other wise we might get errors like this one some times.

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)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Olivier Tilloy (osomon) wrote :

Thanks for improving the test coverage!
With the re-design of the browser app happening now, the entire activity view is going away, so I’m kind of reluctant to merging this now as it will have to be entirely rewritten very soon.
If there are general improvements that can be extracted from this branch and applied to the existing tests, I’d gladly take them in though.

Revision history for this message
Leo Arias (elopio) wrote :

I'll wait for the new bookmarks UI and adapt this branch then.

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

The new UI (phase 1) has landed, with a new way of managing bookmarks, and I made sure the functionality comes with autopilot tests from the start, so this branch isn’t needed anymore. Thanks Leo for your work on it.

review: Disapprove
Revision history for this message
Leo Arias (elopio) wrote :

Nice, thanks!

Unmerged revisions

459. By Leo Arias

Merged with trunk.

458. By Leo Arias

Removed the unnecesary override.

457. By Leo Arias

Fixed the Tabs mess.

456. By Leo Arias

Removed the extra Tabs.

455. By Leo Arias

Typo.

454. By Leo Arias

Updated copyright.

453. By Leo Arias

Use operator.attrgetter.

452. By Leo Arias

Fixed string format.

451. By Leo Arias

Fixed identation.

450. By Leo Arias

Rename.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/app/webbrowser/ActivityView.qml'
2--- src/app/webbrowser/ActivityView.qml 2014-03-12 22:45:12 +0000
3+++ src/app/webbrowser/ActivityView.qml 2014-04-25 02:58:59 +0000
4@@ -1,5 +1,5 @@
5 /*
6- * Copyright 2013 Canonical Ltd.
7+ * Copyright 2013, 2014 Canonical Ltd.
8 *
9 * This file is part of webbrowser-app.
10 *
11@@ -21,6 +21,7 @@
12
13 Tabs {
14 id: activityView
15+ objectName: "activityViewTabs"
16
17 property alias tabsModel: timelineView.tabsModel
18 property alias historyModel: timelineView.historyModel
19@@ -34,6 +35,7 @@
20
21 Tab {
22 title: i18n.tr("Activity")
23+ objectName: "activityTab"
24 page: Page {
25 TimelineView {
26 id: timelineView
27@@ -49,9 +51,9 @@
28 }
29 }
30 }
31-
32 Tab {
33 title: i18n.tr("Bookmarks")
34+ objectName: "bookmarksTab"
35 page: Page {
36 BookmarksView {
37 id: bookmarksView
38
39=== modified file 'src/app/webbrowser/PageDelegate.qml'
40--- src/app/webbrowser/PageDelegate.qml 2014-03-12 22:45:12 +0000
41+++ src/app/webbrowser/PageDelegate.qml 2014-04-25 02:58:59 +0000
42@@ -1,5 +1,5 @@
43 /*
44- * Copyright 2013 Canonical Ltd.
45+ * Copyright 2013, 2014 Canonical Ltd.
46 *
47 * This file is part of webbrowser-app.
48 *
49@@ -65,6 +65,7 @@
50
51 Image {
52 id: starIcon
53+ objectName: "bookmarkImage"
54 source: pageDelegate.bookmarked ? "assets/browser_favourite_on.png"
55 : "assets/browser_favourite_off.png"
56 visible: pageDelegate.canBookmark
57
58=== modified file 'tests/autopilot/webbrowser_app/emulators/browser.py'
59--- tests/autopilot/webbrowser_app/emulators/browser.py 2014-02-06 04:31:01 +0000
60+++ tests/autopilot/webbrowser_app/emulators/browser.py 2014-04-25 02:58:59 +0000
61@@ -1,6 +1,6 @@
62 # -*- Mode: Python; coding: utf-8; indent-tabs-mode: nil; tab-width: 4 -*-
63 #
64-# Copyright 2013 Canonical
65+# Copyright 2013, 2014 Canonical
66 #
67 # This program is free software: you can redistribute it and/or modify it
68 # under the terms of the GNU General Public License version 3, as published
69@@ -14,9 +14,21 @@
70 # You should have received a copy of the GNU General Public License
71 # along with this program. If not, see <http://www.gnu.org/licenses/>.
72
73+import logging
74+import operator
75+
76+from autopilot import logging as autopilot_logging
77+
78 from ubuntuuitoolkit import emulators as uitk
79
80
81+logger = logging.getLogger(__name__)
82+
83+
84+class BrowserEmulatorException(uitk.ToolkitEmulatorException):
85+ """Exception raised when there is an error with the emulator."""
86+
87+
88 class Panel(uitk.Toolbar):
89 pass
90
91@@ -27,6 +39,15 @@
92 An emulator class that makes it easy to interact with the webbrowser app.
93 """
94
95+ def get_tabs(self):
96+ """Return the Tabs emualtor of the MainView.
97+
98+ Overriden because on the browser the Tabs container gets the
99+ ActivityView name.
100+
101+ """
102+ return self.select_single(ActivityView)
103+
104 def get_toolbar(self):
105 # Overridden since the browser doesn’t use the MainView’s Toolbar.
106 return self.select_single(Panel)
107@@ -101,3 +122,113 @@
108 tabs = view.select_many("PageDelegate", objectName="openTabDelegate")
109 tabs.sort(key=lambda tab: tab.x)
110 return tabs
111+
112+ @autopilot_logging.log_action(logger.info)
113+ def open_activity(self):
114+ """Open the Activity view."""
115+ if self.activityViewVisible:
116+ logger.debug('The activity view is already opened.')
117+ else:
118+ toolbar = self.open_toolbar()
119+ toolbar.click_button('activityButton')
120+ self.activityViewVisible.wait_for(True)
121+ return self.select_single(ActivityView)
122+
123+
124+class ActivityView(uitk.Tabs):
125+
126+ def __init__(self, *args):
127+ super(ActivityView, self).__init__(*args)
128+ # XXX we need a better way to keep reference to the main view.
129+ # --elopio - 2014-01-31
130+ self.main_view = self.get_root_instance().select_single(Browser)
131+
132+ def open_activity_tab(self):
133+ self.main_view.switch_to_tab('activityTab')
134+ return self.select_single(TimelineView)
135+
136+ def open_bookmarks_tab(self):
137+ self.main_view.switch_to_tab('bookmarksTab')
138+ return self.select_single(BookmarksView)
139+
140+
141+class TimelineView(uitk.UbuntuUIToolkitEmulatorBase):
142+
143+ @autopilot_logging.log_action(logger.info)
144+ def bookmark_page(self, url):
145+ """Add a page to the bookmarks.
146+
147+ :parameter url: The URL of the page to bookmark.
148+ :raises BrowserEmulatorException: if the page is not in the currently
149+ viewing list.
150+
151+ """
152+ pages = self._get_currently_viewing_page_delegates()
153+ for page in pages:
154+ if page.url == url:
155+ page.bookmark()
156+ break
157+ else:
158+ raise BrowserEmulatorException(
159+ 'The page with URL {!r} is not open.'.format(url))
160+
161+ def _get_currently_viewing_page_delegates(self):
162+ tabs_list = self.select_single('TabsList')
163+ pages = tabs_list.select_many(
164+ PageDelegate, objectName='openTabDelegate')
165+ pages.sort(key=lambda page: page.x)
166+ return pages
167+
168+
169+class PageDelegate(uitk.UbuntuUIToolkitEmulatorBase):
170+
171+ def bookmark(self):
172+ """Add this page to the bookmarks.
173+
174+ :raises BrowserEmulatorException: if the page is already bookmarked.
175+
176+ """
177+ if self.bookmarked:
178+ raise BrowserEmulatorException(
179+ 'The page with URL {!r} is already bookmarked.'.format(
180+ self.url))
181+ else:
182+ bookmark_image = self.select_single(
183+ 'QQuickImage', objectName='bookmarkImage')
184+ self.pointing_device.click_object(bookmark_image)
185+ self.bookmarked.wait_for(True)
186+
187+ def remove_bookmark(self):
188+ """Remove this page from the bookmarks.
189+
190+ :raises BrowserEmulatorException: if the page is not bookmarked.
191+
192+ """
193+ if not self.bookmarked:
194+ raise BrowserEmulatorException(
195+ 'The page with URL {!r} is not bookmarked.'.format(self.url))
196+ else:
197+ bookmark_image = self.select_single(
198+ 'QQuickImage', objectName='bookmarkImage')
199+ self.pointing_device.click_object(bookmark_image)
200+ # We can't just wait for the bookmarked value to be False, because
201+ # on the Bookmarks view the element is removed. The check is left
202+ # to the caller.
203+
204+
205+class BookmarksView(uitk.UbuntuUIToolkitEmulatorBase):
206+
207+ def get_bookmarked_pages(self):
208+ """Return a list with the URLs of the bookmarked pages."""
209+ pages = self._get_bookmarked_page_delegates()
210+ return [page.url for page in pages]
211+
212+ def _get_bookmarked_page_delegates(self):
213+ pages = self.select_many(PageDelegate)
214+ pages.sort(key=operator.attrgetter('x'))
215+ return pages
216+
217+ def remove_all_bookmarks(self):
218+ for page in self._get_bookmarked_page_delegates():
219+ page.remove_bookmark()
220+ page.wait_until_destroyed()
221
222=== added file 'tests/autopilot/webbrowser_app/tests/test_bookmarks.py'
223--- tests/autopilot/webbrowser_app/tests/test_bookmarks.py 1970-01-01 00:00:00 +0000
224+++ tests/autopilot/webbrowser_app/tests/test_bookmarks.py 2014-04-25 02:58:59 +0000
225@@ -0,0 +1,40 @@
226+# -*- Mode: Python; coding: utf-8; indent-tabs-mode: nil; tab-width: 4 -*-
227+#
228+# Copyright 2014 Canonical
229+#
230+# This program is free software: you can redistribute it and/or modify it
231+# under the terms of the GNU General Public License version 3, as published
232+# by the Free Software Foundation.
233+#
234+# This program is distributed in the hope that it will be useful,
235+# but WITHOUT ANY WARRANTY; without even the implied warranty of
236+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
237+# GNU General Public License for more details.
238+#
239+# You should have received a copy of the GNU General Public License
240+# along with this program. If not, see <http://www.gnu.org/licenses/>.
241+
242+from webbrowser_app import tests
243+
244+from testtools.matchers import Contains
245+
246+
247+class BookmarksTestCase(tests.StartOpenRemotePageTestCaseBase):
248+
249+ def setUp(self):
250+ super(BookmarksTestCase, self).setUp()
251+ self.addCleanup(self.remove_all_bookmarks)
252+
253+ def remove_all_bookmarks(self):
254+ activity = self.main_window.open_activity()
255+ bookmarks_tab = activity.open_bookmarks_tab()
256+ bookmarks_tab.remove_all_bookmarks()
257+
258+ def test_bookmark_page_must_add_it_to_bookmarks_tab(self):
259+ activity = self.main_window.open_activity()
260+ activity_tab = activity.open_activity_tab()
261+ activity_tab.bookmark_page(self.url)
262+
263+ bookmarks_tab = activity.open_bookmarks_tab()
264+ self.assertThat(
265+ bookmarks_tab.get_bookmarked_pages(), Contains(self.url))

Subscribers

People subscribed via source and target branches

to all changes:
to status/vote changes: