[py] optimised BaseOptions in common/options.py

[sandeepsuryaprasad]

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

Optimised BaseOptions object by creating _BaseOptions, _PageLoadStrategy, _UnHandledPromptBehavior and _Timeouts descriptors by eliminating multiple properties

Motivation and Context

• In BaseOptions class, different browser attributes are being set through multiple properties like

  browser_version
platform_name
page_load_strategy
unhandled_prompt_behavior
timeouts
accept_insecure_certs
strict_file_interactability
set_window_rect
proxy

• Now that I have replaced multiple properties with below descriptor objects
  _BaseOptions
_PageLoadStrategy
_UnHandledPromptBehavior
_Timeouts

• By doing so, the descriptors has eliminated 18 getter and setter methods.

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.

[codecov-commenter]

Codecov Report

Patch coverage: 17.18% and project coverage change: -0.82 ⚠️

Comparison is base (9772e55) 58.09% compared to head (e9a7837) 57.28%.

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

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk   #12213      +/-   ##
==========================================
- Coverage   58.09%   57.28%   -0.82%     
==========================================
  Files          86       86              
  Lines        5348     5349       +1     
  Branches      207      205       -2     
==========================================
- Hits         3107     3064      -43     
- Misses       2034     2080      +46     
+ Partials      207      205       -2     

Impacted Files	Coverage Δ
py/selenium/webdriver/common/options.py	33.66% <17.18%> (-41.34%)	⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[titusfortner]

I'm still trying to understand the improvement. If we want to keep docstring info for the IDEs, does this new approach still reduce code?

[titusfortner]

Why do we treat PageLoadStrategy, Timeouts, Proxy and UnhandledPromptBehavior differently?

Shouldn't we be documenting all w3c capabilities this way?

[sandeepsuryaprasad]

Why do we treat PageLoadStrategy, Timeouts, Proxy and UnhandledPromptBehavior differently?

Shouldn't we be documenting all w3c capabilities this way?

We can stuff PageLoadStrategy, UnHandledPromptBehavior, Timeouts and Poxy in one single descriptor. But we need to have multiple conditional statements inside __set__ method since the behaviour of __set__ method is slightly different for each of the above 4 descriptors. So from code readability perspective I have just separated them out into different classes. If you observe _BaseOptions class, the behaviour of __set__ method is common across attributes browser_version, platform_name, accept_insecure_certs, strict_file_interactability, set_window_rect. So for all the above attributes we can re-use the same __get__ and __set__ method in _BaseOptions class. We don't have to write multiple getters and setters. Since the behaviour is same across all 5 attributes we can re-use __get__ and __set__ in _BaseOptions.

[titusfortner]

But that means we are only getting the extra docstring details on the items that have non-default getter and/or setter behavior? Is there a way to use these descriptors instead of properties, have docstring details that can be consumed by the constructor, and have it result in less code and/or be more straightforward than what we have now with properties?

[titusfortner]

But that means we are only getting the extra docstring details on the items that have non-default getter and/or setter behavior? Is there a way to use these descriptors instead of properties, have docstring details that can be consumed by the constructor, and have it result in less code and/or be more straightforward than what we have now with properties?

[sandeepsuryaprasad]

I'm still trying to understand the improvement. If we want to keep docstring info for the IDEs, does this new approach still reduce code?

In the current code base we have separate getter and setter methods for each browser attribute. If we take PrintOptions class, we have separate getter and setter method for setting page width, similarly a separate pair of getter and setter method for setting height. Since we have 4 different methods all together, we have written separate doc strings for each of the methods. So IDE's show doc string for each attribute (page_width and page_height) . In the new approach both page_width and page_height are descriptor instances of same class. So the thing is IDE's will not show the doc string at an attribute level, rather it shows doc string of all the descriptor instances of PageSettings class. We can mention doc strings of both page_width and page_height inside PageSettings class. So when are about to create an instance of PageSettings class, the doc strings pops-up giving the information about all the descriptor attributes related to PageSettings class. I am attaching the screen shot for your reference.

[titusfortner]

Ok, looking at this side by side with existing... One of the goals of this PR was to make the classes shorter. Once we add the requirement back in for keeping the comments displayed in the user's IDE, though, it results in a larger class.

I know @isaulv generally prefers the descriptors, but I don't think they are normally combined with code comments (Ruby's attr_accessor aren't).

@KazuCocoa what is your opinion? Is there a common approach that Selenium/Appium can/should take here?

[sandeepsuryaprasad]

