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.

Sign up for GitHub

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

Jump to bottom

[py] disable strict time stamps in Firefox profile #15141

Merged
merged 5 commits into from
Jan 28, 2025
Merged

[py] disable strict time stamps in Firefox profile #15141

merged 5 commits into from
Jan 28, 2025

Conversation

Delta456
Copy link
Member

@Delta456 Delta456 commented Jan 23, 2025 •
edited by qodo-merge-pro 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

Fixes #15138

Motivation and Context

  • Allow zipping profiles with files and directories stamped earlier than 1980

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

  • Fixed install_addon method to handle timestamps before 1980.

  • Updated zipfile.ZipFile usage to disable strict timestamps.

  • Resolved issue with zipping Firefox profiles containing old timestamps.

Changes walkthrough 📝

Relevant files
Bug fix
webdriver.py
Disable strict timestamps in `install_addon` method           

py/selenium/webdriver/firefox/webdriver.py

  • Modified zipfile.ZipFile to include strict_timestamps=False.
  • Ensured compatibility with files having timestamps before 1980.
  • Adjusted install_addon method for improved error handling.
    		• +1/-1     
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    15138 - PR Code Verified

    Compliant requirements:

    • Fix Firefox addon installation error when files have timestamps before 1980
    • Change zipfile.ZipFile to use strict_timestamps=False parameter
    • Fix should be applied to install_addon() method in webdriver.py

    Requires further human verification:

    • Verify that Firefox addons with pre-1980 timestamps can now be installed successfully
    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ No major issues detected

    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add error handling for zip creation

    Add error handling around the zipfile creation to catch potential file access or
    permission errors that could occur during addon installation.

    py/selenium/webdriver/firefox/webdriver.py [134-138]

    -with zipfile.ZipFile(fp, "w", zipfile.ZIP_DEFLATED, strict_timestamps=False) as zipped:
-    for base, _, files in os.walk(path):
-        for fyle in files:
-            filename = os.path.join(base, fyle)
-            zipped.write(filename, filename[path_root:])
+try:
+    with zipfile.ZipFile(fp, "w", zipfile.ZIP_DEFLATED, strict_timestamps=False) as zipped:
+        for base, _, files in os.walk(path):
+            for fyle in files:
+                filename = os.path.join(base, fyle)
+                zipped.write(filename, filename[path_root:])
+except (OSError, zipfile.BadZipFile) as e:
+    raise WebDriverException(f"Failed to create addon zip file: {e}")
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding error handling for zip file operations is crucial for robustness, as file system operations can fail due to permissions or I/O issues. The suggestion properly catches relevant exceptions and provides meaningful error messages.

    		8

    VietND96
    VietND96 requested changes Jan 28, 2025
    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.

    I think we should allow users who want to strict the timestamp also.

        def install_addon(self, path, temporary=False, strict_timestamps=False) -> str:

    @VietND96
    Copy link
    Member

    The change is similar to #14168

    @VietND96 VietND96 merged commit c58a581 into SeleniumHQ:trunk Jan 28, 2025
    17 checks passed
    @Delta456 Delta456 deleted the firefox_zip branch January 28, 2025 08:00
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    @Delta456 @sandeepsuryaprasad
    [py] disable strict time stamps in Firefox profile (SeleniumHQ#15141)
    6ba0cc3
    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
    None yet
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.

    [🐛 Bug]: Firefox ZIP timestamps, part 2
    2 participants
    @Delta456 @VietND96