Skip to content

[ci] Fix pinned browsers fetch different msedgedriver version per OS#14683

Merged
diemol merged 1 commit intotrunkfrom
fix-pinned-browsers
Oct 30, 2024
Merged

[ci] Fix pinned browsers fetch different msedgedriver version per OS#14683
diemol merged 1 commit intotrunkfrom
fix-pinned-browsers

Conversation

@VietND96
Copy link
Copy Markdown
Member

@VietND96 VietND96 commented Oct 30, 2024 •
edited
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

Get stable version, then get major version number, and using API to fetch different versions might have per OS - following MicrosoftEdge/EdgeWebDriver#39 (comment)

Motivation and Context

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

enhancement

Description

  • Updated the EdgeDriver URLs and SHA256 checksums in common/repositories.bzl to reflect the latest versions for Linux and MacOS.
  • Enhanced the scripts/pinned_browsers.py to fetch the major version of the stable EdgeDriver and use it to determine the specific version URLs for Linux and MacOS.
  • Calculated and updated the SHA256 checksums for the new EdgeDriver URLs.

Changes walkthrough 📝

Relevant files
Enhancement
repositories.bzl
Update EdgeDriver URLs and SHA256 checksums                           

common/repositories.bzl

  • Updated the URL and SHA256 for the Linux EdgeDriver.
  • Updated the URL and SHA256 for the Mac EdgeDriver.
    		•  +4/-4     
    pinned_browsers.py
    Fetch and use specific EdgeDriver versions per OS               

    scripts/pinned_browsers.py

  • Added logic to fetch the major version of the stable EdgeDriver.
  • Updated URLs to fetch specific EdgeDriver versions for Linux and
    MacOS.
  • Calculated SHA256 for the new URLs.
    		•  +9/-4     

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

    @VietND96
    [ci] Fix pinned browsers fetch different msedgedriver version per OS
    6635084 
    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Oct 30, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ❌

    39 - Not compliant

    Not compliant requirements:

    • Let WindowsUtils.killPID() kill the whole process tree
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Potential Error Handling
    The code doesn't handle potential exceptions when making HTTP requests or decoding responses. Consider adding try-except blocks to handle possible network errors or decoding issues.

    Code Duplication
    There's repeated code for fetching and decoding versions for Linux and MacOS. Consider refactoring this into a separate function to improve maintainability.

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Oct 30, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Add error handling for HTTP requests and version parsing

    Consider adding error handling for the HTTP requests and version parsing to make the
    code more robust against network issues or unexpected responses.

    scripts/pinned_browsers.py [264-268]

    -r_stable = http.request("GET", "https://msedgedriver.azureedge.net/LATEST_STABLE")
-stable_version = r_stable.data.decode("utf-16").strip()
-major_version = stable_version.split('.')[0]
-r = http.request("GET", f"https://msedgedriver.azureedge.net/LATEST_RELEASE_{major_version}_LINUX")
-linux_version = r.data.decode("utf-16").strip()
+try:
+    r_stable = http.request("GET", "https://msedgedriver.azureedge.net/LATEST_STABLE")
+    stable_version = r_stable.data.decode("utf-16").strip()
+    major_version = stable_version.split('.')[0]
+    r = http.request("GET", f"https://msedgedriver.azureedge.net/LATEST_RELEASE_{major_version}_LINUX")
+    linux_version = r.data.decode("utf-16").strip()
+except Exception as e:
+    raise RuntimeError(f"Failed to fetch EdgeDriver versions: {e}")
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding error handling for HTTP requests and version parsing is crucial for robustness, as it helps the code handle network issues or unexpected responses gracefully, preventing crashes and providing meaningful error messages.

    		8
    Best practice
    Use context managers for HTTP requests to ensure proper resource management

    Consider using a context manager with the http.request() calls to ensure proper
    resource management and connection closure.

    scripts/pinned_browsers.py [264-268]

    -r_stable = http.request("GET", "https://msedgedriver.azureedge.net/LATEST_STABLE")
