CDP replacement candidate: direct python API for BiDi #934

martinpitt · 2024-07-24T10:20:35Z

This is another candidate for https://issues.redhat.com/browse/COCKPIT-1139 , in the similar spirit as #926

This talks to https://w3c.github.io/webdriver-bidi/ directly, requires zero new npm dependencies (in fact, we can drop chrome-remote-interface), and only a new package (chromedriver) in the tasks container (~~installed locally for this demo~~ added to official container now).

This is much further ahead than #926, in the sense of that it actually is a Python test, in our usual testlib shape. We won't need any extra node process to talk to a JS webdriver module any more, and they aren't any good ones for BiDi anyway.

Compared to CDP this has the following advantages:

Fewer moving parts and less code
At the API level, Firefox and Chromium behave the same, so we can drop our {firefox,chromium}-cdp-driver.js forks
Very low input API level, we can even simulate touch swipes and others. With CDP we just synthesize a MouseEvent, while this actually goes through the browser.

Hiccups:

Land and deploy tasks: Install chromedriver cockpituous#622 to use the packaged chromedriver (cockpit-ci: Update container to 2024-07-24 #935 ), then drop the local download
that input.performActions does not understand key names like "Return" or "ArrowUp". input.Key{Down,Up}Action value is undefined w3c/webdriver-bidi#757
clicks in frames miss their target in chrome (works fine with Firefox)
When showing the browser, the test sometimes crashes with something like "target is outside of viewport". needs scrollIntoView() or so (I remember this from Selenium)
Check how to enable coverage (if no API, then via a single CDP call or some other hack)
Test loading/using sizzle

https://issues.redhat.com/browse/COCKPIT-1154

martinpitt · 2024-07-24T10:32:23Z

@jelly @allisonkarlitskaya At this point it seems pretty clear to me that this approach is much better than #926 (where all of the required JS → Python glue isn't even written, it uses the outdated webdriver protocol, and requires umpteen new dependencies, and it's also slower).

This is demo code, so no need for a minutely review, but I'd like to hear your coarse-grained opinion about the approach. Thanks!

allisonkarlitskaya

This is a very cool piece of work.

I would prefer to see more separation between driver/session/context. It might be useful to think about sharing these things (but my experiments also show problems there, particularly with respect to each fresh pytest getting its own new asyncio event loop). So maybe the added complication is not helpful there.

I think the next thing I'd like to see is a version of this integrated into test/common/ in the cockpit repo....

allisonkarlitskaya · 2024-07-24T12:41:29Z

bidi-test.py

+        asyncio.run_coroutine_threadsafe(self.driver.start_session(), self.loop).result()
+
+    def close(self):
+        asyncio.run_coroutine_threadsafe(self.driver.close(), self.loop).result()


This run_coroutine_threadsafe() approach is very cool. Much better than the custom-built machinery I did in my version of this, and probably more efficient, too.

Thanks! I'm surprised that it surprised you 😉

allisonkarlitskaya · 2024-07-24T12:44:19Z

bidi-test.py

+    driver: bidi.WebdriverBidi
+
+    @staticmethod
+    def asyncio_loop_thread(loop: asyncio.AbstractEventLoop) -> None:


I generally like the approach of binding the bidi driver and the asyncio loop thread directly to the Browser object. I think that's probably right. Once we start interacting with the websocket in the asyncio loop running in that thread, we really ought not to touch it from any other context.

We can dream about sharing the session some day, but for now, this seems right.

Wrt. sharing the session: I share the sentiment. Starting up chromium is lighting fast, but starting firefox takes ~ 5 seconds annoyingly. It would be cool to leave the process running and just instantiate sessions. I didn't test how isolated they are from one another though -- standard "tab" isolation isn't enough (we need separate cookie/localStorage for each tab), but if it's more like "private window" that'd be great.

But that isn't a regression -- our current CDP library also starts firefox with each Browser instance, so for now I'd like to keep that in the backlog. This project is huge enough even without doing additional structural changes at the same time 😅

However, for your pytest work the picture is different -- you don't nearly need so much machinery there (no frame tracking, cookies, mouse clicks, all the testlib API, no firefox, no coverage, etc.). While we can certainly look at uniting the bidi driver stuff at some point, it may be easier to just keep it separate for the first version. WDYT?

allisonkarlitskaya · 2024-07-24T12:46:25Z

bidi.py

+    args: list[object]
+    text: str
+
+    def __init__(self, message_params):


How do you feel about the cockpit json helpers? I notice you have your own JsonObject defined above, and indeed, we can't import jsonutil from starter-kit. I wonder if we should move those helpers to a more neutral location where they can also get used by tests.

It's a bit of a weird situation. We might end up with two copies... or maybe we don't have to care about typing too much here.

Heh yes, had the same "wanna but can't" feeling. But TBH we just need it at exactly one place, I feel it's probably not worth sharing bridge with testlib code for that. Maybe if we run into more of these, but I don't see that coming -- the protocol side is basically "done", the remaining work is all on the "outwards" testlib side.

