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

[py] remove python 2.x support from py test file #14504

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

Conversation

navin772
Copy link
Contributor

@navin772 navin772 commented Sep 16, 2024

User description

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

Remove support for python 2.x in the py test file - py/test/selenium/webdriver/common/position_and_size_tests.py

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

Bug fix


Description

  • Removed support for Python 2.x in the test file position_and_size_tests.py by eliminating the use of viewitems() method.
  • Unified the code to use Python 3.x syntax for dictionary item comparisons, ensuring compatibility with Python 3.x.

Changes walkthrough 📝

Relevant files
Bug fix
position_and_size_tests.py
Remove Python 2.x support from test file                                 

py/test/selenium/webdriver/common/position_and_size_tests.py

  • Removed Python 2.x specific code for checking dictionary items.
  • Unified code to use Python 3.x syntax for dictionary item comparison.
  • +2/-8     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ No key issues to review

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Performance
    Use set operations for comparing dictionary items

    Consider using set() to compare dictionary items for better performance and
    readability.

    py/test/selenium/webdriver/common/position_and_size_tests.py [112-114]

    -expected = kwargs.items()
    -actual = location.items()
    -assert expected <= actual
    +assert set(kwargs.items()).issubset(location.items())
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Using set() for comparison improves performance and readability by leveraging set operations, which are more efficient for subset checks.

    8
    Maintainability
    Use more descriptive variable names for clarity

    Consider using a more descriptive variable name instead of expected to clarify that
    it represents the expected key-value pairs.

    py/test/selenium/webdriver/common/position_and_size_tests.py [112-114]

    -expected = kwargs.items()
    -actual = location.items()
    -assert expected <= actual
    +expected_items = kwargs.items()
    +actual_items = location.items()
    +assert expected_items <= actual_items
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using more descriptive variable names enhances code readability and maintainability, making it clearer what the variables represent.

    6

    Copy link
    Member

    @harsha509 harsha509 left a comment

    Choose a reason for hiding this comment

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

    LGTM!

    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