Switch tests to the new Marionette Wait class

RESOLVED FIXED

Status

Firefox OS
Gaia::UI Tests
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: davehunt, Assigned: zac)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
With bug 850881 fixed, we should move to the new Wait class to take advantage of more succinct code and shorter delays.
(Reporter)

Comment 1

4 years ago
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
(Assignee)

Comment 2

4 years ago
Wow, which test is that?
(Assignee)

Comment 3

4 years ago
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.
(Reporter)

Comment 4

4 years ago
Created attachment 8364229 [details] [diff] [review]
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.
(Reporter)

Comment 5

4 years ago
Created attachment 8364230 [details]
results-without-waits.html
(Reporter)

Comment 6

4 years ago
Created attachment 8364231 [details]
results-with-waits.html
(Reporter)

Comment 7

4 years ago
(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.
(Reporter)

Comment 8

4 years ago
(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.
(Reporter)

Comment 9

4 years ago
Created attachment 8364654 [details] [review]
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.
Attachment #8364229 - Attachment is obsolete: true
Attachment #8364230 - Attachment is obsolete: true
Attachment #8364231 - Attachment is obsolete: true
Attachment #8364654 - Flags: review?(zcampbell)
Attachment #8364654 - Flags: review?(bob.silverberg)
Attachment #8364654 - Flags: feedback?(florin.strugariu)
(Reporter)

Updated

4 years ago
Depends on: 956780, 963299
(Reporter)

Updated

4 years ago
Depends on: 963301
No longer depends on: 963299
(Reporter)

Comment 10

4 years ago
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.
Attachment #8364654 - Flags: review?(rwood)
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
Attachment #8364654 - Flags: feedback?(florin.strugariu) → feedback+
(Reporter)

Comment 12

4 years ago
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.
(Assignee)

Comment 13

4 years ago
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?
Attachment #8364654 - Flags: review?(zcampbell) → review-
(Reporter)

Comment 14

4 years ago
(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.
(Assignee)

Comment 15

4 years ago
(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.
(Reporter)

Comment 16

4 years ago
(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 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.
Attachment #8364654 - Flags: review?(bob.silverberg) → review-
(Reporter)

Comment 18

4 years ago
(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 on attachment 8364654 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15659

Wow, great change! LGTM.
Attachment #8364654 - Flags: review?(rwood) → review+
(Reporter)

Comment 20

4 years ago
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.
Attachment #8364654 - Flags: review?(zcampbell)
Attachment #8364654 - Flags: review?(bob.silverberg)
Attachment #8364654 - Flags: review-
Attachment #8364654 - Flags: feedback?(florin.strugariu)
Attachment #8364654 - Flags: feedback+
(Reporter)

Comment 21

4 years ago
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/
(Assignee)

Comment 22

4 years ago
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.
Attachment #8364654 - Flags: review?(zcampbell)
Attachment #8364654 - Flags: review?(bob.silverberg)
Attachment #8364654 - Flags: feedback?(florin.strugariu)
(Assignee)

Updated

4 years ago
Assignee: dave.hunt → zcampbell
(Reporter)

Comment 23

4 years ago
As requested, I've squashed this to a single commit: https://github.com/davehunt/gaia/commit/e29c46d81dddb48b3761afb6da7371afd96b2e0a
(Assignee)

Comment 24

4 years ago
Merged:
https://github.com/mozilla-b2g/gaia/commit/75c585cb163d7338e8624e3edbc5484654fc79c2
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.