unity-scope-click

Merge lp:~canonical-platform-qa/unity-scope-click/wait_for_search_button into lp:unity-scope-click

wait_for_search_button
Merge into trunk

Proposed by Brendan Donegan on 2015-05-01

Status:

Merged

Approved by:

dobey on 2015-05-06

Approved revision:

322

Merged at revision:

321

Proposed branch:

lp:~canonical-platform-qa/unity-scope-click/wait_for_search_button

Merge into:

lp:unity-scope-click

Diff against target:

43 lines (+12/-4)

1 file modified

tests/autopilot/unityclickscope/__init__.py (+12/-4)

To merge this branch:

bzr merge lp:~canonical-platform-qa/unity-scope-click/wait_for_search_button

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	continuous-integration		Approve on 2015-05-06
dobey (community)		2015-05-01	Approve on 2015-05-06
Vincent Ladeuil (community)			Approve on 2015-05-06
Review via email: mp+258002@code.launchpad.net

Commit Message

Wait for the search button to be available/enabled before trying to click it in the enter_search_query AP helper

Description of the Change

In our sanity tests we sometimes find that our test to install a free application fails because it can't find the search box - this seems likely to be because of a timing issue which means the button hasn't been clicked. Changing a select_single to wait_select_single to see if it helps.

lp:~canonical-platform-qa/unity-scope-click/wait_for_search_button updated on 2015-05-01

319. By Brendan Donegan on 2015-05-01: Wait for search button to be available to avoid the possibility it might not be

PS Jenkins bot (ps-jenkins) wrote on 2015-05-01:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scope-click-ci/586/rebuild

review: Approve (continuous-integration)

dobey (dobey) wrote on 2015-05-01:

Should not all of the select_single() calls be changed to wait_select_single()?

review: Needs Information

Brendan Donegan (brendan-donegan) wrote on 2015-05-01:

That certainly wouldn't do any harm (in fact I'm of the opinion that select_single should be mapped to wait_select_single in Autopilot)

lp:~canonical-platform-qa/unity-scope-click/wait_for_search_button updated on 2015-05-05

320. By Brendan Donegan on 2015-05-05: Wait for the search button to be enabled and use wait_select across the board

Brendan Donegan (brendan-donegan) wrote on 2015-05-05:

Turns out I needed to wait for the button to be enabled as well. I updated the other select_singles to as requested by Rodney

PS Jenkins bot (ps-jenkins) wrote on 2015-05-05:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scope-click-ci/587/rebuild

review: Approve (continuous-integration)

Leo Arias (elopio) wrote on 2015-05-05:

On Fri, May 1, 2015 at 8:10 AM, Brendan Donegan <
<email address hidden>> wrote:

> That certainly wouldn't do any harm (in fact I'm of the opinion that
> select_single should be mapped to wait_select_single in Autopilot)
>

I disagree with this. I dislike wait_select_single because generally it
means we don't understand what's happening on the screen. What we should do
in most of the cases is to wait for a loading image to disappear or an
animation to stop. If we use the wait always we will miss some bugs, like:
it takes 10 seconds for a button to appear without any indication on the
screen that the application is not stuck.
Sometimes it's valid to use it of course.

dobey (dobey) wrote on 2015-05-05:

On Tue, 2015-05-05 at 14:27 +0000, Leo Arias wrote:
> On Fri, May 1, 2015 at 8:10 AM, Brendan Donegan <
> <email address hidden>> wrote:
>
> > That certainly wouldn't do any harm (in fact I'm of the opinion that
> > select_single should be mapped to wait_select_single in Autopilot)
> >
>
> I disagree with this. I dislike wait_select_single because generally it
> means we don't understand what's happening on the screen. What we should do
> in most of the cases is to wait for a loading image to disappear or an
> animation to stop. If we use the wait always we will miss some bugs, like:
> it takes 10 seconds for a button to appear without any indication on the
> screen that the application is not stuck.
> Sometimes it's valid to use it of course.

We can't possibly understand what's happening on the screen. Neither the
app nor autopilot owns the framebuffer. We should always do
wait_select_single in all cases. Maybe the 10 second timeout is too long
for a lot of actions, but that doesn't mean we should expect some piece
of the UI to be immediately available. Many things will affect how
quickly things happen, and the app itself has very little control over
how quickly that will happen.

There have been times when I've seen even 10 seconds not be enough time,
when running autopilot tests, for the expected thing to happen, which
normally happens very quickly in live usage of an app on a phone, and
that was with using wait_select_single.

There is just no way for any app to be able to guarantee the speed at
which the UI is drawn by other things; especially in the case of scopes,
where the UI is all entirely in a separate app and process anyway.

lp:~canonical-platform-qa/unity-scope-click/wait_for_search_button updated on 2015-05-06

321. By Brendan Donegan on 2015-05-06: Wait for page header to be totally visible

Brendan Donegan (brendan-donegan) wrote on 2015-05-06:

Pretty sure I found the magic property now. After 20 runs I got no failures related to the original issue (there were 2 related to CPO inheritance and 1 'mystery' failure)

Brendan Donegan (brendan-donegan) wrote on 2015-05-06:

I left the wait_selects in for now. I'm inclined to agree with dobey but Leo has a good point as well.

Vincent Ladeuil (vila) wrote on 2015-05-06:

PageHeader.... nice catch !

I think that if Leo is correct, waiting for the page header would allow coming back to select_single instead of wait_select_single to demonstrate that the wait_select_single calls were workarounds.

But up to you, I'm fine landing as is.

review: Approve

Vincent Ladeuil (vila) wrote on 2015-05-06:

