aerokube / ggr

A lightweight load balancer used to create big Selenium clusters
https://aerokube.com/ggr/latest/
Apache License 2.0
314 stars 74 forks source link

implement Selenium 4 CDP support #350

Closed MoLow closed 2 years ago

MoLow commented 2 years ago

Hi @vania-pooh, This is an implementation of supporting new selenium 4 cdp proxying. a new selenium 4 grid is able to proxy CDP WebSocket from the node through the hub out of the box, So I wanted to have this possibility in GGR as well - as we use GGR and it works great for us. it is pretty much based on VNC, with some tweaks.

Please let me know what I should do to get this PR approved and merged - if there are any documentation/tests missing etc.

vania-pooh commented 2 years ago

@MoLow this is not standard WebDriver and may change in the future. So I don't think we'll merge this.

codecov[bot] commented 2 years ago

Codecov Report

Merging #350 (8a23010) into master (ad43f36) will decrease coverage by 3.96%. The diff coverage is 30.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #350      +/-   ##
==========================================
- Coverage   89.72%   85.76%   -3.97%     
==========================================
  Files           4        4              
  Lines         584      618      +34     
==========================================
+ Hits          524      530       +6     
- Misses         47       73      +26     
- Partials       13       15       +2     
Flag Coverage Δ
go 85.76% <30.00%> (-3.97%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
proxy.go 89.06% <30.00%> (-5.08%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 35133a6...8a23010. Read the comment docs.

MoLow commented 2 years ago

@MoLow this is not standard WebDriver and may change in the future. So I don't think we'll merge this.

thanks @vania-pooh I am not sure about your comment, since also these are not standard: https://github.com/testimio/ggr/blob/master/proxy.go#L54-L58 what will be the downside of adding this?

vania-pooh commented 2 years ago

@aandryashin could you please take a look?

vania-pooh commented 2 years ago

@MoLow

1) The problem with this change is that we don't control these capabilities. What if several months later guys from Selenium team decide to change the name of the capability like they did with /file API (https://github.com/aerokube/selenoid/issues/1079)? Yes, Ggr supports some non-standard APIs like you mentioned above but they are under our control. 2) Also Selenium guys consider Selenium Grid 4 fault-tolerant solution. How about just using it instead of Ggr?

MoLow commented 2 years ago

@MoLow the problem with this change is that we don't control these capabilities. What if several months later guys from Selenium team decide to change the name of the capability like they did with /file API (aerokube/selenoid#1079)? Yes, Ggr supports some non-standard APIs like you mentioned above but they are under our control.

I would be grateful if you will consider adding this, as I think essentially it is not really different than the non-standard API's already supported. I will totally respect if you decide not to add support for this, and we will have to build and use our own fork.

thanks

MoLow commented 2 years ago

@MoLow

Also Selenium guys consider Selenium Grid 4 fault-tolerant solution. How about just using it instead of Ggr? @vania-pooh I took a look at that and still think that GGR is a much better solution as it is stateless

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.