-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade Selenium to 4.1.1 #2995
Conversation
4dfb91a
to
e376ad8
Compare
33be871
to
12e544d
Compare
2207784
to
0e71955
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few remarks. Due to the high volume of refactoring and style changes, this PR makes it very difficult to review.
Is it possible to leave out the changes to spaces?
Is it entirely necessary to use the w3c endpoints? in terms of tests, I don't think it makes any difference if we use jwp or w3c, but it makes it very difficult to review the actual meaningful changes.
lib/api/expect/cookie.js
Outdated
this.resultValue = result ? result.value : {}; | ||
if (result && result.value) { | ||
if (Array.isArray(result.value)) { | ||
this.resultValue = result.value[0].value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If result.value
is an empty array, this will throw an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a ternary here. Looking at the spec, the likelihood of it being empty for named cookies is low because it should error if it can't return anything.
I'm not sure how to add a test as it seems to kick the retry code in so any guidance would be appreciated.
I can revert these but most where put in when running
It makes a big difference. The Mocks are only looking for JWP but Selenium is no longer calling those so we were getting test failures. The necessary changes to get tests passing where done without changing the results of the tests. |
@@ -233,7 +233,7 @@ mocks: | |||
sessionId: *sessionId | |||
status: 0 | |||
|
|||
- url: '/wd/hub/session/1352110219202/log' | |||
- url: '/wd/hub/session/1352110219202/se/log' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a valid endpoint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the /se/
is namespacing it purely for Selenium and showing that it is not in the standard.
9c05648
to
ead8ddc
Compare
ead8ddc
to
9065d6d
Compare
9065d6d
to
5a45ace
Compare
This handles the deprecations and updates to endpoints that selenium only supports now.
This reverts commit 72b7dfa.
8ec5dc5
to
a2a525c
Compare
* Upgrade to Selenium 4.1.1 This handles the deprecations and updates to endpoints that selenium only supports now.
checkResponse was deprecated in Selenium and has been removed. This PR moves to the method that was suggested.
Thanks in advance for your contribution. Please follow the below steps in submitting a pull request, as it will help us with reviewing it quicker.
features/my-new-feature
orissue/123-my-bugfix
);