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
The Wheel class implementation has incomplete JSDoc comments. The scroll method parameters lack proper documentation compared to other methods in the file, which could make it difficult for developers to understand how to use this functionality correctly.
classWheelextendsDevice{/** * @param {string} id the device ID.. */constructor(id){super(Device.Type.WHEEL,id)}/** * Scrolls a page via the coordinates given * @param {number} x starting x coordinate * @param {number} y starting y coordinate * @param {number} deltaX Delta X to scroll to target * @param {number} deltaY Delta Y to scroll to target * @param {WebElement} origin element origin * @param {number} duration duration ratio be the ratio of time delta and duration * @returns {!Action} An action to scroll with this device. */scroll(x,y,deltaX,deltaY,origin,duration){return{type: Action.Type.SCROLL,duration: duration,x: x,y: y,deltaX: deltaX,deltaY: deltaY,origin: origin,}}}
The scroll method in the Actions class doesn't validate its parameters before passing them to the wheel_.scroll method, unlike other methods like dragAndDrop which perform input validation. This could lead to runtime errors if invalid parameters are provided.
The current implementation only takes the first element from the noProxy array. This will discard all other proxy exclusions. Consider preserving all noProxy entries by joining them into a comma-separated string instead of just taking the first element.
let proxy = caps.get(Capability.PROXY)
if (proxy && Array.isArray(proxy.noProxy)) {
- proxy.noProxy = proxy.noProxy[0]+ proxy.noProxy = proxy.noProxy.join(',')
if (!proxy.noProxy) {
proxy.noProxy = undefined
}
}
Apply this suggestion
Suggestion importance[1-10]: 9
__
Why: The current implementation discards all but the first element in the noProxy array, which would cause all other proxy exclusion rules to be lost. Joining the array elements into a comma-separated string preserves all exclusion rules, which is critical for proper proxy configuration. This is a significant bug fix that would prevent unexpected network behavior.
High
Handle rejected promise properly
The test is not properly handling the rejected promise from the file detector. When handleFile returns a rejected promise, the test should verify that the error is properly caught and handled, but currently it just continues as if the promise was resolved successfully.
[To ensure code accuracy, apply this suggestion manually]
Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies a significant issue where the test doesn't properly verify error handling when the file detector returns a rejected promise. The improved code properly tests the error path, which is important for ensuring robust error handling in the WebDriver implementation.
Medium
Fix timing race condition
This test has a race condition. It assumes that 75ms is less than half of 150ms and 175ms (75+100) is greater than 150ms, which may not always be true in all environments. The test should use more reliable timing mechanisms or mock the timer to ensure consistent behavior.
it('waits forever on a zero timeout', function () {
let done = false
- setTimeout(() => (done = true), 150)+ let donePromise = new Promise(resolve => {+ setTimeout(() => {+ done = true+ resolve()+ }, 150)+ })
let executor = new FakeExecutor()
let driver = executor.createDriver()
let waitResult = driver.wait(() => done, 0)
- return driver- .sleep(75)- .then(function () {- assert.ok(!done)- return driver.sleep(100)+ // Use a controlled timing approach+ return Promise.resolve()+ .then(() => {+ assert.ok(!done, 'Should not be done yet')+ return donePromise
})
- .then(function () {- assert.ok(done)+ .then(() => {+ assert.ok(done, 'Should be done now')
return waitResult
})
})
Apply this suggestion
Suggestion importance[1-10]: 7
__
Why: The suggestion addresses an important race condition in the test that relies on specific timing assumptions. The improved approach using a controlled promise-based timing mechanism makes the test more reliable and less susceptible to environment-specific timing variations.
Medium
General
Fix parameter naming inconsistency
The parameter names in the method signature don't match the names used in the function call. The parameters are named deltax and deltay in the documentation but targetDeltaX and targetDeltaY in the implementation, which could cause confusion for developers.
/**
* scrolls a page via the coordinates given
* @param {number} x starting x coordinate
* @param {number} y starting y coordinate
- * @param {number} deltax delta x to scroll to target- * @param {number} deltay delta y to scroll to target+ * @param {number} deltaX delta x to scroll to target+ * @param {number} deltaY delta y to scroll to target+ * @param {!Origin|!./webdriver.WebElement|undefined} origin the origin of the scroll
* @param {number} duration duration ratio be the ratio of time delta and duration
* @returns {!Actions} An action to scroll with this device.
*/
-scroll(x, y, targetDeltaX, targetDeltaY, origin, duration) {- return this.insert(this.wheel_, this.wheel_.scroll(x, y, targetDeltaX, targetDeltaY, origin, duration))+scroll(x, y, deltaX, deltaY, origin, duration) {+ return this.insert(this.wheel_, this.wheel_.scroll(x, y, deltaX, deltaY, origin, duration))
}
[To ensure code accuracy, apply this suggestion manually]
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies a parameter naming inconsistency between the JSDoc comments and the actual implementation. This is important for code maintainability and developer understanding, as the mismatch between documentation and implementation could lead to confusion.
Medium
Prevent promise memory leak
The test creates a deferred promise but never resolves or rejects it, which could lead to memory leaks. Even though the test is checking timeout behavior, it's good practice to clean up by resolving or rejecting the promise after the test completes.
it('promise that does not resolve before timeout', function () {
let d = defer()
let executor = new FakeExecutor()
let driver = executor.createDriver()
- return driver.wait(d.promise, 5).then(fail, (e) => {- assert.ok(e instanceof error.TimeoutError, 'Unexpected error: ' + e)- assert.ok(- /Timed out waiting for promise to resolve after \d+ms/.test(e.message),- 'unexpected error message: ' + e.message,- )- })+ return driver.wait(d.promise, 5)+ .then(fail, (e) => {+ assert.ok(e instanceof error.TimeoutError, 'Unexpected error: ' + e)+ assert.ok(+ /Timed out waiting for promise to resolve after \d+ms/.test(e.message),+ 'unexpected error message: ' + e.message,+ )+ })+ .finally(() => {+ // Clean up the promise to avoid potential memory leaks+ d.resolve();+ })
})
Apply this suggestion
Suggestion importance[1-10]: 5
__
Why: The suggestion correctly identifies a potential memory leak where a deferred promise is created but never resolved. While this is a valid improvement for test hygiene, it's a relatively minor issue compared to the other suggestions since the test process would end anyway.
Low
Learned best practice
Initialize all object properties in a single step or within the same scope to improve code clarity and reduce null reference risks
The defer() function creates an object and then assigns properties to it in multiple steps. This approach can be improved by using a more concise pattern that initializes all properties at once, reducing the risk of null reference issues and making the code more maintainable.
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.
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
Types of changes
Checklist
PR Type
Enhancement, Tests
Description
Relocated
selenium-webdriverto the root directory for better project organization.Enhanced WebDriver functionalities with new modules for Chromium and Firefox clients, including custom commands and options.
Added comprehensive test suites for WebDriver functionalities, input actions, BiDi protocol, DevTools integration, and virtual authenticator.
Introduced new utilities for file system operations, logging, and browsing context management.
Refactored the main module to improve WebDriver API and configuration handling.
Implemented network-related types and classes for BiDi protocol.
Deleted outdated files from the
nodedirectory to streamline the codebase.Changes walkthrough 📝
6 files
webdriver_test.js
Comprehensive test suite for WebDriver functionalities.javascript/selenium-webdriver/test/lib/webdriver_test.js
FakeExecutorandExpectationfor testing.and error handling.
behaviors.
script_test.js
Added BiDi script manager tests for various scenarios.javascript/selenium-webdriver/test/bidi/script_test.js
operations.
scripts.
input_test.js
Added tests for input actions and device synchronization.javascript/selenium-webdriver/test/lib/input_test.js
operations.
devtools_test.js
Added tests for DevTools integration and event handling.javascript/selenium-webdriver/test/devtools_test.js
events.
promise_test.js
Added extensive tests for promise utilities in WebDriver.javascript/selenium-webdriver/test/lib/promise_test.js
WebDriver.
fullyResolved,finally,checkedNodeCall,map, andfiltermethods.virtualAuthenticator_test.js
Added tests for virtual authenticator functionality in WebDriver.javascript/selenium-webdriver/test/virtualAuthenticator_test.js
WebDriver.
protocols.
operations.
8 files
input.js
Input handling and user gesture simulation enhancements.javascript/selenium-webdriver/lib/input.js
Key,Button, andAction.Devicesubclasses likeKeyboard,Pointer, andWheelforinput simulation.
Actionsclass for complex user gesture sequences.management.
index.js
Refactored main module for WebDriver API and configurations.javascript/selenium-webdriver/index.js
Builderclass.
capabilities.
configuration.
chromium.js
Introduced Chromium WebDriver client with custom commands.javascript/selenium-webdriver/chromium.js
and drivers.
firefox.js
Introduced Firefox WebDriver client with custom options.javascript/selenium-webdriver/firefox.js
and services.
networkTypes.js
Introduced network-related types and classes for BiDi protocol.javascript/selenium-webdriver/bidi/networkTypes.js
protocol.
BytesValue,Header,Cookie,FetchTimingInfo,and
RequestDatafor handling network data.methods.
browsingContext.js
Added browsing context management for BiDi protocol.javascript/selenium-webdriver/bidi/browsingContext.js
BrowsingContextand related classes for BiDiprotocol.
management.
Locatorclass for node location andNavigateResultfornavigation results.
operations.
logging.js
Implemented a hierarchical logging system for WebDriver.javascript/selenium-webdriver/lib/logging.js
Level,Entry,Logger, andLogManagerclasses for hierarchicallogging.
various levels.
index.js
Introduced file system utilities for WebDriver.javascript/selenium-webdriver/io/index.js
WebDriver.
temporary file handling.
walking.
101 files