Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/pr-119.md
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.

Copy link
Copy Markdown
Collaborator

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:

  • Session name and status never used the executor — they go through the REST API: _updateJob (packages/browserstack-service/src/service.ts:881) → _update (:912), a PUT/PATCH to api.browserstack.com. BiDi cannot affect that path.
  • The service's own annotate paths already use 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:

  1. User-written browser.execute('browserstack_executor: …') calls — the main win.
  2. util.ts:2153 performO11ySync (reached via cli/modules/observabilityModule.ts:42 on the CLI/binary path).
  3. cli/modules/accessibilityModule.ts:570 _setAnnotation (also CLI path).

Fix: reword to something like "Fixed browserstack_executor commands issued via browser.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.

30 changes: 30 additions & 0 deletions packages/browserstack-service/src/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -888,6 +905,19 @@ export default class BrowserstackService implements Services.ServiceInstance {
})
}

_routeBidiExecutorToHttp (browser: WebdriverIO.Browser) {
if (!browser.isBidi) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The guard checks isBidi but not whether this is a BrowserStack session, which diverges from every other executor path in the package:

  • service.ts:1022 (_executeCommand) — isBrowserstackSession(this._browser)
  • accessibility-handler.ts:602, util.ts:2154, cli/modules/accessibilityModule.ts:570 — same guard

The service does run against non-BrowserStack sessions (see the self-healing branch at service.ts:219, gated on !isBrowserstackSession), so on any non-BrowserStack BiDi session execute still gets overwritten and prefix-matched scripts get re-routed to /execute/sync.

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) => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

executeAsync is routed over BiDi under exactly the same condition as execute, and is left unpatched here.

Evidencewebdriverio@9.28.0, build/index.js:3534-3538:

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 executeAsync, so this is a user-facing gap only — a user doing browser.executeAsync('browserstack_executor: …') still gets it silently swallowed on BiDi.

Question: intentionally out of scope, or worth mirroring the same overwrite for executeAsync (here or as a follow-up)? Either is fine — flagging so it is a decision rather than an omission.

if (typeof script === 'string' && script.startsWith('browserstack_executor:')) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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:

  • packages/browserstack-service/src/accessibility-handler.ts:546-555script.toLowerCase().indexOf('browserstack_executor') !== -1
  • packages/browserstack-service/src/cli/modules/accessibilityModule.ts:401-408 — same check, duplicated

Evidence / risk: a script those two already classify as an executor call (leading whitespace, or BROWSERSTACK_EXECUTOR:) is not matched by startsWith('browserstack_executor:') here. On a BiDi session it therefore still goes out over script.callFunction and is swallowed, while the identical script keeps working on non-BiDi — a BiDi-only behaviour divergence. I could not verify whether the hub itself tolerates those variants; that determines whether this is live today or only latent.

Fix: extract a single predicate and use it in all three places, e.g. in util.ts:

export const isBrowserstackExecutorScript = (script: unknown): script is string =>
    typeof script === 'string' && script.toLowerCase().includes('browserstack_executor')

Question: is the case-sensitive startsWith deliberate (i.e. the hub rejects the variants)? If not, reusing the existing predicate keeps BiDi and non-BiDi behaviour identical.

return browser.executeScript(script, args)
}
return originalExecute(script, ...args)
})
}

