Closed
Bug 960931
Opened 10 years ago
Closed 8 years ago
Marionette should fail in a more obvious way if we try to start more than one session
Categories
(Testing :: Marionette Client and Harness, defect)
Tracking
(firefox49 fixed)
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: wlach, Assigned: automatedtester)
Details
(Keywords: pi-marionette-client, pi-marionette-server)
Attachments
(2 files)
MozReview Request: Bug 960931: Throw SessionNotCreatedError when requesting 2nd Active Session r?ato
58 bytes,
text/x-review-board-request
|
ato
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
ato
:
review+
|
Details |
I just spent quite a few hours debugging an error related to accidentally creating multiple simultaneous marionette sessions in bug 960063 (without realizing that was what was actually happening). We should generate a noisy assertion at the gecko layer if the user tries to create a new session if one is there already, instead of just silently hanging waiting for the existing one to close.
Reporter | ||
Updated•10 years ago
|
Summary: Marionette should fail in a more obvious way if we start more than one simultaneous session → Marionette should fail in a more obvious way if we start more than one session
Reporter | ||
Updated•10 years ago
|
Summary: Marionette should fail in a more obvious way if we start more than one session → Marionette should fail in a more obvious way if we try to start more than one session
Comment 1•10 years ago
|
||
Is it meant to be possible to have multiple sessions? Could start_session return the active session if there is one, rather than throwing?
Reporter | ||
Comment 2•10 years ago
|
||
(In reply to Dave Hunt (:davehunt) from comment #1) > Is it meant to be possible to have multiple sessions? Could start_session > return the active session if there is one, rather than throwing? No, it is not meant to be possible as I understand it. I think having more than one marionette object accessing the session indicates a problem with the code which could cause race conditions or other problems, especially in a multi-threading application. I think we're best off throwing an exception/assertion in this case.
Comment 3•10 years ago
|
||
What we can do is send back a 'A Marionette session is already active' message from the server when someone tries to make another session. What do you two think?
Comment 4•10 years ago
|
||
That would work for me.
Assignee | ||
Updated•9 years ago
|
Keywords: ateam-marionette-client,
ateam-marionette-server
Assignee | ||
Comment 5•8 years ago
|
||
If we have a session we are expected to return a SessionNotCreatedError as part of step 2 of http://w3c.github.io/webdriver/webdriver-spec.html#new-session Review commit: https://reviewboard.mozilla.org/r/55122/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/55122/
Attachment #8756375 -
Flags: review?(ato)
Attachment #8756376 -
Flags: review?(ato)
Assignee | ||
Comment 6•8 years ago
|
||
We create the variables with what is returned from the session creation but never do anything with them. Review commit: https://reviewboard.mozilla.org/r/55124/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/55124/
Comment 7•8 years ago
|
||
There is actually on technical reason Marionette can’t support more than one concurrent session. In fact, the new dispatching technique I put in place about a year ago ensures it can handle >1 command requests at the same time without confusing or messing up the response sequence.
Assignee: nobody → dburns
Status: NEW → ASSIGNED
Comment 8•8 years ago
|
||
Comment on attachment 8756375 [details] MozReview Request: Bug 960931: Throw SessionNotCreatedError when requesting 2nd Active Session r?ato https://reviewboard.mozilla.org/r/55122/#review52170 ::: testing/marionette/driver.js:209 (Diff revision 1) > * JSON serialisable object to send to the listener. > * @param {number=} cmdId > * Command ID to ensure synchronisity. > */ > GeckoDriver.prototype.sendAsync = function(name, msg, cmdId) { > + logger.info(`sendAsync ${this.sessionId}`) Debug? ::: testing/marionette/driver.js:314 (Diff revision 1) > * Window whose browser we need to access. > * @param {boolean=false} isNewSession > * True if this is the first time we're talking to this browser. > */ > GeckoDriver.prototype.startBrowser = function(win, isNewSession=false) { > + logger.info(`startBrowser ${this.sessionId}`) Debug? ::: testing/marionette/driver.js:474 (Diff revision 1) > }; > > /** Create a new session. */ > GeckoDriver.prototype.newSession = function*(cmd, resp) { > + if (this.sessionId) { > + throw new SessionNotCreatedError("Maximum number of active sessions.") Remove . and add ; ::: testing/marionette/harness/marionette/tests/unit/test_session.py:46 (Diff revision 1) > caps = self.marionette.start_session(session_id="ILoveCheese") > > self.assertEqual(self.marionette.session_id, "ILoveCheese") > self.assertTrue(isinstance(self.marionette.session_id, unicode)) > + > + def test_we_only_support_one_active_session_at_a_time(self): `test_maximum_allowed_sessions` is more aligned with the spec prose. ::: testing/marionette/harness/marionette/tests/unit/test_session.py:48 (Diff revision 1) > self.assertEqual(self.marionette.session_id, "ILoveCheese") > self.assertTrue(isinstance(self.marionette.session_id, unicode)) > + > + def test_we_only_support_one_active_session_at_a_time(self): > + self.marionette.start_session() > + self.assertTrue(isinstance(self.marionette.session_id, unicode)) We should have a different test for whether `self.marionette.session_id` is the correct type. The right test here, I think, is if it not `None`.
Attachment #8756375 -
Flags: review?(ato) → review+
Comment 9•8 years ago
|
||
Comment on attachment 8756376 [details] MozReview Request: Bug 960931: Remove unused variables from tests r?ato https://reviewboard.mozilla.org/r/55124/#review52172
Attachment #8756376 -
Flags: review?(ato) → review+
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/050485808a59 https://hg.mozilla.org/integration/mozilla-inbound/rev/8e7bb0f5c506
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/050485808a59 https://hg.mozilla.org/mozilla-central/rev/8e7bb0f5c506
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Updated•1 year ago
|
Product: Testing → Remote Protocol
Comment 12•1 year ago
|
||
Moving bugs for Marionette client due to component changes.
Component: Marionette → Marionette Client and Harness
Product: Remote Protocol → Testing
You need to log in
before you can comment on or make changes to this bug.
Description
•