Skip to content

fixed mypy error and change source var #15853

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

Merged
merged 11 commits into from
Jun 6, 2025

Conversation

pallavigitwork
Copy link
Member

@pallavigitwork pallavigitwork commented Jun 4, 2025

User description

🔗 Related Issues

💥 What does this PR do?

fixed mypy error and change source var

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Bug fix (backwards compatible)
  • New feature (non-breaking change which adds functionality and tests!)
  • Breaking change (fix or feature that would cause existing functionality to change)

PR Type

Bug fix, Enhancement


Description

  • Renamed source attribute to input_source for clarity

  • Fixed mypy type error in KeyActions class

  • Updated superclass initialization to use correct argument

  • Adjusted internal method to use new attribute name


Changes walkthrough 📝

Relevant files
Bug fix
key_actions.py
Refactor KeyActions for type safety and clarity                   

py/selenium/webdriver/common/actions/key_actions.py

  • Renamed self.source to self.input_source throughout the class
  • Changed superclass __init__ call to use KEY instead of source
  • Updated internal method to reference new attribute name
  • Addresses mypy type-checking error and improves code clarity
  • +3/-3     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @selenium-ci selenium-ci added the C-py Python Bindings label Jun 4, 2025
    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 4, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ❌

    1234 - Not compliant

    Non-compliant requirements:

    • Fix issue where Selenium 2.48 doesn't trigger JavaScript in link's href on click()
    • Ensure JavaScript alerts are triggered properly when clicking links with JavaScript in href attributes
    • Make behavior consistent with previous version (2.47.1)
    • Fix specifically for Firefox 42.0

    5678 - Not compliant

    Non-compliant requirements:

    • Fix "ConnectFailure (Connection refused)" error when instantiating ChromeDriver
    • Address issue where subsequent ChromeDriver instantiations fail after first successful one
    • Fix specifically for Chrome 65.0.3325.181 with ChromeDriver 2.35 on Ubuntu 16.04.4

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

    Potential Bug

    The variable meth is assigned but might not be used correctly in the _key_action method. The line after assignment doesn't reference the variable.

    meth = getattr(self.input_source, action)
    meth(letter)

    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 4, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 82a4897

    CategorySuggestion                                                                                                                                    Impact
    Incremental [*]
    Use source's type attribute

    The code determines the source type based on the input object type, but it would
    be more robust to use the source's type attribute directly if available, as it
    might have been customized during initialization.

    py/selenium/webdriver/common/actions/key_actions.py [32-42]

     # Determine the correct source type string based on the input object
    -if isinstance(source, KeyInput):
    +if hasattr(source, 'type'):
    +    source_type = source.type
    +elif isinstance(source, KeyInput):
         source_type = KEY
     elif isinstance(source, PointerInput):
         source_type = POINTER
     elif isinstance(source, WheelInput):
         source_type = WHEEL
     else:
         source_type = KEY  # fallback
         
     super().__init__(source_type)
    • Apply / Chat
    Suggestion importance[1-10]: 6

    __

    Why: This is a reasonable improvement that would make the code more robust by checking for a type attribute first, though it's a minor enhancement since the current isinstance approach works correctly.

    Low
    • Update

    Previous suggestions

    ✅ Suggestions up to commit 1ee594a
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix incorrect constructor parameter
    Suggestion Impact:The commit implemented exactly what was suggested - changing super().__init__(KEY) to super().__init__(self.input_source) to pass the correct input source object to the parent class constructor instead of a string constant

    code diff:

    -        super().__init__(KEY)
    +        super().__init__(self.input_source)

    The super().init(KEY) call is incorrect. The parent class Interaction
    expects an input source object, not a string constant. You should pass source
    (or self.input_source) to the parent constructor instead of KEY.

    py/selenium/webdriver/common/actions/key_actions.py [27-31]

     def __init__(self, source: KeyInput | PointerInput | WheelInput | None = None) -> None:
         if source is None:
             source = KeyInput(KEY)
         self.input_source = source
    -    super().__init__(KEY)
    +    super().__init__(self.input_source)

    [Suggestion processed]

    Suggestion importance[1-10]: 9

    __

    Why: This identifies a critical bug where super().__init__(KEY) passes a string constant instead of the expected input source object. This would likely cause runtime errors or incorrect behavior in the parent class.

    High

    @pallavigitwork
    Copy link
    Member Author

    ran these two commands -
    pytest bazel-selenium/py/test/selenium/webdriver/common/interactions_with_device_tests.py
    pytest bazel-selenium/py/test/selenium/webdriver/common/interactions_tests.py

    are these the correct tests to run for this file ? @navin772 , can you kindly please guide. thanks.

    @navin772
    Copy link
    Member

    navin772 commented Jun 4, 2025

    This is the test command - pytest py/test/selenium/webdriver/common/w3c_interaction_tests.py. You can also run the 2 test files you mentioned.

    @pallavigitwork
    Copy link
    Member Author

    ran this - pytest bazel-selenium/py/test/selenium/webdriver/common/w3c_interaction_tests.py
    got these errors
    bazel-selenium/py/test/selenium/webdriver/common/w3c_interaction_tests.py::test_touch_pointer_properties FAILED
    E AssertionError: assert 9 == 7
    E + where 9 = len([{'altKey': False, 'altitudeAngle': 1.1827416313714896, 'azimuthAngle': 5.932221251670766, 'button': 0, ...}, {'altKey': False, 'altitudeAngle': 1.1827416313714896, 'azimuthAngle': 5.932221251670766, 'button': 0, ...}, {'altKey': False, 'altitudeAngle': 1.1827416313714896, 'azimuthAngle': 5.932221251670766, 'button': 0, ...}, {'altKey': False, 'altitudeAngle': 0.4818905427524743, 'azimuthAngle': 1.7518731617179564, 'button': -1, ...}, {'altKey': False, 'altitudeAngle': 1.5707963267948966, 'azimuthAngle': 0, 'button': -1, ...}, {'altKey': False, 'altitudeAngle': 1.5707963267948966, 'azimuthAngle': 0, 'button': -1, ...}, ...])

    py/test/selenium/webdriver/common/w3c_interaction_tests.py:251: AssertionError

    @navin772 -- i don't think it is related. is it?

    @cgoldberg
    Copy link
    Contributor

    please run the formatter on your branch.

    Copy link
    Contributor

    @cgoldberg cgoldberg left a comment

    Choose a reason for hiding this comment

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

    I think having source and self.input_source refer to the same thing is confusing. I think leaving the old name is better.

    Is there a reason for renaming _key_action to keyaction? Removing the leading underscore means it is public, where it was private previously. Also, key_action better follows our snake case naming convention.

    @pallavigitwork
    Copy link
    Member Author

    pallavigitwork commented Jun 4, 2025

    @cgoldberg if i don't change the source var, the w3c test throws error
    FAILED bazel-selenium/py/test/selenium/webdriver/common/w3c_interaction_tests.py::test_sending_keys_to_active_element_with_modifier - AttributeError: 'str' object has no attribute 'create_key_down'
    FAILED bazel-selenium/py/test/selenium/webdriver/common/w3c_interaction_tests.py::test_can_create_pause_action_on_keyboard - AttributeError: 'str' object has no attribute 'create_pause'

    -using another variable to call on actual object then str.

    --i changed the key action method, thank you for helping.

    @pallavigitwork pallavigitwork requested a review from cgoldberg June 4, 2025 16:52
    Copy link
    Contributor

    @cgoldberg cgoldberg left a comment

    Choose a reason for hiding this comment

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

    if i don't change the source var, the w3c test throws error

    How did it work correctly before your change?

    Could you remove the 3 comments you added in your last change? They aren't needed.

    @pallavigitwork
    Copy link
    Member Author

    @cgoldberg
    if i go back to original file where we don't separate source type and input source, mypy errors come as follows-
    selenium/webdriver/common/actions/key_actions.py:31: error: Incompatible types in assignment (expression has type "KeyInput | PointerInput | WheelInput", variable has type "str") [assignment]
    selenium/webdriver/common/actions/key_actions.py:43: error: Argument 1 to "init" of "Interaction" has incompatible type "KeyInput | PointerInput | WheelInput"; expected "str" [arg-type]

    w3c tests pass.

    with separating these vars, w3c tests passes and mypy errors get fixed.

    i am renaming the variables-
    Interaction class - defines source as str
    but for some we need actual object -like method create key down

    looked at the pointer, wheel and key input files.

    mypy errors arent there now.
    ran pytest for w3c and both interactions
    ran format
    ran ruff

    please let me know if there is anything else? i am trying to understand. thanks Corey.

    @pallavigitwork pallavigitwork requested a review from cgoldberg June 5, 2025 05:06
    @cgoldberg
    Copy link
    Contributor

    if i go back to original file where we don't separate source type and input source, mypy errors come as follows

    OK, I understand now.
    I just removed the comments in your branch.. this looks good now.

    Copy link
    Contributor

    @cgoldberg cgoldberg left a comment

    Choose a reason for hiding this comment

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

    LGTM. thanks!

    @cgoldberg cgoldberg merged commit 523fbc8 into SeleniumHQ:trunk Jun 6, 2025
    4 checks passed
    @pallavigitwork
    Copy link
    Member Author

    Thank you @cgoldberg .

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants