fix: use own cdp implementation #1129

KuznetsovRoman · 2025-09-05T16:24:39Z

No description provided.

pkg-pr-new · 2025-09-05T20:56:20Z

npm i https://pkg.pr.new/gemini-testing/testplane@1129

commit: bb74561

KuznetsovRoman · 2025-09-08T22:09:38Z

package.json

    "vite": "5.1.6",
    "wait-port": "1.1.0",
    "worker-farm": "1.7.0",
+    "ws": "8.18.3",


using this version because we already have it as "@testplane/[email protected]" dependency

KuznetsovRoman · 2025-09-08T22:13:03Z

src/browser/cdp/emitter.ts

+import * as logger from "../../utils/logger";
+import { EventEmitter } from "events";
+
+export class CDPEventEmitter<Events extends { [key in keyof Events]: unknown }> extends EventEmitter {


This class provides typings for eventemitter, like that:

Also it wraps callbacks so they don't produce unhandled rejection

KuznetsovRoman · 2025-09-08T22:13:57Z

src/browser/cdp/index.ts

+    public readonly profiler: CDPProfiler;
+
+    static async create(browser: Browser): Promise<CDP | null> {
+        if (!browser.publicAPI.isChromium) {


cdp is only available in chromium-based browsers

KuznetsovRoman · 2025-09-08T22:14:47Z

src/browser/cdp/profiler.ts

+import { CDPEventEmitter } from "./emitter";
+import type { CDPDebuggerLocation, CDPSessionId, CDPScriptCoverage, CDPProfile } from "./types";
+
+interface StartPreciseCoverageRequest {


copied typings from https://chromedevtools.github.io/devtools-protocol/1-3/Profiler/

KuznetsovRoman · 2025-09-08T22:17:07Z

src/browser/cdp/utils.ts

+    return new Promise(resolve => setTimeout(resolve, delay).unref());
+};
+
+export const extractRequestIdFromBrokenResponse = (message: string): number | null => {


as we have seen, we might get broken message, which can't be parsed with json.parse
This function extracts request "id", so we can stop waiting for response for it until timeout and just retry it

KuznetsovRoman · 2025-09-08T22:43:16Z

src/browser/cdp/connection.ts

+     * @description creates ws connection with retries or returns existing one
+     * @note Concurrent requests with same params produce same ws connection
+     */
+    private async getWsConnection(): Promise<WebSocket> {


Retries "tryToEstablishWsConnection"

KuznetsovRoman · 2025-09-08T22:44:00Z

src/browser/cdp/connection.ts

+    }
+
+    /** @description establishes ws connection, sends request with timeout and waits for response */
+    private async tryToSendRequest(


Makes single try to send a request and resolves its response

KuznetsovRoman · 2025-09-08T22:44:13Z

src/browser/cdp/connection.ts

+    }
+
+    /** @description Performs high-level CDP request with retries and timeouts */
+    async request<T>(


retries "tryToSendRequest"

KuznetsovRoman · 2025-09-08T22:45:28Z

src/browser/cdp/connection.ts

+    /** @description Closes websocket connection, terminating all pending requests */
+    close(): void {


Should only be called once, when we would not need this connection in the future (for example, when deleting browser session)

KuznetsovRoman · 2025-09-08T22:46:56Z

src/browser/cdp/connection.ts

+            if (!this.isWebSocketActive(ws)) {
+                clearInterval(pingInterval);
+                return;
+            }


on each tick we have to check it first, because we might catch a frame where this callback is called when this pingInterval is no longer active because ws connection is reestablished

shadowusr · 2025-10-14T09:56:50Z

src/browser/cdp/index.ts

+    public readonly target: CDPTarget;
+    public readonly profiler: CDPProfiler;
+
+    static async create(browser: Browser): Promise<CDP | null> {


This may be controversial, but IMO create methods don't bring any value and only pollute codebase. But not critical to me

Its async
Since we can't make async constructor, i think its better to encapsulate "get browser ws endpoint" logic to "cdp" scope

That's a common complaint, but you don't need async constructor. You can get all necessary values before calling the constructor and just pass all those values

shadowusr · 2025-10-14T10:01:02Z

src/browser/cdp/connection.ts

+    }
+
+    /** @description Creates CDPConnection without establishing it */
+    static async create(browser: Browser): Promise<CDPConnection> {


Here as well I think there's little value in create() method and in receiving our Browser god-object if we in fact only need headers and wsEndpoint. This increases code coupling. For example when we chose to expose this API, users will be forced to create our Browser object, when they could just pass the endpoint and it would work fine

Not critical

I think i will leave it be for now, and its not critical while it is internals
We can change it when (and if) we will decide to make it public

shadowusr · 2025-10-14T10:06:11Z

src/browser/cdp/connection.ts

+                    ws.close();
+                    done(
+                        new CDPTimeoutError({
+                            message: `Couldn't establish CDP connection to "${endpoint}" in ${CDP_CONNECTION_TIMEOUT}ms`,


It would be really nice if we had more actionable errors here and in all other places as well. I think a good error message states clearly "what happenned and why" and "what can be done to fix". Imagining myself as a user receiving the "Couldn't establish CDP connection to ... in 1000ms", I'd be pretty puzzled.

Not critical, but very nice to have. I don't think that all these new errors are actionable right now.

I would love to leave actionable errors here, explaining "why it happened" and "what to do", if i knew, why could it happen and how to resolve these issues.

My plan to these kind of errors where you dont know, why it might happen, is the following:

Release this version with non-actionable errors

Saturate error messages with actionability, based on user stories (user comes with this error, we check, why it happened and what to do in this case, then update error message)

src/browser/cdp/connection.ts

shadowusr · 2025-10-14T10:19:15Z

src/browser/cdp/connection.ts

+
+        this._wsConnectionStatus = WsConnectionStatus.CONNECTING;
+
+        this._wsConnectionPromise = (async (): Promise<WebSocket> => {


Can we move this iife to a helper/protected method with human readable name? The logic here is pretty hard, especially the part where you reassign wsConnectionPromise inside.

especially the part where you reassign wsConnectionPromise inside

Thats why i keep it inside, so we could see all complex _wsConnectionPromise transitions scoped to just one method, instead it being reassigned in multiple ones

shadowusr · 2025-10-14T10:51:43Z

src/browser/cdp/profiler.ts

I would move domain implementations to a separate directory "domains". Now, they are mixed with types, utils, etc.

Its done in next PR

src/browser/cdp/target.ts

src/browser/cdp/profiler.ts

shadowusr · 2025-10-14T10:58:04Z

src/browser/existing-browser.ts

+            await cdpTarget.activateTarget(incognitoWindowId);
        }

        if (this._session.isBidi) {


We got rid of this in master, because puppeteer could do cleanup of unnecessary contexts with bidi, too. Would it remain possible with our own implementation?

I will take a look at the code

shadowusr · 2025-10-14T10:58:45Z

test/src/browser/cdp/connection.ts

+        });
+
+        it("should handle connection termination and reconnect", async () => {
+            const result1 = await connection.request("Successfull.Method");


minor typo — successful, not critical

KuznetsovRoman force-pushed the TESTPLANE-693.cdp branch 2 times, most recently from 8cc1b46 to 1223919 Compare September 5, 2025 20:53

KuznetsovRoman force-pushed the TESTPLANE-693.cdp branch 4 times, most recently from 9232a6d to 56b81a4 Compare September 8, 2025 22:08

KuznetsovRoman commented Sep 8, 2025

View reviewed changes

KuznetsovRoman force-pushed the TESTPLANE-693.cdp branch 2 times, most recently from fc679c0 to 0805224 Compare September 9, 2025 15:00

KuznetsovRoman force-pushed the TESTPLANE-693.cdp branch 2 times, most recently from 0e0c4ed to 641a566 Compare September 17, 2025 10:24

shadowusr approved these changes Oct 14, 2025

View reviewed changes

KuznetsovRoman force-pushed the TESTPLANE-693.cdp branch from 641a566 to de92444 Compare October 15, 2025 00:20

fix: use own cdp implementation

bb74561

KuznetsovRoman force-pushed the TESTPLANE-693.cdp branch from de92444 to bb74561 Compare October 15, 2025 01:45

KuznetsovRoman merged commit 41647d2 into master Oct 15, 2025
5 checks passed

KuznetsovRoman deleted the TESTPLANE-693.cdp branch October 15, 2025 10:38

		/** @description Closes websocket connection, terminating all pending requests */
		close(): void {


		this._wsConnectionStatus = WsConnectionStatus.CONNECTING;

		this._wsConnectionPromise = (async (): Promise<WebSocket> => {

fix: use own cdp implementation #1129

fix: use own cdp implementation #1129

Uh oh!

Conversation

KuznetsovRoman commented Sep 5, 2025

Uh oh!

pkg-pr-new bot commented Sep 5, 2025 • 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shadowusr Oct 14, 2025 • 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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pkg-pr-new bot commented Sep 5, 2025 •

edited

Loading

shadowusr Oct 14, 2025 •

edited

Loading