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)

enhancement

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.
Priority: -- → P3
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.
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-
Attachment #8957847 - Attachment is obsolete: true
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-
Attachment #8959854 - Attachment is obsolete: true
Attachment #8959873 - Flags: review?(ato)
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+
Makes sense.  I'll amend it in a sec.
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-
My bad!  I'm still getting used to the process.  I'll do that now.
Attachment #8959873 - Attachment is obsolete: true
Attachment #8960122 - Attachment is obsolete: true
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
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+
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c161a8b11ba7
Switch to using rust time for Marionette connection polling r=ato
https://hg.mozilla.org/mozilla-central/rev/c161a8b11ba7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: