[rust] Include support for Safari in Selenium Manager #11609

bonigarcia · 2023-02-01T16:24:40Z

Description

This PR implements the support of Safari in Selenium Manager, as requested in #11522.

In addition to the stable release of Safari, and according to its resources page, there are two more distributions of Safari: beta, and Technology Preview. For symmetry with the rest of the browser (Chrome, Firefox, etc.), the "technology preview" is considered in Selenium Manager using the dev label (for browser version). The beta version is also considered.

Here it is some example executed in macOS (with Safari stable and Technology Preview installed):

bonigarcia@SL-2030 rust % cargo run -- --browser safari --debug     
    Finished dev [unoptimized + debuginfo] target(s) in 0.10s
     Running `target/debug/selenium-manager --browser safari --debug`
DEBUG	Using shell command to find out safari version
DEBUG	Running sh command: "/usr/libexec/PlistBuddy -c \"print :CFBundleShortVersionString\" /Applications/Safari.app/Contents/Info.plist"
DEBUG	Output { status: ExitStatus(unix_wait_status(0)), stdout: "16.1\n", stderr: "" }
DEBUG	The version of safari is 16.1
DEBUG	Required driver: safaridriver (local)
INFO	/usr/bin/safaridriver

bonigarcia@SL-2030 rust % cargo run -- --browser safari --debug --browser-version dev
    Finished dev [unoptimized + debuginfo] target(s) in 0.09s
     Running `target/debug/selenium-manager --browser safari --debug --browser-version dev`
DEBUG	Using shell command to find out safari version
DEBUG	Running sh command: "/usr/libexec/PlistBuddy -c \"print :CFBundleShortVersionString\" /Applications/Safari\\ Technology\\ Preview.app/Contents/Info.plist"
DEBUG	Output { status: ExitStatus(unix_wait_status(0)), stdout: "16.4\n", stderr: "" }
DEBUG	The version of safari is 16.4
DEBUG	Required driver: safaridriver (local)
INFO	/Applications/Safari Technology Preview.app/Contents/MacOS/safaridriver

Motivation and Context

Part of Selenium Manager M3, requested in #11522.

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.

titusfortner · 2023-02-01T17:01:46Z

Does beta come with its own driver? If so bindings don't support that right now, and should. Also would need to figure out what browser name that driver expects. I can check later when back at my laptop.
Does manager support using technology preview when the browser name is "Safari Technology Preview?" that's how Apple differentiates it right now.

bonigarcia · 2023-02-01T18:03:00Z

Does beta come with its own driver? If so bindings don't support that right now, and should. Also would need to figure out what browser name that driver expects. I can check later when back at my laptop.

No. I cannot install the current Safari beta in my macOS (since the beta is for Monterrey and mine is Ventura), but I investigated the content of the package of the current Safari beta (called Safari16.3MoterreySeed.pkg), and there is no safaridriver in it. I found only a different safaridriver the Technology Preview package.

Does manager support using technology preview when the browser name is "Safari Technology Preview?" that's how Apple differentiates it right now.

No. As I said, in Selenium Manager, for symmetry with the rest of the browsers, the "technology preview" is considered in Selenium Manager using the browser version dev. I don't like having a browser name called Safari Technology Preview in Selenium Manager, at the same level of other browsers (Chrome, Firefox, etc.), independently that Apple calls it like that. It would broke the design of Selenium Manager. It does not worth it. And the only impact for the Selenium Manager users (the bindings, I suppose) is calling it through --browser-version dev.

titusfortner · 2023-02-01T18:57:50Z

Except that is how selenium bindings differentiate it. Either the logic goes in the manager or it has to be created in each of the bindings, and removing the need to have special safari logic in the bindings was the whole point of adding this functionality to Selenium Manager in the first place.

titusfortner · 2023-02-01T19:01:32Z

Hmm, maybe we can override the browser version in bindings as part of setting preview...

diemol · 2023-02-01T20:46:56Z

Yes, the bindings differentiate Safari and Safari Technology preview, but I am not sure if that was a correct decision. The driver has the same name, just in a different location.

I think it is fine to have it in Selenium Manager as dev, and we set that version in the bindings when the user wants Safari Technology Preview.

titusfortner · 2023-02-01T22:01:07Z

Only Java has two separate service classes, the rest just toggle the location based on the options.

Whatever we do, the goal is not to have special code for safari in any of the bindings. I think we need to understand how it will be implemented before merging this.

bonigarcia · 2023-02-01T22:40:38Z

@titusfortner In your original request you mentioned the following for Safari TP:

selenium-manager --browser "Safari\ Technology\ Preview"
selenium-manager --browser SafariTechnologyPreview
selenium-manager --browser SafariTP

Instead of that, I am proposing to use the following to get the same result:

selenium-manager --browser safari --browser-version dev

titusfortner · 2023-02-02T00:27:06Z

Right, I'm not sure we can make that work in all of the bindings without adding the custom code I want to avoid, but we can see.

diemol · 2023-02-02T10:24:54Z

