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

[java] Copy SM binary to cache folder and use it from there (#11359) #12539

Merged
merged 6 commits into from
Oct 18, 2023

Conversation

bonigarcia
Copy link
Member

@bonigarcia bonigarcia commented Aug 11, 2023

Description

This PR copies the SM binary from the distribution (jar) to the cache folder (`~/.cache/selenium), which is invoked from then.

This PR has two commits:

  1. The first commit assumes that the cache path is always the path (`~/.cache/selenium), which makes things easier.
  2. The "problem" is that the cache folder can be configured in Selenium Manager with other mechanisms: the configuration file (se-config.toml -if we finally renamed that way-) or environmental variables (in particular, SE_CACHE_PATH is the one for the cache path). The second commit allows to read also the cache path from the configuration file (first) and the env (second).

Motivation and Context

Resolves #11359.

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.

@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (d7dd881) 56.49% compared to head (9cb0041) 56.49%.

❗ Current head 9cb0041 differs from pull request most recent head e6f018e. Consider uploading reports for the commit e6f018e 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   #12539   +/-   ##
=======================================
  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
Copy link
Member

I really don't want the bindings to process the toml file. The goal is to move the extra processing out of the bindings, not create more.

@bonigarcia
Copy link
Member Author

For that reason, I sent two commits in this PR. If the bindings do not want to process the config file (or the envs), the first commit should be enough.

@diemol
Copy link
Member

diemol commented Aug 14, 2023

I agree, only env vars I would say. No TOML config read for now.

Also, this class should give you the Selenium version

@bonigarcia
Copy link
Member Author

I have just sent two additional commits to this PR with the requested changes (read env but not config file and use of BuildInfo to get the Selenium version instead of a constant).

@titusfortner titusfortner added this to the 4.13 milestone Aug 23, 2023
@titusfortner
Copy link
Member

The Java & Ruby implementations are slightly different and we need to make sure we have what we want, and then implement in the others.

Let's prioritize this as primary goal for 4.13

@titusfortner
Copy link
Member

I have no idea what we did that fixed it, but thanks for letting us know.

@diemol
Copy link
Member

diemol commented Sep 3, 2023

It seems that browsers are left open, which could be the consequence of a bug in your code.

Can you please open a new issue and fill out all the information required in the template? With that, we can reproduce the issue.

@diemol
Copy link
Member

diemol commented Sep 3, 2023

We can only troubleshoot your issue if there is a way to reproduce it.

@titusfortner
Copy link
Member

How does this work with when the cache location is not writeable? Do we need to fall back to streaming it?

@bonigarcia
Copy link
Member Author

Yes, that case was not considered, so it will fail when the cache is not writable in the current implementation. Let me review this PR to consider that case.

@bonigarcia
Copy link
Member Author

I've added a new patch to this PR to consider when the cache path is not writable. In that case, the Selenium Manager binary will be extracted in a temporal folder and used from there. That binary is not deleted using the original shutdown hook to avoid the problem related to grid (see #12802).

@titusfortner
Copy link
Member

Will that potentially result in a bunch of Selenium Manager binaries in temp files? (but only if cache directory isn't writeable)

@bonigarcia
Copy link
Member Author

No problem. I added a new commit to restore the shutdown hook to delete the SM binary only when it is in the temporal folder.

@bonigarcia bonigarcia merged commit d8b9333 into trunk Oct 18, 2023
39 of 43 checks passed
@bonigarcia bonigarcia deleted the java_sm_cache branch October 18, 2023 07:38
aguspe pushed a commit to aguspe/selenium that referenced this pull request Oct 22, 2023
…HQ#11359) (SeleniumHQ#12539)

* [java] Copy SM binary to cache folder and use it from there

* [java] Read cache path from the config file and as env

* [java] Read cache-path as env only

* [java] Use BuildInfo class to get current Selenium version

* [java] if cache path is not writable, SM will be extracted to a temporal folder

* [java] Include shutdown hook again to delete binary when stored in tmp folder
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[🚀 Feature]: Cache Selenium Manager
4 participants