chore(cli): refactor browser listing methods #36034
Conversation
Test results for "tests 1"4 failed 6 flaky39209 passed, 804 skipped Merge workflow run. |
yury-s
left a comment
yury-s
left a comment
There was a problem hiding this comment.
It's not obvious to me that this refactoring actually improves anything. It may be easier to just include it into the PR that adds the command.
|
|
||
| // Remove stale browsers. | ||
| await this._validateInstallationCache(linksDir); | ||
| const browserList: Array<{ browserName: string, browserVersion: number, hostDir: string, browserPath: string }> = await this._traverseBrowserInstallations(linksDir); |
There was a problem hiding this comment.
You shouldn't need explicit type declaration here.
|
|
||
| // Remove stale browsers. | ||
| await this._validateInstallationCache(linksDir); | ||
| const browserList: Array<{ browserName: string, browserVersion: number, hostDir: string, browserPath: string }> = await this._traverseBrowserInstallations(linksDir); |
| if (!getAsBooleanFromENV('PLAYWRIGHT_SKIP_BROWSER_GC')) { | ||
| const usedBrowserPaths: Set<string> = new Set(); | ||
| for (const browser of browserList) { | ||
| const browserName = browser.browserName; |
There was a problem hiding this comment.
const { browserName, browserRevision, usedBrowserPath } = browser;
| // We want a location that won't have a node_modules dir anywhere along its path | ||
| const tmpWorkspace = path.join(TMP_WORKSPACES, path.basename(test.info().outputDir)); | ||
| await fs.promises.mkdir(tmpWorkspace); | ||
| await fs.promises.mkdir(tmpWorkspace, { recursive: true }); |
There was a problem hiding this comment.
I was trying to run tests locally, and faced error while trying to create directory: /tmp/pwt/workspace. mkdir_p was the fix.
| } | ||
|
|
||
| private async _deleteStaleBrowsers(browserList: Array<{ browserName: string, browserVersion: number, hostDir: string, browserPath: string }>) { | ||
| if (!getAsBooleanFromENV('PLAYWRIGHT_SKIP_BROWSER_GC')) { |
|
Thanks for the review: @yury-s . I would raise another PR with all the desired changes. Closing this for now. |
2 participants
Prepares for #34183