Thinking more about this, I am not even sure I agree on merging this.

I understand the idea is to avoid the bindings hard coding the SafariDriver path location, but now we need to hard code dev somewhere in the bindings so Selenium Manager returns the path.

I remember the main idea of Selenium Manager was to download the drivers and in the future download the browsers. I perceive this type of work is not adding much value to the user and it is slowing us down in terms of reaching the point where we want to start working on browser download.

titusfortner · 2023-02-02T15:04:27Z

Was just looking at how the Grid chooses Technology Preview, and it also does it based on Browser name:

07:00:32.012 INFO [NodeOptions.report] - Adding Chrome for {"browserName": "chrome"} 12 times
07:00:32.013 INFO [NodeOptions.report] - Adding Firefox for {"browserName": "firefox"} 12 times
07:00:32.014 INFO [NodeOptions.report] - Adding Safari for {"browserName": "safari"} 1 times
07:00:32.015 INFO [NodeOptions.report] - Adding Safari Technology Preview for {"browserName": "Safari Technology Preview"} 1 times
07:00:32.016 INFO [NodeOptions.report] - Adding Edge for {"browserName": "MicrosoftEdge"} 12 times

I could change the bindings code to set "dev" as the browser version when someone specifies STP, but we'd also have to muck about in the grid code.

If we want to support Safari Technology Preview, Selenium Manager should treat STP as a separate browser the same way that Grid does.

diemol · 2023-02-02T16:21:06Z

My point was around having Selenium Manager doing this. The user still needs to download and install the browsers. And it is not that the bindings are having an extremely complicated logic to handle this.

Selenium Manager is supposed to stop the user from doing manual setups, but for STP they will still need to do it anyway.

titusfortner · 2023-02-02T18:15:01Z

The reason for my request was so that the common logic for managing all driver locations exists in the Selenium Manager instead of in the bindings. The bindings don't need conditionals or to store constants, the DriverFinder pings Selenium Manager with browser name, and Selenium Manager verifies the driver exists and provides the location.

diemol · 2023-02-03T22:13:37Z

OK, I guess we can go back and forth for a while with this. We need to find a way to merge this so we can move forward.

@bonigarcia would it be too complicated to add STP as a browser in Selenium Manager so we centralize this logic there?

After this, let's check https://github.com/orgs/SeleniumHQ/projects/5/views/1 and see which ones are a "must" before we dig into browser downloads.

bonigarcia · 2023-02-06T09:43:21Z

@diemol: As you know, Safari differs from the other browsers since its driver is shipped with the browser itself. This difference impacts the internal design of the Selenium Manager. You can see in the current PR that I needed to include several conditions to do differently for Safari (if !self.is_safari()).

Moreover, the current API exposed by Selenium Manager (which will be used as a crate, theoretically) has the methods get_manager_by_browser and get_manager_by_driver to get a manager by browser and driver name, respectively. My concern was the second method since both Safari and SafariTP have the same driver name (safaridriver). Besides, I don't particularly like to have two top-level Safari browsers (meaning, at the same level that Chrome-Edge-Firefox) since I said before, Safari is an anomaly w.r.t. to the rest of browsers (for Selenium Manager).

For all these reasons, and because SafariTP is basically a dev version of Safari (currently SafariTP is 16.4 and Safari is 16.1, and there is a beta in the middle), I decided to create the PR like it is now, i.e., reusing the --browser-version dev flag for SafariTP.

Being all this said, and answering your question, yes, it is perfectly possible to add another browser (SafariTP) in addition to Safari (i.e., a new browser with a different name). And it is not particularly difficult regarding the required code. But even including the SafariTP as a browser, the get_manager_by_driver will not work for SafariTP (i.e., the name safaridriver will be used only to identify Safari, and not SafariTP).

I can submit a new patch to this PR, and decide which option is better (i.e., two Safari browsers, or only one, using the dev version for SafariTP).

bonigarcia · 2023-02-06T10:11:56Z

Some output examples with the last patch:

bonigarcia@SL-2030 rust % cargo run -- --browser safari --debug                       
    Finished dev [unoptimized + debuginfo] target(s) in 0.09s
     Running `target/debug/selenium-manager --browser safari --debug`
DEBUG	Using shell command to find out safari version
DEBUG	Running command: "/usr/libexec/PlistBuddy -c \"print :CFBundleShortVersionString\" /Applications/Safari.app/Contents/Info.plist"
DEBUG	Output: "16.1"
DEBUG	The version of safari is 16.1
DEBUG	Required driver: safaridriver (local)
INFO	/usr/bin/safaridriver

bonigarcia@SL-2030 rust % cargo run -- --browser SafariTP --debug
    Finished dev [unoptimized + debuginfo] target(s) in 0.09s
     Running `target/debug/selenium-manager --browser SafariTP --debug`
DEBUG	Using shell command to find out safaritp version
DEBUG	Running command: "/usr/libexec/PlistBuddy -c \"print :CFBundleShortVersionString\" /Applications/Safari\\ Technology\\ Preview.app/Contents/Info.plist"
DEBUG	Output: "16.4"
DEBUG	The version of safaritp is 16.4
DEBUG	Required driver: safaridriver (local)
INFO	/Applications/Safari Technology Preview.app/Contents/MacOS/safaridriver

