Skip to content

Conversation

@pujagani
Copy link
Contributor

@pujagani pujagani commented Aug 1, 2024

User description

Related to #13992

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

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

  • Introduced getArgument method in LocalValue to handle various argument types including String, Number, Boolean, Instant, Map, List, Set, and RegExpValue.
  • Added execute method to RemoteScript to execute scripts with arguments, utilizing LocalValue.getArgument for argument handling.
  • Updated Script interface to include the execute method.
  • Added comprehensive tests for the execute method in WebScriptExecuteTest to verify handling of different argument types.

Changes walkthrough 📝

Relevant files
Enhancement
LocalValue.java
Add `getArgument` method to handle various argument types

java/src/org/openqa/selenium/bidi/script/LocalValue.java

  • Added getArgument method to handle various argument types.
  • Introduced JSON serialization for object arguments.
  • Enhanced support for different data types including String, Number,
    Boolean, Instant, Map, List, Set, and RegExpValue.
  • +84/-0   
    RemoteScript.java
    Add `execute` method to RemoteScript for script execution

    java/src/org/openqa/selenium/remote/RemoteScript.java

  • Added execute method to execute scripts with arguments.
  • Integrated LocalValue.getArgument for argument handling.
  • Implemented error handling for script execution.
  • +35/-0   
    Script.java
    Add `execute` method to Script interface                                 

    java/src/org/openqa/selenium/remote/Script.java

    • Added execute method signature to Script interface.
    +3/-0     
    Tests
    WebScriptExecuteTest.java
    Add tests for `execute` method with various argument types

    java/test/org/openqa/selenium/WebScriptExecuteTest.java

  • Added multiple tests for execute method with various argument types.
  • Verified handling of undefined, null, special numbers, booleans,
    BigInt, arrays, sets, dates, maps, objects, and RegExp.
  • +330/-0 

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

    @qodo-merge-pro qodo-merge-pro bot added P-enhancement PR with a new feature tests labels Aug 1, 2024
    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Aug 1, 2024

    PR Reviewer Guide 🔍

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

    Code Duplication
    The method getArgument contains repetitive code blocks for handling different types of arguments. Consider refactoring to reduce duplication and improve maintainability.

    Exception Handling
    The method execute throws a generic WebDriverException without specific error handling or categorization. Consider throwing more specific exceptions or handling different error scenarios distinctly.

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Aug 1, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Use String.format to construct exception messages

    Use String.format for constructing exception messages to enhance readability and
    maintainability.

    java/src/org/openqa/selenium/remote/RemoteScript.java [164-166]

    -throw new WebDriverException(
    -    "Error while executing script: "
    -    + ((EvaluateResultExceptionValue) result).getExceptionDetails().getText());
    +throw new WebDriverException(String.format("Error while executing script: %s", 
    +    ((EvaluateResultExceptionValue) result).getExceptionDetails().getText()));
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Using String.format enhances readability and maintainability of the exception message construction, making the code cleaner and easier to understand.

    8
    Replace multiple if statements with a switch statement for type checking

    Consider using a switch statement for the Number type check instead of multiple if
    statements. This will make the code more readable and maintainable.

    java/src/org/openqa/selenium/bidi/script/LocalValue.java [163-168]

    -if (arg instanceof Integer || arg instanceof Long) {
    -  localValue = numberValue(((Number) arg).longValue());
    -} else if (arg instanceof Double || arg instanceof Float) {
    -  localValue = numberValue(((Number) arg).doubleValue());
    -} else if (arg instanceof BigInteger) {
    -  localValue = bigIntValue(arg.toString());
    +switch (arg.getClass().getSimpleName()) {
    +  case "Integer":
    +  case "Long":
    +    localValue = numberValue(((Number) arg).longValue());
    +    break;
    +  case "Double":
    +  case "Float":
    +    localValue = numberValue(((Number) arg).doubleValue());
    +    break;
    +  case "BigInteger":
    +    localValue = bigIntValue(arg.toString());
    +    break;
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves readability and maintainability by using a switch statement instead of multiple if statements. However, the improvement is minor as the original code is already clear.

    7
    Best practice
    Use Optional to handle null values more gracefully

    Consider using Java's Optional to handle potential null values gracefully instead of
    directly returning null.

    java/src/org/openqa/selenium/bidi/script/LocalValue.java [208]

    -return localValue;
    +return Optional.ofNullable(localValue);
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using Optional can help handle potential null values more gracefully, but it introduces additional complexity and may not be necessary in this context.

    6

    @pujagani pujagani merged commit bed411d into SeleniumHQ:trunk Aug 20, 2024
    M1troll pushed a commit to M1troll/selenium that referenced this pull request May 14, 2025
    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.

    1 participant