Client uses unknown command property session_id

RESOLVED FIXED in Firefox 39

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ato, Assigned: gioyik, Mentored)

Tracking

({pi-marionette-client})

unspecified
mozilla39
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [good first bug][language=py])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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

4 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)

Updated

4 years ago
No longer blocks: webdriver
(Reporter)

Comment 2

4 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.
(Reporter)

Updated

4 years ago
Depends on: 1118313
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 4

4 years ago
Posted patch 1118298.patch (obsolete) — Splinter Review
v1 patch
Attachment #8554095 - Flags: review?(ato)
(Assignee)

Comment 5

4 years ago
Hi submitted a patch for this bug. Could you check it and tell if is correct?
(Reporter)

Comment 6

4 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

4 years ago
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.
(Assignee)

Comment 8

4 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

4 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

4 years ago
Posted patch 1118298-v2.patch (obsolete) — Splinter Review
v2 patch
Attachment #8554095 - Attachment is obsolete: true
Attachment #8554965 - Flags: review?(ato)
(Reporter)

Comment 11

4 years ago
Do you have access to do a try run on this?
Flags: needinfo?(gioyik)
(Reporter)

Comment 12

4 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

4 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)
(Assignee)

Comment 14

4 years ago
Posted patch 1118298-v3.patch (obsolete) — Splinter Review
updated patch
Attachment #8555424 - Flags: review?(ato)
(Reporter)

Comment 15

4 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

4 years ago
Attachment #8555424 - Flags: review?(ato) → review+
(Assignee)

Comment 16

4 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

4 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

4 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

4 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

4 years ago
Posted patch 1118298.patchSplinter Review
updated patch
Attachment #8554965 - Attachment is obsolete: true
Attachment #8555424 - Attachment is obsolete: true
Attachment #8567716 - Flags: review?(ato)
(Reporter)

Comment 21

4 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+
(Assignee)

Comment 23

4 years ago
Thank you Andreas. I really appreciate your help. I will see more bugs.
https://hg.mozilla.org/mozilla-central/rev/64f26b5ed9f4
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.