Closed
Bug 1364228
Opened 7 years ago
Closed 7 years ago
Increase Marionette startup timeout, at least for linux mochitests
Categories
(Remote Protocol :: Marionette, enhancement)
Remote Protocol
Marionette
Tracking
(firefox-esr52 fixed, firefox54 fixed, firefox55 fixed)
RESOLVED
FIXED
mozilla55
People
(Reporter: gbrown, Assigned: gbrown)
References
Details
Attachments
(1 file, 1 obsolete file)
4.72 KB,
patch
|
ato
:
review+
|
Details | Diff | Splinter Review |
As discussed in bug 1261598, it appears that firefox intermittently requires more than 120 seconds to start up, at least in our aws linux32-debug environment. Increasing the wait to 180 seconds seems to eliminate the failures in that bug, at least in try pushes.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8866983 -
Flags: review?(hskupin)
Comment 2•7 years ago
|
||
Comment on attachment 8866983 [details] [diff] [review] increase marionette startup timeout to 180 seconds Review of attachment 8866983 [details] [diff] [review]: ----------------------------------------------------------------- I'm against changing the default value of the harness here. Given that this is only an issue for our poor performance Linux32 workers. Instead we should update the TaskCluster configuration to pass in this higher timeout value to the Marionette job. Preferred only for Linux 32. Otherwise I feel we could miss some potential regressions in the startup time. Btw. the startup time was initially increased from 60, because it is also used for restarts and Firefox has a shutdown timeout of 60s before non-stopping threads are getting killed.
Attachment #8866983 -
Flags: review?(hskupin) → review-
Assignee | ||
Comment 3•7 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #2) > I'm against changing the default value of the harness here. Given that this > is only an issue for our poor performance Linux32 workers. > Instead we should update the TaskCluster configuration to pass in this > higher timeout value to the Marionette job. Preferred only for Linux 32. > Otherwise I feel we could miss some potential regressions in the startup time. I checked that again and found typical mochitest startup times of about 10 seconds or less on Windows and OSX and 30 seconds or less on linux64. I agree, only linux32 (debug especially, but also opt) is typically 60+ seconds. On the other hand, I think there's no real upper limit or even a target for **browser startup time in a unit test environment**. Even if Windows opt startup time suddenly jumps to 120+ seconds for, say, a mochitest run, I predict we'll still shrug it off, blaming the test hardware, or the unnatural browser configuration used for mochitests (or reftests, etc) -- and that might be the right call. Startup performance regressions are better tracked and responded to in the context of specific performance tests like Talos. I'll have a look at the TC configuration alternative anyway...
Assignee | ||
Comment 4•7 years ago
|
||
This adds a --marionette-startup-timeout option for mochitests and reftests and uses that for linux mochitests. We normally use the same test options for all linux mochitests. I don't see a convenient way to distinguish linux32/linux64 and I'm comfortable using the higher timeout for linux64 mochitests, even though it doesn't seem to be strictly necessary on linux64. https://treeherder.mozilla.org/#/jobs?repo=try&revision=36de5b84000aab5c6bf85d0bb7970c170cfa8c19 has this change + logging to report the startup timeout (search on "ZZZ"). It verifies that linux mochitests use 180 seconds while other platforms (like Windows) and other tests (like reftests) continue to use 120 seconds. My first attempt at this change was not effective: I found I needed to adjust timeout handling in start_session() to incorporate the specified startup_timeout -- easy to fix, but an unexpected complication.
Attachment #8866983 -
Attachment is obsolete: true
Attachment #8867782 -
Flags: review?(hskupin)
Assignee | ||
Updated•7 years ago
|
Summary: Increase Marionette DEFAULT_STARTUP_TIMEOUT → Increase Marionette startup timeout, at least for linux mochitests
Comment 5•7 years ago
|
||
Comment on attachment 8867782 [details] [diff] [review] increase marionette startup timeout to 180 seconds, for linux mochitests Review of attachment 8867782 [details] [diff] [review]: ----------------------------------------------------------------- I'm currently away for the Rust training and won't have the time to check this until Thursday. Andreas, could you have a look instead? I'm not sure if that is the right approach, given that I see that other harnesses do not call ´raise_for_port()´ after creating the Marionette driver instance. They directly try to start the session. Is that what we want?
Attachment #8867782 -
Flags: review?(hskupin) → review?(ato)
Comment 6•7 years ago
|
||
Comment on attachment 8867782 [details] [diff] [review] increase marionette startup timeout to 180 seconds, for linux mochitests Review of attachment 8867782 [details] [diff] [review]: ----------------------------------------------------------------- Looks in line with older code to modify the socket timeout.
Attachment #8867782 -
Flags: review?(ato) → review+
Pushed by gbrown@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/75ec8cde6e20 Increase marionette startup timeout for Linux mochitests; r=ato
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/75ec8cde6e20
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•7 years ago
|
Whiteboard: [checkin-needed-beta][checkin-needed-esr52]
Comment 9•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/63e5df872250
status-firefox54:
--- → fixed
Whiteboard: [checkin-needed-beta][checkin-needed-esr52] → [checkin-needed-esr52]
Comment 10•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/1515f76d2922
status-firefox-esr52:
--- → fixed
Whiteboard: [checkin-needed-esr52]
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•