Throw Error When Using Unsupported Linux ARM
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
SeleniumManagerto 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 |
|
💡 PR-Agent usage: Comment
/help "your question"on any pull request to receive relevant information
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 Architecture Detection |
PR Code Suggestions ✨
Explore these optional code suggestions:
| Category | Suggestion | Score |
| Enhancement |
Improve architecture detection specificity for more accurate unsupported platform identificationConsider using a more specific check for ARM64 architecture instead of a general ARM java/src/org/openqa/selenium/manager/SeleniumManager.java [197-201]
Suggestion importance[1-10]: 8Why: 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 characterSuggestion Impact:The suggestion impacted the commit by removing the '=' sign from the error message, although the commit also changed "ARM64" to "ARM".code diff:
Remove the '=' sign from the error message as it appears to be a typo and doesn't java/src/org/openqa/selenium/manager/SeleniumManager.java [198]
Suggestion importance[1-10]: 7Why: 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 exceptionConsider logging a warning message before throwing the exception to provide more java/src/org/openqa/selenium/manager/SeleniumManager.java [197-201]
Suggestion importance[1-10]: 6Why: 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
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.
the weird git diff error in the workflow should be fixed now - for some reason the linux word got removed, I put it back
@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)
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?
Yes, the test is not a concern. Thank you @shbenzer!