You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Thanks for the PR and for digging into the macOS "Chrome for Testing" process not exiting — that's a real issue, and
awaiting the close with a timeout guard (closeWithTimeout) is a genuinely good idea. A few things need to be addressed
before this can be merged:
The branch is out of date with main. Most of the PageController changes in this diff (the getDashboardDataFromHtml
extraction, the multi-line logging, the parsed.filter().join() reflow) already exist on main. Please rebase/merge main
into your branch — right now the diff conflicts with code that's already shipped, which makes it impossible to review
the actual fix.
As written, this regresses assertBundledChromiumAvailable(). On main that method launches a test browser, closes
it, and has a catch that throws a helpful "run npx patchright install chromium" message. The version in this PR drops
the .close() and the catch — that leaks a browser and loses the error handling. This looks like a merge artifact;
rebasing should resolve it.
The parent browser is already closed elsewhere — let's not close it twice. In BrowserManager.createBrowser, the
context already has:
context.once('close', () => {
this.activeBrowsers.delete(browser)
void browser.close().catch(() => {})
})
plus activeBrowsers tracking and a closeAll(). Your change calls context.close() and then browser.close() directly in
closeBrowser, so browser.close() ends up being invoked twice. Rather than duplicating, please build on the existing
mechanism — e.g. keep the closeWithTimeout improvement by making that context.once('close') handler (or closeAll)
await the close with the timeout, instead of the current fire-and-forget void browser.close(). That fixes the
lingering-process race in one place.
Please keep the PR scoped to the fix. The dashboard/logging reformatting is unrelated to "browser not closing" and
should drop out once you rebase. A focused diff is much easier to review and safer to merge.
How was this tested? Since the fix targets macOS specifically, could you share how you verified it (e.g. confirming
no Google Chrome for Testing processes remain after a run)? We don't have a macOS CI runner, so your repro steps help
a lot.
Once it's rebased on main, scoped down to the close logic, and the closeWithTimeout guard is wired into the existing
close path, I'd be happy to take another look. Thanks again!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.