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

feat: support selenium 4.26+: support ClientConfig and refactoring internal implementation #1054

Merged
merged 31 commits into from
Nov 11, 2024

Conversation

KazuCocoa
Copy link
Member

@KazuCocoa KazuCocoa commented Oct 31, 2024

Python selenium client has two major changes:

test

Current unit tests cover find_element/s, user agent, extra headers and commands.
The functional tests cover session creation, do some actions and deleting the session.

@KazuCocoa KazuCocoa changed the title applying selenium 4.26 feat: bump selenium 4.26+ dependency Nov 2, 2024
@KazuCocoa KazuCocoa changed the title feat: bump selenium 4.26+ dependency feat: support selenium 4.26+ dependency Nov 2, 2024
@KazuCocoa KazuCocoa changed the title feat: support selenium 4.26+ dependency feat: support selenium 4.26+: support ClientConfig and refactoring internal implementation Nov 5, 2024
@KazuCocoa KazuCocoa marked this pull request as ready for review November 8, 2024 07:47
@@ -200,32 +213,34 @@ class WebDriver(
Sms,
SystemBars,
):
def __init__(
def __init__( # noqa: PLR0913
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to extract the custom pre-init code into a class or static helper method, so we don't need to ignore the linter warning?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean something like:

def __init__(self, **kwargs):
    args = getSomething(**kwargs)
    command_executor = args.command_executor
    ...

?

I was thinking of defining a custom client config after this PR to drop existing ones such as keep_alive, direct_connect and strict_ssl like AppiumClientConfig and:

def __init__(
  self,
  command_executor = 'http://127.0.0.1:4444/wd/hub',
  extensions = None,
  client_config = AppiumClientConfig
)

with major version update (as a breaking change.) So this lint ignore is a temporary one.

Copy link
Contributor

@mykola-mokhnach mykola-mokhnach Nov 9, 2024

Choose a reason for hiding this comment

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

the original linter error is about super init method is not the first call in the class constructor. In order to address that we would need to move the preceeding code into separate method call(s). for example:

def __init__(self, x, y):
    z = ... # lint warning here
    super().__init__(z=z)

could be changed to

def __init__(self, x, y):
    super().__init__(z=self.calculate_z(x, y)) # no warning

@staticmethod
def calculate_z(x, y):
    return z

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, i see

Copy link
Member Author

Choose a reason for hiding this comment

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

hm, the error I suppressed was:

appium/webdriver/webdriver.py:216:9: PLR0913 Too many arguments in function definition (7 > 6)
    |
214 |     SystemBars,
215 | ):
216 |     def __init__(
    |         ^^^^^^^^ PLR0913
217 |         self,
218 |         command_executor: Union[str, AppiumConnection] = 'http://127.0.0.1:4444/wd/hub',
    |

Found 1 error.

so the number of args for the init itself...?

I did very roughly as below but this didn't help:

def get_config(command_executor, keep_alive, strict_ssl, client_config):
    if isinstance(command_executor, str):
        client_config = client_config or ClientConfig(
            remote_server_addr=command_executor, keep_alive=keep_alive, ignore_certificates=not strict_ssl
        )
        client_config.remote_server_addr = command_executor
        return client_config
    return None

def get_command_executor(command_executor, keep_alive, strict_ssl, client_config):
    if isinstance(command_executor, str):
        client_config = client_config or ClientConfig(
            remote_server_addr=command_executor, keep_alive=keep_alive, ignore_certificates=not strict_ssl
        )
        client_config.remote_server_addr = command_executor
        return AppiumConnection(remote_server_addr=command_executor, client_config=client_config)
    return command_executor

class WebDriver(
    ...
):
    def __init__(  # noqa: PLR0913
        self,
        command_executor: Union[str, AppiumConnection] = 'http://127.0.0.1:4444/wd/hub',
        keep_alive: bool = True,
        direct_connection: bool = True,
        extensions: Optional[List['WebDriver']] = None,
        strict_ssl: bool = True,
        options: Union[AppiumOptions, List[AppiumOptions], None] = None,
        client_config: Optional[ClientConfig] = None,
    ):
        super().__init__(
            command_executor=get_command_executor(command_executor, keep_alive, strict_ssl, client_config),
            options=options,
            locator_converter=AppiumLocatorConverter(),
            web_element_cls=MobileWebElement,
            client_config=get_config(command_executor, keep_alive, strict_ssl, client_config),
        )

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought this warning is related to a different cause. Please keep the noqa then. Nevertheless I like the above change. Please do not forget to add type declarations to new functions


super().__init__(
command_executor=command_executor,
options=options,
locator_converter=AppiumLocatorConverter(),
Copy link
Contributor

Choose a reason for hiding this comment

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

does this cache AppiumLocatorConverter instance or a new one is created every time a new WebDriver instance is constructed?

Copy link
Member Author

Choose a reason for hiding this comment

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

setup.py Outdated Show resolved Hide resolved
@KazuCocoa KazuCocoa merged commit 94a6da7 into master Nov 11, 2024
10 checks passed
@KazuCocoa KazuCocoa deleted the selenium-4.26 branch November 11, 2024 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants