feat: added BrowserType.connect by mxschmitt · Pull Request #630 · microsoft/playwright-python

mxschmitt · 2021-04-19T15:16:43Z

Closes #529

Quite an early implementation, need to think about the last skipped test.

playwright/_impl/_browser_type.py

playwright/_impl/_transport.py

playwright/async_api/_generated.py

tests/async/test_browsertype_connect.py

tests/sync/test_browsertype_connect.py

tests/conftest.py

kumaraditya303 · 2021-04-22T04:19:16Z

I was able to fix the last skipped test test_browser_type_connect_should_reject_navigation_when_browser_closes with this code, what it does essentially is runs the greenlet nested with other greenlets hence it does not block, you can try the code given below also it is passing the tests locally to me:

def test_browser_type_connect_should_reject_navigation_when_browser_closes(
    server: Server, browser_type: BrowserType, launch_server
):
    import greenlet

    remote_server = launch_server()
    browser = browser_type.connect(remote_server.ws_endpoint)
    page = browser.new_page()
    server.set_route("/one-style.css", lambda r: None)
    g = greenlet.greenlet(browser.close)
    page.on("request", lambda: g.switch())

    with pytest.raises(Error) as exc_info:
        page.goto(server.PREFIX + "/one-style.html")
    assert "Playwright connection closed" in exc_info.value.message

playwright/_impl/_transport.py

tests/conftest.py

kumaraditya303

The failing tests seems flaky

mxschmitt · 2021-04-22T14:06:36Z

Let me summarize the test failures:

test_wait_for_url_should_work -> not related to this PR, failed before needs to be checked some day
test_browser_type_connect_should_reject_navigation_when_browser_closes (all browsers which happens sometimes, and sync and async)-> flaky, can't repro locally -> fixed

So its only the second one which is relevant but causes 3 bots to fail. Will take a more detailed look now.

pavelfeldman · 2021-04-22T19:07:59Z

playwright/_impl/_browser.py

        return page

    async def close(self) -> None:
+        if self._is_connected_over_websocket:


Do you want to move it to under _is_closed_or_closing = True? Also, why is logic different from the one in TS? Does TS close the browser on browser.close?

To me, the ideal workflow would be: close terminates the underlying connection, connection termination calls _on_close everywhere. That way, terminating connection due to networking issues would result in the same proper cleanup.

move it below it.

TS does it via an event, see here in another place: https://github.com/microsoft/playwright/blob/fff1f3d45c243bec65c7e6c43e6fe5e36607dc59/src/client/browserType.ts#L163-L179

pavelfeldman · 2021-04-22T19:10:47Z

playwright/_impl/_browser.py

                raise e

+    def _notify_remote_closed(self) -> None:
+        # Emulate all pages, contexts and the browser closing upon disconnect.


I.e I would expect this to be called on web socket disconnect somehow.

the naming came originally from Java, but open to change.

So I looked at the TS version, looks like you don't need any special code here, just close the browser. But our server seems to be missing the connection termination upon close for some reason.

playwright/_impl/_browser_type.py

playwright/_impl/_connection.py

playwright/_impl/_transport.py

pavelfeldman · 2021-04-22T19:33:03Z

playwright/_impl/_transport.py

+
+    def stop(self) -> None:
+        self._stopped = True
+        self._loop.create_task(self._connection.close())


Declare self._connection in init, did our mypy break?

Yes its broken currently (only the impl folder - most important one...), will follow up to fix it.

It can automatically infer that I set it in WebSocketTransport.run()

But yes your assumption was also correct that mypy was broken. Fixed in #643

pavelfeldman · 2021-04-22T19:41:50Z

playwright/_impl/_browser_type.py

+        pre_launched_browser = playwright._initializer.get("preLaunchedBrowser")
+        assert pre_launched_browser
+        browser = cast(Browser, from_channel(pre_launched_browser))
+        browser._is_remote = True


Now that you are adding it, we need to introduce the code that depends on it. That would be our artifacts, namely 'download' and 'video'.

good catch, added it.

tests/async/test_browsertype_connect.py

local-requirements.txt

pavelfeldman · 2021-04-23T16:14:47Z

playwright/_impl/_browser.py

        self._is_connected = True
        self._is_closed_or_closing = False
        self._is_remote = False
+        self._is_connected_over_websocket = False


It seems like you can use is_remote here.

remote is also used for connect_over_cdp, we only want to close the underlying connection when connect is used.

pavelfeldman · 2021-04-23T16:17:15Z

playwright/_impl/_browser_type.py

+        assert pre_launched_browser
+        browser = cast(Browser, from_channel(pre_launched_browser))
+        browser._is_remote = True
+        browser._is_connected_over_websocket = True


Yeah, it looks the same as is_remote...

pavelfeldman · 2021-04-23T16:18:33Z

playwright/_impl/_browser_type.py

+        browser._is_remote = True
+        browser._is_connected_over_websocket = True
+
+        transport.once("close", browser._on_close)


nit: this looks like a layering violation, should go transport -> connection -> browser. Not a big deal though.