allisonkarlitskaya · 2024-07-24T12:48:34Z

bidi.py

+                if data["type"] == "event":
+                    if data["method"] == "log.entryAdded":
+                        log = LogMessage(data["params"])
+                        self.logs.append(log)


I think this isn't useful enough for qunit tests. This needs to be at least an async queue or something else that can be awaited.

Ah, sure -- cf. my question above with "does it already make sense at this point to use that exact API for the unit tests"? Do you have a pointer for what a unit test is trying to do that relies on waiting for log messages? There could certainly be one method for "set up a log watch future", and resolve it here if it's set up; is that enough?

See ./test/common/tap-cdp and cdp.read_log()

https://github.com/allisonkarlitskaya/cockpit/blob/test-server.py/test/pytest/webdriver_bidi.py#L81

bidi.py

martinpitt · 2024-07-24T13:38:03Z

I think the next thing I'd like to see is a version of this integrated into test/common/ in the cockpit repo....

Right, that's the plan. This is strictly "draft", mostly because it's a convenient place for a demo, and we wanted to compare different approaches.

Unfortunately the "interesting" commands like "click" don't have a BiDi command, and need classic webdriver.

That's what we do in CDP as well

Re-eliminate geckodriver. Turns out we can do everything through BiDi.

Thanks Lis for the idea!

Too small window sizes make the test unstable. Now it works reliably in a loop.

Hint from w3c/webdriver-bidi#418

This makes waiting for text robust. script.addPreloadScript() doesn't export the declared functions, so we need to attach them to `window`. We also don't need all of them.

This is all sync code, with sketch of what the updated Browser class looks like.

This better lives in the sync Browser class.

This works fine with Firefox, and conforms to the spec. However, Chromium gets confused and clicks on the wrong position. Work around that for now by keeping our old `ph_mouse()` event synthesizer for Chromium.

Use the same `document` tracking fix/hack as in our CDP driver.

martinpitt · 2024-07-27T01:18:57Z

Wow, it's very straightforward to talk to CDP. chromedriver already enables CDP and gives you the address in the returned capabilities. And looks like not only the bidi sessions/tabs appear there, they even have the exact same session IDs as bidi/webdriver:

❱❱❱ curl http://localhost:37773/json/list
[ {
   "description": "",
   "devtoolsFrontendUrl": "/devtools/inspector.html?ws=localhost:37773/devtools/page/9E5C285BA99CC696EE4FD2390D303029",
   "id": "9E5C285BA99CC696EE4FD2390D303029",
   "title": "fedora-40-127-0-0-2-2201",
   "type": "page",
   "url": "http://127.0.0.2:9091/",
   "webSocketDebuggerUrl": "ws://localhost:37773/devtools/page/9E5C285BA99CC696EE4FD2390D303029"
}, {
   "description": "",
   "devtoolsFrontendUrl": "/devtools/inspector.html?ws=localhost:37773/devtools/page/401D4B511DB168958EFF68E8F2F32087",
   "id": "401D4B511DB168958EFF68E8F2F32087",
   "title": "BiDi-CDP Mapper",
   "type": "page",
   "url": "data:,",
   "webSocketDebuggerUrl": "ws://localhost:37773/devtools/page/401D4B511DB168958EFF68E8F2F32087"
} ]

Unfortunately, websocat ws://localhost:37773/devtools/page/9E5C285BA99CC696EE4FD2390D303029 immediately fails with "websocat: WebSocketError: I/O failure" in both default text and binary (-b) mode. However, it works fine with aiohttp, and I can send commands and receive replies. I just wonder about this:

DEBUG:bidi.proto:CDP ← '{"id": 0, "method": "Profiler.enable", "params": {}}'
DEBUG:bidi.proto:CDP → {'id': 0, 'result': {}}
DEBUG:bidi.proto:CDP ← '{"id": 1, "method": "Profiler.startPreciseCoverage", "params": {"callCount": false, "detailed": true}}'
DEBUG:bidi.proto:CDP → {'id': 1, 'error': {'code': -32000, 'message': 'Profiler is not enabled'}}

That's pretty much exactly what CDP.start() does in our current code.. 🤔

I am currently trying to only connect to the CDP websocket when necessary, it seems a big waste to let it run all the time -- then we have to read all the messages and discard them sigh. But that's my next attempt,

This is not accessible via BiDi or webdriver, but fortunately the CDP and BiDi sessions are compatible.

martinpitt · 2024-07-27T19:22:04Z

Look ma, CDP Profiler (aka coverage) support! That was my final TODO item that I could think of. Now I think it's time to aim higher and put it into actual cockpit.

martinpitt · 2024-08-06T06:42:13Z

This was just a demo and has fulfilled its purpose. cockpit-project/cockpit#20832 is Ze Real Zing.

martinpitt closed this Jul 24, 2024

martinpitt reopened this Jul 24, 2024

martinpitt mentioned this pull request Jul 24, 2024

tasks: Install chromedriver cockpit-project/cockpituous#622

Merged

martinpitt force-pushed the direct-bidi branch from f590063 to fcf61c8 Compare July 24, 2024 12:22

allisonkarlitskaya reviewed Jul 24, 2024

View reviewed changes

martinpitt force-pushed the direct-bidi branch 2 times, most recently from 8d9eca3 to 0f31b7f Compare July 24, 2024 14:18

martinpitt added 15 commits July 24, 2024 17:15

first PoC of Python bidi runner

fb93356

collect log messages

e34f982

move to aiohttp

577ec26

add headless mode

7cc9b6c

API for classic webdriver; text + click

3df6155

Unfortunately the "interesting" commands like "click" don't have a BiDi command, and need classic webdriver.

Clean up open-coded driver startup

eb6fd8a

Clean up session tracking

025731f

Convert to BiDi click and add page load wait/check

3a8a279

Convert /text to bidi evaluate

a6c4ab0

That's what we do in CDP as well

Bring back firefox marionette

b9b9f0a

Re-eliminate geckodriver. Turns out we can do everything through BiDi.

Dynamic ports

23ff405

Thanks Lis for the idea!

Refactor bidi session

c1496f1

Refactor run and page helpers

4c18ab0

page wait load timeout and fix

6cd41a5

Robustify interactive browser

5b31add

Too small window sizes make the test unstable. Now it works reliably in a loop.

martinpitt force-pushed the direct-bidi branch 2 times, most recently from 218f7fa to ff3cb22 Compare July 26, 2024 14:24

martinpitt added 4 commits July 26, 2024 16:48

frame tracking and cockpit testing

6cae2c6

Hint from w3c/webdriver-bidi#418

log exceptions from ws_reader()

42b0311

screenshot on failure

ecb9420

Add test helpers

91be31b

This makes waiting for text robust. script.addPreloadScript() doesn't export the declared functions, so we need to attach them to `window`. We also don't need all of them.

martinpitt added 2 commits July 26, 2024 16:48

make bidi.py importable

5581ec4

Use custom high-level Error class like testlib.py

700037e

martinpitt force-pushed the direct-bidi branch from 855cdb2 to dc133f3 Compare July 26, 2024 14:48

martinpitt added 6 commits July 27, 2024 02:32

Add sync bidi-test.py

a68e5a0

This is all sync code, with sketch of what the updated Browser class looks like.

Drop obsolete bidi.py async code

5d89be7

This better lives in the sync Browser class.

add cockpit CI glue

130c7a0

Add input of single/special keys

9653a60

scroll element into view for mouse actions

49317d7

This works fine with Firefox, and conforms to the spec. However, Chromium gets confused and clicks on the wrong position. Work around that for now by keeping our old `ph_mouse()` event synthesizer for Chromium.

Add sizzle support

6c97825

Use the same `document` tracking fix/hack as in our CDP driver.

martinpitt force-pushed the direct-bidi branch from dc133f3 to 6c97825 Compare July 27, 2024 01:33

Add coverage measurement via CDP

8dc9256

This is not accessible via BiDi or webdriver, but fortunately the CDP and BiDi sessions are compatible.

martinpitt mentioned this pull request Jul 30, 2024

CDP replacement candidate: webdriver npm module bidi demo #926

Closed

martinpitt closed this Aug 6, 2024

martinpitt deleted the direct-bidi branch August 6, 2024 06:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CDP replacement candidate: direct python API for BiDi #934

CDP replacement candidate: direct python API for BiDi #934

martinpitt commented Jul 24, 2024 •

edited

Loading

martinpitt commented Jul 24, 2024

allisonkarlitskaya left a comment

allisonkarlitskaya Jul 24, 2024

martinpitt Jul 24, 2024

allisonkarlitskaya Jul 24, 2024

martinpitt Jul 24, 2024

allisonkarlitskaya Jul 24, 2024

martinpitt Jul 24, 2024

allisonkarlitskaya Jul 24, 2024

martinpitt Jul 24, 2024

martinpitt Jul 24, 2024

martinpitt Jul 24, 2024

martinpitt commented Jul 24, 2024

martinpitt commented Jul 27, 2024

martinpitt commented Jul 27, 2024

martinpitt commented Aug 6, 2024

CDP replacement candidate: direct python API for BiDi #934

CDP replacement candidate: direct python API for BiDi #934

Conversation

martinpitt commented Jul 24, 2024 • edited Loading

martinpitt commented Jul 24, 2024

allisonkarlitskaya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt commented Jul 24, 2024

martinpitt commented Jul 27, 2024

martinpitt commented Jul 27, 2024

martinpitt commented Aug 6, 2024

martinpitt commented Jul 24, 2024 •

edited

Loading