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

[rb] implement remote downloads #12037

Merged
merged 7 commits into from
Nov 1, 2023
Merged

[rb] implement remote downloads #12037

merged 7 commits into from
Nov 1, 2023

Conversation

titusfortner
Copy link
Member

@titusfortner titusfortner commented May 15, 2023

Status

Ruby delete fails because of #13025
I rebased this PR on that fix to make sure it passes

Description

  • creates a new HasFileDownloads module with #downloadable_files and #download_file
  • Implemented Options#enableDownloads to allow downloading
  • Only RemoteWebDriver includes this new module
  • Rather than put the implementation of these methods into Remote::Bridge, I created Remote::Features similar to how we've done the browser features files
  • Moved the upload implementation into that new feature file
  • In order to support both browser & remote command list, a new #command_list created and Remote::Features#add_commands for Remote::Driver to use
  • Had to do custom processing of se:downloadsEnabled

Motivation and Context

This implements #11657

@titusfortner titusfortner added this to the 4.11 milestone May 15, 2023
@titusfortner titusfortner marked this pull request as draft May 15, 2023 16:53
@titusfortner
Copy link
Member Author

I want to hold off on this until after we releases 4.10 to keep pace with other bindings. Wanted a proof of concept in Ruby to make it easier to do it in other languages (maybe)

Copy link
Member

@diemol diemol left a comment

Choose a reason for hiding this comment

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

Maybe I missed this because I am not very fluent in Ruby... But I think we should return an error when the capability se:downloadsEnabled is not present.

@titusfortner
Copy link
Member Author

Won't the node do this? We're not currently doing this elsewhere in ruby code, though I guess we could...

@diemol
Copy link
Member

diemol commented Jun 16, 2023

I guess I was thinking of disabling the method in some way or throwing an error. People will try this with any Grid they have available because most users do not read the prerequisites. Also, it won't work with most cloud vendors for now, so trying to avoid throwing work to their customer support teams.

@titusfortner
Copy link
Member Author

Makes sense. We should do the same with bidi capability, then.

Oddly, ruby used to do this with the javascript enabled capability and was the only one, and this is what prevented selenium 2 ruby tests from working with Selenium 3 grid.

@codecov-commenter
Copy link

codecov-commenter commented Oct 23, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (11b4efe) 56.49% compared to head (3d09c14) 56.49%.
Report is 19 commits behind head on trunk.

❗ Current head 3d09c14 differs from pull request most recent head cb0abf9. Consider uploading reports for the commit cb0abf9 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##            trunk   #12037   +/-   ##
=======================================
  Coverage   56.49%   56.49%           
=======================================
  Files          86       86           
  Lines        5255     5255           
  Branches      187      187           
=======================================
  Hits         2969     2969           
  Misses       2099     2099           
  Partials      187      187           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@titusfortner titusfortner marked this pull request as ready for review October 23, 2023 14:16
@titusfortner titusfortner force-pushed the rb_downloads branch 4 times, most recently from a8febe0 to 8e3db89 Compare October 31, 2023 03:23
@giladshanan
Copy link

@titusfortner any insight on #13307?

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