Ok, looking at this side by side with existing... One of the goals of this PR was to make the classes shorter. Once we add the requirement back in for keeping the comments displayed in the user's IDE, though, it results in a larger class.

I know @isaulv generally prefers the descriptors, but I don't think they are normally combined with code comments (Ruby's attr_accessor aren't).

@KazuCocoa what is your opinion? Is there a common approach that Selenium/Appium can/should take here?

@titusfortner @KazuCocoa , I have gone though options in Appium python client and observed tons and tons of python modules with hundreds of properties. Most of them doing very similar work, all getters calling get_capability method and all setters calling set_capability method. We can greatly optimise it by having one single descriptor. I am posting the optimised code that I have written for common options in options/common without breaking the existing code. That way a lot of boiler plate code is avoided.

class OptionsDescriptor:
    def __init__(self, name):
        self.name = name

    def __get__(self, obj, cls):
        return getattr(obj, "get_capabilities")(self.name)

    def __set__(self, obj, value):
        getattr(obj, "set_capabilities")(self.name, value)

We can make the rest of the option classes to inherit from the main OptionsDescriptor class.

class AppOption(SupportsCapabilities):
        """
        Set the absolute local path for the location of the App.
        The app must be located on the same machine where Appium
        server is running.
        Could also be a valid URL.
        """
    APP = 'app'
    
   # Create a descriptor object (Indirectly we are creating a getter and setter here)
    app = OptionsDescriptor("APP")

class AutoWebViewOption(SupportsCapabilities):
        """
        Set whether the driver should try to automatically switch
        a web view context after the session is started.
        """
    AUTO_WEB_VIEW = 'autoWebView'
    auto_web_view = OptionsDescriptor("AUTO_WEB_VIEW")

class AutomationNameOption(SupportsCapabilities):
       """Gets and Sets the automation driver name to use for the given platform."""
    AUTOMATION_NAME = 'automationName'
    automation_name = OptionsDescriptor("AUTOMATION_NAME")

class BundleIdOption(SupportsCapabilities):
       """Gets and Sets the bundle identifier of the application to automate."""
    BUNDLE_ID = 'bundleId'
    bundle_id = OptionsDescriptor("BUNDLE_ID")

class ClearSystemFilesOption(SupportsCapabilities):
       """ Set whether the driver should delete generated files at the end of a session."""
    CLEAR_SYSTEM_FILES = 'clearSystemFiles'
    clear_system_files = OptionsDescriptor("CLEAR_SYSTEM_FILES")

class DeviceNameOption(SupportsCapabilities):
    """Gets and Sets the name of the device to be used in the test."""
    DEVICE_NAME = 'deviceName'
    device_name = OptionsDescriptor("DEVICE_NAME")

class EnablePerformanceLoggingOption(SupportsCapabilities):
        """Gets and Sets whether to enable additional performance logging."""
    ENABLE_PERFORMANCE_LOGGING = 'enablePerformanceLogging'
    enable_performance_logging = OptionsDescriptor("ENABLE_PERFORMANCE_LOGGING")

class EventTimingsOption(SupportsCapabilities):
        """
        Get Whether the driver should to report the timings
        for various Appium-internal events.

        Set whether the driver should to report the timings
        for various Appium-internal events.
        """
    EVENT_TIMINGS = 'eventTimings'
    event_timings = OptionsDescriptor("EVENT_TIMINGS")

class FullResetOption(SupportsCapabilities):
       """
        Get Whether the driver should perform a full reset.
        Set whether the driver should perform a full reset.
        """
    FULL_RESET = 'fullReset'
    full_reset = OptionsDescriptor("FULL_RESET")

class IsHeadlessOption(SupportsCapabilities):
       """
        Whether the driver should start emulator/simulator in headless mode.
        Set emulator/simulator to start in headless mode (e.g. no UI is shown).
        It is only applied if the emulator is not running before the test starts.
        """
    IS_HEADLESS = 'isHeadless'
    is_headless = OptionsDescriptor("IS_HEADLESS")

class LanguageOption(SupportsCapabilities):
        """
         Gets Language abbreviation to use in a test session
        Set language abbreviation to use in a test session.
        """
    LANGUAGE = 'language'
    language = OptionsDescriptor("LANGUAGE")

class LocaleOption(SupportsCapabilities):
        """
       Gets Locale abbreviation to use in a test session.
        Set locale abbreviation to use in a test session.
        """
    LOCALE = 'locale'
    locale = OptionsDescriptor("LOCALE")

