Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Validation for response in ff58 #141

Closed

Conversation

cuff-links
Copy link
Contributor

Fixes #139

@jsf-clabot
Copy link

jsf-clabot commented Mar 15, 2018

CLA assistant check
All committers have signed the CLA.

@cuff-links cuff-links force-pushed the validation_for_response_in_ff58 branch from 679b155 to 460c8ed Compare March 15, 2018 22:05
@cuff-links
Copy link
Contributor Author

@dylans Yep. One commit was in the wrong email. My bad. Thanks for the help.

@dylans
Copy link

dylans commented Mar 16, 2018

Reopening as it hasn't landed in Leadfoot yet.

@dylans dylans reopened this Mar 16, 2018
Server.js Outdated
var responseData = null;
var sessionId = null;
// At least geckodriver 0.19.0 returns the response data in a 'value.capabilities'
// property, whereas Selenium does not.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there's a problem with the whitespace lines 369 - 375. I think this code uses spaces to indent whereas the rest of the file uses tabs.

@cuff-links cuff-links force-pushed the validation_for_response_in_ff58 branch from fbd30ab to 621a650 Compare March 19, 2018 21:04
Copy link
Member

@jason0x43 jason0x43 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The addition of responseData and sessionId variables is good, but one possible response structure is no longer handled.

@@ -363,12 +363,19 @@ Server.prototype = {
desiredCapabilities: desiredCapabilities,
requiredCapabilities: requiredCapabilities
}).then(function (response) {
// At least geckodriver 0.15.0 returns the response data in a 'value' property, whereas Selenium does not.
if (response.value && response.value.sessionId) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case is no longer covered (where the structure would have response.value.sessionId && response.value.value).

{
  value: {
    sessionId: 123
    value: { 
      // ...
    }
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jason0x43 Fixed. I think we should be good now. Thanks for catching that.

Server.js Outdated
var sessionId = null;
// At least geckodriver 0.19.0 returns the response data in a 'value.capabilities'
// property, whereas Selenium does not.
if (!response.sessionId && response.value.capabilities) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than check for !response.sessionId, consider checking for response.value.sessionId && response.value.capabilities.

@cuff-links cuff-links force-pushed the validation_for_response_in_ff58 branch from 5c97ddc to 32742bc Compare March 20, 2018 16:31
@cuff-links cuff-links force-pushed the validation_for_response_in_ff58 branch from 32742bc to d6a7824 Compare March 20, 2018 16:35
@cuff-links
Copy link
Contributor Author

@jason0x43 The edits have been made.

Server.js Outdated
var responseData = null;
var sessionId = null;
// At least geckodriver 0.19.0 returns the response data in a 'value.capabilities'
// property, whereas Selenium does not.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow my commit to fix the mixed use of tabs and spaces in this and the next few lines was lost with the recent push...

@cuff-links
Copy link
Contributor Author

@dylans @jason0x43 I set webstorm up for smart tabs with 4 indentation. I hope this worked.

Copy link

@dylans dylans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

jason0x43 pushed a commit that referenced this pull request Mar 24, 2018
@jason0x43
Copy link
Member

Merged in b861870. Thanks!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants