1074117 - Use expected conditions where appropriate and optimise element lookups

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::UI Tests, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

We have expected conditions, which can make the syntax for a Marionette wait easier to parse. Alongside this change we should optimise our current element lookups as demonstrated below. This is a low priority task, and it may be worth waiting for bug 1015242 to be addressed.

Current syntax waiting for an element to be displayed:

Wait(m, ignored_exceptions=NoSuchElementException).until(m.find_element(by, locator).is_displayed())

Syntax with lookup optimised and expected conditions:

el = Wait(m).until(expected.element_present(by, locator))
Wait(m).until(expected.element_displayed(element))

The first example will repeatedly lookup the element even after it's been found while it checks the visibility. The second will store the element once it's discovered and then only check the element for visibility without further lookups.

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Camera

Doing this in one go will be a large amount of work and will require a large review and testing effort. Let's take each app object as a separate patch. This patch is for the Camera app object. The final patch should be able to remove the majority of wait_for_ methods in the test class and base page class.

I did a bit of analysis over the changes in this patch. This reduces the number of method calls by 12,337 (~4%) and specifically reduces the number of times we call find_element by ~14%. While the impact on total time is difficult to measure, it's likely to be a positive (if negligible) effect.

Note also that we'll need to ensure future patches use the Wait class and expected conditions so that we don't revert to the less optimal approach currently in place.

Comment 2

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Calendar

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Clock

Comment 5

[Florin Strugariu [:Bebe]]

Comment on attachment 8516638 [details] [review]
Camera

https://github.com/mozilla-b2g/gaia/commit/c704e65f50870cbe49edd080312f86ca851c7700

Comment 6

[Florin Strugariu [:Bebe]]

Comment on attachment 8516683 [details] [review]
Calendar

https://github.com/mozilla-b2g/gaia/commit/ea9438b0ea56c14318c2873dc9e8ccccdaf2c994

Comment 7

[Florin Strugariu [:Bebe]]

Comment on attachment 8516975 [details] [review]
Clock

ate my gaia repo a couple of hours ago

Comment 8

[Florin Strugariu [:Bebe]]

Comment on attachment 8516975 [details] [review]
Clock

Wrong copy/paste Sorry

https://github.com/mozilla-b2g/gaia/commit/bf8d513adab5c5bbd9ddd144cfdfd2aca3858544

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Contacts

I haven't been able to run test_sms_contact as I don't have an active SIM in my device. Every other contacts test is passing for me though.

Comment 10

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Patches that have been merged are not obsolete.

Comment 11

[Florin Strugariu [:Bebe]]

Comment on attachment 8520641 [details]
Contacts

treeherder tests are failing

Comment 12

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8520641 [details]
Contacts

The single failure is a known intermittent, and it didn't reproduce on a retrigger.

Comment 13

[Florin Strugariu [:Bebe]]

Comment on attachment 8520641 [details]
Contacts

Merged in https://github.com/mozilla-b2g/gaia/commit/c98c9c68fa914b379a1b1beabc63389bece5c9c1

Comment 14

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Cost control

Comment 15

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Email

Note that this does not resolve bug 1090281, which I have seen locally whilst testing.

Comment 16

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Emergency call

Comment 17

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: FM radio

Comment 18

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: First time use

Comment 19

[Florin Strugariu [:Bebe]]

Comment on attachment 8524485 [details] [review]
Emergency call

https://github.com/mozilla-b2g/gaia/commit/5fd781a5d3f80b94dce8d17a1ab5b3a3601045ff

Comment 20

[Florin Strugariu [:Bebe]]

Comment on attachment 8524510 [details] [review]
FM radio

https://github.com/mozilla-b2g/gaia/commit/ae8083210d21edffdbd014c86cc06b71d08c9c3a

Comment 21

[Florin Strugariu [:Bebe]]

Comment on attachment 8524415 [details] [review]
Cost control

https://github.com/mozilla-b2g/gaia/commit/e9b1a8c222bf674cbf960015832a306bacfe3000

Comment 22

[Florin Strugariu [:Bebe]]

Comment on attachment 8524513 [details] [review]
First time use

https://github.com/mozilla-b2g/gaia/commit/b32e14bc9bc0483ee26b785d75f08f7d1c8ff59a

Comment 23

[Florin Strugariu [:Bebe]]

Comment on attachment 8524479 [details] [review]
Email

merged https://github.com/mozilla-b2g/gaia/commit/0da467aba9ecf354b905f9cc08dfa1a4169659e4

Comment 24

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Gallery

Comment 25

[Florin Strugariu [:Bebe]]

Comment on attachment 8528329 [details] [review]
Gallery

merged in https://github.com/mozilla-b2g/gaia/commit/625239c492bc7c7e00db815e15448fbfe87f983c

Comment 26

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Homescreen