class NewCommandTimeoutOption(SupportsCapabilities):
        """
        The allowed time before seeing a new server command.
        Set the allowed time before seeing a new server command.
        The value could either be provided as timedelta instance or an integer number of seconds.
        """
    NEW_COMMAND_TIMEOUT = 'newCommandTimeout'
     new_command_timeout = OptionsDescriptor("NEW_COMMAND_TIMEOUT")

class NoResetOption(SupportsCapabilities):
        """
       Whether the driver should not perform a reset.
        Set whether the driver should not perform a reset.
        """
    NO_RESET = 'noReset'
    no_reset = OptionsDescriptor("NO_RESET")

class OrientationOption(SupportsCapabilities):
       """
        Gets The orientation of the device's screen.
        Usually this is either 'PORTRAIT' or 'LANDSCAPE'.
       
        Set the orientation of the device's screen.
        Usually this is either 'PORTRAIT' or 'LANDSCAPE'.
        """
    ORIENTATION = 'orientation'
    orientation = OptionsDescriptor("ORIENTATION")

class OtherAppsOption(SupportsCapabilities):
      """
        Gets Locations of apps to install before running a test.
        Sets locations of apps to install before running a test.
        Each item could be separated with a single comma.
        """
    OTHER_APPS = 'otherApps'
    other_apps = OptionsDescriptor("OTHER_APPS")

class PostrunOption(SupportsCapabilities):
       """
       Gets System script which is supposed to be executed upon
        driver session quit.
        Sets a system script to execute upon driver session quit.
        """
    POSTRUN = 'postrun'
   postrun = OptionsDescriptor("POSTRUN")

class PrerunOption(SupportsCapabilities):
       """
       Gets System script which is supposed to be executed before
        a driver session is initialised.

        Sets a system script which is supposed to be executed before
        a driver session is initialised.
        """
    PRERUN = 'prerun'
    prerun = OptionsDescriptor("PRERUN")

class PrintPageSourceOnFindFailureOption(SupportsCapabilities):
        """
       Gets Whether the driver should print the page source to the log
        if a find failure occurs.

        Sets whether the driver should print the page source to the log
        if a find failure occurs.
        """
    PRINT_PAGE_SOURCE_ON_FIND_FAILURE = 'printPageSourceOnFindFailure'
    print_page_source_on_find_failure = OptionsDescriptor("PRINT_PAGE_SOURCE_ON_FIND_FAILURE")

class SkipLogCaptureOption(SupportsCapabilities):
       """
       Whether the driver should not record device logs.
        Set whether the driver should not record device logs.
        """
    SKIP_LOG_CAPTURE = 'skipLogCapture'
    skip_log_capture = OptionsDescriptor("SKIP_LOG_CAPTURE")

class SystemHostOption(SupportsCapabilities):
       """
        Gets The name of the host for the internal server to listen on.
        Sets the name of the host for the internal server to listen on.
        """
    SYSTEM_HOST = 'systemHost'
    system_host = OptionsDescriptor("SYSTEM_HOST")

class SystemPortOption(SupportsCapabilities):
       """
        Gets The number of the port for the internal server to listen on.
        Sets the number of the port for the internal server to listen on.
        """
    SYSTEM_PORT = 'systemPort'
    system_port = OptionsDescriptor("SYSTEM_PORT")

class UdidOption(SupportsCapabilities):
       """
       The unique identifier of the device under test.
        Set the unique identifier of the device under test.
        """
    UDID = 'udid'
    udid = OptionsDescriptor("UDID")

[KazuCocoa]

Oh, great. It should help to reduce the boiler place in Python client code. Let me cc @mykola-mokhnach as well

[AutomatedTester]

Please remove other PR commits from this PR and I will review

[sandeepsuryaprasad]

@AutomatedTester I have removed all previous commits of this PR and kept only the relevant change specific to this PR.
All tests (Unit as well as UI) passing in my local.

[sandeepsuryaprasad]

@AutomatedTester , I have also added type validations for all browser options in __set__ method. This was missing in all setter methods. Also, I have fixed the test method test_browser_version_is_used_for_sm where in we were setting an int value to browser_version instead of str.

[sandeepsuryaprasad]

As per discussion with @titusfortner I have removed the type checks that I had added in previous commit (1a22b11) from all base browser options. Also, the browser version is now being passed as an int as it was being done previously in test method as well test_browser_version_is_used_for_sm. All UI as well as Unit tests are passing in my local.

[sandeepsuryaprasad]

Please remove other PR commits from this PR and I will review

@AutomatedTester , I have removed previous commit. All tests are passing in my local. Request you to review this PR. Thank You.

[sandeepsuryaprasad]

now assigning browser version as string in selenium_manager_tests options.browser_version = "110"