Skip to content

[java] implement file downloads #12979

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

Merged
merged 6 commits into from
Nov 1, 2023
Merged

[java] implement file downloads #12979

merged 6 commits into from
Nov 1, 2023

Conversation

titusfortner
Copy link
Member

@titusfortner titusfortner commented Oct 20, 2023

Status

Update: I re-implemented this because I realized it shouldn't use AutoService; no bazel changes required.

This code works, but it is in draft because:

  • I think I'm putting the interface and the implementation in the right directories, but bazel complains about AutoService, and I'm not sure I've added that dependency in the right place
  • I think the tests might need to get moved somewhere else, but to be honest, I'm not quite sure how to add this one to the test files The test I updated here is more of a unit test for the grid, so I think it should stay the same and I need to add this test someplace else, but I'm not sure where that someplace else needs to be. Advice appreciated.
  • I'd like to get Python & .NET implemented before these get merged.

Description

  • Implement getDownloadableFiles(), downloadFile(filename) and deleteDownloadableFiles()

Motivation and Context

Implements #11657

@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (11b4efe) 56.49% compared to head (0cf879c) 56.49%.
Report is 26 commits behind head on trunk.

❗ Current head 0cf879c differs from pull request most recent head 69a9ae4. Consider uploading reports for the commit 69a9ae4 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   #12979   +/-   ##
=======================================
  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 force-pushed the file_down_java branch 4 times, most recently from 48720af to e219478 Compare October 21, 2023 17:21
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.

I think the updated test is enough, you are testing both the bindings and the Grid.

If you wish, you could add unit tests for the bindings and then mock all the requests and responses, which I guess should go where the other RemoteWebDriver tests are.

@titusfortner titusfortner marked this pull request as ready for review October 23, 2023 14:14
@titusfortner titusfortner force-pushed the file_down_java branch 2 times, most recently from 69a9ae4 to 9a594fb Compare October 30, 2023 16:42
@titusfortner titusfortner merged commit 96f13f8 into trunk Nov 1, 2023
@titusfortner titusfortner deleted the file_down_java branch November 1, 2023 04:18
@rbri
Copy link
Contributor

rbri commented Nov 2, 2023

Hi @titusfortner,
just saw this mentioned in the release note and i'm thinking about making this possible with the HtmlUnit driver also.
But i'm a bit confused - i can't find a description of how the file list you get by get(/se/files) is constructed. Does this mean the browser checks all links in the page for something that might point to a file?
Any hint will be great!
Thanks
ronald

@titusfortner
Copy link
Member Author

It works with the grid, not with the individual drivers. If you are local you can set whatever directory you want in the options.

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

Successfully merging this pull request may close these issues.

6 participants