Please note that I have been unable to run the tests that install an app due an issue reaching my local web server from the device. All other homescreen tests are passing for me.

Comment 27

[Florin Strugariu [:Bebe]]

Comment on attachment 8529193 [details] [review]
Homescreen

https://github.com/mozilla-b2g/gaia/commit/609f3985df16ac2505a48a699ae9406b978ac03c

Comment 28

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Keyboard

Comment 29

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Lockscreen

Comment 30

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Messages

I've not been able to run the tests as I'm traveling and they require a carrier. I have triggered an adhoc build in Jenkins though: http://jenkins1.qa.scl3.mozilla.com:8080/view/UI/job/flame-kk.ui.adhoc/371/

Comment 31

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Dave Hunt (:davehunt) from comment #30)
> Created attachment 8530894 [details] [review]
> Messages
> 
> I've not been able to run the tests as I'm traveling and they require a
> carrier. I have triggered an adhoc build in Jenkins though:
> http://jenkins1.qa.scl3.mozilla.com:8080/view/UI/job/flame-kk.ui.adhoc/371/

I've fixed an issue, rebased, and pushed. New adhoc: http://jenkins1.qa.scl3.mozilla.com:8080/view/UI/job/flame-kk.ui.adhoc/372/

Comment 32

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Dave Hunt (:davehunt) from comment #30)
> Created attachment 8530894 [details] [review]
> Messages
> 
> I've not been able to run the tests as I'm traveling and they require a
> carrier. I have triggered an adhoc build in Jenkins though:
> http://jenkins1.qa.scl3.mozilla.com:8080/view/UI/job/flame-kk.ui.adhoc/371/

I've fixed an issue, rebased, and pushed. New adhoc: http://jenkins1.qa.scl3.mozilla.com:8080/view/UI/job/flame-kk.ui.adhoc/373/

Comment 33

[Florin Strugariu [:Bebe]]

Comment on attachment 8530884 [details] [review]
Keyboard

https://github.com/mozilla-b2g/gaia/commit/d5b610ecce6ccd791d4275ff730f6af717d08b9b

Comment 34

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Dave Hunt (:davehunt) from comment #30)
> Created attachment 8530894 [details] [review]
> Messages
> 
> I've not been able to run the tests as I'm traveling and they require a
> carrier. I have triggered an adhoc build in Jenkins though:
> http://jenkins1.qa.scl3.mozilla.com:8080/view/UI/job/flame-kk.ui.adhoc/371/

I've fixed an issue, rebased, and pushed. New adhoc: http://jenkins1.qa.scl3.mozilla.com:8080/view/UI/job/flame-kk.ui.adhoc/377/

Comment 35

[Florin Strugariu [:Bebe]]

Comment on attachment 8530893 [details] [review]
Lockscreen

LGTM

Comment 36

[Florin Strugariu [:Bebe]]

Comment on attachment 8530893 [details] [review]
Lockscreen

https://github.com/mozilla-b2g/gaia/commit/1dee279381fbc188fa1f083d8f17c20f8c3af07d

Comment 37

[Florin Strugariu [:Bebe]]

Comment on attachment 8530894 [details] [review]
Messages

LGTM

Tested this locally and it works OK

Comment 38

[Florin Strugariu [:Bebe]]

Comment on attachment 8530894 [details] [review]
Messages

merged in https://github.com/mozilla-b2g/gaia/commit/12606843268c9c0478d67388cd6fb6f86eb6d923

Comment 39

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Music

Comment 40

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Persona

Comment 41

[Florin Strugariu [:Bebe]]

Comment on attachment 8534934 [details] [review]
Music

merged in https://github.com/mozilla-b2g/gaia/commit/b0e5a07bd0d96764fdce566ca13cb4395c69bbaf

Comment 42

[Florin Strugariu [:Bebe]]

Comment on attachment 8534936 [details] [review]
Persona

merged in: https://github.com/mozilla-b2g/gaia/commit/35f2f7dbecd05f1ee0ffca3994a894a1c90ad2ef

Comment 43

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Phone

The DSDS tests were failing for me locally, but then I only have one SIM.

Comment 44

[Florin Strugariu [:Bebe]]

Comment on attachment 8535243 [details] [review]
Phone

some dialer tests are failing 
see: http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/436/console

Comment 45

[Martijn Wargers (dead)]

I guess this has to be:
https://github.com/mozilla-b2g/gaia/pull/26724/files#diff-193d67b00241c0a135318265c7b61febR102
+ Wait(self.marionette).until(call.is_displayed() and contact.text != u'')

Comment 46

[Martijn Wargers (dead)]

