Check if we should not reset the timeout during restart() and quit()

RESOLVED FIXED in Firefox 52

Status

Testing
Marionette
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

Trunk
mozilla53
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox52 fixed, firefox53 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
As David mentioned on bug 1296597 comment 8 we should not reset the timeouts in restart() and quit(). They should be kept at their value until it's explicitly reset. 

David, can you please add more details? Is there a requirement we have to meet regarding the webdriver specs?
Flags: needinfo?(dburns)
Thinking about this more we don't need to do it.
Status: NEW → RESOLVED
Last Resolved: a year ago
Flags: needinfo?(dburns)
Resolution: --- → INVALID
(Assignee)

Comment 2

a year ago
(In reply to David Burns :automatedtester from comment #1)
> Thinking about this more we don't need to do it.

I stumbled over this again today while investigating bug 1316552. So I think we should get a clear answer if we need it or not. David, the reply you gave here, doesn't tell me why we don't have to do it. Can you please explain further?

When I check the code I would expect a misbehavior in tests after a restart. Lets say a test sets the timeouts to specific values and expects that those timeouts are kept also after a restart. Now we restart or quit and the timeouts are reset. Isn't that a broken behavior from the test side? Shouldn't a set timeout be persisted for the whole test?
Status: RESOLVED → REOPENED
Flags: needinfo?(dburns)
Resolution: INVALID → ---
What I said, is we don't need need to make sure that our information goes across a restart.

Originally my thinking is that we should maintain this across the entire "session" of the test. I don't that that is required as long as we can keep the information in the test and when we know that we are resetting, we can pass this on.

We also now have getTimeout that has been added we can do things differently.
Flags: needinfo?(dburns)
(Assignee)

Comment 4

a year ago
Thanks David. Given that Andreas has spend a fair amount of time on timeouts lately it would be wise to also get this feedback.
Flags: needinfo?(ato)
I think it’s generally a good idea to reset state across test boundaries.  My expectation would be for tests to handle state between individual subtests themselves, but for the harness to ensure no state bleeds over between tests (that is, Python test_*.py files).

Since the restart function is called _inside_ tests, I would actually not expect it to reset the timeouts explicitly.  However, does this not happen _implicitly_ because a new Marionette session is started?  To my knowledge the different timeouts are reset when GeckODriver#startSession or GeckoDriver#tearDownSession is called.
Flags: needinfo?(ato)
(Assignee)

Comment 6

a year ago
(In reply to Andreas Tolfsen ‹:ato› from comment #5)
> _implicitly_ because a new Marionette session is started?  To my knowledge
> the different timeouts are reset when GeckODriver#startSession or
> GeckoDriver#tearDownSession is called.

Could be. Lets wait until you got your patch on bug 1316622 landed, and we can come back to this one.
Depends on: 1316622
(Assignee)

Comment 7

a year ago
So when I understand the current code correctly we reset the timeout values for each restart of the application:

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver.js#113

It means we won't have to do it via the client.

But the question is now, if we want to keep the timeouts as set for the current test whether how often Firefox gets restarted or quit. If we want that we need the following two steps:

* Get current timeouts before a restart/quit and update them with the old values after a restart
* Reset timeouts at the end of a test method

Given that the webdriver spec doesn't cover restarts of the application, we should be free in defining our wanted behavior.

Personally I would be fine with the reset and that tests have to set their own values again if needed after a restart. So we should simply get rid of the reset_timeout() call in the client.

Andreas, what would you prefer?
Flags: needinfo?(ato)
I’d expect a new Marionette session to be exactly that: new.  If we want timeouts to persist we should reuse the session instead of working around it by storing timeout state in the harness.
Flags: needinfo?(ato)
(Assignee)

Comment 9

a year ago
(In reply to Andreas Tolfsen ‹:ato› from comment #8)
> I’d expect a new Marionette session to be exactly that: new.  If we want
> timeouts to persist we should reuse the session instead of working around it
> by storing timeout state in the harness.

Ok, so that would mean that we should remove the call to reset_timeout in quit() and restart() and have it done in start_session() instead.
Assignee: nobody → hskupin
Status: REOPENED → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8814895 - Flags: review?(ato)

Comment 11

a year ago
mozreview-review
Comment on attachment 8814895 [details]
Bug 1299414 - Always reset timeout parameters for a new session.

https://reviewboard.mozilla.org/r/95984/#review95982

::: testing/marionette/client/marionette_driver/marionette.py:1268
(Diff revision 1)
> +        # For a new session we should always reset all timeout values
> +        self.timeout.reset()

Timeouts are reset implicitly when you start a new session.  I don’t think this is needed.
Attachment #8814895 - Flags: review?(ato) → review+
(Assignee)

Comment 12

a year ago
mozreview-review-reply
Comment on attachment 8814895 [details]
Bug 1299414 - Always reset timeout parameters for a new session.

https://reviewboard.mozilla.org/r/95984/#review95982

> Timeouts are reset implicitly when you start a new session.  I don’t think this is needed.

The tests have proven that this is the case! ;) So I can indeed remove it. Will push an update in a bit.
Comment hidden (mozreview-request)

Comment 14

a year ago
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b6d9e6164194
Always reset timeout parameters for a new session. r=ato

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b6d9e6164194
Status: ASSIGNED → RESOLVED
Last Resolved: a year agoa year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Assignee)

Comment 16

a year ago
test-only change which we would like to have for aurora. Please uplift. Thanks.
status-firefox52: --- → affected
Whiteboard: [checkin-needed-aurora]
https://hg.mozilla.org/releases/mozilla-aurora/rev/bd22bcd16b03
status-firefox52: affected → fixed
Flags: in-testsuite+
Whiteboard: [checkin-needed-aurora]
You need to log in before you can comment on or make changes to this bug.