newSession looks for non-conforming session_id property on Command

RESOLVED FIXED in Firefox 39

Status

Testing
Marionette
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ato, Assigned: gioyik, Mentored)

Tracking

(Blocks: 1 bug, {ateam-marionette-server})

unspecified
mozilla39
ateam-marionette-server
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
The server looks for the property "session_id" on the Command object in newSession.  According to the WebDriver protocol it should be "sessionId".
(Reporter)

Updated

3 years ago
Blocks: 721859
Keywords: ateam-marionette-server
(Reporter)

Updated

3 years ago
Blocks: 1118298
Mentor: dburns@mozilla.com
Whiteboard: [good first bug][language=js]
(Assignee)

Comment 1

3 years ago
Hi, I like to work on this bug. Could you tell me the file where I need to make the change?
Flags: needinfo?(ato)
(Assignee)

Comment 2

3 years ago
Redirecting question to bug mentor.
Flags: needinfo?(ato) → needinfo?(dburns)
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
Flags: needinfo?(dburns)
(Assignee)

Comment 4

3 years ago
Created attachment 8568769 [details] [diff] [review]
1118313.patch
Attachment #8568769 - Flags: review?(dburns)
(Assignee)

Comment 5

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

Thanks
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
Attachment #8568769 - Flags: review?(dburns) → review-
(Assignee)

Comment 7

3 years ago
Created attachment 8570972 [details] [diff] [review]
1118313.patch

Patch v2
Attachment #8570972 - Flags: review?(dburns)
(Assignee)

Comment 8

3 years ago
Sorry was my fault
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 ;
Attachment #8570972 - Flags: review?(dburns) → review-
(Assignee)

Comment 10

3 years ago
Created attachment 8574498 [details] [diff] [review]
1118313.patch

Patch v3
Attachment #8568769 - Attachment is obsolete: true
Attachment #8570972 - Attachment is obsolete: true
Attachment #8574498 - Flags: review?(dburns)
(Assignee)

Comment 11

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

Thanks
(Assignee)

Updated

3 years ago
Assignee: nobody → gioyik
Attachment #8574498 - Flags: review?(dburns) → review+
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!
Keywords: checkin-needed
This needs rebasing, sorry.
Keywords: checkin-needed
(Assignee)

Comment 14

3 years ago
Created attachment 8582137 [details] [diff] [review]
Patch

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

Updated

3 years ago
Keywords: checkin-needed
Attachment #8574498 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/66e7de1f1e8e
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/66e7de1f1e8e
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.