Closed Bug 1118298 Opened 6 years ago Closed 5 years ago

Client uses unknown command property session_id

Categories

(Testing :: Marionette, defect)

defect
Not set
normal

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)

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.
No longer blocks: webdriver
(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.
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]
Attached patch 1118298.patch (obsolete) — Splinter Review
v1 patch
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?
Flags: needinfo?(ato)
(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)
Attached patch 1118298-v2.patch (obsolete) — Splinter Review
v2 patch
Attachment #8554095 - Attachment is obsolete: true
Attachment #8554965 - Flags: review?(ato)
Do you have access to do a try run on this?
Flags: needinfo?(gioyik)
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?
Flags: needinfo?(gioyik)
Attached patch 1118298-v3.patch (obsolete) — Splinter Review
updated patch
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.
Flags: needinfo?(gioyik)
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
Flags: needinfo?(gioyik)
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)
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)
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)
Attached patch 1118298.patchSplinter Review
updated patch
Attachment #8554965 - Attachment is obsolete: true
Attachment #8555424 - Attachment is obsolete: true
Attachment #8567716 - Flags: review?(ato)
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+
Thank you Andreas. I really appreciate your help. I will see more bugs.
https://hg.mozilla.org/mozilla-central/rev/64f26b5ed9f4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.