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

Use the Safari 10 WebDriver to run tests #22

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

RLovelett
Copy link

@RLovelett RLovelett commented Mar 3, 2017

For all the reasons addressed in a WebKit blog post, https://webkit.org/blog/6901/webdriver-support-in-safari-10/, this method should be preferred over the previous method.

How does this work?

  1. It launches /usr/bin/safaridriver on a specified port (defaults to 4444).
  2. It then "ping"s the safaridriver until it starts accepting incoming connections.
  3. Creates a new WebDriver session (e.g., opens Safari).
  4. Navigates to the Karma provided URL.

Caveats

By going this route the launcher named Safari now only supports Safari 10 on OS X El Capitan and newer. SafariLegacy works as it currently does.

Notes

I'm not thrilled with the retry method. It just does not feel right. Though I'm not sure how else to know when the safaridriver is fully launched and accepting incoming connections.

This should address the concerns of both #6 and #20.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@RLovelett
Copy link
Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@RLovelett RLovelett force-pushed the safari-webdriver branch 2 times, most recently from 690fe97 to c8ce913 Compare March 3, 2017 02:50
@dignifiedquire
Copy link
Member

To make it possible to test both old and new safari versions could you keep the current one as a legacy launcher? similiar to how the chrome-launcher exposes multiple different launchers

@RLovelett
Copy link
Author

I think I understand how that works in the chrome-launcher; so I can give that a try. Do you have any suggestions for names?

The reason why I ask is that in the chrome-launcher it truly launches different applications. Yet it this instance it's really launching different versions of the same browser. Technically the "legacy" version covers all versions of Safari that the new one does while the new one only covers Safari 10 and newer.

Suggestions:

  • SafariWebDriver and Safari
  • Safari and SafariLegacy
  • ???

@dignifiedquire
Copy link
Member

I think Safari and SafariLegacy are pretty good

Mischa1610 added a commit to sovanta/karma-safari-launcher that referenced this pull request Oct 2, 2017
…lution).

Merge branch 'safari-webdriver' into develop

* safari-webdriver:
  Use the Safari 10 WebDriver to run tests

# Conflicts:
#	index.js
#	package.json
@RLovelett RLovelett force-pushed the safari-webdriver branch 2 times, most recently from 26a9695 to 2623b3e Compare August 16, 2018 16:52
For all the reasons addressed in a WebKit blog post,
https://webkit.org/blog/6901/webdriver-support-in-safari-10/,
this method should be preferred over the previous method.
@RLovelett
Copy link
Author

RLovelett commented Aug 16, 2018

@dignifiedquire better late than never right? 😏

I've incorporated the feedback. There are now two exported launchers Safari and SafariLegacy.

@shinnn
Copy link

shinnn commented Sep 25, 2018

When I tried to use this branch with macOS Mojave 10.14 and Safari 12.0, Karma threw the following error.

Error: [init({"browserName":"safari"})] The environment you requested was unavailable.
Request body does not contain required parameter 'capabilities'.

SeleniumHQ/selenium#6026 (comment) is the reason why it stopped working:

This issue happens because Safari 12 uses a new (W3C) webdriver protocol which appears incompatible with selenium-webdriver v3.6

We should pass --legacy flag to safaridriver somehow, I'm not familiar with the API of wd module though.

@RLovelett
Copy link
Author

The biggest problem is that is not a backwards compatible solution. We cannot globally just pass --legacy because on Safari 10/11 it does not understand that flag and exits with a non-zero status code. That leaves needing to gate the flag based on Safari version.

Not thrilled with that. Better yet would be to update the wd library to support W3C or to use a new library all-together that supports W3C.

Update to a pre-release version of WD to add suppor for the W3C
WebDriver protocol. This means that it supports Safari 10-12 with the
same configuration.
@RLovelett
Copy link
Author

@shinnn I've added a commit, 5b77a2f, in my environment I am now able to support Safari 10-12 with this change.

@mgol
Copy link

mgol commented Jan 16, 2019

One concern with using safaridriver is that the window is not interactive so you cannot click Debug, open the inspector etc. That may sometimes be necessary so I wonder if we should support running new Safari the old way as well (not as a default)?

For example, jQuery's Karma setup requires clicking Debug to actually run the tests.

@RLovelett
Copy link
Author

... so I wonder if we should support running new Safari the old way as well (not as a default)?

This change does just that. The old way is now called SafariLegacy as discussed above.

One concern with using safaridriver is that the window is not interactive so you cannot click Debug, open the inspector etc.

It is still possible in the new one. I have personally made use of "breaking the glass pane" over the automation window as discussed in the WebKit.org article: WebDriver Support in Safari 10. Granted it is a bit more clicking and as I recall required me to use some debugger statements but it was possible.

@pdesjardins90
Copy link

@dignifiedquire can you merge this PR?

@gergo-papp
Copy link

Hey @RLovelett,
I tried to test this change on Travis and I can't seem to get it to work. I've seen from previous comments that this is a known issue but I think we should make sure this works without requiring any manual configuration. The most important use case should be supporting this on a CI (such as on OSX running on TravisCI) - at least that's my opinion...

Some further info in case you need it:

I used "karma-safari-launcher": "git://github.com/RLovelett/karma-safari-launcher.git#safari-webdriver" in my package.json file and tried to launch Safari 11.0.3 (Mac OS X 10.13.3) on Travis CI.

