Skip to content

[py]: replace dead battery imghdr with filetype#14771

Merged
VietND96 merged 3 commits intoSeleniumHQ:trunkfrom
navin772:update-dep-imghdr
Nov 21, 2024
Merged

[py]: replace dead battery imghdr with filetype#14771
VietND96 merged 3 commits intoSeleniumHQ:trunkfrom
navin772:update-dep-imghdr

Conversation

@navin772
Copy link
Member

@navin772 navin772 commented Nov 18, 2024 •
edited by qodo-code-review bot
Loading

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

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Nov 18, 2024

    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.

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Nov 18, 2024

    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

    VietND96 commented Nov 18, 2024

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

    @navin772
    Copy link
    Member Author

    navin772 commented Nov 19, 2024

    @VietND96 ran the formatter

    VietND96
    VietND96 approved these changes Nov 19, 2024
    View reviewed changes
    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 Python Bindings label Nov 19, 2024
    @VietND96 VietND96 merged commit b1828bf into SeleniumHQ:trunk Nov 21, 2024
    @navin772 navin772 deleted the update-dep-imghdr branch November 21, 2024 13:17
    @navin772 navin772 mentioned this pull request Oct 16, 2025
    Merged
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @VietND96 VietND96 VietND96 approved these changes

    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.

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

    2 participants

    @navin772 @VietND96