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

Add NumberFormatException when the port isn't correctly formatted for Firefox, Edge and Chrome Drivers #14946

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from

Conversation

MustafaAgamy
Copy link
Contributor

@MustafaAgamy MustafaAgamy commented Dec 26, 2024

User description

Description

Add "NumberFormatException" when the port isn't correctly formatted for Firefox, Edge and Chrome Drivers at:
**- GeckoDriverService

  • EdgeDriverService
  • ChromeDriverService**

Motivation and Context

Due to Operating System langauge set in Arabic, middle east users were getting "SessionNotCreatedException" which wasn't detailed enough, now the users will get a detailed "NumberFormatException" with the recommended solution and a link to the docs for more info
*Please note that I already had tests created for previous PR, now I just adjusted those Tests to work better with the given scenario (Expected to Throw NumberFormatException)

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, Enhancement


Description

  • Added proper error handling for non-English system locales in Chrome, Edge, and Firefox WebDrivers
  • Throws NumberFormatException with detailed message when system locale is Arabic
  • Added validation check for port number formatting in all three driver services
  • Updated test cases to verify the new NumberFormatException behavior
  • Includes helpful error message with instructions to set JVM language arguments
  • Added documentation link in error message for more information

Changes walkthrough 📝

Relevant files
Error handling
ChromeDriverService.java
Add locale validation for Chrome driver port formatting   