-stable_version = r_stable.data.decode("utf-16").strip()
+with http.request("GET", "https://msedgedriver.azureedge.net/LATEST_STABLE") as r_stable:
+    stable_version = r_stable.data.decode("utf-16").strip()
 major_version = stable_version.split('.')[0]
-r = http.request("GET", f"https://msedgedriver.azureedge.net/LATEST_RELEASE_{major_version}_LINUX")
-linux_version = r.data.decode("utf-16").strip()
+with http.request("GET", f"https://msedgedriver.azureedge.net/LATEST_RELEASE_{major_version}_LINUX") as r:
+    linux_version = r.data.decode("utf-16").strip()
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using context managers for HTTP requests is a good practice as it ensures proper resource management and connection closure, which can prevent potential resource leaks and improve code reliability.

    		7
    Use f-strings consistently for string formatting

    Consider using f-strings consistently throughout the code for string formatting, as
    they are more readable and less error-prone.

    scripts/pinned_browsers.py [272-299]

    -linux = "https://msedgedriver.azureedge.net/%s/edgedriver_linux64.zip" % linux_version
+linux = f"https://msedgedriver.azureedge.net/{linux_version}/edgedriver_linux64.zip"
 ...
-mac = "https://msedgedriver.azureedge.net/%s/edgedriver_mac64.zip" % macos_version
+mac = f"https://msedgedriver.azureedge.net/{macos_version}/edgedriver_mac64.zip"
    Suggestion importance[1-10]: 5

    Why: Using f-strings consistently enhances code readability and reduces the likelihood of errors in string formatting, aligning with modern Python practices, though it is a minor stylistic improvement.

    		5
    Maintainability
    Extract version fetching logic into a separate function for better code organization

    Consider extracting the URL construction and version fetching logic into a separate
    function to improve code readability and reusability.

    scripts/pinned_browsers.py [264-299]

    -r_stable = http.request("GET", "https://msedgedriver.azureedge.net/LATEST_STABLE")
-stable_version = r_stable.data.decode("utf-16").strip()
-major_version = stable_version.split('.')[0]
-r = http.request("GET", f"https://msedgedriver.azureedge.net/LATEST_RELEASE_{major_version}_LINUX")
-linux_version = r.data.decode("utf-16").strip()
+def get_edgedriver_version(platform):
+    r_stable = http.request("GET", "https://msedgedriver.azureedge.net/LATEST_STABLE")
+    stable_version = r_stable.data.decode("utf-16").strip()
+    major_version = stable_version.split('.')[0]
+    r = http.request("GET", f"https://msedgedriver.azureedge.net/LATEST_RELEASE_{major_version}_{platform.upper()}")
+    return r.data.decode("utf-16").strip()
 
+linux_version = get_edgedriver_version("linux")
 ...
+macos_version = get_edgedriver_version("macos")
 
-r = http.request("GET", f"https://msedgedriver.azureedge.net/LATEST_RELEASE_{major_version}_MACOS")
-macos_version = r.data.decode("utf-16").strip()
-
    Suggestion importance[1-10]: 6

    Why: Extracting the version fetching logic into a separate function improves code readability and reusability, making it easier to maintain and understand, although it does not address any critical issues.

    		6

    💡 Need additional feedback ? start a PR chat

    @diemol diemol merged commit 8ccf021 into trunk Oct 30, 2024
    @diemol diemol deleted the fix-pinned-browsers branch October 30, 2024 09:12
    @dependabot dependabot Bot mentioned this pull request Sep 18, 2025
    Merged
    @dependabot dependabot Bot mentioned this pull request Oct 1, 2025
    Closed
    @dependabot dependabot Bot mentioned this pull request Oct 24, 2025
    Closed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @diemol diemol diemol approved these changes

    Assignees

    No one assigned

    Labels

    Review effort [1-5]: 2

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @VietND96 @diemol