Skip to content

[ci] Update script pinned_browsers for changes in Edge API product fetch#14865

Merged
VietND96 merged 1 commit intotrunkfrom
pinned-browsers
Dec 6, 2024
Merged

[ci] Update script pinned_browsers for changes in Edge API product fetch#14865
VietND96 merged 1 commit intotrunkfrom
pinned-browsers

Conversation

@VietND96
Copy link
Copy Markdown
Member

@VietND96 VietND96 commented Dec 6, 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

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 URLs and SHA256 checksums for various browser archives in common/repositories.bzl, including Firefox, Edge, and Chrome, to reflect new version releases.
  • Introduced new functions in scripts/pinned_browsers.py to handle JSON data in a case-insensitive manner, ensuring compatibility with changes in the Edge API response format.

Changes walkthrough 📝

Relevant files
Enhancement
repositories.bzl
Update browser version URLs and checksums                               

common/repositories.bzl

  • Updated URLs and SHA256 checksums for Firefox, Edge, and Chrome
    archives.
  • Incremented version numbers for browser releases.
    		•  +25/-25 
    pinned_browsers.py
    Add case-insensitive JSON handling for Edge API                   

    scripts/pinned_browsers.py

  • Added functions for case-insensitive JSON loading.
  • Modified Edge API data handling to be case-insensitive.
    		•  +28/-14 

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

    @VietND96
    [ci] Update script pinned_browsers for changes in Edge API product fetch
    dd1fc11 
    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Dec 6, 2024

    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

    Code Smell
    The case-insensitive JSON handling functions are implemented but not used consistently. The code still directly accesses 'productversion' after converting keys to lowercase.

    Error Handling
    No error handling for JSON parsing or HTTP requests. The code assumes the Edge API response will always be valid and contain the expected data structure.

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Dec 6, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add error handling for external API requests to prevent silent failures

    Add error handling for the HTTP request to the Edge API endpoint. If the request
    fails, the script will currently fail silently. Add a try-except block to handle
    potential connection errors or invalid responses.

    scripts/pinned_browsers.py [194-195]

    -r = http.request("GET", "https://edgeupdates.microsoft.com/api/products")
-all_data = case_insensitive_json_loads(r.data)
+try:
+    r = http.request("GET", "https://edgeupdates.microsoft.com/api/products")
+    if r.status != 200:
+        raise RuntimeError(f"Edge API request failed with status {r.status}")
+    all_data = case_insensitive_json_loads(r.data)
+except Exception as e:
+    raise RuntimeError(f"Failed to fetch Edge browser data: {e}")
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion adds crucial error handling for an external API call, which could prevent silent failures and make debugging easier. This is important for reliability and maintainability.

    		8
    Fix potential undefined variable issue by using consistent version tracking

    Validate that mac_version is defined before using it. Currently, mac_version is
    assigned but never used, and could be undefined if the matching conditions aren't
    met.

    scripts/pinned_browsers.py [207-212]

     if "MacOS" == release.get("platform"):
     for artifact in release["artifacts"]:
         if "pkg" == artifact["artifactname"]:
             mac = artifact["location"]
             mac_hash = artifact["hash"]
-            mac_version = release["productversion"]
+            version = release["productversion"]  # Use the common version variable
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion fixes a potential bug where mac_version is assigned but never used, while the common 'version' variable should be used instead for consistency throughout the code.

    		7

    💡 Need additional feedback ? start a PR chat

    @VietND96 VietND96 merged commit a3af929 into trunk Dec 6, 2024
    @VietND96 VietND96 deleted the pinned-browsers branch December 6, 2024 08:30
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Dec 7, 2024
    @VietND96 @sandeepsuryaprasad
    [ci] Update script pinned_browsers for changes in Edge API product fe…
    e44e7e2 
    …tch (SeleniumHQ#14865)

Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    @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

    No reviews

    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.

    1 participant

    @VietND96