java/src/org/openqa/selenium/chrome/ChromeDriverService.java

  • Added check for non-English system locale when formatting port numbers
  • Throws NumberFormatException with detailed message for Arabic locale
  • +5/-1     
    EdgeDriverService.java
    Add locale validation for Edge driver port formatting       

    java/src/org/openqa/selenium/edge/EdgeDriverService.java

  • Added check for non-English system locale when formatting port numbers
  • Throws NumberFormatException with detailed message for Arabic locale
  • +5/-1     
    GeckoDriverService.java
    Add locale validation for Firefox driver port formatting 

    java/src/org/openqa/selenium/firefox/GeckoDriverService.java

  • Added import for java.util.Locale
  • Added check for non-English system locale when formatting port numbers
  • Throws NumberFormatException with detailed message for Arabic locale
  • +5/-0     
    Tests
    ChromeDriverFunctionalTest.java
    Update Chrome driver tests for locale validation                 

    java/test/org/openqa/selenium/chrome/ChromeDriverFunctionalTest.java

  • Renamed test method to shouldThrowNumberFormatException
  • Updated test to verify NumberFormatException is thrown for Arabic
    locale
  • Removed successful launch test case
  • +7/-6     
    EdgeDriverFunctionalTest.java
    Update Edge driver tests for locale validation                     

    java/test/org/openqa/selenium/edge/EdgeDriverFunctionalTest.java

  • Renamed test method to shouldThrowNumberFormatException
  • Updated test to verify NumberFormatException is thrown for Arabic
    locale
  • Removed successful launch test case
  • +6/-6     
    FirefoxDriverTest.java
    Update Firefox driver tests for locale validation               

    java/test/org/openqa/selenium/firefox/FirefoxDriverTest.java

  • Renamed test method to shouldThrowNumberFormatException
  • Updated test to verify NumberFormatException is thrown for Arabic
    locale
  • Removed successful launch test case
  • +7/-6     

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

    Hardcoded Language Check

    The code assumes English is the only valid language for port formatting. Consider supporting other languages or having a more robust port number validation approach.

    if(!Locale.getDefault(Locale.Category.FORMAT).getLanguage().equals("en")) {
      throw new NumberFormatException("Couldn't format the port numbers because the System Language is arabic: \"" + String.format("--port=%d", getPort()) +
      "\", please make sure to add the required arguments \"-Duser.language=en -Duser.region=US\" to your JVM, for more info please visit :" + "\n  https://www.selenium.dev/documentation/webdriver/browsers/");
    }
    Error Message Accuracy

    The error message specifically mentions Arabic language but the check is for any non-English locale. This could be confusing for users with other non-English locales.

    throw new NumberFormatException("Couldn't format the port numbers because the System Language is arabic: \"" + String.format("--port=%d", getPort()) +
    "\", please make sure to add the required arguments \"-Duser.language=en -Duser.region=US\" to your JVM, for more info please visit :" + "\n  https://www.selenium.dev/documentation/webdriver/browsers/");

    Copy link
    Contributor

    qodo-merge-pro bot commented Dec 26, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Use locale-independent number formatting to handle port numbers consistently across all system languages

    Instead of checking only for non-English locale, implement proper number formatting
    using a Locale-independent approach. Use String.valueOf() or Integer.toString() for
    port number conversion to avoid locale-specific formatting issues.

    java/src/org/openqa/selenium/chrome/ChromeDriverService.java [287-291]

    -args.add(String.format("--port=%d", getPort()));
    -if(!Locale.getDefault(Locale.Category.FORMAT).getLanguage().equals("en")) {
    -  throw new NumberFormatException("Couldn't format the port numbers because the System Language is arabic: \"" + String.format("--port=%d", getPort()) +
    -  "\", please make sure to add the required arguments \"-Duser.language=en -Duser.region=US\" to your JVM, for more info please visit :" + "\n  https://www.selenium.dev/documentation/webdriver/browsers/");
    -}
    +args.add("--port=" + String.valueOf(getPort()));
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion addresses a critical issue by proposing a more robust solution using locale-independent number formatting, which would prevent formatting issues across all locales without requiring system language changes.

    9
    Ensure consistent locale-independent number formatting across all port number handling

    The websocket port formatting is inconsistent with the main port handling and could
    face the same locale issues. Apply the same locale-independent formatting to the
    websocket port.

    java/src/org/openqa/selenium/firefox/GeckoDriverService.java [223-230]

    -args.add(String.format("--port=%d", getPort()));
    -if(!Locale.getDefault(Locale.Category.FORMAT).getLanguage().equals("en")) {
    -  throw new NumberFormatException("Couldn't format the port numbers because the System Language is arabic: \"" + String.format("--port=%d", getPort()) +
    -  "\", please make sure to add the required arguments \"-Duser.language=en -Duser.region=US\" to your JVM, for more info please visit :" + "\n  https://www.selenium.dev/documentation/webdriver/browsers/");
    -}
    +args.add("--port=" + String.valueOf(getPort()));
    +int wsPort = PortProber.findFreePort();
    +args.add("--websocket-port=" + String.valueOf(wsPort));
     
    -int wsPort = PortProber.findFreePort();
    -args.add(String.format("--websocket-port=%d", wsPort));
    -
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion identifies an important consistency issue where websocket port formatting could face the same locale-related problems as the main port, making the fix more comprehensive.

    8
    General
    Provide accurate error messages that reflect the actual system locale instead of assuming Arabic

    The error message incorrectly assumes the system language is Arabic when it's any
    non-English locale. Update the message to be more accurate and include the actual
    locale in the error message.

    java/src/org/openqa/selenium/chrome/ChromeDriverService.java [289]

    -throw new NumberFormatException("Couldn't format the port numbers because the System Language is arabic: \"" + String.format("--port=%d", getPort()) +
    +throw new NumberFormatException("Couldn't format the port numbers due to non-English system locale '" + Locale.getDefault(Locale.Category.FORMAT) + "': \"" + String.format("--port=%d", getPort()) +
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves error message accuracy by including the actual system locale rather than assuming Arabic, making debugging easier and preventing confusion for users with non-Arabic locales.

    7

    @MustafaAgamy
    Copy link
    Contributor Author

    @pujagani FYI

    @Delta456
    Copy link
    Contributor

    I wonder if can use this string as a const and use that everywhere. I feel that would be a good idea.

    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.

    2 participants