-
Notifications
You must be signed in to change notification settings - Fork 9
Bidi browserstack executor http #119
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
base: main
Are you sure you want to change the base?
Changes from all commits
e6dc8d2
1370fb6
b9bb4eb
1deec85
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@wdio/browserstack-service": patch | ||
| --- | ||
|
|
||
| - Fixed BrowserStack executor commands (session name, status, annotations) being ignored in WebDriver BiDi sessions. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -242,6 +242,23 @@ export default class BrowserstackService implements Services.ServiceInstance { | |
| PerformanceTester.scenarioThatRan = this._scenariosThatRan | ||
|
|
||
| if (this._browser) { | ||
| const patchBidiExecutorRouting = (resolveBrowser: () => WebdriverIO.Browser, label?: string) => { | ||
| try { | ||
| this._routeBidiExecutorToHttp(resolveBrowser()) | ||
| } catch (err) { | ||
| BStackLogger.warn(`Failed to patch execute for BiDi browserstack_executor routing${label ? ` on ${label}` : ''}; executor commands may not work in BiDi sessions: ${err}`) | ||
| } | ||
| } | ||
|
|
||
| if (this._browser.isMultiremote) { | ||
| const multiRemoteBrowser = this._browser as unknown as WebdriverIO.MultiRemoteBrowser | ||
| Object.keys(this._caps).forEach((browserName) => { | ||
| patchBidiExecutorRouting(() => multiRemoteBrowser.getInstance(browserName), browserName) | ||
| }) | ||
| } else { | ||
| patchBidiExecutorRouting(() => this._browser as WebdriverIO.Browser) | ||
| } | ||
|
|
||
| try { | ||
| const sessionId = this._browser.sessionId | ||
|
|
||
|
|
@@ -888,6 +905,19 @@ export default class BrowserstackService implements Services.ServiceInstance { | |
| }) | ||
| } | ||
|
|
||
| _routeBidiExecutorToHttp (browser: WebdriverIO.Browser) { | ||
| if (!browser.isBidi) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The guard checks
The service does run against non-BrowserStack sessions (see the self-healing branch at Low impact in practice — nobody sends executor payloads to a non-BrowserStack grid — but it is a one-condition fix that keeps this consistent with the rest of the file: if (!browser.isBidi || !isBrowserstackSession(browser)) {
return
} |
||
| return | ||
| } | ||
|
|
||
| browser.overwriteCommand('execute', async (originalExecute, script, ...args) => { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Evidence — async function executeAsync(script, ...args) {
...
if (this.isBidi && !this.isMultiremote) { // same gate as execute() at :3509
...
const result = await browser.scriptCallFunction(params);No internal caller passes an executor payload to Question: intentionally out of scope, or worth mirroring the same overwrite for |
||
| if (typeof script === 'string' && script.startsWith('browserstack_executor:')) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the third copy of "is this an executor script?" in the package, and the only one with these semantics. The two existing ones are case-insensitive substring matches:
Evidence / risk: a script those two already classify as an executor call (leading whitespace, or Fix: extract a single predicate and use it in all three places, e.g. in export const isBrowserstackExecutorScript = (script: unknown): script is string =>
typeof script === 'string' && script.toLowerCase().includes('browserstack_executor')Question: is the case-sensitive |
||
| return browser.executeScript(script, args) | ||
| } | ||
| return originalExecute(script, ...args) | ||
| }) | ||
| } | ||
|
|
||
| _multiRemoteAction (action: MultiRemoteAction) { | ||
| if (!this._browser) { | ||
| return Promise.resolve() | ||
|
|
||
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.
This line ships to the public CHANGELOG, and two of the three things it names were not affected by the BiDi issue.
Evidence:
_updateJob(packages/browserstack-service/src/service.ts:881) →_update(:912), aPUT/PATCHtoapi.browserstack.com. BiDi cannot affect that path.executeScript(classic HTTP/execute/sync), so they were never swallowed either:_executeCommand(service.ts:1018-1038),AccessibilityHandler._setAnnotation(accessibility-handler.ts:603),InsightsHandler(insights-handler.ts:139).What this PR actually fixes:
browser.execute('browserstack_executor: …')calls — the main win.util.ts:2153 performO11ySync(reached viacli/modules/observabilityModule.ts:42on the CLI/binary path).cli/modules/accessibilityModule.ts:570 _setAnnotation(also CLI path).Fix: reword to something like "Fixed
browserstack_executorcommands issued viabrowser.execute()being ignored in WebDriver BiDi sessions." Otherwise customers will attribute unrelated session-name/status problems to BiDi. The same wording appears in the PR description and the internal release notes.