-
Notifications
You must be signed in to change notification settings - Fork 63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: auto browser download #1029
Conversation
@@ -54,6 +54,7 @@ | |||
"@babel/code-frame": "7.24.2", | |||
"@gemini-testing/commander": "2.15.4", | |||
"@jspm/core": "2.0.1", | |||
"@puppeteer/browsers": "2.4.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- download chrome
- download chromium (replaces chrome for versions >73, <115)
- download firefox
@@ -64,12 +65,16 @@ | |||
"bluebird": "3.5.1", | |||
"chalk": "2.4.2", | |||
"clear-require": "1.0.1", | |||
"cli-progress": "3.12.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"debug": "2.6.9", | ||
"devtools": "8.39.0", | ||
"edgedriver": "5.6.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- download edge driver
"error-stack-parser": "2.1.4", | ||
"expect-webdriverio": "3.6.0", | ||
"extract-zip": "2.0.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- extract zip archive with chromedriver, which is installed manually (>73, <115)
"fastq": "1.13.0", | ||
"fs-extra": "5.0.0", | ||
"geckodriver": "4.5.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- download and run geckodriver
if (platform === BrowserPlatform.MAC_ARM && Number(milestone) < MIN_CHROMIUM_MAC_ARM_VERSION) { | ||
return BrowserPlatform.MAC; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for chrome, use mac64
platform, if chromium version is old and mac_arm
version does not exist
export const getFirefoxBrowserDir = (): string => getBrowsersDir(); // path is set by @puppeteer/browsers.install | ||
export const getChromeBrowserDir = (): string => getBrowsersDir(); // path is set by @puppeteer/browsers.install | ||
|
||
export const retryFetch = async ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we fetch some data from github, retries wont hurt
@@ -26,10 +28,11 @@ export class BasicPool implements Pool { | |||
|
|||
this._activeSessions = {}; | |||
this._cancelled = false; | |||
this._wdPool = new WebdriverPool(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type ChildProcessWithStatus = { process: ChildProcess; gridUrl: string; isBusy: boolean }; | ||
export type WdProcess = { gridUrl: string; free: () => void; kill: () => void }; | ||
|
||
export class WebdriverPool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have N browsers.
This class handles webdriver processes:
- we can reuse existing webdriver process, which is free because browser, which requested webdriver process, no longer needs it (
browser.quit
is called) - we need to launch new webdriver process (if all existing webdriver processes are busy and we need to launch NewBrowser)
WebdriverPool can reuse webdriver process between browsers with different browserId, if their browser and browserVersion matches
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
port
in this class acts like process id as it is unique
warn(`WARNING: Can not close session: ${(e as Error).message}`); | ||
this._wdProcess?.kill(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If session can't be closed, we kill webdriver process, so later we could launch the new one
src/browser/new-browser.ts
Outdated
if (this._isLocalGridUrl() && config.automationProtocol === "webdriver") { | ||
gridUrl = await this._getLocalWebdriverGridUrl(); | ||
} else { | ||
// if automationProtocol is not "webdriver", fallback to default grid url from "local" | ||
// because in "devtools" protocol we dont need gridUrl, but it still has to be valid URL | ||
gridUrl = config.gridUrl === LOCAL_GRID_URL ? DEFAULT_GRID_URL : config.gridUrl; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"isLocalGridUrl" === either "gridUrl" is "local", either testplane
is launched with --local
- If isLocalGridUrl == true and automationProtocol === "webdriver", we get webdriver process and use its gridUrl
- if isLocalGridUrl == true and automationProtocol is "devtools", gridUrl won't be used either way, but we need to pass any valid url
return this._isLocalGridUrl() | ||
? this._addExecutablePath(config, capabilitiesWithAddedHeadless) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we don't check for automationProtocol, so we could use our binaries for "devtools" automation protocol
const executablePath = await installBrowser( | ||
this._config.desiredCapabilities?.browserName as SupportedBrowser, | ||
this._config.desiredCapabilities?.browserVersion as string, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Installs browser (if necessary) and returns its executable path
For safari and edge null
is returned (we won't set binary path in capabilities, but wdio will be able to find them, if they are installed)
unpack: true, | ||
}).then(result => result.executablePath); | ||
|
||
return installBinary(Browser.CHROME, platform, buildId, installFn); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
registry.installBinary does not mean we will call "installFn" function.
If multiple "installBinary" are called with same browser platform and buildId, "installFn" would only be called at most once. And if we already have it installed, "installFn" wouldn't be called
a3693cd
to
58c27d2
Compare
await installEdgeDriver(browserVersion, { force }); | ||
} | ||
|
||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we return null
here but not binary (as for chrome/firefox)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we can't download edge
or safari
. We return null
so NewBrowser
wouldn't set desiredCapabilities.
With safari
and edge
currently, as we are using globally installed browser version, we don't need to specify binaryPath. This will change after edge autodownload support though.
@@ -0,0 +1,61 @@ | |||
import { resolveBuildId, canDownload, install as puppeteerInstall } from "@puppeteer/browsers"; | |||
import { MIN_CHROME_FOR_TESTING_VERSION } from "../constants"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
min_supported_chrome_version
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No.
We have these chrome versions:
- MIN_CHROMIUM_VERSION (73)
- MIN_CHROMIUM_MAC_ARM_VERSION (93)
- MIN_CHROME_FOR_TESTING_VERSION (113)
So there are 3 ranges:
- [73, 92]. Minimal chrome to use. Chromium is used. (mac x64 version for mac arm, because there is no mac arm for these versions)
- [93, 112]. Chromium is still used, but now Mac_arm version for mac.
- 113+. Chrome for testing is used.
const milestone = getMilestone(version); | ||
|
||
if (Number(milestone) < MIN_CHROME_FOR_TESTING_VERSION) { | ||
browserInstallerDebug(`couldn't install chrome@${version}, installing chromium instead`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to inform user about min supported version here. it's not clear why we are going to install chromium instead of chrome
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not user warning log. It won't be logged out to the user, and user don't need to distinguish between "chromium" and "chrome for testing" It is debug log.
getPortStub = sandbox.stub().resolves(12345); | ||
waitPortStub = sandbox.stub().resolves(); | ||
|
||
runChromeDriver = proxyquire("../../../../src/browser-installer/chrome", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it might be better to test this functionality with integration tests. There are too many stubs right now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, everything is stubbed, because it is not important at chrome/index.ts
level
We only need to know installChromeDriver
is called/not called here, as well as spawn
is called/not called.
installChromeDriver
is tested in another test file, and none of those stubs contains any important logic
I still think those tests do test chrome/index.ts
functionality (and nothing more)
|
||
afterEach(() => sandbox.restore()); | ||
|
||
[true, false, undefined].forEach(debug => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
o_O
installBrowsersWithDriversStub = sandbox.stub(); | ||
|
||
cli = proxyquire("../../../../../src/cli", { | ||
[path.resolve(process.cwd(), "src/cli/commands/install-browsers")]: proxyquire( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, you can't do that. Double proxyquire
nesting is too much :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just copied it from here: https://github.com/gemini-testing/testplane/blob/master/test/src/cli/commands/list-tests/index.ts#L39-L50
Is there any better way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes - integration test ;)
7e5fe69
to
61f8403
Compare
What is done
Added ability to automatically download browsers and/or webdrivers for these browsers.
Supported browsers to download:
Supported drivers to download:
With "--local" (or gridUrl: "local") and webdriver protocol, webdiver instances are automatically run for these browsers:
When launching tests with "--local" (and optionally webdriver protocol) all supported browsers/drivers will be installed automatically, if necessary. With "--local" we at all cost are trying to avoid network requests to google/mozilla/... servers.
Example
npx testplane --local
, either by setting "gridUrl": "local"In order to just download browser,
npx testplane install-browsers
could be used. This way we also check if our browsers are up-to-date, so if we have 114.0.5696.0 downloaded and user wants [email protected], we check for newest [email protected] version and see it is "114.0.5735.133". Then this new version is downloadedThese can be combined:
npx testplane install-browsers my-chrome chrome@115
Directory to save testplane browsers and drivers can be set with
TESTPLANE_BROWSERS_PATH
env variable. By default,~/.testplane
is used.