bito-code-review[bot] commented on code in PR #44608: URL: https://github.com/apache/superset/pull/44608#discussion_r4131246256
########## superset-embedded-sdk/testrig/drive.mjs: ########## @@ -0,0 +1,573 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +// Drives the rig in headless Chromium over the DevTools protocol, with no +// dependencies beyond a chromium binary. `node drive.mjs [--headed] [--verbose]` + +import { spawn, execFileSync } from "node:child_process"; +import { mkdtempSync, existsSync, statSync, readdirSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join, dirname } from "node:path"; +import { fileURLToPath } from "node:url"; +import { start, HOST_PORT } from "./server.mjs"; + +const here = dirname(fileURLToPath(import.meta.url)); +const sdkRoot = join(here, ".."); +const headed = process.argv.includes("--headed"); +const verbose = process.argv.includes("--verbose"); + +const CHROMIUM = + process.env.CHROMIUM_PATH || + ["chromium", "chromium-browser", "google-chrome", "google-chrome-stable"].find( + (bin) => { + try { + execFileSync("which", [bin], { stdio: "ignore" }); + return true; + } catch { + return false; + } + }, + ); + +// ------------------------------------------------------------------ results +const results = []; +function check(name, ok, detail = "") { + results.push({ name, ok, detail }); + const mark = ok ? "\x1b[32m✓\x1b[0m" : "\x1b[31m✗\x1b[0m"; + console.log(` ${mark} ${name}${detail && !ok ? `\n ${detail}` : ""}`); +} + +// Waits for something to become true, and records the wait itself as a check, +// so a run against code that never gets there reports a failure per scenario +// instead of stopping at the first one. +async function expect(name, predicate, timeoutMs = 15_000) { + try { + await waitFor(predicate, name, timeoutMs); + check(name, true); + return true; + } catch (err) { + check(name, false, err.message); + return false; + } +} + +// ---------------------------------------------------------------- cdp client +class CDP { + constructor(ws) { + this.ws = ws; + this.nextId = 0; + this.pending = new Map(); + this.listeners = []; + ws.addEventListener("message", (event) => { + const msg = JSON.parse(event.data); + if (msg.id && this.pending.has(msg.id)) { + const { resolve, reject } = this.pending.get(msg.id); + this.pending.delete(msg.id); + if (msg.error) reject(new Error(JSON.stringify(msg.error))); + else resolve(msg.result); + } else { + this.listeners.forEach((fn) => fn(msg)); + } + }); + } + + send(method, params = {}, sessionId) { + const id = (this.nextId += 1); + const payload = { id, method, params }; + if (sessionId) payload.sessionId = sessionId; + this.ws.send(JSON.stringify(payload)); + return new Promise((resolve, reject) => { + this.pending.set(id, { resolve, reject }); + setTimeout(() => { + if (this.pending.delete(id)) reject(new Error(`${method} timed out`)); + }, 30_000); + }); + } +} + +async function launchBrowser() { + const userDataDir = mkdtempSync(join(tmpdir(), "embedded-sdk-rig-")); + const args = [ + ...(headed ? [] : ["--headless=new"]), + "--remote-debugging-port=0", + `--user-data-dir=${userDataDir}`, + "--no-first-run", + "--no-default-browser-check", + "--disable-gpu", + "--disable-dev-shm-usage", + "about:blank", + ]; + const child = spawn(CHROMIUM, args, { stdio: ["ignore", "pipe", "pipe"] }); + const wsUrl = await new Promise((resolve, reject) => { + let buffered = ""; + const onChunk = (chunk) => { + buffered += chunk; + const match = buffered.match(/ws:\/\/\S+/); + if (match) resolve(match[0]); + }; + child.stdout.on("data", onChunk); + child.stderr.on("data", onChunk); + child.on("exit", (code) => + reject(new Error(`chromium exited (${code}) before listening:\n${buffered}`)), + ); + setTimeout(() => reject(new Error("chromium never reported a devtools url")), 20_000); + }); + const ws = new WebSocket(wsUrl); + await new Promise((resolve, reject) => { + ws.addEventListener("open", resolve, { once: true }); + ws.addEventListener("error", reject, { once: true }); + }); Review Comment: <!-- Bito Reply --> The fix implemented in the referenced commit correctly addresses the issue of orphaned Chromium processes. By wrapping the post-spawn WebSocket handshake in a try/catch block, the code ensures that if the handshake fails, the child process is terminated and the temporary directory is removed before the error propagates, preventing resource leaks. **superset-embedded-sdk/testrig/drive.mjs** ``` const ws = new WebSocket(wsUrl); try { await new Promise((resolve, reject) => { ws.addEventListener("open", resolve, { once: true }); ws.addEventListener("error", reject, { once: true }); }); } catch (err) { child.kill(); rmSync(userDataDir, { recursive: true, force: true }); throw err; } ``` ########## superset-embedded-sdk/testrig/drive.mjs: ########## @@ -0,0 +1,573 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +// Drives the rig in headless Chromium over the DevTools protocol, with no +// dependencies beyond a chromium binary. `node drive.mjs [--headed] [--verbose]` + +import { spawn, execFileSync } from "node:child_process"; +import { mkdtempSync, existsSync, statSync, readdirSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join, dirname } from "node:path"; +import { fileURLToPath } from "node:url"; +import { start, HOST_PORT } from "./server.mjs"; + +const here = dirname(fileURLToPath(import.meta.url)); +const sdkRoot = join(here, ".."); +const headed = process.argv.includes("--headed"); +const verbose = process.argv.includes("--verbose"); + +const CHROMIUM = + process.env.CHROMIUM_PATH || + ["chromium", "chromium-browser", "google-chrome", "google-chrome-stable"].find( + (bin) => { + try { + execFileSync("which", [bin], { stdio: "ignore" }); + return true; + } catch { + return false; + } + }, + ); + +// ------------------------------------------------------------------ results +const results = []; +function check(name, ok, detail = "") { + results.push({ name, ok, detail }); + const mark = ok ? "\x1b[32m✓\x1b[0m" : "\x1b[31m✗\x1b[0m"; + console.log(` ${mark} ${name}${detail && !ok ? `\n ${detail}` : ""}`); +} + +// Waits for something to become true, and records the wait itself as a check, +// so a run against code that never gets there reports a failure per scenario +// instead of stopping at the first one. +async function expect(name, predicate, timeoutMs = 15_000) { + try { + await waitFor(predicate, name, timeoutMs); + check(name, true); + return true; + } catch (err) { + check(name, false, err.message); + return false; + } +} + +// ---------------------------------------------------------------- cdp client +class CDP { + constructor(ws) { + this.ws = ws; + this.nextId = 0; + this.pending = new Map(); + this.listeners = []; + ws.addEventListener("message", (event) => { + const msg = JSON.parse(event.data); + if (msg.id && this.pending.has(msg.id)) { + const { resolve, reject } = this.pending.get(msg.id); + this.pending.delete(msg.id); + if (msg.error) reject(new Error(JSON.stringify(msg.error))); + else resolve(msg.result); + } else { + this.listeners.forEach((fn) => fn(msg)); + } + }); + } + + send(method, params = {}, sessionId) { + const id = (this.nextId += 1); + const payload = { id, method, params }; + if (sessionId) payload.sessionId = sessionId; + this.ws.send(JSON.stringify(payload)); + return new Promise((resolve, reject) => { + this.pending.set(id, { resolve, reject }); + setTimeout(() => { + if (this.pending.delete(id)) reject(new Error(`${method} timed out`)); + }, 30_000); + }); + } +} + +async function launchBrowser() { + const userDataDir = mkdtempSync(join(tmpdir(), "embedded-sdk-rig-")); + const args = [ + ...(headed ? [] : ["--headless=new"]), + "--remote-debugging-port=0", + `--user-data-dir=${userDataDir}`, + "--no-first-run", + "--no-default-browser-check", + "--disable-gpu", + "--disable-dev-shm-usage", + "about:blank", + ]; + const child = spawn(CHROMIUM, args, { stdio: ["ignore", "pipe", "pipe"] }); + const wsUrl = await new Promise((resolve, reject) => { + let buffered = ""; + const onChunk = (chunk) => { + buffered += chunk; + const match = buffered.match(/ws:\/\/\S+/); + if (match) resolve(match[0]); + }; + child.stdout.on("data", onChunk); + child.stderr.on("data", onChunk); + child.on("exit", (code) => + reject(new Error(`chromium exited (${code}) before listening:\n${buffered}`)), + ); + setTimeout(() => reject(new Error("chromium never reported a devtools url")), 20_000); + }); + const ws = new WebSocket(wsUrl); + await new Promise((resolve, reject) => { + ws.addEventListener("open", resolve, { once: true }); + ws.addEventListener("error", reject, { once: true }); + }); + return { + cdp: new CDP(ws), + stop: () => { + try { + child.kill("SIGKILL"); + } finally { + rmSync(userDataDir, { recursive: true, force: true }); + } + }, + }; +} + +// ------------------------------------------------------------------- a page +async function openPage(cdp, url) { + const { targetId } = await cdp.send("Target.createTarget", { url: "about:blank" }); + const { sessionId } = await cdp.send("Target.attachToTarget", { + targetId, + flatten: true, + }); + await cdp.send("Runtime.enable", {}, sessionId); + await cdp.send("Page.enable", {}, sessionId); + if (verbose) { + cdp.listeners.push((msg) => { + if (msg.method === "Runtime.consoleAPICalled" && msg.sessionId === sessionId) { + const text = msg.params.args + .map((a) => a.value ?? a.description ?? a.type) + .join(" "); + console.log(` [page] ${text}`); + } + }); + } + + const evaluate = async (expression) => { + const r = await cdp.send( + "Runtime.evaluate", + { expression, awaitPromise: true, returnByValue: true }, + sessionId, + ); + if (r.exceptionDetails) { + throw new Error( + r.exceptionDetails.exception?.description || + r.exceptionDetails.text || + JSON.stringify(r.exceptionDetails), + ); + } + return r.result.value; + }; + + await cdp.send("Page.navigate", { url }, sessionId); + // Wait for the host app's own globals rather than a load event. + await waitFor( + () => evaluate("typeof window.rig === 'object' && !!window.supersetEmbeddedSdk"), + "the host app to load", + ); + return { evaluate, sessionId }; +} + +async function waitFor(predicate, what, timeoutMs = 15_000, intervalMs = 100) { + const deadline = Date.now() + timeoutMs; + let last; + for (;;) { + last = await predicate(); + if (last) return last; Review Comment: <!-- Bito Reply --> The user is correct. Since `waitFor()` now handles predicate rejections by retrying until the deadline, the `expect()` wrapper is no longer required for `openPage()` to avoid crashing the test run on transient CDP errors. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