bonigarcia@SL-2030 rust % cargo run -- --browser SafariTechnologyPreview --debug 
    Finished dev [unoptimized + debuginfo] target(s) in 0.09s
     Running `target/debug/selenium-manager --browser SafariTechnologyPreview --debug`
DEBUG	Using shell command to find out safaritp version
DEBUG	Running command: "/usr/libexec/PlistBuddy -c \"print :CFBundleShortVersionString\" /Applications/Safari\\ Technology\\ Preview.app/Contents/Info.plist"
DEBUG	Output: "16.4"
DEBUG	The version of safaritp is 16.4
DEBUG	Required driver: safaridriver (local)
INFO	/Applications/Safari Technology Preview.app/Contents/MacOS/safaridriver

bonigarcia@SL-2030 rust % cargo run -- --browser "Safari\ Technology\ Preview" --debug
    Finished dev [unoptimized + debuginfo] target(s) in 0.09s
     Running `target/debug/selenium-manager --browser 'Safari\ Technology\ Preview' --debug`
DEBUG	Using shell command to find out safaritp version
DEBUG	Running command: "/usr/libexec/PlistBuddy -c \"print :CFBundleShortVersionString\" /Applications/Safari\\ Technology\\ Preview.app/Contents/Info.plist"
DEBUG	Output: "16.4"
DEBUG	The version of safaritp is 16.4
DEBUG	Required driver: safaridriver (local)
INFO	/Applications/Safari Technology Preview.app/Contents/MacOS/safaridriver

titusfortner · 2023-02-06T19:43:52Z

That output above is exactly what will help the selenium code.

So, fyi, Selenium bindings are going to stop using get_manager_by_driver approach entirely. Everything is going to come from Browser Options including: browserName, browserVersion, proxy, goog:chromeOptions: binary, moz:firefoxOptions: binary.

I kind of wish from the Selenium/Apple side we managed Safari Technology Preview as a browserVersion toggle instead of a browserName difference, but someone made a decision many years ago and we're still stuck with it. 😄

bonigarcia added 2 commits February 2, 2023 16:25

[rust] Include support for Safari in Selenium Manager

e7f3a61

[rust] Include separate test for Safari

9af252b

bonigarcia force-pushed the se_mgr_safari branch from 102f6a2 to 9af252b Compare February 2, 2023 15:27

[rust] Fix Safari logic after latest refactoring

9ee98d5

[rust] Include SafariTP as a different browser than Safari

380ecac

bonigarcia force-pushed the se_mgr_safari branch from dcd04da to 380ecac Compare February 6, 2023 10:09

bonigarcia added the C-rust label Feb 20, 2023

bonigarcia requested a review from diemol February 20, 2023 15:42

diemol approved these changes Feb 20, 2023

View reviewed changes

bonigarcia merged commit 50d1ae9 into trunk Feb 21, 2023

bonigarcia deleted the se_mgr_safari branch February 21, 2023 08:12

bonigarcia mentioned this pull request Mar 15, 2023

[🚀 Feature]: Selenium Manager support Safari & Technology Preview #11522

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[rust] Include support for Safari in Selenium Manager #11609

[rust] Include support for Safari in Selenium Manager #11609

bonigarcia commented Feb 1, 2023

titusfortner commented Feb 1, 2023

bonigarcia commented Feb 1, 2023

titusfortner commented Feb 1, 2023

titusfortner commented Feb 1, 2023

diemol commented Feb 1, 2023

titusfortner commented Feb 1, 2023

bonigarcia commented Feb 1, 2023

titusfortner commented Feb 2, 2023

diemol commented Feb 2, 2023

titusfortner commented Feb 2, 2023

diemol commented Feb 2, 2023

titusfortner commented Feb 2, 2023

diemol commented Feb 3, 2023

bonigarcia commented Feb 6, 2023

bonigarcia commented Feb 6, 2023

titusfortner commented Feb 6, 2023

[rust] Include support for Safari in Selenium Manager #11609

[rust] Include support for Safari in Selenium Manager #11609

Conversation

bonigarcia commented Feb 1, 2023

Description

Motivation and Context

Types of changes

Checklist

titusfortner commented Feb 1, 2023

bonigarcia commented Feb 1, 2023

titusfortner commented Feb 1, 2023

titusfortner commented Feb 1, 2023

diemol commented Feb 1, 2023

titusfortner commented Feb 1, 2023

bonigarcia commented Feb 1, 2023

titusfortner commented Feb 2, 2023

diemol commented Feb 2, 2023

titusfortner commented Feb 2, 2023

diemol commented Feb 2, 2023

titusfortner commented Feb 2, 2023

diemol commented Feb 3, 2023

bonigarcia commented Feb 6, 2023

bonigarcia commented Feb 6, 2023

titusfortner commented Feb 6, 2023