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

[bidi][js] Add high-level script command #14293

Merged
merged 6 commits into from
Aug 6, 2024

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Jul 22, 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.

Related to #13992

Description

Motivation and Context

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.

PR Type

Enhancement, Tests


Description

  • Added support for special number types and argument handling in LocalValue class.
  • Introduced execute method in Script class to execute scripts with arguments.
  • Updated existing tests to validate object argument handling.
  • Added comprehensive tests for the new execute method covering various argument types.

Changes walkthrough 📝

Relevant files
Enhancement
protocolValue.js
Add support for special number types and argument handling

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

  • Added SpecialNumberType to imports.
  • Introduced createReferenceValue and getArgument static methods in
    LocalValue class.
  • Enhanced deserializeValue method in RemoteValue class to handle
    NonPrimitiveType.OBJECT.
  • +88/-2   
    script.js
    Add execute method to Script class                                             

    javascript/node/selenium-webdriver/lib/script.js

  • Added execute method to Script class for executing scripts with
    arguments.
  • +16/-0   
    Tests
    local_value_test.js
    Update tests for object argument handling                               

    javascript/node/selenium-webdriver/test/bidi/local_value_test.js

    • Updated test to validate object argument handling in LocalValue.
    +3/-3     
    webdriver_script_execute_test.js
    Add tests for Script execute method with various arguments

    javascript/node/selenium-webdriver/test/lib/webdriver_script_execute_test.js

  • Added comprehensive tests for execute method in Script class.
  • Covered various argument types including undefined, null, special
    numbers, arrays, sets, maps, objects, and regex.
  • +311/-0 

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

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Code Complexity
    The method getArgument in LocalValue class is overly complex and long, making it hard to maintain and understand. Consider refactoring it into smaller, more manageable methods.

    Possible Bug
    The deserializeValue method in RemoteValue class has changed its behavior by only deserializing NonPrimitiveType.OBJECT and not handling NonPrimitiveType.MAP as before. Ensure this change doesn't introduce bugs.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Add a default case to the switch statement to handle unexpected types

    Consider using a default case in the switch statement to handle unexpected type
    values. This can prevent the function from returning undefined implicitly, which
    might lead to harder-to-debug errors in the future.

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

     switch (type) {
       case PrimitiveType.STRING:
         localValue = LocalValue.createStringValue(argument)
         break
       ...
    +  default:
    +    throw new Error(`Unhandled type: ${type}`);
     }
     
    Suggestion importance[1-10]: 9

    Why: Adding a default case in the switch statement is a best practice that improves code robustness by handling unexpected type values, preventing potential runtime errors.

    9
    Maintainability
    Refactor repeated logic into a separate function for better code reusability

    Refactor the repeated calls to LocalValue.getArgument(value) inside the forEach
    loops for Map, Set, and Array to a separate function that handles these conversions.
    This will improve code reusability and maintainability.

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

    +function convertCollectionItem(item) {
    +  return typeof item === 'string' ? item : LocalValue.getArgument(item);
    +}
     argument.forEach((value, key) => {
    -  let objectKey
    -  if (typeof key === 'string') {
    -    objectKey = key
    -  } else {
    -    objectKey = LocalValue.getArgument(key)
    -  }
    -  const objectValue = LocalValue.getArgument(value)
    -  map.push([objectKey, objectValue])
    +  const objectKey = convertCollectionItem(key);
    +  const objectValue = convertCollectionItem(value);
    +  map.push([objectKey, objectValue]);
     })
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Refactoring the repeated calls to LocalValue.getArgument(value) into a separate function enhances code maintainability and reusability, making the code cleaner and easier to manage.

    8
    Performance
    Use a map-based lookup to handle different instance types more efficiently

    Replace the multiple if-else conditions with a more concise and efficient map-based
    lookup for handling instances of different classes like Date, Map, Set, Array, and
    RegExp.

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

    -if (argument instanceof Date) {
    -  localValue = LocalValue.createDateValue(argument)
    -} else if (argument instanceof Map) {
    -  ...
    -} else if (argument instanceof Set) {
    -  ...
    -} else if (argument instanceof Array) {
    -  ...
    -} else if (argument instanceof RegExp) {
    -  ...
    +const typeHandlers = {
    +  Date: LocalValue.createDateValue,
    +  Map: handleMap,
    +  Set: handleSet,
    +  Array: handleArray,
    +  RegExp: handleRegExp
    +};
    +const handler = typeHandlers[argument.constructor.name];
    +if (handler) {
    +  localValue = handler(argument);
     }
     
    Suggestion importance[1-10]: 7

    Why: Using a map-based lookup for handling different instance types can improve performance and readability, though the current if-else structure is also clear and functional.

    7

    @pujagani pujagani force-pushed the add-script-high-level-execute branch from 66e3251 to 5794cd0 Compare July 23, 2024 06:55
    @harsha509 harsha509 merged commit 7a18a86 into SeleniumHQ:trunk Aug 6, 2024
    10 of 11 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants