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][java] Add methods to allow all parameters for script callFunction and evaluate method #13873

Merged
merged 3 commits into from
Apr 29, 2024

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Apr 25, 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

All a method to all parameters (mandatory and optional) for Script module commands to call a function and evaluation a script.

Motivation and Context

Keep up with the w3C BiDi spec

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, Tests


Description

  • Added new methods in Script.java for enhanced script evaluation and function calls.
  • Introduced CallFunctionParameters and EvaluateParameters classes to encapsulate parameters for script functions and evaluations.
  • Implemented ContextTarget, RealmTarget, and Target classes to represent different script targets.
  • Added comprehensive tests for new classes and methods to ensure functionality aligns with expected behaviors.

Changes walkthrough

Relevant files
Enhancement
Script.java
Add new methods for script evaluation and function calls in Script
module

java/src/org/openqa/selenium/bidi/module/Script.java

  • Added callFunction method to handle function calls with
    CallFunctionParameters.
  • Added evaluateFunction method to handle script evaluation with
    EvaluateParameters.
  • Removed unused imports by replacing them with wildcard imports.
  • +11/-11 
    CallFunctionParameters.java
    Implement CallFunctionParameters class for script function parameters

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

  • Created a new class CallFunctionParameters to encapsulate parameters
    for the callFunction method.
  • Added methods to set various parameters like arguments, result
    ownership, serialization options, etc.
  • +62/-0   
    ContextTarget.java
    Add ContextTarget class for context-specific script targets

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

  • Created a new class ContextTarget extending Target to represent
    context-specific targets.
  • +29/-0   
    EvaluateParameters.java
    Implement EvaluateParameters class for script evaluation parameters

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

  • Created a new class EvaluateParameters to encapsulate parameters for
    the evaluateFunction method.
  • Added methods to set various parameters like result ownership,
    serialization options, etc.
  • +51/-0   
    RealmTarget.java
    Add RealmTarget class for realm-specific script targets   

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

  • Created a new class RealmTarget extending Target to represent
    realm-specific targets.
  • +25/-0   
    Target.java
    Implement abstract Target class for script targets             

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

  • Created an abstract class Target to serve as a base for specific
    target types like ContextTarget and RealmTarget.
  • +29/-0   
    Tests
    CallFunctionParameterTest.java
    Add tests for CallFunctionParameters in script module       

    java/test/org/openqa/selenium/bidi/script/CallFunctionParameterTest.java

  • Added extensive tests for the CallFunctionParameters class, covering
    various scenarios and parameter configurations.
  • +459/-0 
    EvaluateParametersTest.java
    Add tests for EvaluateParameters in script module               

    java/test/org/openqa/selenium/bidi/script/EvaluateParametersTest.java

  • Added tests for the EvaluateParameters class, ensuring correct
    handling of script evaluation parameters.
  • +240/-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 Description updated to latest commit (ef0551b)

    Copy link
    Contributor

    PR Review

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves multiple new classes and methods which require understanding of the existing BiDi module in Selenium, as well as the new functionality being added. The changes are moderate in size and the logic is not overly complex, but the integration with existing code and the impact on the overall system need careful consideration.

    🧪 Relevant tests

    Yes

    🔍 Possible issues

    Possible Bug: The use of wildcard imports (e.g., import org.openqa.selenium.bidi.script.*;) in Script.java could lead to namespace conflicts and make the code less readable. It's generally a good practice to list imported classes explicitly.

    Code Clarity: The new methods in Script.java could benefit from JavaDoc comments explaining their purpose, parameters, and the expected behavior, especially since they are public API methods.

    🔒 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
    Contributor

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Add exception handling around server start-up in tests.

    Consider handling potential exceptions when starting the server in the setUp method to
    prevent test failures due to unhandled exceptions.

    java/test/org/openqa/selenium/bidi/script/CallFunctionParameterTest.java [45-46]

     server = new NettyAppServer();
    -server.start();
    +try {
    +  server.start();
    +} catch (Exception e) {
    +  fail("Failed to start server: " + e.getMessage());
    +}
     
    Best practice
    Improve type checking in assertions.

    Use a more specific assertion to check the type of the result in
    canCallFunctionWithDeclaration to ensure type safety.

    java/test/org/openqa/selenium/bidi/script/CallFunctionParameterTest.java [63]

    -assertThat(successResult.getResult().getType()).isEqualTo("number");
    +assertThat(successResult.getResult()).isInstanceOf(NumberResult.class);
     
    Use constants for URLs to enhance maintainability.

    Instead of hardcoding the URL in the test method canCallFunctionToGetElement, use a
    constant or retrieve it from a configuration to enhance flexibility and maintainability.

    java/test/org/openqa/selenium/bidi/script/CallFunctionParameterTest.java [190-191]

    -String url = appServer.whereIs("/bidi/logEntryAdded.html");
    +String url = Constants.BIDI_LOG_ENTRY_ADDED_URL;
     driver.get(url);
     
    Ensure resources are closed properly to prevent leaks.

    To avoid potential resource leaks, ensure that resources like Script are properly closed
    in a finally block or use try-with-resources consistently.

    java/test/org/openqa/selenium/bidi/script/CallFunctionParameterTest.java [54-58]

    -try (Script script = new Script(id, driver)) {
    +Script script = null;
    +try {
    +  script = new Script(id, driver);
       ...
    +} finally {
    +  if (script != null) {
    +    script.close();
    +  }
     }
     
    Enhance code readability by using descriptive variable names.

    Consider using a more descriptive variable name instead of id for driver.getWindowHandle()
    to enhance code readability.

    java/test/org/openqa/selenium/bidi/script/EvaluateParametersTest.java [45]

    -String id = driver.getWindowHandle();
    +String windowHandle = driver.getWindowHandle();
     
    Enhance type safety by using generics in map declaration.

    Use generics for the Map declaration in CallFunctionParameters to ensure type safety.

    java/src/org/openqa/selenium/bidi/script/CallFunctionParameters.java [26]

    -private final Map<String, Object> map = new HashMap<>();
    +private final Map<String, Object> map = new HashMap<String, Object>();
     
    Maintainability
    Refactor repeated script execution code into a helper method.

    Refactor repeated code for creating Script and EvaluateResult objects into a helper method
    to improve code maintainability.

    java/test/org/openqa/selenium/bidi/script/CallFunctionParameterTest.java [54-58]

    -try (Script script = new Script(id, driver)) {
    -  ...
    +private EvaluateResult executeScript(String scriptText) {
    +  try (Script script = new Script(driver.getWindowHandle(), driver)) {
    +    return script.callFunction(new CallFunctionParameters(new ContextTarget(driver.getWindowHandle()), scriptText, true));
    +  }
     }
     
    Improve maintainability and testability by using a factory method for server instantiation.

    Replace the direct instantiation of NettyAppServer with a factory method to allow for
    easier testing and future changes in server implementations.

    java/test/org/openqa/selenium/bidi/script/EvaluateParametersTest.java [39]

    -server = new NettyAppServer();
    +server = ServerFactory.createAppServer();
     
    Robustness
    Improve robustness by adding error handling for server start-up in tests.

    Add error handling for potential exceptions when starting the server in the setUp method
    to prevent the test from failing without proper cleanup.

    java/test/org/openqa/selenium/bidi/script/EvaluateParametersTest.java [40]

    -server.start();
    +try {
    +    server.start();
    +} catch (Exception e) {
    +    // handle exception or fail the test with a meaningful message
    +    fail("Failed to start the server: " + e.getMessage());
    +}
     
    Prevent potential runtime exceptions by adding null checks.

    Consider checking for null values in the constructor of EvaluateParameters to prevent
    potential NullPointerExceptions.

    java/src/org/openqa/selenium/bidi/script/EvaluateParameters.java [28]

    +if (target == null) {
    +    throw new IllegalArgumentException("Target cannot be null.");
    +}
     map.put("target", target.toMap());
     

    ✨ 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.

    @codecov-commenter
    Copy link

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 58.72%. Comparing base (7f25fd1) to head (0494596).

    ❗ Current head 0494596 differs from pull request most recent head 2bb853f. Consider uploading reports for the commit 2bb853f to get more accurate results

    ❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

    Additional details and impacted files
    @@           Coverage Diff           @@
    ##            trunk   #13873   +/-   ##
    =======================================
      Coverage   58.72%   58.72%           
    =======================================
      Files          86       86           
      Lines        5298     5298           
      Branches      226      226           
    =======================================
      Hits         3111     3111           
      Misses       1961     1961           
      Partials      226      226           

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    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.

    2 participants