I get the following error:

15 02 2019 10:08:11.884:INFO [karma-server]: Karma v3.1.1 server started at http://0.0.0.0:8082/
15 02 2019 10:08:11.897:INFO [launcher]: Launching browsers Safari with concurrency unlimited
15 02 2019 10:08:11.988:INFO [launcher]: Starting browser Safari via WebDriver at http://127.0.0.1:4444/

Could not create a session: You must enable the 'Allow Remote Automation' option in Safari's Develop menu to control Safari via WebDriver.
    at /Users/travis/build/ni-kismet/helium-webserver/content/node_modules/wd/lib/webdriver.js:148:15
    at Request._callback (/Users/travis/build/ni-kismet/helium-webserver/content/node_modules/wd/lib/http-utils.js:89:7)
    at Request.self.callback (/Users/travis/build/ni-kismet/helium-webserver/content/node_modules/request/request.js:185:22)
    at Request.emit (events.js:197:13)
    at Request.EventEmitter.emit (domain.js:446:20)
    at Request.<anonymous> (/Users/travis/build/ni-kismet/helium-webserver/content/node_modules/request/request.js:1161:10)
    at Request.emit (events.js:197:13)
    at Request.EventEmitter.emit (domain.js:446:20)
    at IncomingMessage.<anonymous> (/Users/travis/build/ni-kismet/helium-webserver/content/node_modules/request/request.js:1083:12)
    at Object.onceWrapper (events.js:285:13)
    at IncomingMessage.emit (events.js:202:15)
    at IncomingMessage.EventEmitter.emit (domain.js:446:20)
    at endReadableNT (_stream_readable.js:1129:12)
    at processTicksAndRejections (internal/process/next_tick.js:76:17)
  data:
   `{"status":33,"sessionId":"","value":{"message":"Could not create a session: You must enable the 'Allow Remote Automation' option in Safari's Develop menu to control Safari via WebDriver."}}` }
15 02 2019 10:09:12.041:WARN [launcher]: Safari via WebDriver at http://127.0.0.1:4444/ have not captured in 60000 ms, killing.

I used karma start karma.conf.js --single-run --browsers Safari to start my karma tests.

As far as I understand webdriver support is not enabled by default but I don't know how to set it programatically. I also think this should be set by karma-safari-launcher (if it's possible).

Or am I missing something?
Thanks!

@RLovelett
Copy link
Author

RLovelett commented Feb 26, 2019

@gergo-papp this is the relevant part of the error message:

Could not create a session: You must enable the 'Allow Remote Automation' option in Safari's Develop menu to control Safari via WebDriver.

That seems to indicate that you have not configured Safari to enable support for the web driver. There are a few articles out there: Testing with WebDriver in Safari: Configure Safari to Enable WebDriver Support and WebDriver Support in Safari 10.

The steps are different depending on the version of Safari you have but on the latest versions it is effectively just safaridriver --enable.

Hopefully that helps.

@gergo-papp
Copy link

@RLovelett thanks for the response. In this case I’m happy with this change. Is this repo still maintained? I haven’t seen too much activity

@RLovelett
Copy link
Author

Is this repo still maintained?

That is a good question. I think so but I could be wrong.

@LeviticusMB
Copy link

Can somebody please merge this and get it out on NPM ASAP?

@mgol
Copy link

mgol commented Aug 21, 2019

@dignifiedquire Any chance of merging this?

@mgol
Copy link

mgol commented Jan 8, 2020

@dignifiedquire Could you merge the PR?

package.json Outdated Show resolved Hide resolved
@LeviticusMB
Copy link

FYI: I've published @onslip/karma-safari-launcher from https://github.com/Onslip/karma-safari-launcher/tree/onslip.

@taymoork2
Copy link

So I've been using the code from this PR for a while now and it runs fine.
I use npm so I've been using patch-package to patch the changes made in this PR to the package installed in my repo.
You can manually create the patch using the package or just copy the patch I've linked below into a patches folder.
I believe yarn has built in patching

https://gist.github.com/taymoork2/8eb2be290579467f0d0090671e031ca8

@leobalter
Copy link

Seconding @mgol requests...

@dignifiedquire Could you merge the PR, please?

@johnjbarton
Copy link

Any volunteers to maintain this repo?

@leobalter
Copy link

I mean, I'd be happy to merge and publish this if I have access to the npm package. I'd prefer to share this with @mgol if he's +1 and has the bandwidth, considering the work he's done at the jQuery foundation.

@mgol
Copy link

mgol commented Mar 15, 2021

I'd prefer to share this with @mgol if he's +1 and has the bandwidth

I'm not able to commit to a timely feedback to issues but at least looking at available PRs and an occasional publish should be fine, especially if shared with you, @leobalter.

@LeviticusMB
Copy link

FYI: I've published @onslip/karma-safari-launcher from https://github.com/Onslip/karma-safari-launcher/tree/onslip.

FYI again: We've switched to Onslip/karma-playwright-launcher#v0.2.0 and I don't think we'll ever go back to testing against real Safari (or any other browser, for that matter). Playwright as been rock solid for us and I can't say I miss all the communication errors, hangups or failed unit tests because of Safaris aggressive low-power mode ...

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

Successfully merging this pull request may close these issues.