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]: replace dead battery imghdr with filetype #14771

Merged
merged 3 commits into from
Nov 21, 2024

Conversation

navin772
Copy link
Contributor

@navin772 navin772 commented Nov 18, 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

Closes #14765

imghdr module will be a dead battery starting with Python 3.13, replaced the tests to use filetype instead

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


Description

  • Replaced the deprecated imghdr module with filetype for image type checking in various screenshot tests.
  • Updated test assertions to validate MIME types using filetype.guess.
  • Added filetype as a dependency in requirements.txt and Bazel build configuration.
  • Updated cryptography and debugpy versions in the lock file and removed unused dependencies.

Changes walkthrough 📝

Relevant files
Enhancement
takes_screenshots_tests.py
Replace `imghdr` with `filetype` in screenshot tests         

py/test/selenium/webdriver/common/takes_screenshots_tests.py

  • Replaced imghdr with filetype for image type checking.
  • Updated assertions to use filetype.guess for MIME type validation.
  • +7/-4     
    ff_takes_full_page_screenshots_tests.py
    Update full page screenshot tests to use `filetype`           

    py/test/selenium/webdriver/firefox/ff_takes_full_page_screenshots_tests.py

  • Replaced imghdr with filetype for full page screenshot tests.
  • Updated assertions to use filetype.guess for MIME type validation.
  • +5/-3     
    remote_connection_tests.py
    Use `filetype` for image type checking in remote tests     

    py/test/selenium/webdriver/remote/remote_connection_tests.py

  • Replaced imghdr with filetype for remote connection tests.
  • Updated assertions to use filetype.guess for MIME type validation.
  • +3/-2     
    Dependencies
    BUILD.bazel
    Add `filetype` to Bazel test dependencies                               

    py/BUILD.bazel

    • Added filetype as a new test dependency.
    +1/-0     
    requirements.txt
    Add `filetype` to Python requirements                                       

    py/requirements.txt

    • Added filetype==1.2.0 to the requirements.
    +1/-0     
    requirements_lock.txt
    Update locked dependencies and add `filetype`                       

    py/requirements_lock.txt

  • Updated cryptography and debugpy versions.
  • Added filetype with specific hashes.
  • Removed jeepney and secretstorage.
  • +64/-67 

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

    Copy link
    Contributor

    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

    Code Duplication
    Similar validation code for checking PNG file type is duplicated across multiple test functions. Consider extracting the validation into a helper function.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Add defensive error handling around file type detection to provide clearer test failures

    Add error handling around filetype.guess() calls since they could return None for
    corrupted images or non-image data. This would make the tests more robust.

    py/test/selenium/webdriver/common/takes_screenshots_tests.py [29-30]

     kind = filetype.guess(result)
    -assert kind is not None and kind.mime == "image/png"
    +if kind is None:
    +    raise AssertionError("Failed to determine file type - image data may be corrupted")
    +assert kind.mime == "image/png", f"Expected PNG image but got {kind.mime}"
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves error handling and test diagnostics by providing more specific error messages when image validation fails, making it easier to debug test failures.

    7

    💡 Need additional feedback ? start a PR chat

    @VietND96
    Copy link
    Member

    As CI result, need to run ./scripts/format.sh once

    @navin772
    Copy link
    Contributor Author

    @VietND96 ran the formatter

    Copy link
    Member

    @VietND96 VietND96 left a comment

    Choose a reason for hiding this comment

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

    LGTM!
    @AutomatedTester PTAL.

    @VietND96 VietND96 added the C-py label Nov 19, 2024
    @VietND96 VietND96 merged commit b1828bf into SeleniumHQ:trunk Nov 21, 2024
    17 checks passed
    @navin772 navin772 deleted the update-dep-imghdr branch November 21, 2024 13:17
    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.

    [🐛 Bug]: Some tests in py/test/selenium/webdriver using 'dead battery' library imghdr
    2 participants