Skip to content

bugfix: prevent this error when running WebDriver without Bidi protocol: 'Error: Failed to execute WebDriver Bidi command "sessionSubscribe"' #5095

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Sep 16, 2025
Merged

bugfix: prevent this error when running WebDriver without Bidi protocol: 'Error: Failed to execute WebDriver Bidi command "sessionSubscribe"' #5095

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
68bba2d
bugfix: prevent error 'Error: Failed to execute WebDriver Bidi comman…
Aug 22, 2025
1e3f15c
bugfix: prevent error 'Error: Failed to execute WebDriver Bidi comman…
Aug 22, 2025
a4c2c03
bugfix: prevent error 'Error: Failed to execute WebDriver Bidi comman…
Sep 2, 2025
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
8 changes: 6 additions & 2 deletions lib/helper/WebDriver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,7 +491,7 @@ class WebDriver extends Helper {
}
config.capabilities.browserName = config.browser || config.capabilities.browserName

// WebDriver Bidi Protocol. Default: false
// WebDriver Bidi Protocol. Default: true
config.capabilities.webSocketUrl = config.bidiProtocol ?? config.capabilities.webSocketUrl ?? true

config.capabilities.browserVersion = config.browserVersion || config.capabilities.browserVersion
Expand Down Expand Up @@ -629,7 +629,11 @@ class WebDriver extends Helper {

this.browser.on('dialog', () => {})

await this.browser.sessionSubscribe({ events: ['log.entryAdded'] })
// Check for Bidi, because "sessionSubscribe" is an exclusive Bidi protocol feature. Otherwise, error will be thrown.
if (this.browser.capabilities && this.browser.capabilities.webSocketUrl) {
await this.browser.sessionSubscribe({ events: ['log.entryAdded'] })
}

this.browser.on('log.entryAdded', logEvents)

Copy link

Copilot AI Aug 23, 2025

Choose a reason for hiding this comment

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

The event listener for 'log.entryAdded' is being registered regardless of whether Bidi protocol is enabled, but the corresponding sessionSubscribe call that enables these events is now conditional. This will result in the event listener never being triggered when Bidi is disabled, creating dead code.

Suggested change
}
this.browser.on('log.entryAdded', logEvents)
this.browser.on('log.entryAdded', logEvents)
}

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ngraf may you advise this please? Thanks!

Copy link
Contributor Author

@ngraf ngraf Sep 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Fair point from Copilot.

I made an additional commit and moved
"this.browser.on('log.entryAdded', logEvents)" into the "if" condition:

    if (this.browser.capabilities && this.browser.capabilities.webSocketUrl) {
      await this.browser.sessionSubscribe({ events: ['log.entryAdded'] })
      this.browser.on('log.entryAdded', logEvents)
    }

@kobenguyent , what is actually the downpart of not having the browser logs added?
Will it break some essential functionality? Or is it just a bonus feature?
I mean, as of today the feature does not work for "bidiProtocol: false" anyway. I wonder if this fact needs to be documented in the documentation that the feature of tracking browser logs is only supported when "bidiProtocol" is true. Alternatively the feature to collect logs could be rewritten in a way, so that the feature works for "bidiProtocol" false and true. But I guess it may be hard to impossible to collect logs without "bidiProtocol" enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The unit test failed. Isn a flaky test failing. At least the failing unit test is about some timing and wait mechanism. I don't really see a relationship to my tiny changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair point from Copilot.

I made an additional commit and moved "this.browser.on('log.entryAdded', logEvents)" into the "if" condition:

    if (this.browser.capabilities && this.browser.capabilities.webSocketUrl) {
      await this.browser.sessionSubscribe({ events: ['log.entryAdded'] })
      this.browser.on('log.entryAdded', logEvents)
    }

@kobenguyent , what is actually the downpart of not having the browser logs added? Will it break some essential functionality? Or is it just a bonus feature? I mean, as of today the feature does not work for "bidiProtocol: false" anyway. I wonder if this fact needs to be documented in the documentation that the feature of tracking browser logs is only supported when "bidiProtocol" is true. Alternatively the feature to collect logs could be rewritten in a way, so that the feature works for "bidiProtocol" false and true. But I guess it may be hard to impossible to collect logs without "bidiProtocol" enabled.

imho, the current implementation is quite basic and only handles log events.

return this.browser
Expand Down