959217 - Switch tests to the new Marionette Wait class

Status: RESOLVED FIXED

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

Description

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

With bug 850881 fixed, we should move to the new Wait class to take advantage of more succinct code and shorter delays.

Comment 1

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

I've started working on this, and am finding that several waits are unfortunately relying on a 0.5 second sleep for them to work appropriately. I'll continue working through these issues and rebasing as I go. I'm also planning to incorporate bug 948075 in my final patch.

One particular test already shows promising results:

Desktop: 178 seconds
Desktop (with patch): 48 seconds

Device: 335 seconds
Device (with patch): 182 seconds

Comment 2

[Zac C (:zac)]

Wow, which test is that?

Comment 3

[Zac C (:zac)]

btw I never really got the whole advantage of the expected conditions class,  it seems like just another layer of abstraction to wade through. I would be happy to take the new waits without it.

Comment 4

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

Attachment: Work in progress patch

This is still a work in progress, but I wanted to share what I have so far. Changing to the new Wait class uncovered a number of issues, and there are still some intermittents to fix. So far my testing has been on device, and I'm certain there will be additional failures on desktop considering it's so much faster.

I'd like this to land after a new Marionette client has been released with the fix for bug 957248, but my current patch contains a TODO that allows it to work with the current Marionette client version.

I ran the full suite of applicable tests locally with and without this patch. The number of failures was about the same, but with the patch the tests took around 30 minutes less! I'll attached the HTML reports in case anyone is interested.

I'll continue working on this patch, and if I have to suddenly drop it due to my time off, I'll post the latest version and someone is welcome to pick it up. Otherwise, I'm happy to pick it up when I return.

Comment 5

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

Attachment: results-without-waits.html

Comment 6

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

Attachment: results-with-waits.html

Comment 7

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

(In reply to Zac C (:zac) from comment #2)
> Wow, which test is that?

I believe that was one of the email tests. I've attached reports for with/without the patch applied.

Comment 8

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

(In reply to Zac C (:zac) from comment #3)
> btw I never really got the whole advantage of the expected conditions class,
> it seems like just another layer of abstraction to wade through. I would be
> happy to take the new waits without it.

I won't be including the expected conditions in this patch, as the waits are important to get right first. I'll work on a follow-up that will add these and optimise the about of element lookups we're doing.

Comment 9

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

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15659

Okay, here's an initial working patch. I've had a 100% success rate on my Leo (don't ask, but my Hamachi and Inari are now bricks) and 100% on desktop, aside from a crash that's mentioned (and skipped) in the pull. I wouldn't be surprised if there are a few intermittents that I've missed, but I have run the tests *a lot* :)

Asking for review from both Bob and Zac because I'd like them *both* to review this. Also asking feedback from Bebe, but I wouldn't want this merged without the former both reviewing.

Chances are that there may be some changes necessary, which I even allude to in my pull request (I've commented fairly heavily already). In the event that I'm not around to update, please feel free to build on this commit. It will likely rot fairly quickly but in the worst case I can recover this when I return.

Phew, what a patch! Should save about 30 minutes on device and 8 minutes on desktop, from my testing.

Comment 10

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

Comment on attachment 8364654 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15659

Adding Rob Wood as a reviewer due to some endurance test changes.

Comment 11

[Florin Strugariu [:Bebe]]

Comment on attachment 8364654 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15659

generally it looks OK
some small questions and nits
Looking forward to the final version  :D

Comment 12

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

Thanks for taking the time to go through this Bebe, I've addressed all of your comments and updated the pull request. There are still a few failing tests in Travis-CI, so there's a bit more work to do in this patch.

Comment 13

[Zac C (:zac)]

Comment on attachment 8364654 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15659

r-, 
want message= override on the wait_for_condition class because this wait often results in vague TimeoutExceptions so adding custom messages really helps with debugging the first time.

also the _call_ stuff seems a bit overkill for a test case, I'd prefer to keep the coding concepts fewer and more consistent if we can avoid it, to keep the barrier to entry lower especially for non-Python devs and contributors.

Do you reckon we can split this up a bit and take some new waits that aren't blocked now and others later?

Comment 14

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

(In reply to Zac C (:zac) from comment #13)
> Comment on attachment 8364654 [details] [review]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15659
> 
> r-, 
> want message= override on the wait_for_condition class because this wait
> often results in vague TimeoutExceptions so adding custom messages really
> helps with debugging the first time.

Please raise this bug against Marionette and we'll see if we can get it into the next release.

> also the _call_ stuff seems a bit overkill for a test case, I'd prefer to
> keep the coding concepts fewer and more consistent if we can avoid it, to
> keep the barrier to entry lower especially for non-Python devs and
> contributors.

I feel these are necessary. Only needed them twice, which speaks for the general testability of the code. I think removing them would require dev assistance.

> Do you reckon we can split this up a bit and take some new waits that aren't
> blocked now and others later?

I don't see how - the changes are pretty core, and we're pretty much there already.

Comment 15

[Zac C (:zac)]

(In reply to Dave Hunt (:davehunt) from comment #14)
> > Do you reckon we can split this up a bit and take some new waits that aren't
> > blocked now and others later?
> 
> I don't see how - the changes are pretty core, and we're pretty much there
> already.

The main blocking bit for me is the custom message in until, we're regressing a bit.

Comment 16

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

(In reply to Zac C (:zac) from comment #15)
> (In reply to Dave Hunt (:davehunt) from comment #14)
> > > Do you reckon we can split this up a bit and take some new waits that aren't
> > > blocked now and others later?
> > 
> > I don't see how - the changes are pretty core, and we're pretty much there
> > already.
> 
> The main blocking bit for me is the custom message in until, we're
> regressing a bit.

This has been raised as bug 963598 (thanks Zac), which is blocking this bug via bug 963301.

Comment 17

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8364654 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15659

Overall very nice patch, which also cleans up some other things. Just a few comments/questions in the PR, plus there is a test that is failing frequently for me on desktop.

Comment 18

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

(In reply to Bob Silverberg [:bsilverberg] from comment #17)
> Comment on attachment 8364654 [details] [review]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15659
> 
> Overall very nice patch, which also cleans up some other things. Just a few
> comments/questions in the PR, plus there is a test that is failing
> frequently for me on desktop.

I was able to reproduce the wallpaper test failure, and have fixed it with the latest commit. This will likely bitrot (or be bitrotted by bug 936505).

Comment 19

[Robert Wood [:rwood]]

Comment on attachment 8364654 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15659

Wow, great change! LGTM.

Comment 20

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

Comment on attachment 8364654 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15659

Okay, updated the pull request to address all comments. Let's go for another round of reviews.

Comment 21

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

I've scheduled an adhoc Jenkins build against Hamachi. It's currently in the queue: http://qa-selenium.mv.mozilla.com:8080/view/B2G%20Hamachi/job/b2g.hamachi.mozilla-central.ui.adhoc/133/

Comment 22

[Zac C (:zac)]

Comment on attachment 8364654 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15659

This is OK for device but needs a bit more work to be ready for Travis.

I'm going to take over now as Dave is heading onto PTO.

Comment 23

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

As requested, I've squashed this to a single commit: https://github.com/davehunt/gaia/commit/e29c46d81dddb48b3761afb6da7371afd96b2e0a

Comment 24

[Zac C (:zac)]

Merged:
https://github.com/mozilla-b2g/gaia/commit/75c585cb163d7338e8624e3edbc5484654fc79c2