playwright/_impl/_connection.py

x0day · 2021-04-25T02:35:38Z

I check the code from master(9d79662), but BrowserType.connect not handle the ConnectionRefusedError ?

import asyncio
from playwright.async_api import async_playwright


async def main():
    async with async_playwright() as p:
        browser = await p.chromium.connect("ws://127.0.0.1:12345/9c2251a08a8f1092b72af9b0f9d035bf")
        print(browser)
        page = await browser.new_page()
        print(page)
        await page.goto('http://whatsmyuseragent.org/')
        await browser.close()

asyncio.run(main())

mxschmitt · 2021-04-25T06:16:35Z

Good catch, will take a look.

kumaraditya303 suggested changes Apr 19, 2021

View reviewed changes

kumaraditya303 reviewed Apr 20, 2021

View reviewed changes

tests/async/test_browsertype_connect.py Outdated Show resolved Hide resolved

kumaraditya303 mentioned this pull request Apr 20, 2021

feat: firefox-stable channel #632

Merged

mxschmitt force-pushed the feature/browser-type-connect branch 2 times, most recently from d2cd2df to e0c873d Compare April 21, 2021 08:19

kumaraditya303 reviewed Apr 21, 2021

View reviewed changes

tests/async/test_browsertype_connect.py Show resolved Hide resolved

mxschmitt force-pushed the feature/browser-type-connect branch from e0c873d to d212d33 Compare April 21, 2021 08:57

mxschmitt commented Apr 21, 2021

View reviewed changes

tests/sync/test_browsertype_connect.py Outdated Show resolved Hide resolved

mxschmitt force-pushed the feature/browser-type-connect branch from d212d33 to 7895112 Compare April 21, 2021 09:11

kumaraditya303 reviewed Apr 21, 2021

View reviewed changes

tests/conftest.py Outdated Show resolved Hide resolved

mxschmitt force-pushed the feature/browser-type-connect branch 5 times, most recently from 5cb2cd3 to bd6820d Compare April 21, 2021 10:15

microsoft deleted a comment from mxschmitt Apr 21, 2021

mxschmitt force-pushed the feature/browser-type-connect branch 3 times, most recently from ad3eebb to d779566 Compare April 21, 2021 13:04

mxschmitt marked this pull request as ready for review April 21, 2021 13:53

mxschmitt force-pushed the feature/browser-type-connect branch from 9322212 to 8b5a21f Compare April 21, 2021 15:10

kumaraditya303 reviewed Apr 22, 2021

View reviewed changes

playwright/_impl/_transport.py Outdated Show resolved Hide resolved

kumaraditya303 reviewed Apr 22, 2021

View reviewed changes

playwright/_impl/_transport.py Show resolved Hide resolved

kumaraditya303 reviewed Apr 22, 2021

View reviewed changes

tests/conftest.py Outdated Show resolved Hide resolved

mxschmitt force-pushed the feature/browser-type-connect branch from 02bb278 to 4b6490b Compare April 22, 2021 09:38

kumaraditya303 approved these changes Apr 22, 2021

View reviewed changes

mxschmitt force-pushed the feature/browser-type-connect branch from 927f992 to a7847ed Compare April 22, 2021 16:05

pavelfeldman reviewed Apr 22, 2021

View reviewed changes

kumaraditya303 reviewed Apr 23, 2021

View reviewed changes

tests/async/test_browsertype_connect.py Outdated Show resolved Hide resolved

kumaraditya303 reviewed Apr 23, 2021

View reviewed changes

local-requirements.txt Show resolved Hide resolved

mxschmitt added 9 commits April 23, 2021 14:52

feat: added BrowserType.connect

3dcc411

fixed tests

be149ed

debug

b132745

fix sync test

54271cc

debug flaky tests

9071e43

idea

f9ddbdb

fix flaky test

89ffa16

nits

7fbce4b

reworked closing logic

ae908a3

mxschmitt force-pushed the feature/browser-type-connect branch from 79b8d72 to ae908a3 Compare April 23, 2021 12:55

added artifacts/video remote handling

3349e4d

mxschmitt force-pushed the feature/browser-type-connect branch from d4adea9 to 3349e4d Compare April 23, 2021 15:45

pavelfeldman approved these changes Apr 23, 2021

View reviewed changes

fix tests

140ad2e

mxschmitt merged commit 9d79662 into microsoft:master Apr 24, 2021

x0day mentioned this pull request Apr 26, 2021

[internal] hang bug in PlaywrightContextManager and Transport #650

Closed

Conversation

mxschmitt commented Apr 19, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kumaraditya303 commented Apr 22, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kumaraditya303 left a comment

Choose a reason for hiding this comment

Uh oh!

mxschmitt commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mxschmitt Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

x0day commented Apr 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mxschmitt commented Apr 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mxschmitt commented Apr 22, 2021 •

edited

Loading

mxschmitt Apr 22, 2021 •

edited

Loading

x0day commented Apr 25, 2021 •

edited

Loading