Skip to content

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

Merged
harsha509 merged 6 commits intoSeleniumHQ:trunkfrom
pujagani:add-script-high-level-execute
Aug 6, 2024
Merged

[bidi][js] Add high-level script command#14293
harsha509 merged 6 commits intoSeleniumHQ:trunkfrom
pujagani:add-script-high-level-execute

Conversation

@pujagani
Copy link
Contributor

@pujagani pujagani commented Jul 22, 2024 •
edited by qodo-code-review bot
Loading

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

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Jul 22, 2024

    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.

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Jul 22, 2024

    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
    @pujagani pujagani mentioned this pull request Jul 25, 2024
    Open
    @harsha509 harsha509 merged commit 7a18a86 into SeleniumHQ:trunk Aug 6, 2024
    M1troll pushed a commit to M1troll/selenium that referenced this pull request May 14, 2025
    @pujagani @harsha509 @M1troll
    [bidi][js] Add high-level script command (SeleniumHQ#14293)
    d35a38f 
    * [bidi][js] Add high-level script command

* Fix formatting

* Fix tests

* Fix linting

---------

Co-authored-by: Sri Harsha <12621691+harsha509@users.noreply.github.com>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @harsha509 harsha509 harsha509 approved these changes

    Assignees

    No one assigned

    Labels

    P-enhancement PR with a new feature Review effort [1-5]: 4

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @pujagani @harsha509