selenium icon indicating copy to clipboard operation
selenium copied to clipboard

Throw Error When Using Unsupported Linux ARM

Open shbenzer opened this issue 1 year ago • 3 comments

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

  • [x] 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

  • [x] 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

    shbenzer avatar Oct 17 '24 21:10 shbenzer

    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.

    qodo-code-review[bot] avatar Oct 17 '24 21:10 qodo-code-review[bot]

    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 <!-- /improve --apply_suggestion=0 -->
    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 <!-- /improve --apply_suggestion=1 -->
    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 <!-- /improve --apply_suggestion=2 -->
    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

    qodo-code-review[bot] avatar Oct 17 '24 21:10 qodo-code-review[bot]

    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 avatar Oct 17 '24 21:10 shbenzer

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

    shbenzer avatar Oct 23 '24 14:10 shbenzer

    @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 avatar Nov 06 '24 15:11 shbenzer

    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?

    shbenzer avatar Nov 06 '24 18:11 shbenzer

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

    pujagani avatar Nov 07 '24 03:11 pujagani