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
Revert "Revert "Update selenium-webdriver
gem to 4.2.0""
#51960
Conversation
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.
Testing story looks good!
@@ -41,7 +41,7 @@ def create_response(code, body, content_type) | |||
if (msg = exception.message.match(/unexpected response, code=(?<code>\d+).*\n(?<error>.*)/)) | |||
error = msg[:error] | |||
error = JSON.parse(error)['value']['error'] rescue error | |||
exception.message.replace("Error #{msg[:code]}: #{error}") | |||
raise exception, "Error #{msg[:code]}: #{error}", exception.backtrace |
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 definitely like this approach better than mutating the exception.
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.
Likewise! Shout out to @snickell, who actually came up with this before I copied it out of his WIP PR. I assume it was in response to the fact that the message string is now frozen internally by Selenium and throws an error when we try to modify it, but I agree that overall it's much cleaner this way. Also, I learned that you can pass the backtrace manually as an optional third argument to Kernel#raise
!
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.
Looks good!
Wish [test all browsers]
passed on drone :-P, good workaround. For my future ref, here's the drone run for test all browsers: https://drone.cdn-code.org/code-dot-org/code-dot-org/29313/2/5
Reverts #51958, reapplying #51635. The original attempt consistently failed UI tests on iPhone and iPad when trying to interact with dropdowns, because we were trying to apply a desktop-Safari-specific workaround when executing in mobile Safari.
In Selenium 3,
@browser.browser
is:Safari
on desktop Safari and:safari
on mobile. In Selenium 4, it's:safari
on both, so in addition to browser detection I explicitly inspect browser capabilities (as we do here) to determine whether or not to apply the workaround.Testing Story
Verified locally that the tests pass with the new modifications; also ran with
[test all browsers]
and verified that the only tests which failed in that run did not fail during the actual DTT.