Update to selenium 3.x #12
Conversation
…sing selenium 3.x
I _think_ this is updated to reflect the new return types (we used to get `null` but now get an object with a uuid `sessionId` and a `result` property.
|
I am running into issues in Protractor - the root cause is that Protractor is using a combination of Selenium v2 and v3. I am having to explicitly cast objects to @sjelin is there anything I can do to help get this merged? |
|
I am not really involved with protractor. @qiyigg probably knows more :) |
|
Sorry, I don't know much about webdriver-js-extender. Probably I can get some time to take a look at it this week, sorry for late response. |
spec/index_spec.ts
Outdated
| @@ -1,5 +1,5 @@ | |||
| import * as webdriver from 'selenium-webdriver'; | |||
| import {extend, patch} from '../lib'; | |||
| import {extend} from '../lib'; | |||
| import {DeferredExecutor} from '../lib/deferred_executor'; | |||
spec/command_tests/table_spec.ts
Outdated
|
|
||
| if (testcase.result != null) { | ||
| expect(result).toEqual(testcase.result); | ||
| } else if (result != null) { |
There was a problem hiding this comment.
Is there a scenario/possibility that the "testcase.result" is null, but "result" is not null. It seems the test can still pass without these changes.
There was a problem hiding this comment.
I believe this failed because of tslint but i'd be 👍 on restoring the original line. Let me investigate that
|
The PR looks good to me in general, but I am not familiar with webdriver-js-extender. It would be better if any of you(@sjelin @juliemr @vikerman or who else might know this) could also take a look at it since it is a breaking change which will break all the tests based on webdriver-js-extender which tries to patch the required lib by themselves. (Since we have a common lib to do this: initMockSeleniumStandaloneServerAndGetDriverFactory, it's probably not that bad) I'll leave this PR for a while and merge it if no response was got. |
|
@qiyigg thanks for the review! I am going out of town but I will make the edits you've suggested early next week. I haven't been involved in this project as of late, so i'm also on very shaky ground here. If we can't get feedback from folks who know more perhaps we could get some volunteers to beta test these changes? |
|
@NickTomlin Thanks, Nick. If you know who might be volunteers, it would be great. I don't even know who is using it. |
|
@NickTomlin Hi, Nick, do you have some time to take a look at those comments?
I think it is ok to merge this change. |
|
I have created a new PR and replicated changes here #20 |
|
Guys, can you merge this to solve the malicious version package issue? Thanks! :) |
|
@qiyigg thanks! I finally got around to resolving your comments. I also included a node engine bump to match protractor core. Let me know if that's something you want split out into another PR. |
|
Looks good to me, merged. I will make a new release of this and Protractor next week. |
This is a very much breaking change to bring webdriver-js-extender inline with the switch to selenium
3.x. This isn response to angular/protractor#4148 and related to the work in angular/protractor#4158.I understand what this library is doing at a very high level, but I'm a bit lost when it comes to some of the internals. I took a semi heavy-handed approach in ripping all the
2.xfunctionality out; let me know if that is not a good thing to do.Assumptions:
es6is finepatchanddeferred_executoris fine{ sessionId: 'f724cfd8-419d-4c74-82dc-adf8f43335cd', status: 0 }where we once receivednullis fine (I'm the least confident about this)This is more of a conversation starter than a sure thing so please educate me if there's something obvious I'm missing. 😄