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

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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?

Comment 1

[David Burns :automatedtester]

Thinking about this more we don't need to do it.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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?

Comment 3

[David Burns :automatedtester]

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.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks David. Given that Andreas has spend a fair amount of time on timeouts lately it would be wise to also get this feedback.

Comment 5

[Andreas Tolfsen ❲: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.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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?

Comment 8

[Andreas Tolfsen ❲: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.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1299414 - Always reset timeout parameters for a new session.

Review commit: https://reviewboard.mozilla.org/r/95984/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95984/

Comment 11

[Andreas Tolfsen ❲: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.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95984/diff/1-2/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b6d9e6164194
Always reset timeout parameters for a new session. r=ato

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b6d9e6164194

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

test-only change which we would like to have for aurora. Please uplift. Thanks.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bd22bcd16b03