search

sitespeedio / browsertime

Measure and Optimize Web Performance
https://www.sitespeed.io/documentation/browsertime/
Apache License 2.0
600 stars 137 forks source link

Safari Technology Preview not being recognized as a browser? #2114

Closed 92kns closed 4 months ago

92kns commented 5 months ago

Your question

It is possible I am doing something wrong.

When using the following flags --browser safari and --safari.useTechnologyPreview I get the following error

 safari failed to start, trying 2 more time(s): Could not create a session: Browser name does not match (requested: safari; available: Safari Technology Preview

Possible bug? Safari-tp should be part of the Safari group right?

The driver isn't being configured here it seems https://github.com/sitespeedio/browsertime/blob/d17537bdffb6ca9e18e3933978b4013e9650307b/lib/core/seleniumRunner.js#L84-L89

which goes here https://github.com/sitespeedio/browsertime/blob/d17537bdffb6ca9e18e3933978b4013e9650307b/lib/core/webdriver/index.js#L43-L46

but this file seems fine https://github.com/sitespeedio/browsertime/blob/d17537bdffb6ca9e18e3933978b4013e9650307b/lib/safari/webdriver/builder.js since builder seems to return just fine. So I am a bit unsure why it is failing.

it is worth mentioning that I have ran safaridriver --enable with the safaridriver that comes bundled within safari technology preview, and if I manually open safari-tp myself, the automation field in the developer setting is all checked.

any ideas what I might be doing wrong?

soulgalore commented 5 months ago

Hi @92kns hmm I think something has changes in either Selenium or Safaridriver. This used to work but we do not have any automatic testing for it, I need to check how we can install the preview on a GitHub Action.

I could reproduce what you get. I looked in the Selenium code and this seems to fix setting the correct driver: https://github.com/SeleniumHQ/selenium/blob/864089606c3780574630d28c0173cb567516b8cf/javascript/node/selenium-webdriver/safari.js#L119-L122

I couldn't find where "Browser name does not match" comes from so I guess it's from Safaridriver? I tried to hack to pass on the preview name into Selenium but that did't help. The next step is to make a small reproducible test case for Selenium and see if we get the same thing.

soulgalore commented 5 months ago

I think maybe this is a bug in the NodeJS flavour of Selenium but I need to make a test case to test it. Looking at the Selenium code for Python that handles the browser name "Safari Technology Preview" but I couldn't find the same in NodeJS.

92kns commented 5 months ago

@soulgalore thanks for taking a look!

That error also threw me off.

After some further digging, in the NodeJS part of selenium here https://github.com/SeleniumHQ/selenium/blob/12344fe74af55ee6731ccda860b63fd77e6ffd96/javascript/node/selenium-webdriver/safari.js#L65 It seems safari is "hard coded" here, when it should be Safari Technology Preview when running safari-tp

and here there is no option for that https://github.com/SeleniumHQ/selenium/blob/12344fe74af55ee6731ccda860b63fd77e6ffd96/javascript/node/selenium-webdriver/lib/capabilities.js#L31-L37

On the browsertime side I think we could just add o.setBrowserName("Safari Technology Preview") here in between https://github.com/sitespeedio/browsertime/blob/d17537bdffb6ca9e18e3933978b4013e9650307b/lib/safari/webdriver/builder.js#L46-L47? and that seems to work

I can submit a PR for that shortly

soulgalore commented 4 months ago

Thank you @92kns for the fix!