selenium icon indicating copy to clipboard operation
selenium copied to clipboard

[java] refactor(remote/command): Merge overload's business logic

Open nnnnoel opened this issue 1 year ago • 4 comments

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

Merge overload's business logic to make it easy to see at a glance

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

  • [x] 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.
  • [x] All new and existing tests passed.

PR Type

enhancement, configuration changes


Description

  • Refactored overloaded methods in DriverCommand to streamline business logic by delegating deprecated methods to their Duration counterparts.
  • Updated Java language level in java.iml from JDK 11 to JDK 17 to ensure compatibility with newer Java features.

Changes walkthrough 📝

Relevant files
Enhancement
DriverCommand.java
Refactor overloaded methods for session and timeout commands

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

  • Refactored overloaded methods for NEW_SESSION and timeout settings.
  • Deprecated methods now delegate to their Duration counterparts.
  • Simplified method calls by removing redundant code.
  • +4/-8     
    Configuration changes
    java.iml
    Update Java language level to JDK 17                                         

    java/java.iml

    • Updated Java language level from JDK 11 to JDK 17.
    +1/-1     

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

    nnnnoel avatar Sep 03 '24 08:09 nnnnoel

    CLA assistant check
    All committers have signed the CLA.

    CLAassistant avatar Sep 03 '24 08:09 CLAassistant

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Potential Regression
    The removal of Require.nonNull("Capabilities", capabilities) check in the NEW_SESSION method might lead to null pointer exceptions if null capabilities are passed.

    Language Level Change
    The LANGUAGE_LEVEL has been changed from JDK_11 to JDK_17, which might affect compatibility with older Java versions.

    qodo-code-review[bot] avatar Sep 03 '24 08:09 qodo-code-review[bot]

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    ✅ Add a null check for the input parameter to prevent potential null pointer exceptions
    Suggestion Impact:A null check for the capabilities parameter was added using Require.nonNull to prevent potential NullPointerException.

    code diff:

    +    Require.nonNull("Capabilities", capabilities);
    

    Consider adding a null check for the capabilities parameter before calling
    singleton() to prevent potential NullPointerException.

    java/src/org/openqa/selenium/remote/DriverCommand.java [165-166]

     static CommandPayload NEW_SESSION(Capabilities capabilities) {
    +  Require.nonNull("Capabilities", capabilities);
       return NEW_SESSION(singleton(capabilities));
     }
     
    
    • [ ] Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding a null check for the capabilities parameter is crucial to prevent potential NullPointerException, which enhances the robustness of the code.

    9
    Maintainability
    Add a comment to explain the rationale behind a significant version upgrade

    Consider adding a comment explaining the reason for upgrading the LANGUAGE_LEVEL
    from JDK_11 to JDK_17, as this change might have implications for the project's
    compatibility and requirements.

    java/java.iml [6]

    +<!-- Upgraded to JDK 17 for improved performance and new language features -->
     <component name="NewModuleRootManager" LANGUAGE_LEVEL="JDK_17">
     
    
    • [ ] Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding a comment to explain the upgrade to JDK 17 is beneficial for maintainability, as it provides context for future developers about the project's compatibility and requirements.

    7
    Enhancement
    Use a more readable time unit conversion method for improved code clarity

    Consider using Duration.ofSeconds() instead of Duration.ofMillis() for better
    readability when converting from TimeUnit to Duration.

    java/src/org/openqa/selenium/remote/DriverCommand.java [347-348]

     static CommandPayload SET_IMPLICIT_WAIT_TIMEOUT(long time, TimeUnit unit) {
    -  return SET_IMPLICIT_WAIT_TIMEOUT(Duration.ofMillis(unit.toMillis(time)));
    +  return SET_IMPLICIT_WAIT_TIMEOUT(Duration.ofSeconds(unit.toSeconds(time)));
     }
     
    
    • [ ] Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: While using Duration.ofSeconds() can improve readability, it is a minor enhancement and does not significantly impact functionality or performance.

    5

    qodo-code-review[bot] avatar Sep 03 '24 08:09 qodo-code-review[bot]

    @shs96c I applied your review. Please can you see the submitted commit?

    nnnnoel avatar Sep 27 '24 10:09 nnnnoel

    @shs96c Please can you provide any updates?

    nnnnoel avatar Nov 06 '24 09:11 nnnnoel

    PR Compliance Guide 🔍

    Below is a summary of compliance checks for this PR:

    Security Compliance
    🟢
    No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
    Ticket Compliance
    🎫 No ticket provided
    • [ ] Create ticket/issue
    Codebase Duplication Compliance
    Codebase context is not defined

    Follow the guide to enable codebase context checks.

    Custom Compliance
    No custom compliance provided

    Follow the guide to enable custom compliance check.

    • [ ] Update
    Compliance status legend 🟢 - Fully Compliant
    🟡 - Partial Compliant
    🔴 - Not Compliant
    ⚪ - Requires Further Human Verification
    🏷️ - Compliance label

    qodo-code-review[bot] avatar Oct 14 '25 08:10 qodo-code-review[bot]

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Learned
    best practice
    Guard against null elements in collection

    Also validate that the collection does not contain null elements to avoid NPEs
    in downstream code. Add a check and clear error message.

    java/src/org/openqa/selenium/remote/DriverCommand.java [170-177]

     static CommandPayload NEW_SESSION(Collection<Capabilities> capabilities) {
       Require.nonNull("Capabilities", capabilities);
       if (capabilities.isEmpty()) {
         throw new IllegalArgumentException("Capabilities for new session must not be empty");
       }
    +  if (capabilities.stream().anyMatch(c -> c == null)) {
    +    throw new IllegalArgumentException("Capabilities collection must not contain null elements");
    +  }
     
       return new CommandPayload(NEW_SESSION, Map.of("capabilities", capabilities));
     }
    
    • [ ] Apply / Chat
    Suggestion importance[1-10]: 6

    __

    Why: Relevant best practice - Validate inputs and states early to prevent NPEs and logic errors, especially for null checks on parameters to public APIs.

    Low
    Null-check TimeUnit in deprecated setters

    Add null checks for the TimeUnit parameter (and consider negative times) to
    prevent NPEs and undefined behavior.

    java/src/org/openqa/selenium/remote/DriverCommand.java [347-368]

     @Deprecated
     static CommandPayload SET_IMPLICIT_WAIT_TIMEOUT(long time, TimeUnit unit) {
    +  Require.nonNull("Time unit", unit);
       return SET_IMPLICIT_WAIT_TIMEOUT(Duration.ofMillis(unit.toMillis(time)));
     }
     
     @Deprecated
     static CommandPayload SET_SCRIPT_TIMEOUT(long time, TimeUnit unit) {
    +  Require.nonNull("Time unit", unit);
       return SET_SCRIPT_TIMEOUT(Duration.ofMillis(unit.toMillis(time)));
     }
     
     @Deprecated
     static CommandPayload SET_PAGE_LOAD_TIMEOUT(long time, TimeUnit unit) {
    +  Require.nonNull("Time unit", unit);
       return SET_PAGE_LOAD_TIMEOUT(Duration.ofMillis(unit.toMillis(time)));
     }
    
    • [ ] Apply / Chat
    Suggestion importance[1-10]: 5

    __

    Why: Relevant best practice - Validate inputs early with precise checks; ensure deprecated overloads converting time values guard against null TimeUnit.

    Low
    • [ ] More

    qodo-code-review[bot] avatar Oct 14 '25 08:10 qodo-code-review[bot]