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

[java] increasing of properties scope for better appium compatibility #14183

Merged
merged 9 commits into from
Oct 1, 2024

Conversation

iampopovich
Copy link
Contributor

@iampopovich iampopovich commented Jun 24, 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

according to request in #13949 scopes of properties were increased
here's a part of request

Increase the scope of private properties of the below classes to 'protected':
RemoteWebDriver -> capabilities

HttpCommandExecutor -> commandCodec

HttpCommandExecutor -> responseCodec

FluentWait -> clock
FluentWait -> timeout
FluentWait -> interval
FluentWait -> sleeper
FluentWait -> ignoredExceptions
FluentWait -> messageSupplier
FluentWait -> input

Make public accessor for the property:
HttpCommandExecutor -> client

DriverService.Builder -> exe

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


Description

  • Changed the access level of several fields in HttpCommandExecutor to improve compatibility with Appium:
    • client field changed to public.
    • commandCodec and responseCodec fields changed to protected.
  • Changed the access level of the capabilities field in RemoteWebDriver to protected.
  • Changed the access level of the exe field in DriverService.Builder to public.
  • Changed the access level of multiple fields in FluentWait to protected:
    • input, clock, sleeper, timeout, interval, messageSupplier, ignoredExceptions.

Changes walkthrough 📝

Relevant files
Enhancement
HttpCommandExecutor.java
Modify access levels for HttpCommandExecutor fields           

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

  • Changed client field to public.
  • Changed commandCodec and responseCodec fields to protected.
  • +3/-3     
    RemoteWebDriver.java
    Modify access level for RemoteWebDriver capabilities field

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

    • Changed capabilities field to protected.
    +1/-1     
    DriverService.java
    Modify access level for DriverService.Builder exe field   

    java/src/org/openqa/selenium/remote/service/DriverService.java

    • Changed exe field to public.
    +1/-1     
    FluentWait.java
    Modify access levels for FluentWait fields                             

    java/src/org/openqa/selenium/support/ui/FluentWait.java

  • Changed multiple fields to protected: input, clock, sleeper, timeout,
    interval, messageSupplier, ignoredExceptions.
  • +7/-7     

    💡 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 [1-5] 2
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review Access Level Changes:
    The PR changes the access levels of several fields in multiple classes. This could potentially expose internal implementation details and should be carefully reviewed to ensure that it does not violate encapsulation principles or introduce unintended side effects.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Change the access modifier of the client field from public to protected to improve encapsulation

    Consider making the client field protected instead of public to restrict access to
    subclasses only, maintaining encapsulation and reducing the risk of unintended
    modifications.

    java/src/org/openqa/selenium/remote/HttpCommandExecutor.java [47]

    -public final HttpClient client;
    +protected final HttpClient client;
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Changing the access modifier from public to protected for the client field enhances encapsulation and limits access to subclasses, which is a significant improvement in design.

    8
    Make the capabilities field final to prevent it from being reassigned after initialization

    Consider making the capabilities field final to indicate that it should not be reassigned
    after being initialized, which can help prevent bugs.

    java/src/org/openqa/selenium/remote/RemoteWebDriver.java [117]

    -protected Capabilities capabilities;
    +protected final Capabilities capabilities;
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Making the capabilities field final is a good practice to ensure it is not modified after initialization, which can help in maintaining immutability and preventing bugs.

    7
    Enhancement
    Initialize the messageSupplier field with a more meaningful default message

    Consider initializing the messageSupplier field with a more meaningful default message to
    improve debugging and error reporting.

    java/src/org/openqa/selenium/support/ui/FluentWait.java [75]

    -protected Supplier<String> messageSupplier = () -> null;
    +protected Supplier<String> messageSupplier = () -> "Condition not met within timeout";
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Providing a more meaningful default message for messageSupplier can enhance debugging and error reporting, which improves the usability of the class.

    6

    @iampopovich iampopovich changed the title 13949 better appium compatibility [java] increasing of properties scope for better appium compatibility Jun 24, 2024
    Copy link
    Contributor

    @pujagani pujagani left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Thank you @iampopovich!

    @pujagani pujagani merged commit e455840 into SeleniumHQ:trunk Oct 1, 2024
    3 of 4 checks passed
    @iampopovich
    Copy link
    Contributor Author

    iampopovich commented Oct 1, 2024

    thanks god . it took a bit more time than usual 🙂
    guess you have a lot of work with bidi

    @iampopovich iampopovich deleted the 13949-appium-compatibility branch October 1, 2024 11:54
    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