Skip to content
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

[js] Add JS comments for BiDi related files #13763

Merged
merged 5 commits into from Apr 3, 2024

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Apr 1, 2024

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.

Description

Add JS comments for BiDi-related files.

Motivation and Context

Ensure API docs are available for BiDi.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Type

enhancement


Description

  • Added comprehensive JS doc comments across various BiDi-related files to enhance code documentation.
  • Documented classes, methods, enums, and parameters for better understanding and maintenance.

Changes walkthrough

Relevant files
Documentation
28 files
addInterceptParameters.js
Add JS Doc Comments for AddInterceptParameters                     

javascript/node/selenium-webdriver/bidi/addInterceptParameters.js

  • Added doc comments for AddInterceptParameters class and its methods.
  • +26/-0   
    browser.js
    Add JS Doc Comments for Browser Module                                     

    javascript/node/selenium-webdriver/bidi/browser.js

    • Added doc comments for Browser class and its methods.
    +17/-0   
    browsingContext.js
    Add JS Doc Comments for BrowsingContext and Related Classes

    javascript/node/selenium-webdriver/bidi/browsingContext.js

  • Added doc comments for BrowsingContext class, Locator class,
    NavigateResult class, and PrintResult class.
  • Documented methods for navigating, capturing screenshots, and locating
    nodes.
  • +130/-4 
    browsingContextInspector.js
    Add JS Doc Comments for BrowsingContextInspector                 

    javascript/node/selenium-webdriver/bidi/browsingContextInspector.js

  • Added doc comments for BrowsingContextInspector class and its methods.

  • +51/-0   
    browsingContextTypes.js
    Add JS Doc Comments for BrowsingContext Types                       

    javascript/node/selenium-webdriver/bidi/browsingContextTypes.js

  • Added doc comments for BrowsingContextInfo and NavigationInfo classes.

  • +31/-0   
    captureScreenshotParameters.js
    Add JS Doc Comments for CaptureScreenshotParameters           

    javascript/node/selenium-webdriver/bidi/captureScreenshotParameters.js

  • Added doc comments for CaptureScreenshotParameters class and its
    methods.
  • +32/-0   
    clipRectangle.js
    Add JS Doc Comments for ClipRectangle Classes                       

    javascript/node/selenium-webdriver/bidi/clipRectangle.js

  • Added doc comments for ClipRectangle, ElementClipRectangle, and
    BoxClipRectangle classes.
  • +40/-0   
    continueRequestParameters.js
    Add JS Doc Comments for ContinueRequestParameters               

    javascript/node/selenium-webdriver/bidi/continueRequestParameters.js

  • Added doc comments for ContinueRequestParameters class and its
    methods.
  • +39/-0   
    continueResponseParameters.js
    Add JS Doc Comments for ContinueResponseParameters             

    javascript/node/selenium-webdriver/bidi/continueResponseParameters.js

  • Added doc comments for ContinueResponseParameters class and its
    methods.
  • +40/-0   
    cookieFilter.js
    Add JS Doc Comments for CookieFilter                                         

    javascript/node/selenium-webdriver/bidi/cookieFilter.js

    • Added doc comments for CookieFilter class and its methods.
    +60/-0   
    evaluateResult.js
    Add JS Doc Comments for EvaluateResult and Related Classes

    javascript/node/selenium-webdriver/bidi/evaluateResult.js

  • Added doc comments for EvaluateResultType, EvaluateResultSuccess,
    EvaluateResultException, and ExceptionDetails classes.
  • +17/-0   
    input.js
    Add JS Doc Comments for Input Module                                         

    javascript/node/selenium-webdriver/bidi/input.js

    • Added doc comments for Input class and its methods.
    +26/-0   
    interceptPhase.js
    Add JS Doc Comments for InterceptPhase                                     

    javascript/node/selenium-webdriver/bidi/interceptPhase.js

    • Added doc comments for InterceptPhase enum.
    +4/-0     
    logEntries.js
    Add JS Doc Comments for LogEntries and Related Classes     

    javascript/node/selenium-webdriver/bidi/logEntries.js

  • Added doc comments for BaseLogEntry, GenericLogEntry, ConsoleLogEntry,
    and JavascriptLogEntry classes.
  • +66/-0   
    network.js
    Add JS Doc Comments for Network Module                                     

    javascript/node/selenium-webdriver/bidi/network.js

    • Added doc comments for Network class and its methods.
    +102/-0 
    networkTypes.js
    Add JS Doc Comments for Network Types                                       

    javascript/node/selenium-webdriver/bidi/networkTypes.js

  • Added doc comments for SameSite, BytesValue, Header, Cookie,
    FetchTimingInfo, RequestData, BaseParameters, Initiator,
    BeforeRequestSent, FetchError, ResponseData, and ResponseStarted
    classes.
  • +346/-1 
    partialCookie.js
    Add JS Doc Comments for PartialCookie                                       

    javascript/node/selenium-webdriver/bidi/partialCookie.js

    • Added doc comments for PartialCookie class and its methods.
    +49/-1   
    partitionDescriptor.js
    Add JS Doc Comments for PartitionDescriptor and Related Classes

    javascript/node/selenium-webdriver/bidi/partitionDescriptor.js

  • Added doc comments for PartitionDescriptor,
    BrowsingContextPartitionDescriptor, and StorageKeyPartitionDescriptor
    classes.
  • +32/-0   
    partitionKey.js
    Add JS Doc Comments for PartitionKey                                         

    javascript/node/selenium-webdriver/bidi/partitionKey.js

    • Added doc comments for PartitionKey class.
    +17/-0   
    protocolType.js
    Add JS Doc Comments for Protocol Types                                     

    javascript/node/selenium-webdriver/bidi/protocolType.js

  • Added doc comments for PrimitiveType, NonPrimitiveType, RemoteType,
    and SpecialNumberType enums.
  • +20/-0   
    protocolValue.js
    Add JS Doc Comments for ProtocolValue and Related Classes

    javascript/node/selenium-webdriver/bidi/protocolValue.js

  • Added doc comments for LocalValue, RemoteValue, ReferenceValue,
    RegExpValue, SerializationOptions, and ChannelValue classes.
  • +129/-2 
    provideResponseParameters.js
    Add JS Doc Comments for ProvideResponseParameters               

    javascript/node/selenium-webdriver/bidi/provideResponseParameters.js

  • Added doc comments for ProvideResponseParameters class and its
    methods.
  • +40/-0   
    realmInfo.js
    Add JS Doc Comments for RealmInfo and Related Classes       

    javascript/node/selenium-webdriver/bidi/realmInfo.js

  • Added doc comments for RealmType, RealmInfo, and WindowRealmInfo
    classes.
  • +27/-0   
    resultOwnership.js
    Add JS Doc Comments for ResultOwnership                                   

    javascript/node/selenium-webdriver/bidi/resultOwnership.js

    • Added doc comments for ResultOwnership enum.
    +4/-0     
    scriptManager.js
    Add JS Doc Comments for ScriptManager                                       

    javascript/node/selenium-webdriver/bidi/scriptManager.js

    • Added doc comments for ScriptManager class and its methods.
    +117/-1 
    scriptTypes.js
    Add JS Doc Comments for ScriptTypes                                           

    javascript/node/selenium-webdriver/bidi/scriptTypes.js

    • Added doc comments for Message and Source classes.
    +38/-0   
    storage.js
    Add JS Doc Comments for Storage Module                                     

    javascript/node/selenium-webdriver/bidi/storage.js

    • Added doc comments for Storage class and its methods.
    +30/-0   
    urlPattern.js
    Add JS Doc Comments for UrlPattern                                             

    javascript/node/selenium-webdriver/bidi/urlPattern.js

    • Added doc comments for UrlPattern class and its methods.
    +35/-0   

    PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR Description updated to latest commit (fa72e55)

    Copy link

    PR Review

    ⏱️ Estimated effort to review [1-5]

    5, because the PR introduces extensive changes across multiple files, adding comprehensive documentation and functionality related to the BiDi protocol. The changes span across various modules including script execution, storage, network interception, and more. Each addition or modification requires careful review to ensure correctness, adherence to the BiDi protocol specifications, and integration with existing functionalities. The PR also involves understanding the context and purpose of the BiDi protocol within the Selenium project, making the review process more time-consuming and complex.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Possible Bug: The implementation of various classes and methods should be thoroughly tested to ensure they conform to the BiDi protocol specifications and correctly handle edge cases.

    Performance Concerns: The addition of new functionalities, especially those involving network interception and script execution, could potentially impact the performance. It's crucial to assess the performance implications of these changes.

    Compatibility Issues: The changes should be reviewed for compatibility with existing functionalities and across different browsers to ensure that the introduction of BiDi protocol support does not break existing features.

    🔒 Security concerns

    No


    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

    See the review usage page for a comprehensive guide on using this tool.

    Copy link

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Validate the value parameter in locator creation methods to ensure it is a non-empty string.

    Consider validating the value parameter in the css, xpath, and innerText static methods of
    the Locator class to ensure it is a non-empty string. This can prevent creating locators
    with invalid or empty selectors, which could lead to unexpected behavior or errors during
    execution.

    javascript/node/selenium-webdriver/bidi/browsingContext.js [55-79]

     static css(value) {
    +  if (typeof value !== 'string' || value.trim() === '') throw new Error('Value must be a non-empty string.');
       return new Locator(Locator.Type.CSS, value)
     }
     static xpath(value) {
    +  if (typeof value !== 'string' || value.trim() === '') throw new Error('Value must be a non-empty string.');
       return new Locator(Locator.Type.XPATH, value)
     }
     static innerText(value, ignoreCase = undefined, matchType = undefined, maxDepth = undefined) {
    +  if (typeof value !== 'string' || value.trim() === '') throw new Error('Value must be a non-empty string.');
       return new Locator(Locator.Type.INNER_TEXT, value, ignoreCase, matchType, maxDepth)
     }
     
    Validate the type of captureScreenshotParameters in the captureScreenshot method.

    For the captureScreenshot method, consider adding a validation step to ensure that if
    captureScreenshotParameters is provided, it is indeed an instance of
    CaptureScreenshotParameters. This can help catch errors early if an incorrect parameter
    type is passed to the method.

    javascript/node/selenium-webdriver/bidi/browsingContext.js [245-247]

     async captureScreenshot(captureScreenshotParameters = undefined) {
    -  if (
    -    captureScreenshotParameters !== undefined &&
    +  if (captureScreenshotParameters !== undefined && !(captureScreenshotParameters instanceof CaptureScreenshotParameters)) {
    +    throw new InvalidArgumentError('captureScreenshotParameters must be an instance of CaptureScreenshotParameters.');
    +  }
     
    Validate the locator parameter in the locateNodes and locateNode methods.

    In the locateNodes and locateNode methods, consider adding validation for the locator
    parameter to ensure it is an instance of Locator. This can prevent runtime errors when the
    methods are called with incorrect types, improving the robustness of the code.

    javascript/node/selenium-webdriver/bidi/browsingContext.js [473-544]

     async locateNodes(
       locator,
       maxNodeCount = undefined,
    +  if (!(locator instanceof Locator)) {
    +    throw new Error('locator must be an instance of Locator.');
    +  }
     async locateNode(
       locator,
       ownership = undefined,
    +  if (!(locator instanceof Locator)) {
    +    throw new Error('locator must be an instance of Locator.');
    +  }
     
    Add a default value for the ignoreCache parameter in the reload method.

    For the reload method, consider adding a default value for the ignoreCache parameter to
    explicitly define the method's behavior when the parameter is not provided. This can
    improve code readability and make the method's behavior more predictable.

    javascript/node/selenium-webdriver/bidi/browsingContext.js [384-385]

    -async reload(ignoreCache = undefined, readinessState = undefined) {
    +async reload(ignoreCache = false, readinessState = undefined) {
     
    Validate parameters in the setViewport method to ensure they are positive numbers.

    In the setViewport method, consider adding validation for the width, height, and
    devicePixelRatio parameters to ensure they are positive numbers. This can prevent runtime
    errors and ensure the method behaves as expected when called with invalid arguments.

    javascript/node/selenium-webdriver/bidi/browsingContext.js [409-410]

     async setViewport(width, height, devicePixelRatio = undefined) {
    +  if (width <= 0 || height <= 0 || (devicePixelRatio !== undefined && devicePixelRatio <= 0)) {
    +    throw new Error('Width, height, and devicePixelRatio must be positive numbers.');
    +  }
     
    Use destructuring for cleaner imports from modules.

    Consider using destructuring to improve code readability when importing multiple exports
    from a single module. This can make the imports section cleaner and easier to manage,
    especially when importing several items.

    javascript/node/selenium-webdriver/bidi/scriptManager.js [26-29]

    -const { RemoteValue, LocalValue } = require('./protocolValue')
    -const { ResultOwnership } = require('./resultOwnership')
    +const { RemoteValue, LocalValue, ResultOwnership } = require('./values')
     
    Use Object.fromEntries() for converting arrays to objects.

    Consider using Object.fromEntries() for a more concise and readable way to convert an
    array of key-value pairs into an object. This can be applied in the createMapValue and
    createObjectValue methods.

    javascript/node/selenium-webdriver/bidi/protocolValue.js [139]

    -let value = []
    -Object.entries(map).forEach((entry) => {
    -  value.push(entry)
    -})
    +let value = Object.fromEntries(Object.entries(map))
     
    Improve error messages by including the actual type of received parameters.

    When throwing errors for incorrect parameter types, include the actual type of the
    received parameter to aid debugging. Use typeof params to dynamically get the parameter
    type.

    javascript/node/selenium-webdriver/bidi/network.js [151]

    -throw new Error(`Params must be an instance of AddInterceptParameters. Received:'${params}'`)
    +throw new Error(`Params must be an instance of AddInterceptParameters. Received type: '${typeof params}'`)
     
    Add validation for the expiry parameter in the expiry method.

    Consider validating the expiry parameter in the expiry method to ensure it represents a
    valid future time or a specific format if required. This can prevent setting cookies with
    invalid expiry times.

    javascript/node/selenium-webdriver/bidi/cookieFilter.js [129-130]

     expiry(expiry) {
    +  if (!isValidExpiry(expiry)) {
    +    throw new Error(`Invalid expiry time: ${expiry}.`);
    +  }
       this.#map.set('expiry', expiry)
       return this
     
    Improve error messages for type validation in value and sameSite methods.

    To improve error handling, consider including more specific error messages in the value
    and sameSite methods when throwing errors for invalid types. This could include the type
    of the received value.

    javascript/node/selenium-webdriver/bidi/cookieFilter.js [46-47]

     if (!(value instanceof BytesValue)) {
    -  throw new Error(`Value must be an instance of BytesValue. Received:'${value}'`)
    +  throw new Error(`Value must be an instance of BytesValue. Received type: ${typeof value}, value: '${value}'`)
     
    Add validation for the size parameter in the size method.

    Consider adding input validation for the size method to ensure that the size is a positive
    integer. This can prevent setting cookies with invalid sizes.

    javascript/node/selenium-webdriver/bidi/cookieFilter.js [81-82]

     size(size) {
    +  if (!Number.isInteger(size) || size < 0) {
    +    throw new Error(`Size must be a positive integer. Received: ${size}`);
    +  }
       this.#map.set('size', size)
       return this
     
    Best practice
    Use an options object for method parameters to enhance flexibility.

    To ensure the flexibility and future-proofing of the disownRealmScript and
    disownBrowsingContextScript methods, consider adding an options object parameter instead
    of multiple specific parameters. This approach allows for easier extension of the method's
    capabilities without changing its signature.

    javascript/node/selenium-webdriver/bidi/scriptManager.js [57-78]

    -async disownRealmScript(realmId, handles) {
    -async disownBrowsingContextScript(browsingContextId, handles, sandbox = null) {
    +async disownRealmScript(options) {
    +async disownBrowsingContextScript(options) {
     
    Validate input parameters for method calls to ensure robustness.

    For the callFunctionInRealm and callFunctionInBrowsingContext methods, it's recommended to
    validate the input parameters to ensure they meet expected criteria (e.g., realmId and
    browsingContextId are not empty, functionDeclaration is a valid function). This can
    prevent runtime errors and make the API more robust.

    javascript/node/selenium-webdriver/bidi/scriptManager.js [107-145]

     async callFunctionInRealm(realmId, functionDeclaration, awaitPromise, argumentValueList = null, thisParameter = null, resultOwnership = null) {
    +    // Validate parameters here
    +}
     async callFunctionInBrowsingContext(browsingContextId, functionDeclaration, awaitPromise, argumentValueList = null, thisParameter = null, resultOwnership = null) {
    +    // Validate parameters here
    +}
     
    Add error handling around asynchronous operations for graceful failure.

    For better error handling, consider adding try-catch blocks around asynchronous operations
    that might fail, such as this.bidi.send(params) in the disownRealmScript,
    disownBrowsingContextScript, and other similar methods. This can help in providing more
    descriptive error messages and handling errors gracefully.

    javascript/node/selenium-webdriver/bidi/scriptManager.js [68]

    -await this.bidi.send(params)
    +try {
    +    await this.bidi.send(params)
    +} catch (error) {
    +    console.error("Failed to send command:", error);
    +    // Handle error appropriately
    +}
     
    Maintainability
    Refactor similar methods into a single method to reduce duplication.

    The evaluateFunctionInRealm and evaluateFunctionInBrowsingContext methods have similar
    signatures and functionality. Consider refactoring these into a single method with an
    additional parameter to specify the context type (realm or browsing context). This can
    reduce code duplication and improve maintainability.

    javascript/node/selenium-webdriver/bidi/scriptManager.js [183-205]

    -async evaluateFunctionInRealm(realmId, expression, awaitPromise, resultOwnership = null) {
    -async evaluateFunctionInBrowsingContext(browsingContextId, expression, awaitPromise, resultOwnership = null) {
    +async evaluateFunction(contextType, contextId, expression, awaitPromise, resultOwnership = null) {
     
    Group related methods together for better readability.

    For better code organization and readability, consider grouping all event subscription
    methods (beforeRequestSent, responseStarted, etc.) together, followed by all action
    methods (addIntercept, removeIntercept, etc.). This helps in quickly understanding the
    class capabilities and finding related methods.

    javascript/node/selenium-webdriver/bidi/network.js [50-150]

    +// Grouped event subscription methods
     async beforeRequestSent(callback) {
       await this.subscribeAndHandleEvent('network.beforeRequestSent', callback)
     }
     ...
    +// Grouped action methods
     async addIntercept(params) {
       if (!(params instanceof AddInterceptParameters)) {
         throw new Error(`Params must be an instance of AddInterceptParameters. Received:'${params}'`)
     }
     
    Standardize error message terminology across methods.

    For consistency and to avoid potential bugs, consider using a consistent naming convention
    for error messages. The sameSite method refers to parameters, while the value method
    refers to values.

    javascript/node/selenium-webdriver/bidi/cookieFilter.js [117]

    -throw new Error(`Params must be a value in SameSite. Received:'${sameSite}'`)
    +throw new Error(`SameSite value must be an instance of SameSite. Received:'${sameSite}'`)
     
    Abstract repetitive code patterns into a private method for better maintainability.

    To enhance code readability and maintainability, consider abstracting the repetitive
    pattern of setting values in the map and returning this into a private method.

    javascript/node/selenium-webdriver/bidi/cookieFilter.js [33-35]

    +#setAndReturn(key, value) {
    +  this.#map.set(key, value);
    +  return this;
    +}
     name(name) {
    -  this.#map.set('name', name)
    -  return this
    +  return this.#setAndReturn('name', name);
     }
     
    Bug
    Correct the condition to assign this.#handle in ReferenceValue constructor.

    For the ReferenceValue constructor, the condition to assign this.#handle seems incorrect.
    It checks if handle equals RemoteReferenceType.HANDLE, but handle is expected to be a
    string identifier. This condition should likely check if handle is not null or undefined
    instead.

    javascript/node/selenium-webdriver/bidi/protocolValue.js [268-269]

    -if (handle === RemoteReferenceType.HANDLE) {
    -  this.#handle = sharedId
    +if (handle != null) {
    +  this.#handle = handle
     }
     
    Possible issue
    Ensure cancelAuth uses the correct command or parameters to cancel authentication.

    The method continueWithAuthNoCredentials and cancelAuth seem to use the same command
    method 'network.continueWithAuth'. If cancelAuth is intended to cancel the authentication
    without providing credentials, it should likely use a different command or modify the
    command parameters to indicate the cancellation.

    javascript/node/selenium-webdriver/bidi/network.js [241-242]

     async cancelAuth(requestId) {
       const command = {
    -    method: 'network.continueWithAuth',
    +    method: 'network.cancelAuth', // Assuming there's a separate command for canceling auth
     }
     

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

    @pujagani pujagani merged commit 911b312 into SeleniumHQ:trunk Apr 3, 2024
    12 of 14 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    None yet

    1 participant