Closed Bug 1299414 Opened 3 years ago Closed 3 years ago

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

Categories

(Testing :: Marionette, defect, P3)

defect

Tracking

(firefox52 fixed, firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(1 file)

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
Closed: 3 years ago
Flags: needinfo?(dburns)
Resolution: --- → INVALID
(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)
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)
(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
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)
(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
Attachment #8814895 - Flags: review?(ato)
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+
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.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b6d9e6164194
Always reset timeout parameters for a new session. r=ato
https://hg.mozilla.org/mozilla-central/rev/b6d9e6164194
Status: ASSIGNED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
test-only change which we would like to have for aurora. Please uplift. Thanks.
Whiteboard: [checkin-needed-aurora]
https://hg.mozilla.org/releases/mozilla-aurora/rev/bd22bcd16b03
Flags: in-testsuite+
Whiteboard: [checkin-needed-aurora]
You need to log in before you can comment on or make changes to this bug.