>>>>> Rodney Dawes <email address hidden> writes:

    > On Tue, 2015-05-05 at 14:27 +0000, Leo Arias wrote:
    >> On Fri, May 1, 2015 at 8:10 AM, Brendan Donegan <
    >> <email address hidden>> wrote:
    >>
    >> > That certainly wouldn't do any harm (in fact I'm of the opinion that
    >> > select_single should be mapped to wait_select_single in Autopilot)
    >> >
    >>
    >> I disagree with this. I dislike wait_select_single because generally it
    >> means we don't understand what's happening on the screen. What we should do
    >> in most of the cases is to wait for a loading image to disappear or an
    >> animation to stop. If we use the wait always we will miss some bugs, like:
    >> it takes 10 seconds for a button to appear without any indication on the
    >> screen that the application is not stuck.
    >> Sometimes it's valid to use it of course.

> We can't possibly understand what's happening on the screen.

Famous last words ;-)

I sincerely hope we, as humans, can understand how things work and find
the right spot to confirm the *app* has reach a specific state instead
of checking for all *widgets* to reach a specific state. That's one key
point of the page object model right ?

Vincent

PS Jenkins bot (ps-jenkins) wrote on 2015-05-06:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scope-click-ci/588/rebuild

review: Approve (continuous-integration)

Leo Arias (elopio) wrote on 2015-05-06:

8- store_scope.isCurrent.wait_for(True)

I wouldn't remove this one. I would leave the two waits.

About the waits, I'm also ok leaving them. I don't fully disagree with dobey, because I'm talking about an ideal scenario where design gives us details about each transition. Every time there's a transition that takes 1 second or more, there should be an indication on the screen. We wait for that indication, and afterwards we know the screen is ready and we don't need to wait for anything else.
So I'd say we can't possibly understand what's happening on the screen *now*

Brendan Donegan (brendan-donegan) wrote on 2015-05-06:

Actually it has next to no value - isCurrent becomes True immediately when
the animation starts so doesn't correspond with the button being ready to
click. I suppose it doesn't do any harm though

On Wed, May 6, 2015 at 3:54 PM, Leo Arias <email address hidden> wrote:

> 8- store_scope.isCurrent.wait_for(True)
>
> I wouldn't remove this one. I would leave the two waits.
>
> About the waits, I'm also ok leaving them. I don't fully disagree with
> dobey, because I'm talking about an ideal scenario where design gives us
> details about each transition. Every time there's a transition that takes 1
> second or more, there should be an indication on the screen. We wait for
> that indication, and afterwards we know the screen is ready and we don't
> need to wait for anything else.
> So I'd say we can't possibly understand what's happening on the screen
> *now*
> --
>
> https://code.launchpad.net/~canonical-platform-qa/unity-scope-click/wait_for_search_button/+merge/258002
> Your team Canonical Platform QA Team is subscribed to branch
> lp:~canonical-platform-qa/unity-scope-click/wait_for_search_button.
>

lp:~canonical-platform-qa/unity-scope-click/wait_for_search_button updated on 2015-05-06

322. By Brendan Donegan on 2015-05-06: Final attempt using the store_scope globalRect.y attribute

dobey (dobey) on 2015-05-06:

review: Approve

PS Jenkins bot (ps-jenkins) wrote on 2015-05-06:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scope-click-ci/590/rebuild

review: Approve (continuous-integration)

Brendan Donegan (brendan-donegan) wrote on 2015-05-06:

Okay I'm quite confident this does the right thing now.

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:

Canonical Platform QA Team

Unity API Team

1	=== modified file 'tests/autopilot/unityclickscope/__init__.py'
2	--- tests/autopilot/unityclickscope/__init__.py 2014-11-28 18:52:47 +0000
3	+++ tests/autopilot/unityclickscope/__init__.py 2015-05-06 18:11:01 +0000
4	@@ -18,6 +18,8 @@
5
6	import autopilot.logging
7	from autopilot import exceptions, introspection
8	+from autopilot.matchers import Eventually
9	+from testtools.matchers import Equals
10	from unity8.shell.emulators import dash
11
12
13	@@ -62,6 +64,9 @@
14	store_scope = self.get_root_instance().select_single(
15	'GenericScopeView', objectName='dashTempScopeItem')
16	store_scope.isCurrent.wait_for(True)
17	+ # The store scope slides from the right. Wait until it has finished
18	+ # sliding before trying to press the search button
19	+ Eventually(Equals(0)).match(lambda: store_scope.globalRect.y)
20	return store_scope
21
22	def _swipe_to_bottom(self):
23	@@ -89,13 +94,16 @@
24	# XXX the enter_search_query of the dash provided by unity doesn't
25	# work for the temp store scope.
26	# TODO file a bug. --elopio - 2014-11-28
27	- search_button = self.select_single(objectName='search_header_button')
28	+ search_button = self.wait_select_single(
29	+ objectName='search_header_button')
30	self.pointing_device.click_object(search_button)
31	- headerContainer = self.select_single(objectName='headerContainer')
32	+ headerContainer = self.wait_select_single(
33	+ objectName='headerContainer')
34	headerContainer.contentY.wait_for(0)
35	- search_text_field = self.select_single(objectName='searchTextField')
36	+ search_text_field = self.wait_select_single(
37	+ objectName='searchTextField')
38	search_text_field.write(query)
39	- self.get_root_instance().select_single(
40	+ self.get_root_instance().wait_select_single(
41	objectName='processingIndicator').visible.wait_for(False)
42
43	def open_preview(self, category, app_name):