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

Created Method to return Keys.CONTROL or Keys.COMMAND based on User OS #15026

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from

Conversation

shbenzer
Copy link
Contributor

@shbenzer shbenzer commented Jan 4, 2025

User description

Description

Created Method to return Keys.CONTROL or Keys.COMMAND based on User OS
Added test to typing_tests.py

Motivation and Context

Issue #14835

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


Description

  • Added ctrl_or_command method to return OS-specific key.

  • Enhanced Keys class with platform-aware functionality.

  • Introduced a test for ctrl_or_command in typing_tests.py.

  • Ensured compatibility with Mac, Linux, and Windows platforms.


Changes walkthrough 📝

Relevant files
Enhancement
keys.py
Introduced `ctrl_or_command` method in `Keys` class           

py/selenium/webdriver/common/keys.py

  • Added ctrl_or_command method to return OS-specific key.
  • Imported sys module for platform detection.
  • Enhanced Keys class functionality with platform-aware logic.
  • +7/-0     
    Tests
    typing_tests.py
    Added test for `ctrl_or_command` method                                   

    py/test/selenium/webdriver/common/typing_tests.py

  • Added test for ctrl_or_command method functionality.
  • Verified key behavior for different operating systems.
  • Ensured proper key handling in keyReporter element.
  • +9/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 4, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Platform Detection

    The platform detection logic only checks for 'darwin' but should also validate other platforms like Windows/Linux to ensure proper key mapping in all environments

    return Keys.COMMAND if sys.platform == "darwin" else Keys.CONTROL
    Test Coverage

    The test only verifies text selection and deletion but does not explicitly validate that the correct key (Control vs Command) is being returned based on the OS

    def test_should_type_ctrl_or_command_based_on_os(driver, pages):
        pages.load("javascriptPage.html")
        element = driver.find_element(by=By.ID, value="keyReporter")
        element.send_keys(1234)
        assert element.get_attribute("value") == "1234"
        element.send_keys(Keys.ctrl_or_command() + "a")
        element.send_keys(Keys.BACKSPACE)
        assert element.get_attribute("value") == ""

    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 4, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add platform validation to prevent unexpected behavior on unsupported operating systems

    Add error handling for unsupported platforms by checking if sys.platform is neither
    'darwin' nor 'win32'/'linux'. This prevents unexpected behavior on other operating
    systems.

    py/selenium/webdriver/common/keys.py [94-97]

     @staticmethod
     def ctrl_or_command():
         """Returns the control (if Linux/Windows) or command (if Mac) key value."""
    -    return Keys.COMMAND if sys.platform == "darwin" else Keys.CONTROL
    +    if sys.platform == "darwin":
    +        return Keys.COMMAND
    +    elif sys.platform in ("win32", "linux"):
    +        return Keys.CONTROL
    +    raise ValueError(f"Unsupported platform: {sys.platform}")
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion adds important error handling for unsupported platforms, preventing potential runtime issues and making the code more robust. The explicit platform validation improves code reliability and maintainability.

    8
    General
    Enhance test coverage by validating the correct OS-specific key is returned

    The test should verify the actual OS-specific behavior by checking if the correct
    key (CONTROL or COMMAND) is returned based on the current platform.

    py/test/selenium/webdriver/common/typing_tests.py [350-357]

     def test_should_type_ctrl_or_command_based_on_os(driver, pages):
         pages.load("javascriptPage.html")
         element = driver.find_element(by=By.ID, value="keyReporter")
         element.send_keys(1234)
         assert element.get_attribute("value") == "1234"
    +    expected_key = Keys.COMMAND if sys.platform == "darwin" else Keys.CONTROL
    +    assert Keys.ctrl_or_command() == expected_key
         element.send_keys(Keys.ctrl_or_command() + "a")
         element.send_keys(Keys.BACKSPACE)
         assert element.get_attribute("value") == ""
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion strengthens the test by explicitly verifying that the correct key (CONTROL/COMMAND) is returned based on the platform, improving test coverage and catching potential platform-specific issues.

    7

    @nvborisenko
    Copy link
    Member

    What about remote webdriver?

    @shbenzer
    Copy link
    Contributor Author

    shbenzer commented Jan 5, 2025

    What about remote webdriver?

    We're discussing that on slack right now - short answer is that we haven't found a convenient solution for that. We're not sure if Playwright's implementation ControlOrMeta works on remote drivers either.

    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