Skip to content
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

Remove require of websocket in selenium-devtools gem #10868

Merged
merged 2 commits into from
Jul 13, 2022
Merged

Remove require of websocket in selenium-devtools gem #10868

merged 2 commits into from
Jul 13, 2022

Conversation

kevindew
Copy link
Contributor

Description

In cd69898 the dependency of websocket was removed from selenium-devtools gem. When this gem is required it raises an error of: cannot load such file -- websocket (LoadError)

We experienced this error because we updated selenium-devtools and hadn't updated selenium-webdriver. We found the problem was resolved by updating selenium-webdriver as that has a websocket dependency cd69898

Motivation and Context

Resolve an error when requiring the selenium-devtools gem

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

In cd69898 the dependency of websocket was removed from selenium-devtools gem. When this gem is required it raises an error of: `cannot load such file -- websocket (LoadError)`

We experienced this error because we updated selenium-devtools and hadn't updated selenium-webdriver. We found the problem was resolved by updating selenium-webdriver as that has a websocket dependency cd69898
@CLAassistant
Copy link

CLAassistant commented Jul 12, 2022

CLA assistant check
All committers have signed the CLA.

@diemol diemol requested review from p0deje and titusfortner July 12, 2022 11:37
@diemol diemol added the C-rb label Jul 12, 2022
Copy link
Member

@titusfortner titusfortner left a comment

Choose a reason for hiding this comment

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

Excellent, thanks. This also needs to change the required version of selenium-webdriver to the one that added the requirement.

@kevindew
Copy link
Contributor Author

Excellent, thanks. This also needs to change the required version of selenium-webdriver to the one that added the requirement.

Hmm do you mean in selenium-devtools? That doesn't currently have a defined dependency on selenium-webdriver

@titusfortner
Copy link
Member

Hah, yes, then we need to add it for this.

@kevindew
Copy link
Contributor Author

Hah, yes, then we need to add it for this.

Sure, it seems a little odd to add because selenium-devtools doesn't actually reference selenium-webdrivers. So won't make use of the dependency.

So I guess the value in doing so would just be to just to force a constraint onto selenium-webdriver given Rubygems doesn't have a concept like peer dependencies which this seems to be. I'd guess for that it'd be a constraint of >= 4.2.0 since that is the first version which bundles websocket gem.

If you've got a clear idea of what's needed do feel free to just push it to my branch if that's easier.

@p0deje
Copy link
Member

p0deje commented Jul 12, 2022

It's not really possible to use selenium-devtools without selenium-webdriver, so let's just add the latter as a runtime dependency:

  s.add_runtime_dependency 'selenium-webdriver', '~> 4.2'

@titusfortner
Copy link
Member

I agree it is weird, but it isn't really a peer dependency because selenium-webdriver is never going to reference selenium-devtools directly in any way, so I can't think of any reason where adding selenium-webdriver as a requirement to selenium-devtools gemspec will be a problem.

Selenium-devtools is an optional add-on for selenium-webdriver, thus the
only way that we can define interoperability is from a dependency on
webdriver from devtools.

In order for devtools to operate correctly with webdriver it needs to be
using a minimum version of 4.2 (as there was a dependency change of
websocket [1]).

As per the suggestion in
#10868 (comment)
this adopts a pessimistic version constraint that will need to be
updated if selenium-webdriver has a major version update.

[1]: cd69898
@kevindew
Copy link
Contributor Author

Thanks, I've added 38c1ea0 to add the requested dependency to selenium-devtools.

As per @p0deje's suggestion I went for the pessimistic constraint of ~> 4.2.

@p0deje p0deje requested a review from titusfortner July 13, 2022 14:35
@titusfortner titusfortner merged commit bb23fba into SeleniumHQ:trunk Jul 13, 2022
@titusfortner
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

5 participants