_multiRemoteAction (action: MultiRemoteAction) {
if (!this._browser) {
return Promise.resolve()
Expand Down
81 changes: 81 additions & 0 deletions packages/browserstack-service/tests/service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ beforeEach(() => {
browser = {
execute: vi.fn(),
executeScript: vi.fn(),
overwriteCommand: vi.fn(),
on: vi.fn(),
sessionId: sessionId,
config: {},
Expand Down Expand Up @@ -626,6 +627,86 @@ describe('before', () => {
expect(service['_failReasons']).toEqual([])
expect(service['_sessionBaseUrl']).toEqual('https://api.browserstack.com/automate-turboscale/v1/sessions')
})

it('should overwrite execute command to route browserstack_executor via executeScript', async () => {
(browser as any).isBidi = true
const service = new BrowserstackService({} as any, [{}] as any, { user: 'foo', key: 'bar', capabilities: {} })
await service.before(service['_config'] as any, [], browser)

expect(browser.overwriteCommand).toHaveBeenCalledWith('execute', expect.any(Function))

const overwrite = vi.mocked(browser.overwriteCommand).mock.calls[0][1] as Function
const originalExecute = vi.fn()

await overwrite(originalExecute, 'browserstack_executor: {"action":"annotate"}')
expect(browser.executeScript).toHaveBeenCalledWith('browserstack_executor: {"action":"annotate"}', [])
expect(originalExecute).not.toHaveBeenCalled()

await overwrite(originalExecute, 'return document.title')
expect(originalExecute).toHaveBeenCalledWith('return document.title')

const extraArg = { key: 'value' }
await overwrite(originalExecute, 'return arguments[0]', extraArg)
expect(originalExecute).toHaveBeenCalledWith('return arguments[0]', extraArg)
})

it('should not overwrite execute command for non-BiDi sessions', async () => {
(browser as any).isBidi = false
const service = new BrowserstackService({} as any, [{}] as any, { user: 'foo', key: 'bar', capabilities: {} })
await service.before(service['_config'] as any, [], browser)

expect(browser.overwriteCommand).not.toHaveBeenCalled()
})

it('should overwrite execute on each instance for multiremote', async () => {
const browserA = { executeScript: vi.fn(), overwriteCommand: vi.fn(), sessionId: 'sessionA', isBidi: true }
const browserB = { executeScript: vi.fn(), overwriteCommand: vi.fn(), sessionId: 'sessionB', isBidi: true }
const multiRemoteBrowser = {
...browser,
isMultiremote: true,
getInstance: vi.fn().mockImplementation((name: string) => name === 'browserA' ? browserA : browserB)
} as unknown as WebdriverIO.MultiRemoteBrowser

const service = new BrowserstackService({} as any, { browserA: {}, browserB: {} } as any, {
user: 'foo', key: 'bar'
})
await service.before(service['_config'] as any, [], multiRemoteBrowser as any)

expect(browserA.overwriteCommand).toHaveBeenCalledWith('execute', expect.any(Function))
expect(browserB.overwriteCommand).toHaveBeenCalledWith('execute', expect.any(Function))

const overwriteA = vi.mocked(browserA.overwriteCommand).mock.calls[0][1] as Function
await overwriteA(vi.fn(), 'browserstack_executor: {"action":"annotate"}')
expect(browserA.executeScript).toHaveBeenCalledWith('browserstack_executor: {"action":"annotate"}', [])
expect(browserB.executeScript).not.toHaveBeenCalled()

const originalExecuteA = vi.fn()
const extraArg = { key: 'value' }
await overwriteA(originalExecuteA, 'return arguments[0]', extraArg)
expect(originalExecuteA).toHaveBeenCalledWith('return arguments[0]', extraArg)
})

it('should keep patching remaining multiremote instances when one instance fails to resolve', async () => {
const browserA = { executeScript: vi.fn(), overwriteCommand: vi.fn(), sessionId: 'sessionA', isBidi: true }
const browserB = { executeScript: vi.fn(), overwriteCommand: vi.fn(), sessionId: 'sessionB', isBidi: true }
const multiRemoteBrowser = {
...browser,
isMultiremote: true,
getInstance: vi.fn()
.mockImplementationOnce(() => {
throw new Error('no such instance')
})
.mockImplementation((name: string) => name === 'browserA' ? browserA : browserB)
} as unknown as WebdriverIO.MultiRemoteBrowser

const service = new BrowserstackService({} as any, { browserA: {}, browserB: {} } as any, {
user: 'foo', key: 'bar'
})
await service.before(service['_config'] as any, [], multiRemoteBrowser as any)

expect(browserA.overwriteCommand).not.toHaveBeenCalled()
expect(browserB.overwriteCommand).toHaveBeenCalledWith('execute', expect.any(Function))
})
})

describe('beforeHook', () => {
Expand Down
Loading