983684 - Detect if it's a device and use an increased timeout

Status: RESOLVED FIXED

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

Description

[Zac C (:zac)]

Lots of people who are using the ui test suite are not using an appropriate timeout for device testing. 
The default timeout is too low for device testing.

Let's detect where it's a device and increase the default timeout to one appropriate for device.

This will make it more user friendly.

Of course if you use a command line timeout then that's the one the suite will use.

Comment 1

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

Which timeout does this refer to? Note that we have the following default timeouts:

Search: 10 seconds
Page load: 30 seconds
Wait: 5 seconds

Maybe it makes sense to finally do away with the various timeout types in gaiatest, as I believe these are almost always overwritten on the command line using --timeout. We could just use a default value for this, which can then vary depending on the target.

Comment 2

[Zac C (:zac)]

I was referring to just the command line timeout, but in practice mostly not passing in --timeout for device testing mostly causes isses for Wait timeouts.
We could probably fine tune all 3 of those waits if we knew the target and its performance.

Comment 3

[Zac C (:zac)]

Attachment: github pr

Dave, I followed your train of thought from comment #1.

I briefly played with trying to 'detect' device but the best way I could fine was just "not desktopb2g and not emulator".. device is a broad definition.

I also structured it to cover https://bugzilla.mozilla.org/show_bug.cgi?id=987504

Comment 4

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

Comment on attachment 8396141 [details] [review]
github pr

f+ but some comments that should be addressed before a patch is ready for review. I'm not a big fan of making all timeouts the same value, but do understand that this change will make it easier for anyone new to running the tests. If we could somehow keep the varied timeouts and provide suitable defaults, that would be ideal.

Comment 5

[Zac C (:zac)]

I am not really mad on it either (hence f?) but if we come to a resolution in bug 987504 we could open up this patch to be done properly.

Comment 6

[Zac C (:zac)]

Dave, how about setting the marionette timeouts to:

desktopb2g = 5000ms
device = 10000ms
emulator = 30000ms

then:
TIMEOUT_SEARCH = default timeout (as above)
TIMEOUT_SCRIPT = default timeout x2
TIMEOUT_PAGE = default timeout x2

So implicit waits would be low and script and page would get a multiple of the default.

The command line timeout would override all 3 as per normal as a conservative fallback, eg if it were a really slow device or emulator. As 1 [command line arg] does not go into 3 very well we just accept the imperfect case in this scenario.

Thoughts?

Comment 7

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

So you're suggesting something like the following?

timeouts = {}

if desktop:
    timeouts[self.marionette.TIMEOUT_SEARCH] = 5000
    timeouts[self.marionette.TIMEOUT_SCRIPT] = 10000
    timeouts[self.marionette.TIMEOUT_PAGE] = 10000
elif device:
    timeouts[self.marionette.TIMEOUT_SEARCH] = 10000
    timeouts[self.marionette.TIMEOUT_SCRIPT] = 20000
    timeouts[self.marionette.TIMEOUT_PAGE] = 20000
elif emulator:
    timeouts[self.marionette.TIMEOUT_SEARCH] = 30000
    timeouts[self.marionette.TIMEOUT_SCRIPT] = 60000
    timeouts[self.marionette.TIMEOUT_PAGE] = 60000

for k, v in timeouts.items():
    self.marionette.timeouts(k, v)

I'm okay with this, though I prefer to be explicit rather than use multipliers. Perhaps the best place for this change would be in the Marionette test runner so that we can update the help text at the same time.

Comment 8

[Zac C (:zac)]

Yes, but somewhere in there it would be nice to set self.marionette.timeout, so we could hit bug 987504 at the same time.

My comment #6 wasn't clear but that's what I intended; set self.marionette.timeout then multiply it to get the more explicit timeouts.

How does that fit into your comment #7 example?

Also I updated the readme.md in the pull request as I thought it more appropriate than updating the test runner.

Comment 9

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

(In reply to Zac C (:zac) from comment #8)
> Yes, but somewhere in there it would be nice to set self.marionette.timeout,
> so we could hit bug 987504 at the same time.
> 
> My comment #6 wasn't clear but that's what I intended; set
> self.marionette.timeout then multiply it to get the more explicit timeouts.

I believe setting self.marionette.timeout would only have the effect of affecting the explicit waits. I would just set this directly to fit in with whatever you've used to work out the other timeouts. The default is 5 seconds. That said, self.marionette.timeout may already be set by the --timeout argument, in which case you want to set all timeouts to that value?

> Also I updated the readme.md in the pull request as I thought it more
> appropriate than updating the test runner.

It sounds like this change could benefit more than just the UI tests, but updating the README is certainly the least we could do.

Comment 10

[Zac C (:zac)]

OK let me update this today and we can 'iterate' on it again.

Comment 11

[Zac C (:zac)]

Comment on attachment 8396141 [details] [review]
github pr

Let's have another look.

The addition of setting self.marionette.timeout is to encompass https://bugzilla.mozilla.org/show_bug.cgi?id=987504 at the same time.

Comment 12

[Zac C (:zac)]

Comment on attachment 8396141 [details] [review]
github pr

Dave I was anticipating the usual feedback!

Can I carry this as a review then?

Comment 13

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

Comment on attachment 8396141 [details] [review]
github pr

(In reply to Zac C (:zac) from comment #12)
> Comment on attachment 8396141 [details] [review]
> github pr
> 
> Dave I was anticipating the usual feedback!

Usual feedback?

> Can I carry this as a review then?

No, I would not like a f+ to be implicitly treated as an r+. Note that I have not tested this code.

Comment 14

[Zac C (:zac)]

(In reply to Dave Hunt (:davehunt) from comment #13)
> Comment on attachment 8396141 [details] [review]
> github pr
> 
> (In reply to Zac C (:zac) from comment #12)
> > Comment on attachment 8396141 [details] [review]
> > github pr
> > 
> > Dave I was anticipating the usual feedback!
> 
> Usual feedback?
> 

Not every day one slips a review past you so easily :)

I'll get a 2nd review from SV/Bob to get the code tested.

Comment 15

[Zac C (:zac)]

Comment on attachment 8396141 [details] [review]
github pr

Hey Bob and SV, can I get a review on this, to fill where Dave was not able to run the tests?

Comment 16

[Florin Strugariu [:Bebe]]

Comment on attachment 8396141 [details] [review]
github pr

the time-outs are off

Comment 17

[Zac C (:zac)]

Comment on attachment 8396141 [details] [review]
github pr

Bebe I pushed a change. The timeouts set are correct, so I just changed the readme.md to state that if there is no --timeout then a reasonable default will be set.

Comment 18

[Zac C (:zac)]

Great! a good usability change.

Merged:
https://github.com/mozilla-b2g/gaia/commit/95f99edd544418e6b808dd40318e43fcf3ef73e8