Closed
Bug 1444068
Opened 6 years ago
Closed 6 years ago
Log how long geckodriver will attempt connecting to Marionette
Categories
(Testing :: geckodriver, enhancement, P3)
Testing
geckodriver
Tracking
(firefox61 fixed)
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: ato, Assigned: gsfraley)
Details
Attachments
(1 file, 4 obsolete files)
We should log how long geckodriver will attempt connecting to Marionette, similar to the changes I’ve made in bug 1443853.
Reporter | ||
Updated•6 years ago
|
Priority: -- → P3
Comment hidden (mozreview-request) |
Comment 2•6 years ago
|
||
mozreview-review |
Comment on attachment 8957847 [details] Bug 1444068 - Log the seconds geckodriver will wait to connect to the browser https://reviewboard.mozilla.org/r/226818/#review232728 Thank you for the patch Greg! I had a quick look and added an issue. When you fixed that and you think the patch is ready for review, please ask :ato to review it. Thanks. ::: testing/geckodriver/src/marionette.rs:1378 (Diff revision 1) > Ok(stream) => { > self.stream = Some(stream); > break > }, > Err(e) => { > trace!(" connection attempt {}/{}", poll_attempt, poll_attempts); With the change we don't want to add a line to the trace log for each iteration. So this line should be removed.
Reporter | ||
Comment 3•6 years ago
|
||
mozreview-review |
Comment on attachment 8957847 [details] Bug 1444068 - Log the seconds geckodriver will wait to connect to the browser https://reviewboard.mozilla.org/r/226818/#review232742 Apart from whimboo’s one issue this looks OK. I would prefer us to migrate to use time::Duration::elapsed(), like we do in mozrunner [1], but I’m happy to accept this patch as-is modulo removing the debug log line. [1] https://hg.mozilla.org/mozilla-central/file/tip/testing/mozbase/rust/mozrunner/src/runner.rs#l139
Attachment #8957847 -
Flags: review-
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8957847 -
Attachment is obsolete: true
Reporter | ||
Comment 5•6 years ago
|
||
mozreview-review |
Comment on attachment 8959854 [details] Bug 1444068 - Switch to using rust time for Marionette connection polling https://reviewboard.mozilla.org/r/228616/#review234440 Good patch! Just a few minor things before we can land it. ::: commit-message-29dcc:3 (Diff revision 1) > +Bug 1444068 - Switch to using rust time for Marionette connection polling > + > +Foremost, this patch switches to using time::Instant and time::Duration to handle the state of polling against the Marionette connection. It also replaces the trace call to printout the progress of the polling with a debug statement upfront indicating total poll time. Please limit this at ~72 characters. fmt(1) does an excellent job of this. ::: testing/geckodriver/src/marionette.rs:1323 (Diff revision 1) > - let timeout = 60 * 1000; // ms > - let poll_interval = 100; // ms > + let timeout_sec = 60; > + let timeout = time::Duration::from_secs(timeout_sec); Remove timeout_sec and pass 60 directly to time::Duration::from_secs. ::: testing/geckodriver/src/marionette.rs:1328 (Diff revision 1) > - let timeout = 60 * 1000; // ms > - let poll_interval = 100; // ms > - let poll_attempts = timeout / poll_interval; > - let mut poll_attempt = 0; > + let timeout_sec = 60; > + let timeout = time::Duration::from_secs(timeout_sec); > + let poll_interval = time::Duration::from_millis(100); > + let now = time::Instant::now(); > > + debug!("Waiting {}s to connect to browser", timeout_sec); Use timeout.as_secs().
Attachment #8959854 -
Flags: review-
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8959854 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8959873 -
Flags: review?(ato)
Reporter | ||
Comment 7•6 years ago
|
||
mozreview-review |
Comment on attachment 8959873 [details] Bug 1444068 - Switch to using rust time for Marionette connection polling https://reviewboard.mozilla.org/r/228640/#review234568 ::: testing/geckodriver/src/marionette.rs:1355 (Diff revision 1) > Ok(stream) => { > self.stream = Some(stream); > break; > } > Err(e) => { > - trace!(" connection attempt {}/{}", poll_attempt, poll_attempts); > + if now.elapsed() <= timeout { I didn’t spot this in my first pass, but I guess technically this should not be lesser-than-or-equal-to but lesser-than, right? If the time has elapsed, e.g. now.elapsed() evaluates to the exact same value as the timeout duration, then we have reached the timeout.
Attachment #8959873 -
Flags: review?(ato) → review+
Assignee | ||
Comment 8•6 years ago
|
||
Makes sense. I'll amend it in a sec.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8960122 [details] Bug 1444068 - Make Marionette connection polling stop at timeout https://reviewboard.mozilla.org/r/228914/#review234574 Hm. Something appears to have gone wrong here: we now have two commits instead of one. Can you squash/fold the last one into the first?
Attachment #8960122 -
Flags: review?(ato) → review-
Assignee | ||
Comment 12•6 years ago
|
||
My bad! I'm still getting used to the process. I'll do that now.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8959873 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8960122 -
Attachment is obsolete: true
Reporter | ||
Comment 14•6 years ago
|
||
try: -b do -p linux,linux64,macosx64,win64,android-api-16 -u marionette,marionette-e10s,marionette-headless-e10s,xpcshell,web-platform-tests,firefox-ui-functional-local-e10s,firefox-ui-functional-remote-e10s -t none
Assignee: nobody → gsfraley
Status: NEW → ASSIGNED
Reporter | ||
Comment 15•6 years ago
|
||
Uh, wrong paste. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=75adba7d96a4d51c22fc1ab35df2e2a7f4dd1123
Reporter | ||
Comment 16•6 years ago
|
||
mozreview-review |
Comment on attachment 8960123 [details] Bug 1444068 - Switch to using rust time for Marionette connection polling https://reviewboard.mozilla.org/r/228916/#review234650
Attachment #8960123 -
Flags: review?(ato) → review+
Comment 17•6 years ago
|
||
Pushed by atolfsen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c161a8b11ba7 Switch to using rust time for Marionette connection polling r=ato
Comment 18•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c161a8b11ba7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•