-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
chore: expose adb devices and actions #4647
Conversation
f6aff88
to
7fe1027
Compare
version: '0.0.2', // Manually manage playwright-android version. | ||
description: 'A high-level API to automate Chrome for Android', | ||
browsers: [], | ||
files: [...PLAYWRIGHT_CORE_FILES, ...FFMPEG_FILES, 'android-types.d.ts', 'bin/android-driver.apk', 'bin/android-driver-target.apk'], |
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.
Please take a look at npmignore file. I think we are either shipping apks to all packages, or ignoring them in playwright-android.
try { | ||
socket = await this._backend.open(`localabstract:playwright_android_driver_socket`); | ||
} catch (e) { | ||
await new Promise(f => setTimeout(f, 100)); |
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.
I guess we'll need a timeout for this operation.
} | ||
} | ||
|
||
class AndroidBrowser extends EventEmitter { |
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.
Shall we mark this as implements Transport
? IIUC, this serves as a transport for protocol connection?
const controller = new ProgressController(); | ||
await controller.run(async progress => { | ||
await browser._defaultContext!._loadDefaultContext(progress); | ||
}); |
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 proper timeout managing, you have to wrap everything in this method into controller.run
and pass TimeoutSettings.timeout(options)
to it.
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.
Doing it in a follow up...
@@ -111,10 +113,11 @@ DEPS['src/server/common/'] = []; | |||
DEPS['src/server/injected/'] = ['src/server/common/']; | |||
|
|||
// Electron and Clank use chromium internally. | |||
DEPS['src/server/android/'] = [...DEPS['src/server/'], 'src/server/chromium/', 'src/protocol/transport.ts']; | |||
DEPS['src/server/electron/'] = [...DEPS['src/server/'], 'src/server/chromium/']; | |||
DEPS['src/server/clank/'] = [...DEPS['src/server/'], 'src/server/chromium/']; |
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.
Drop this line 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.
Nothing works w/o it!
#1122