Closed Bug 1118298 Opened 6 years ago Closed 5 years ago
Client uses unknown command property session
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.
FWIW this is the sort of thing that the proxy has to deal with anyway, so it doesn't seem like a blocker.
(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.
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
Whiteboard: [good first bug][language=py]
Attachment #8554095 - Flags: review?(ato)
Hi submitted a patch for this bug. Could you check it and tell if is correct?
(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
Attachment #8554095 - Flags: review?(ato) → review-
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.
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?
(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.
Do you have access to do a try run on this?
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-
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?
Attachment #8555424 - Flags: review?(ato)
(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.
Attachment #8555424 - Flags: review?(ato) → review+
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
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/
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. (-:
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.
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+
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
Thank you Andreas. I really appreciate your help. I will see more bugs.
You need to log in before you can comment on or make changes to this bug.