Closed
Bug 1118298
Opened 9 years ago
Closed 9 years ago
Client uses unknown command property session_id
Categories
(Testing :: Marionette Client and Harness, defect)
Testing
Marionette Client and Harness
Tracking
(firefox39 fixed)
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: ato, Assigned: gioyik, Mentored)
References
Details
(Keywords: pi-marionette-client, Whiteboard: [good first bug][language=py])
Attachments
(1 file, 3 obsolete files)
2.42 KB,
patch
|
ato
:
review+
|
Details | Diff | Splinter Review |
The WebDriver protocol says that the session ID property to be provided with the Command object should be "sessionId". We are currently sending "session_id" which doesn't conform.
Reporter | ||
Updated•9 years ago
|
Blocks: webdriver
Keywords: ateam-marionette-client
Comment 1•9 years ago
|
||
FWIW this is the sort of thing that the proxy has to deal with anyway, so it doesn't seem like a blocker.
Reporter | ||
Comment 2•9 years ago
|
||
(In reply to James Graham [:jgraham] from comment #1) > FWIW this is the sort of thing that the proxy has to deal with anyway, so it > doesn't seem like a blocker. Sorry, this isn't a blocker. It's marionette-client Python client specific. It should be a dependent for the bug 1118313 that I just filed.
Comment 3•9 years ago
|
||
We need to update https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/marionette.py#824 to use sessionId instead of session_id as the kwarg
Mentor: dburns
Whiteboard: [good first bug][language=py]
Assignee | ||
Comment 5•9 years ago
|
||
Hi submitted a patch for this bug. Could you check it and tell if is correct?
Reporter | ||
Comment 6•9 years ago
|
||
(In reply to Giovanny Andres Gongora Granada from comment #5) > Hi submitted a patch for this bug. Could you check it and tell if is correct? Your patch is half way correct. We also need to change the server to look for sessionId instead of session_id: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-server.js?from=marionette-server.js#589
Assignee: nobody → gioyik
Mentor: dburns → ato
Status: NEW → ASSIGNED
Reporter | ||
Updated•9 years ago
|
Attachment #8554095 -
Flags: review?(ato) → review-
Comment 7•9 years ago
|
||
The server change needs to handle both sessionId and session_id. We will also need to send out a breaking change notification especially when we remove the session_id stuff from the server.
Assignee | ||
Comment 8•9 years ago
|
||
So the modifications in marionette-server.js (https://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-server.js?from=marionette-server.js#589) are: this.session_id = aRequest.parameters.session_id ? aRequest.parameters.session_id : null; this.sessionId = aRequest.parameters.sessionId ? aRequest.parameters.sessionId : null; It's correct?
Flags: needinfo?(ato)
Reporter | ||
Comment 9•9 years ago
|
||
(In reply to David Burns :automatedtester from comment #7) > The server change needs to handle both sessionId and session_id. We will > also need to send out a breaking change notification especially when we > remove the session_id stuff from the server. David is right, we need to handle session_id for backwards compatibility. (In reply to Giovanny Andres Gongora Granada from comment #8) > So the modifications in marionette-server.js > (https://dxr.mozilla.org/mozilla-central/source/testing/marionette/ > marionette-server.js?from=marionette-server.js#589) are: > > this.session_id = aRequest.parameters.session_id ? > aRequest.parameters.session_id : null; > this.sessionId = aRequest.parameters.sessionId ? > aRequest.parameters.sessionId : null; > > It's correct? I'd probably do this instead: this.sessionId = aRequest.parameters.sessionId || aRequest.parameters.session_id || null; Because aRequest.parameters is an object, sessionId or session_id will return undefined. And since undefined evaluates to false, it will fall back to null if both of them are undefined.
Flags: needinfo?(ato)
Assignee | ||
Comment 10•9 years ago
|
||
v2 patch
Attachment #8554095 -
Attachment is obsolete: true
Attachment #8554965 -
Flags: review?(ato)
Reporter | ||
Comment 11•9 years ago
|
||
Do you have access to do a try run on this?
Flags: needinfo?(gioyik)
Reporter | ||
Comment 12•9 years ago
|
||
Comment on attachment 8554965 [details] [diff] [review] 1118298-v2.patch Review of attachment 8554965 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/marionette/client/marionette/marionette.py @@ +815,5 @@ > > :param desired_capabilities: An optional dict of desired > capabilities. This is currently ignored. > :param timeout: Timeout in seconds for the server to be ready. > + :param sessionId: unique identifier for the session. If no session id is This should still be "session_id" because it describes the argument passed into the Python function start_session.
Attachment #8554965 -
Flags: review?(ato) → review-
Assignee | ||
Comment 13•9 years ago
|
||
Andreas, I don't have access to do a try run on this because I need someone who would want to vouch me. Could you be that person?
Flags: needinfo?(gioyik)
Reporter | ||
Comment 15•9 years ago
|
||
(In reply to Giovanny Gongora [:gioyik] from comment #13) > Andreas, I don't have access to do a try run on this because I need someone > who would want to vouch me. Could you be that person? I'd be happy to vouch for you! I've filed bug 1126671 for this. Can you upload your public SSH key there? You'll also need to sign a form. In the meantime, would you like me to trigger a try run to get this patch landed or should we wait for your access to be granted? It shouldn't take long.
Flags: needinfo?(gioyik)
Reporter | ||
Updated•9 years ago
|
Attachment #8555424 -
Flags: review?(ato) → review+
Assignee | ||
Comment 16•9 years ago
|
||
Hi Andreas, I would want wait to get my access granted to trigger a try run. So I could get familiar using it. I will ni you when I trigger a try. Thanks
Flags: needinfo?(gioyik)
Assignee | ||
Comment 17•9 years ago
|
||
Andreas, Today I got Level 1 access, thanks so much. Could you help me with the try access? I am seeing the wiki page and the steps are: hg qref --message "try: <your-computed-syntax-here>" hg push -f try But I have questions about computed-syntax. What could be the correct computed syntax for this patch? I found this and seems useful one time I understand the basis :) http://trychooser.pub.build.mozilla.org/
Flags: needinfo?(ato)
Reporter | ||
Comment 18•9 years ago
|
||
Level 1 access should give you rights to to pushes to the try server. If you're using a hg based workflow I've found the trychooser extension (https://bitbucket.org/sfink/trychooser) to be quite useful. Personally I never work directly with mq or patch queues like the official documentation suggests. I typically use three different try syntaxes when running tests: basic: -b o -p linux64,linux64_gecko -u marionette,marionette-webapi,gaia-ui-test -t none desktop: -b o -p linux,macosx64,win32,linux_gecko,linux64_gecko -u marionette,marionette-webapi,gaia-ui-test -t none mobile: -b o -p emulator,panda -u marionette-webapi -t none The basic syntax should I think be enough for this patch, but you can also experiment with desktop if you want to be more thorough. Ping me on IRC (ato) if you need help getting this to work. (-:
Flags: needinfo?(ato)
Assignee | ||
Comment 19•9 years ago
|
||
Sorry for not answer this bug. I was reorganizing my time with my new university schedule. Finally I know how Treeherder works and I pushed it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dcc99ad5a7d Let me know if there's something I need to do. Also I attach it to an update version of the patch. Sorry for the delay.
Flags: needinfo?(ato)
Assignee | ||
Comment 20•9 years ago
|
||
updated patch
Attachment #8554965 -
Attachment is obsolete: true
Attachment #8555424 -
Attachment is obsolete: true
Attachment #8567716 -
Flags: review?(ato)
Reporter | ||
Comment 21•9 years ago
|
||
Comment on attachment 8567716 [details] [diff] [review] 1118298.patch Review of attachment 8567716 [details] [diff] [review]: ----------------------------------------------------------------- The try run looks good! The next step is to land this on mozilla-inbound (m-i). Since you don't have commit level 3 yet, we can request the sheriff's to land it using the checkin-needed keyword.
Attachment #8567716 -
Flags: review?(ato) → review+
Reporter | ||
Comment 22•9 years ago
|
||
I've gone ahead an added the checkin-needed keyword. Now that you've (soon) landed your patch, there are more good first bugs here: https://bugzilla.mozilla.org/buglist.cgi?resolution=---&status_whiteboard_type=allwordssubstr&query_format=advanced&status_whiteboard=[good%20first%20bug]&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&component=Marionette&product=Testing&list_id=12029558
Flags: needinfo?(ato)
Keywords: checkin-needed
Assignee | ||
Comment 23•9 years ago
|
||
Thank you Andreas. I really appreciate your help. I will see more bugs.
Comment 24•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/64f26b5ed9f4
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/64f26b5ed9f4
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Updated•1 year ago
|
Product: Testing → Remote Protocol
Comment 26•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
•