Closed Bug 1299414 Opened 3 years ago Closed 3 years ago
Check if we should not reset the timeout during restart() and quit()
58 bytes, text/x-review-board-request
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?
Thinking about this more we don't need to do it.
Status: NEW → RESOLVED
Closed: 3 years ago
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
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.
Thanks David. Given that Andreas has spend a fair amount of time on timeouts lately it would be wise to also get this feedback.
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.
(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?
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.
(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 firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/b6d9e6164194 Always reset timeout parameters for a new session. r=ato
test-only change which we would like to have for aurora. Please uplift. Thanks.
You need to log in before you can comment on or make changes to this bug.