Closed Bug 1397734 Opened 6 years ago Closed 6 years ago

Centralize defaults for socket and startup timeouts

Categories

(Remote Protocol :: Marionette, enhancement)

Product:
Remote Protocol
Component:
Marionette
Version:
57 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(1 file)

Bug 1397734 - Centralize defaults for socket and startup timeouts
59 bytes, text/x-review-board-request
impossibus
: review+
Details 
Currently we define the defaults for startup_timeout and socket_timeout at two different places. Which means that in this case startup_timeout has different values. While Marionette driver proposes 120s, the harness only uses 60s.

That means that for slow running builds like debug and ASAN we make use of a wrong and too short value. As result Marionette client itself kills the process because it cannot connect to the too late started server component of Marionette.

With this bug we will unify the defaults to the same value.

This might actually help us for bug 1261598 and bug 1362293.
Blocks: 1362293
Comment on attachment 8905495 [details]
Bug 1397734 - Centralize defaults for socket and startup timeouts

https://reviewboard.mozilla.org/r/177310/#review182576

r+wc

::: commit-message-3ecda:1
(Diff revision 2)
> +Bug 1397734 - Centralize defaults for socket and startup timeouts

I think you might also have to update the Mn harness tests (maybe just the mach_parsed_kwargs fixture)
Attachment #8905495 - Flags: review?(mjzffr) → review+
Comment on attachment 8905495 [details]
Bug 1397734 - Centralize defaults for socket and startup timeouts

https://reviewboard.mozilla.org/r/177310/#review182576

> I think you might also have to update the Mn harness tests (maybe just the mach_parsed_kwargs fixture)

No, the code changes here do not affect the harness tests, so I didn't have to update those. Those still run fine. The only thing I wanted to change is the float/int combination of both, but defered that for a later time given that it might have larger implications for invoking the harness.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bc05e6c94a17
Centralize defaults for socket and startup timeouts r=maja_zf
Backed out for mass mochitest failures (passed str to timedelta):

https://hg.mozilla.org/integration/autoland/rev/95430ac5c638f16cb20bca4abcffd02f9092b81c

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=bc05e6c94a1725d5561810e7a8d78b6f2484e60c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=129445749&repo=autoland

[task 2017-09-08T06:27:21.198674Z] 06:27:21     INFO - Application command: /builds/worker/workspace/build/application/firefox/firefox -marionette -foreground -profile /tmp/tmp2Hbax9.mozrunner
[task 2017-09-08T06:27:21.339709Z] 06:27:21     INFO - runtests.py | Application pid: 1096
[task 2017-09-08T06:27:21.342206Z] 06:27:21     INFO - TEST-INFO | started process GECKO(1096)
[task 2017-09-08T06:27:21.349900Z] 06:27:21     INFO -  Traceback (most recent call last):
[task 2017-09-08T06:27:21.358855Z] 06:27:21     INFO -    File "/builds/worker/workspace/build/tests/mochitest/runtests.py", line 2635, in doTests
[task 2017-09-08T06:27:21.370135Z] 06:27:21     INFO -      marionette_args=marionette_args,
[task 2017-09-08T06:27:21.379457Z] 06:27:21     INFO -    File "/builds/worker/workspace/build/tests/mochitest/runtests.py", line 2165, in runApp
[task 2017-09-08T06:27:21.396029Z] 06:27:21     INFO -      self.marionette.start_session(timeout=port_timeout)
[task 2017-09-08T06:27:21.400619Z] 06:27:21     INFO -    File "/builds/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_driver/decorators.py", line 23, in _
[task 2017-09-08T06:27:21.418200Z] 06:27:21     INFO -      return func(*args, **kwargs)
[task 2017-09-08T06:27:21.425515Z] 06:27:21     INFO -    File "/builds/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_driver/marionette.py", line 1207, in start_session
[task 2017-09-08T06:27:21.436279Z] 06:27:21     INFO -      self.wait_for_port(timeout=timeout)
[task 2017-09-08T06:27:21.448270Z] 06:27:21     INFO -    File "/builds/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_driver/marionette.py", line 674, in wait_for_port
[task 2017-09-08T06:27:21.460669Z] 06:27:21     INFO -      while datetime.datetime.now() - starttime < datetime.timedelta(seconds=timeout):
[task 2017-09-08T06:27:21.474742Z] 06:27:21     INFO -  TypeError: unsupported type for timedelta seconds component: str
Flags: needinfo?(hskupin)
Hurray for type checks in Python! As it looks like some test harnesses inappropriately use a string value instead of float/int to specify the timeout. As such we fail here.

Given that we don't know how many consumers this affects, I would simply put back the conversion to int for the arguments of the Marionette class as what we had before.
Flags: needinfo?(hskupin)
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/629ecdd561c4
Centralize defaults for socket and startup timeouts r=maja_zf
https://hg.mozilla.org/mozilla-central/rev/629ecdd561c4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.