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

adds support for BrowserType.connect_over_cdp() (for chromium browser only) #37

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

oliverswitzer
Copy link

@oliverswitzer oliverswitzer commented Feb 7, 2022

This PR adds support for playwright's BrowserType.connectOverCdp() function, which attaches Playwright to an existing browser instance using the Chrome DevTools Protocol.

  • Adds httpoison as test dependency in order to fetch WS url from chrome debugger tools endpoint
  • Implements BrowserType.connect_over_cdp
  • Add skipped test that makes sure connect_over_cdp raises if using a different browser than chromium

TODOs

  • Add support for passing remaining options (headers,logger, slowMo, timeout)
  • Figure out a way to test that connect_over_cdp/3 can only be invoked when using chromium client

# For some reason this still launches a chromium browser... I could not
# figure out why.

browser = Playwright.launch(:firefox)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coreyti any thoughts about how to better test this error case? I couldn't quite figure out why but it appears that no matter whether I call Playwright.launch() with (ie :webkit or :firefox) I still get a chromium %Playwright.Browser{} back.

Perhaps this is just something to note and move on from since technically this library only supports chromium at the moment.

@oliverswitzer oliverswitzer requested a review from coreyti February 7, 2022 03:38
@coreyti
Copy link
Member

coreyti commented Feb 9, 2022

Hey @oliverswitzer, I'm getting test suite timeouts when running mix test test/integration and with the ConnectCDPTest. Those failures go away if I run that test alone, or if I remove that test and run the whole suite. I haven't spent a lot of time trying to figure out why, but perhaps we have some test cleanup issue.

Regarding the Playwright.launch with other than Chromium, I'll take a look into that. It's not too surprising that things are not working as expected there because I've not spent any time yet on browsers other than Chromium. BrowserType could also use some improvement to come more in-line with official implementations... it might be about time to do that work :)

Copy link
Member

@coreyti coreyti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing... see comment above.

@oliverswitzer
Copy link
Author

Seems that @coreyti and I narrowed the test issue down to setup_browser getting called directly from test/integration/browser_test.exs:8

It is the only test that calls setup_browser, and we are able to reproduce the failing test suite when running test/integration/browser_type/connect_cdp_test.exs before test/integration/browser_test.exs

@oliverswitzer
Copy link
Author

oliverswitzer commented Feb 11, 2022

After pairing a bit more through this with @coreyti, it became apparent that browser_test was not the culprit and that more tests started failing when run right after connect_over_cdp_test.exs if we were to comment out that specific test.

From poking around in the playwright-python and playwright-js client code, we discovered that each of the other libraries seem to have instances of BrowserType that are isolated from each other and have their own channels/state.

The playwright server sends __create__ messages for a new browser, and all the pages and contexts that already exists on the playwright browser you are connecting to over CDP. As implemented in in playwright-elixir right now, connect_over_cdp/3 adds these objects to the same session catalog as the browser you passed into the function.

This duplication in the same catalog may be leading to some of the order-dependent test failures that we're seeing. It seems like our implementation of connectOverCDP should probably spin up a separate playwright session/catalog/supervision tree instead of sharing a session / catalog with the browser that gets passed into connect_over_cdp/3.

NOTES:
* Fixes bug where Channel.list was not correctly filter on parent guid
* Adds HTTPoison in test only

TODOs:
* Adds skipped test that makes sure connect_over_cdp raises if using a
  different browser than chromium. Currently not working because
  Playwright.launch(:firefox) still returns a chromium browser for some
  reason
@oliverswitzer oliverswitzer force-pushed the add-connect-cdp-support branch from 5baae86 to 4983ed5 Compare February 11, 2022 22:44
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.

2 participants