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

Throw Error When Using Unsupported Linux ARM #14616

Merged
merged 17 commits into from
Nov 8, 2024

Conversation

shbenzer
Copy link
Contributor

@shbenzer shbenzer commented Oct 17, 2024

User description

Added code for Issue #13793 in Selenium Manager

Description

Selenium Manager Java should now throw an error if using unsupported Linux ARM architecture

Motivation and Context

Issue #13793

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

  • Added error handling in SeleniumManager to throw an exception when an unsupported Linux ARM64 architecture is detected.
  • Ensures that users are informed about the lack of support for Linux ARM64 in Selenium Manager.

Changes walkthrough 📝

Relevant files
Error handling
SeleniumManager.java
Add error handling for unsupported Linux ARM architecture

java/src/org/openqa/selenium/manager/SeleniumManager.java

  • Added a check for Linux ARM architecture.
  • Throws a WebDriverException if ARM64 architecture is detected.
  • Ensures compatibility warning for unsupported architectures.
  • +4/-0     

    💡 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 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Message Clarity
    The error message for unsupported Linux ARM architecture could be more specific and informative. Consider providing more details about supported architectures or alternative solutions.

    Architecture Detection
    The current check for ARM architecture using System.getProperty("os.arch").contains("arm") might be too broad. Consider using a more precise method to detect specifically ARM64 architecture.

    Copy link
    Contributor

    codiumai-pr-agent-pro bot commented Oct 17, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Improve architecture detection specificity for more accurate unsupported platform identification

    Consider using a more specific check for ARM64 architecture instead of a general ARM
    check. This allows for potential future support of other ARM variants.

    java/src/org/openqa/selenium/manager/SeleniumManager.java [197-201]

    -if (System.getProperty("os.arch").contains("arm")) {
    -  throw new WebDriverException("Linux ARM64= is not supported by Selenium Manager");
    +if (System.getProperty("os.arch").equals("aarch64")) {
    +  throw new WebDriverException("Linux ARM64 is not supported by Selenium Manager");
     } else {
       folder = "linux";
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion improves the specificity of the architecture check by targeting "aarch64" directly, which is more precise for detecting ARM64 architecture. This change enhances future compatibility and reduces the risk of false positives for other ARM variants.

    8
    Best practice
    ✅ Correct the error message by removing an unnecessary character
    Suggestion Impact:The suggestion impacted the commit by removing the '=' sign from the error message, although the commit also changed "ARM64" to "ARM".

    code diff:

    -            throw new WebDriverException("Linux ARM64= is not supported by Selenium Manager");
    +            throw new WebDriverException("Linux ARM is not supported by Selenium Manager");

    Remove the '=' sign from the error message as it appears to be a typo and doesn't
    add any meaningful information.

    java/src/org/openqa/selenium/manager/SeleniumManager.java [198]

    -throw new WebDriverException("Linux ARM64= is not supported by Selenium Manager");
    +throw new WebDriverException("Linux ARM64 is not supported by Selenium Manager");
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Removing the '=' sign from the error message corrects a likely typo, improving the clarity and professionalism of the message without altering functionality.

    7
    Enhance error handling by adding a warning log before throwing an exception

    Consider logging a warning message before throwing the exception to provide more
    context about the unsupported architecture.

    java/src/org/openqa/selenium/manager/SeleniumManager.java [197-201]

     if (System.getProperty("os.arch").contains("arm")) {
    -  throw new WebDriverException("Linux ARM64= is not supported by Selenium Manager");
    +  LOG.warning("Detected unsupported Linux ARM architecture");
    +  throw new WebDriverException("Linux ARM64 is not supported by Selenium Manager");
     } else {
       folder = "linux";
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Adding a warning log provides additional context before an exception is thrown, which can be useful for debugging and understanding the cause of the error. However, it is a minor enhancement since the exception message already conveys the issue.

    6

    💡 Need additional feedback ? start a PR chat

    @shbenzer
    Copy link
    Contributor Author

    shbenzer commented Oct 17, 2024

    I think this should be sufficient - but let me know if I misunderstood the decision y'all made in Issue #13793 . Used System.getProperty() instead of current.Is() since ARM is part of the system architecture.

    @shbenzer
    Copy link
    Contributor Author

    shbenzer commented Oct 23, 2024

    the weird git diff error in the workflow should be fixed now - for some reason the linux word got removed, I put it back

    Copy link

    @A1exKH A1exKH left a comment

    Choose a reason for hiding this comment

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

    LGTM.

    @shbenzer
    Copy link
    Contributor Author

    shbenzer commented Nov 6, 2024

    @pujagani Failing test is unrelated:

    java.lang.Exception: org.openqa.selenium.JavascriptEnabledDriverTest.testShouldBeAbleToFindElementAfterJavascriptCausesANewPageToLoad is not yet expected to work on remote builds using CHROME, but it already works!
    at org.openqa.selenium.testing.SeleniumExtension.afterEach(SeleniumExtension.java:160)
    at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
    at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

    @shbenzer
    Copy link
    Contributor Author

    shbenzer commented Nov 6, 2024

    looks like the same test is failing @pujagani

    java.lang.Exception: org.openqa.selenium.JavascriptEnabledDriverTest.testShouldBeAbleToFindElementAfterJavascriptCausesANewPageToLoad is not yet expected to work on remote builds using CHROME, but it already works!

    Seems like a good thing though?

    @pujagani
    Copy link
    Contributor

    pujagani commented Nov 7, 2024

    Yes, the test is not a concern. Thank you @shbenzer!

    @VietND96 VietND96 merged commit 0a9d689 into SeleniumHQ:trunk Nov 8, 2024
    31 checks passed
    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.

    5 participants