Skip to content

Fix Python formatting in tests #2866

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 3 commits into from
Jun 17, 2025
Merged

Conversation

cgoldberg
Copy link
Contributor

@cgoldberg cgoldberg commented Jun 16, 2025

User description

Description

This PR fixes formatting inconsistencies in the Python files under /tests/ to make them adhere more closely to PEP8 standards.

Formatting was done with:

isort tests/
black --line-length=120 --skip-string-normalization tests/

There are no functional changes in this PR.

Motivation and Context

Better readability/maintainability of Python code.

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

Other


Description

• Standardize import ordering using isort across test files
• Apply black code formatting with 120 character line length
• Fix indentation and spacing inconsistencies throughout codebase
• Break long lines and function calls for better readability


Changes walkthrough 📝

Relevant files
Formatting
12 files
common.py
Reorder imports and format code structure                               
+39/-12 
test_scale_chaos.py
Fix import order and dictionary formatting                             
+23/-15 
test_scale_up.py
Standardize imports and dictionary structure                         
+23/-15 
__init__.py
Reformat imports and fix indentation issues                           
+68/-65 
__init__.py
Fix import order and conditional formatting                           
+16/-8   
builder.py
Standardize imports and fix indentation                                   
+31/-27 
fetch_firefox_version.py
Reorder imports and add spacing                                                   
+5/-2     
fetch_version.py
Fix import order and function formatting                                 
+8/-2     
test.py
Format imports and improve line breaks                                     
+160/-80
get_started.py
Reorder imports and add function spacing                                 
+4/-1     
test.py
Fix indentation and function call formatting                         
+48/-48 
test_grid_ui.py
Standardize imports and add function spacing                         
+8/-5     

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Formatting Issue

    Line 447 has an extra blank line that creates inconsistent spacing. This appears to be a formatting artifact that should be removed for consistency.

    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 16, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add exception handling for cleanup

    Add exception handling to prevent cleanup failures from crashing the
    application. Container operations can fail if containers are already stopped or
    removed.

    tests/test.py [14-28]

     def clean_up():
         logger.info("Cleaning up...")
     
    -    test_container = client.containers.get(test_container_id)
    -    test_container.kill()
    -    test_container.remove()
    +    try:
    +        test_container = client.containers.get(test_container_id)
    +        test_container.kill()
    +        test_container.remove()
    +    except Exception as e:
    +        logger.warning(f"Failed to clean up test container: {e}")
     
         if standalone:
             logger.info("Standalone Cleaned up")
         else:
    -        # Kill the launched hub
    -        hub = client.containers.get(hub_id)
    -        hub.kill()
    -        hub.remove()
    -        logger.info("Hub / Node Cleaned up")
    +        try:
    +            # Kill the launched hub
    +            hub = client.containers.get(hub_id)
    +            hub.kill()
    +            hub.remove()
    +            logger.info("Hub / Node Cleaned up")
    +        except Exception as e:
    +            logger.warning(f"Failed to clean up hub container: {e}")
    • Apply / Chat
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly points out a potential robustness issue in the clean_up function. Adding try...except blocks for Docker operations is crucial to prevent the script from crashing if a container is already stopped or removed, making the cleanup process more resilient.

    Medium
    General
    Replace bare except with specific handling

    Using a bare except clause can mask important errors and make debugging
    difficult. Catch specific exceptions or at least log the error details.

    tests/test.py [56-59]

     try:
         client = docker.from_env()
    -except:
    +except Exception as e:
    +    logger.error(f"Failed to initialize Docker client: {e}")
         client = None

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies a bare except clause, which is a bad practice as it can hide unexpected errors. The proposed change to catch a specific Exception and log it improves debuggability and makes the code more robust.

    Medium
    Avoid mutating global lists directly

    Consider using a more explicit approach to handle None values in version lists.
    The current logic modifies global lists based on a configuration flag, which
    could lead to unexpected behavior if the lists are used elsewhere.

    tests/SeleniumTests/init.py [43-45]

     if not TEST_MULTIPLE_VERSIONS_EXPLICIT:
    -    LIST_CHROMIUM_VERSIONS.append(None)
    -    LIST_FIREFOX_VERSIONS.append(None)
    +    LIST_CHROMIUM_VERSIONS = LIST_CHROMIUM_VERSIONS + [None]
    +    LIST_FIREFOX_VERSIONS = LIST_FIREFOX_VERSIONS + [None]
    • Apply / Chat
    Suggestion importance[1-10]: 5

    __

    Why: The suggestion correctly identifies the mutation of global lists (LIST_CHROMIUM_VERSIONS and LIST_FIREFOX_VERSIONS). Replacing in-place modification with reassignment is a good practice that enhances code clarity and reduces potential side effects.

    Low
    • Update

    @VietND96 VietND96 merged commit c672b0d into SeleniumHQ:trunk Jun 17, 2025
    25 of 28 checks passed
    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