You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Fix array method usageSuggestion Impact:The commit partially implemented the suggestion by changing 'self' to 'this' in the condition and method calls, but did not replace the append() method with push() as suggested.
code diff:
- if (!self.args.includes('--connect-existing')) {- self.args.append('--websocket-port')- self.args.append(`${portprober.findFreePort()}`)+ if (!this.args.includes('--connect-existing')) {+ this.args.append('--websocket-port')+ this.args.append(`${portprober.findFreePort()}`)
JavaScript arrays use push() method, not append(). Replace the append() calls with push() to correctly add the arguments to the array.
Why: This is a critical bug fix. JavaScript arrays use push() not append(), so the current code would throw an error and completely break the functionality. This correction is essential for the code to work at all.
High
✅ Fix object referenceSuggestion Impact:The commit implemented the suggestion by replacing 'self' with 'this' in the class method to correctly reference the current instance, fixing a potential reference error
code diff:
- if (!self.args.includes('--connect-existing')) {- self.args.append('--websocket-port')- self.args.append(`${portprober.findFreePort()}`)+ if (!this.args.includes('--connect-existing')) {+ this.args.append('--websocket-port')+ this.args.append(`${portprober.findFreePort()}`)
Use this instead of self to reference the current instance in JavaScript class methods. self is undefined in this context and will cause a reference error.
Why: Using 'self' instead of 'this' in a JavaScript class method is a critical error that would cause a reference error, as 'self' is undefined in this context. This fix is essential for the code to function.
High
✅ Handle asynchronous port findingSuggestion Impact:The commit completely removed the code that was using the problematic findFreePort() method, which effectively addresses the issue raised in the suggestion. Instead of replacing it with findFreePortSync() as suggested, the developers chose to remove the entire block of code that was using portprober.
The findFreePort() method is likely asynchronous and returns a Promise. You need to handle this asynchronously or use an alternative synchronous method to find a free port.
Why: If findFreePort() is indeed asynchronous, this would cause issues with port assignment. The suggestion correctly identifies a potential problem, but without confirming the actual API of portprober, we can't be certain this is a critical issue.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Delta456
force-pushed
the
js_connect_exist
branch
from
User description
Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it
Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.
Motivation and Context
By having the
--websocket-portparameter by default we avoid port collisions when using BiDi with firefoxFixes #15451 for JS binding
Types of changes
Checklist
PR Type
Bug fix, Enhancement
Description
Added
--websocket-portflag to avoid port collisions in Firefox.Integrated
portproberto dynamically find free ports.Modified
ServiceBuilderto include the new flag when not connecting to an existing connection.Changes walkthrough 📝
firefox.js
Add dynamic `--websocket-port` flag in Firefox servicejavascript/node/selenium-webdriver/firefox.js
portproberfor finding free ports.ServiceBuilderto append--websocket-portwith a freeport.
connection.