(In reply to Dave Hunt (:davehunt) from comment #39)
> Created attachment 8534934 [details] [review]
> Music

While testing one of the music testcase, I fail on this line:
 Wait(self.marionette).until(expected.element_not_displayed(
+ *self._loading_spinner_locator))

I think element_not_displayed takes an element as an argument, not a locator, right? I'm surprised other people don't get the same failure as me for this.

Comment 47

[Martijn Wargers (dead)]

Also a problem in the wait_for_music_tiles_displayed function.

Comment 48

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #46)
> (In reply to Dave Hunt (:davehunt) from comment #39)
> > Created attachment 8534934 [details] [review]
> > Music
> 
> While testing one of the music testcase, I fail on this line:
>  Wait(self.marionette).until(expected.element_not_displayed(
> + *self._loading_spinner_locator))
> 
> I think element_not_displayed takes an element as an argument, not a
> locator, right? I'm surprised other people don't get the same failure as me
> for this.

Please update to the latest Marionette client. The element visibility checks were updated in bug  	1015242 to accept locators.

Comment 49

[Martijn Wargers (dead)]

Attachment: System

I did the rewrite of the system app. Locally, the tests in system/ are working with these changes.

Comment 50

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8544849 [details] [review]
System

Thanks Martijn. A few of these waits can be improved now that we can combine present/visible checking. Also, when checking that an element is not displayed after an action we should ideally first assert that it is displayed before that action.

Comment 51

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8535243 [details] [review]
Phone

I believe I've fixed the issues. New adhoc run: http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/559

Comment 52

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Search

Adhoc: http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/561/

Comment 53

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Dave Hunt (:davehunt) from comment #51)
> Comment on attachment 8535243 [details] [review]
> Phone
> 
> I believe I've fixed the issues. New adhoc run:
> http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/559

There are a couple of failures, but I also see these in the other Jenkins jobs.

Comment 54

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Dave Hunt (:davehunt) from comment #52)
> Adhoc: http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/561/

The failures appear to be unrelated.

Comment 55

[Florin Strugariu [:Bebe]]

Comment on attachment 8545322 [details] [review]
Search

Merged in https://github.com/mozilla-b2g/gaia/commit/165833be6075194e9d75dc6da97d5ed16b529878

Comment 56

[Martijn Wargers (dead)]

Comment on attachment 8544849 [details] [review]
System

Ok, I think I addressed all of your review comments.

Comment 57

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8544849 [details] [review]
System

A few minor fixes left, and I found a use of wait_for_element_not_displayed in UtilityTray.

Comment 58

[Florin Strugariu [:Bebe]]

Comment on attachment 8535243 [details] [review]
Phone

Looks ok to me
The fails are not related with the pull

Comment 59

[Florin Strugariu [:Bebe]]

Comment on attachment 8535243 [details] [review]
Phone

Dave can you update the pull it has merge conflicts 
Thanks!

Comment 60

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Florin Strugariu [:Bebe] from comment #59)
> Comment on attachment 8535243 [details] [review]
> Phone
> 
> Dave can you update the pull it has merge conflicts 
> Thanks!

Updated but due to the changes probably worth another review and adhoc run.

Comment 61

[Martijn Wargers (dead)]

(In reply to Dave Hunt (:davehunt) from comment #57)
> Comment on attachment 8544849 [details] [review]
> System
> 
> A few minor fixes left, and I found a use of wait_for_element_not_displayed
> in UtilityTray.

Thanks for the review, just updated the PR with your review comments, sorry for the delay.

Comment 62

[Martijn Wargers (dead)]

Comment on attachment 8544849 [details] [review]
System

Updated PR again.

Comment 63

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8544849 [details] [review]
System

Still a couple of outstanding issues. Also, it's easier for the reviewer if you do not rebase/squash while a pull request is still in review. If you push additional commits it's much clearer to see what has been addressed from previous reviews. Rebasing/squashing can then happen after the pull has passed review.

Comment 64

[Martijn Wargers (dead)]

Comment on attachment 8544849 [details] [review]
System

Ok, like this then?

Comment 65

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8544849 [details] [review]
System

Failures in gaia try: https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=6e847c8624bb

Caused by a missed import of expected, and passing an element to element_not_present when a locator is needed.

I've also triggered a full adhoc:
http://jenkins1.qa.scl3.mozilla.com/view/UI/job/flame-kk.ui.adhoc/604/

Comment 66

[Florin Strugariu [:Bebe]]

Comment on attachment 8535243 [details] [review]
Phone

Tested this locally and works as expected.

Comment 67

[Florin Strugariu [:Bebe]]

Comment on attachment 8535243 [details] [review]
Phone

https://github.com/mozilla-b2g/gaia/commit/3060d16e5abd0f5a01a2b72c350be6aceb5fba46

Comment 68

[Martijn Wargers (dead)]

Attachment: systemapp2

Sorry, I updated my master branch and updated the pull request with that updated branch. This should have fixed the failures in comment 65:
https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=d51315b1f5c0

Comment 69

[Martijn Wargers (dead)]

Comment on attachment 8553106 [details] [review]
systemapp2

Treeherder try is green with this.

Comment 70

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8553106 [details] [review]
systemapp2

Unfortunately the imports changed while this patch has been in progress. We need to make sure they're in a try/except block so that we can gracefully switch to the marionette driver package.

Comment 71

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Settings

Comment 72

[Florin Strugariu [:Bebe]]

Comment on attachment 8555808 [details] [review]
Settings

all looks OK

Comment 73

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Florin Strugariu [:Bebe] from comment #72)
> Comment on attachment 8555808 [details] [review]

Landed in:
https://github.com/mozilla-b2g/gaia/commit/a0529167d625c8a99d58ff249b12b98bd2aff973

Comment 74

[GitHub Autolander Bot]

Attachment: System

Comment 75

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8558644 [details] [review]
System

Looks good, thanks. Landed in:
https://github.com/mozilla-b2g/gaia/commit/3912b4b5594688758c248f825020ed184df1e69f

Comment 76

[Martijn Wargers (dead)]

Dave, what needs to be still done for this bug to get resolved?

Comment 77

[Dave Hunt [:davehunt] [he/him] ⌚BST]

The follow app objects still need to be updated:
* testapp
* ui_tests
* ui_tests_privileged
* videoplayer
* wallpaper

Following that, we should remove any remaining uses of the wait_for methods in the tests and base classes. Something that might speed this up is the removal of the endurance tests - I believe they're no longer being run/maintained.

This is not a high priority task. I'm happy for it to remain assigned to me and I'll submit patches as and when I can. I typically work on these patches while commuting to the London office, and I've not been in for a few weeks.

Comment 78

[Martijn Wargers (dead)]

(In reply to Dave Hunt (:davehunt) from comment #77)
> Something that might speed this up is the
> removal of the endurance tests - I believe they're no longer being
> run/maintained.

That's bug 1117144. Thanks for the info.

Comment 79

[GitHub Autolander Bot]

Attachment: Test container

Comment 80

[GitHub Autolander Bot]

Attachment: UI tests

Comment 81

[Martijn Wargers (dead)]

Comment on attachment 8579311 [details] [review]
Test container

The changes looke fine to me.
Also tested test_system_message.py locally on device and desktop and it still passes with these changes.

Comment 82

[Martijn Wargers (dead)]

Comment on attachment 8579314 [details] [review]
UI tests

The changes look fine to me. There is 1 obvious mistake you have in the patch which caused some local failures. Other than that, it seems fine to me.
I tested on device: test_privileged_app_geolocation_prompt.py, keyboard tests, test_a11y_keyboard_key_names.py and system tests.

Comment 83

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8579314 [details] [review]
UI tests

Fixed the copy/paste issue.

Comment 84

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #81)
> Comment on attachment 8579311 [details] [review]
> Test container
> 
> The changes looke fine to me.
> Also tested test_system_message.py locally on device and desktop and it
> still passes with these changes.

Landed in:
https://github.com/mozilla-b2g/gaia/commit/984af3a2eeb978d5125454196ea7b0d9074fcf1a

Comment 85

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Dave Hunt (:davehunt) from comment #83)
> Comment on attachment 8579314 [details] [review]
> UI tests
> 
> Fixed the copy/paste issue.

Landed in:
https://github.com/mozilla-b2g/gaia/commit/0409755bd0e93d229bd43f8d5378e6dbc5e2f74c

Comment 86

[GitHub Autolander Bot]

Attachment: Video player

Comment 87

[GitHub Autolander Bot]

Attachment: Wallpaper

Comment 88

[Martijn Wargers (dead)]

Comment on attachment 8584492 [details] [review]
Video player

Looks fine and the relevant tests work locally on b2g desktop for me.

Comment 89

[Martijn Wargers (dead)]

Comment on attachment 8584493 [details] [review]
Wallpaper

It looks fine, just some question I have in the pull request. If you can answer that, then it's an r+.
The test_settings_wallpaper.py test that is affected by it, works fine on b2g desktop, anyway.

Comment 90

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #88)
> Comment on attachment 8584492 [details] [review]
> Video player
> 
> Looks fine and the relevant tests work locally on b2g desktop for me.

Landed in:
https://github.com/mozilla-b2g/gaia/commit/71c3b0860eea690a088860192b578fc2aab0c497

Comment 91

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #89)
> Comment on attachment 8584493 [details] [review]
> Wallpaper
> 
> It looks fine, just some question I have in the pull request. If you can
> answer that, then it's an r+.
> The test_settings_wallpaper.py test that is affected by it, works fine on
> b2g desktop, anyway.

Landed in:
https://github.com/mozilla-b2g/gaia/commit/8da1599f7742ad3f90ad324cd31ce2f7dbca4025

Comment 92

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I'm going to mark this as resolved as we covered most of the explicit waits.