1118313 - newSession looks for non-conforming session_id property on Command

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Andreas Tolfsen ❲:ato❳]

The server looks for the property "session_id" on the Command object in newSession.  According to the WebDriver protocol it should be "sessionId".

Comment 1

[Giovanny Gongora [:gioyik]]

Hi, I like to work on this bug. Could you tell me the file where I need to make the change?

Comment 2

[Giovanny Gongora [:gioyik]]

Redirecting question to bug mentor.

Comment 3

[David Burns :automatedtester]

We need to update https://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-server.js#606 to check both session_id or sessionId to maintain backwards compatibility

Comment 4

[Giovanny Gongora [:gioyik]]

Attachment: 1118313.patch

Comment 5

[Giovanny Gongora [:gioyik]]

David, could you check it and tell me if it's ok?

Thanks

Comment 6

[David Burns :automatedtester]

Comment on attachment 8568769 [details] [diff] [review]
1118313.patch

Review of attachment 8568769 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/marionette/marionette-server.js
@@ +602,5 @@
>      }
>  
>      this.scriptTimeout = 10000;
>      if (aRequest && aRequest.parameters) {
> +      this.sessionId = aRequest.parameters.session_id || aRequest.parameters.session_id || null;

one of these should be sessionId

Comment 7

[Giovanny Gongora [:gioyik]]

Attachment: 1118313.patch

Patch v2

Comment 8

[Giovanny Gongora [:gioyik]]

Sorry was my fault

Comment 9

[David Burns :automatedtester]

Comment on attachment 8570972 [details] [diff] [review]
1118313.patch

Review of attachment 8570972 [details] [diff] [review]:
-----------------------------------------------------------------

sorry this took so long to review, I have an attention span of a goldfish and kept starting and not finishing the review :/

::: testing/marionette/marionette-server.js
@@ +602,5 @@
>      }
>  
>      this.scriptTimeout = 10000;
>      if (aRequest && aRequest.parameters) {
> +      this.sessionId = aRequest.parameters.session_id || aRequest.parameters.sessionId || null;

I would prefer this since if it doesnt hit either of them it will be undefined and the null check below would still work

this.sessionId = aRequest.parameters.sessionId ? aRequest.parameters.sessionId : aRequest.parameters.session_id ;

Comment 10

[Giovanny Gongora [:gioyik]]

Attachment: 1118313.patch

Patch v3

Comment 11

[Giovanny Gongora [:gioyik]]

Please let me know if this is what you suggest my or I misunderstand your feedback.

Thanks

Comment 12

[David Burns :automatedtester]

Perfect! THanks! Please have a look at https://wiki.mozilla.org/Auto-tools/Projects/Marionette/Auto-tools/Projects/Marionette/Roadmap#Good_First_Bugs if there are any other good first bugs you would like to tackle!

Comment 13

[Ryan VanderMeulen [:RyanVM]]

This needs rebasing, sorry.

Comment 14

[Giovanny Gongora [:gioyik]]

Attachment: Patch

Sorry I forget this issue. I attach a rebased version of the patch.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/66e7de1f1e8e

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/66e7de1f1e8e