Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 10 commits into from
Closed

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

wants to merge 10 commits into from

Conversation

@shbenzer
Copy link
Contributor

@shbenzer shbenzer commented Jan 4, 2025 •
edited by qodo-merge-pro bot
Loading

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 COMMAND_OR_CONTROL key to Keys class for OS-specific behavior.

  • Implemented logic to return COMMAND for macOS and CONTROL for other OS.

  • Added a test to validate COMMAND_OR_CONTROL functionality in typing_tests.py.

  • Ensured compatibility with existing functionality and test coverage.

Changes walkthrough 📝

Relevant files
Enhancement
keys.py
Introduced `COMMAND_OR_CONTROL` key for OS-specific behavior

py/selenium/webdriver/common/keys.py

  • Added COMMAND_OR_CONTROL attribute to Keys class.
  • Implemented OS-specific logic using sys.platform.
    		• +4/-0     
    Tests
    typing_tests.py
    Added test for `COMMAND_OR_CONTROL` key functionality       

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

  • Added a test for COMMAND_OR_CONTROL key functionality.
  • Validated behavior for typing and clearing input using OS-specific
    keys.
    		• +9/-0     
    Need help?
  • Type /help how to ... in the comments thread for any question about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 4, 2025 •
    edited
    Loading

    PR Reviewer Guide 🔍

    (Review updated until commit 77e8a04)

    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 uses only 'darwin' check which may not cover all macOS versions or variants. Consider using a more comprehensive OS detection approach.

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

    The test only verifies text clearing functionality but does not validate that the correct key (COMMAND vs CONTROL) is actually being used 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.COMMAND_OR_CONTROL + "a")
    element.send_keys(Keys.BACKSPACE)
    assert element.get_attribute("value") == ""

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 4, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    Latest suggestions up to 77e8a04
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Strengthen test coverage by validating OS-specific key selection

    Enhance the test by verifying the correct key (COMMAND or CONTROL) is used based on
    the current operating system.

    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.COMMAND_OR_CONTROL == expected_key
     element.send_keys(Keys.COMMAND_OR_CONTROL + "a")
     element.send_keys(Keys.BACKSPACE)
     assert element.get_attribute("value") == ""
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion significantly improves test coverage by adding explicit verification of the OS-specific key selection logic, which is crucial for ensuring the correct behavior of COMMAND_OR_CONTROL across different platforms.

    		8

    Previous suggestions

    ✅ Suggestions up to commit e8772e6
    CategorySuggestion                                                                                                                                    Score
    Possible issue
    ✅ Add platform validation to prevent unexpected behavior on unsupported operating systems
    Suggestion Impact:The commit addresses the same code section but takes a different approach by replacing the method with a constant. Instead of adding platform validation, it simplifies the logic by pre-computing the value.

    code diff:

    -    @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
+    
+    COMMAND_OR_CONTROL = COMMAND if sys.platform == "darwin" else CONTROL

    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}")
    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") == ""
    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

    nvborisenko commented Jan 5, 2025

    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.

    @shbenzer shbenzer closed this Jan 16, 2025
    @shbenzer shbenzer deleted the ctrl-or-meta branch January 16, 2025 00:38
    @shbenzer shbenzer restored the ctrl-or-meta branch January 16, 2025 19:37
    @shbenzer shbenzer reopened this Jan 16, 2025
    @VietND96 VietND96 changed the title Created Method to return Keys.CONTROL or Keys.COMMAND based on User OS [py] Created Method to return Keys.CONTROL or Keys.COMMAND based on User OS Jan 20, 2025
    @VietND96 VietND96 added the C-py Python Bindings label Feb 9, 2025
    @diemol
    Copy link
    Member

    diemol commented Aug 17, 2025

    I am closing this one due to the issue's resolution.

    @diemol diemol closed this Aug 17, 2025
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @AutomatedTester AutomatedTester Awaiting requested review from AutomatedTester

    Assignees

    No one assigned

    Labels

    C-py Python Bindings Review effort [1-5]: 2

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    4 participants

    @shbenzer @nvborisenko @diemol @VietND96