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

MustafaAgamy · 2024-12-26T22:50:42Z

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

qodo-merge-pro · 2024-12-26T22:51:22Z

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/");

qodo-merge-pro · 2024-12-26T22:51:54Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	✅ Use locale-independent number formatting to handle port numbers consistently across all system languages Suggestion Impact: The commit removes the locale check and error message, which aligns with the suggestion's goal of handling port numbers consistently across locales code diff: - 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/"); - } 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
Possible issue	✅ Ensure consistent locale-independent number formatting across all port number handling Suggestion Impact: The commit removes the locale-dependent formatting check, which is part of addressing the locale formatting issue, though it doesn't implement the suggested String.valueOf() solution code diff: 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/"); - } 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 Suggestion Impact: The commit addresses the same issue but takes a different approach - instead of showing the actual locale, it fixes the incorrect assumption by only throwing the error when the locale is actually Arabic code diff: - 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/"); - } - + if(Locale.getDefault(Locale.Category.FORMAT).getLanguage().equals("ar")) { + throw new NumberFormatException(String.format("Couldn't format the port numbers because the System Language is arabic: \"--port=%d\"," + + " 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/", getPort())); + } 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 · 2024-12-27T04:08:12Z

@pujagani FYI

Delta456 · 2024-12-27T12:39:32Z

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

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

MustafaAgamy · 2025-01-12T10:51:18Z

@pujagani
@diemol
Bump

diemol

Can't this be placed in the super class?

Tests added for: InternetExplorerDriverTests SafariDriverTests

MustafaAgamy · 2025-01-12T22:33:45Z

Can't this be placed in the super class?

@diemol Done, please check

Also added Tests for Safari and InternetExplorer alongside the added tests for Firefox, Chrome and Edge

pujagani

LGTM. Thank you @MustafaAgamy!

MustafaAgamy · 2025-01-13T06:28:35Z

LGTM. Thank you @MustafaAgamy!

@pujagani
I fixed the formatting issues related to the class files,

The test failures aren't related to the fix

Thank you

MustafaAgamy · 2025-01-13T07:48:10Z

@pujagani

Sorry one more time, I forgot another line fix my bad

MustafaAgamy · 2025-01-13T08:49:56Z

@pujagani
can you please re-reun the PR tests?

And I see tests failure related to something else, not sure what's the root cause

thank you

.idea/misc.xml

.idea/kotlinc.xml

MustafaAgamy · 2025-01-13T10:11:58Z

@diemol @pujagani done changes reverted

Please check

MustafaAgamy · 2025-01-13T13:59:25Z

@diemol Bump

MustafaAgamy · 2025-01-15T09:57:43Z

@pujagani
@diemol

Test failures not related to the change so everything is good, I guess

pujagani · 2025-01-17T05:42:16Z

Thank you! @MustafaAgamy

…ted for Firefox, Edge and Chrome Drivers (SeleniumHQ#14946)

MustafaAgamy added 2 commits December 26, 2024 22:26

commit

31c5d61

Merge branch 'trunk' into trunk

0d42b9f

qodo-merge-pro bot added the Review effort [1-5]: 2 label Dec 26, 2024

Revisit the logic to compare it against Arabic langauge only

2e4634b

Delta456 reviewed Dec 27, 2024

View reviewed changes

java/src/org/openqa/selenium/chrome/ChromeDriverService.java Outdated Show resolved Hide resolved

MustafaAgamy added 2 commits December 29, 2024 09:25

Merge branch 'SeleniumHQ:trunk' into trunk

38396bb

revisiting the formattnig of the port

6eb62d3

diemol reviewed Jan 12, 2025

View reviewed changes

MustafaAgamy added 7 commits January 12, 2025 18:41

Merge branch 'trunk' into trunk

6fa20f5

Merge branch 'SeleniumHQ:trunk' into trunk

2269afb

Added the Login to DriverService class

53cf3ff

Tests added for: InternetExplorerDriverTests SafariDriverTests

Merge branch 'SeleniumHQ:trunk' into trunk

0a28657

fix spaces

9f546ae

fix spacing in service classes

85a4243

Merge branch 'trunk' into trunk

8de6ddf

pujagani approved these changes Jan 13, 2025

View reviewed changes

fix formatting

15fa449

fix comma in driverservice

8a839ea

MustafaAgamy added 2 commits January 13, 2025 07:48

line break

eda3947

fix formatting in FirefoxDriverTests

e41b87b

remove spacing

a47a8f5

diemol requested changes Jan 13, 2025

View reviewed changes

.idea/misc.xml Outdated Show resolved Hide resolved

.idea/kotlinc.xml Outdated Show resolved Hide resolved

MustafaAgamy added 3 commits January 13, 2025 12:59

Merge branch 'trunk' into trunk

7b428ad

fix changes

454e6d5

revert

b70dae0

MustafaAgamy added 3 commits January 13, 2025 14:10

Merge branch 'trunk' into trunk

4ed3807

fix driverService

2864e29

Merge branch 'trunk' into trunk

00a52e9

Merge branch 'trunk' into trunk

0a3916d

pujagani merged commit fad6fbe into SeleniumHQ:trunk Jan 17, 2025
33 of 34 checks passed

VietND96 mentioned this pull request Jan 19, 2025

[java]: Fix the message from NumberFormatException #15116

Merged

8 tasks

sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025

[java] Add NumberFormatException when the port isn't correctly format…

71cdebb

…ted for Firefox, Edge and Chrome Drivers (SeleniumHQ#14946)

gryznar pushed a commit to gryznar/selenium that referenced this pull request May 17, 2025

[java] Add NumberFormatException when the port isn't correctly format…

d959d14

…ted for Firefox, Edge and Chrome Drivers (SeleniumHQ#14946)

Uh oh!

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

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

Uh oh!

Conversation

MustafaAgamy commented Dec 26, 2024 • edited by qodo-merge-pro bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

Description

Motivation and Context

Types of changes

Checklist

PR Type

Description

Changes walkthrough 📝

Uh oh!

qodo-merge-pro bot commented Dec 26, 2024

PR Reviewer Guide 🔍

Uh oh!

qodo-merge-pro bot commented Dec 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Uh oh!

MustafaAgamy commented Dec 27, 2024

Uh oh!

Delta456 commented Dec 27, 2024

Uh oh!

Uh oh!

MustafaAgamy commented Jan 12, 2025

Uh oh!

diemol left a comment

Choose a reason for hiding this comment

Uh oh!

MustafaAgamy commented Jan 12, 2025

Uh oh!

pujagani left a comment

Choose a reason for hiding this comment

Uh oh!

MustafaAgamy commented Jan 13, 2025

Uh oh!

MustafaAgamy commented Jan 13, 2025

Uh oh!

MustafaAgamy commented Jan 13, 2025

Uh oh!

Uh oh!

Uh oh!

MustafaAgamy commented Jan 13, 2025

Uh oh!

MustafaAgamy commented Jan 13, 2025

Uh oh!

MustafaAgamy commented Jan 15, 2025

Uh oh!

Uh oh!

pujagani commented Jan 17, 2025

Uh oh!

Uh oh!

MustafaAgamy commented Dec 26, 2024 •

edited by qodo-merge-pro bot

Loading

qodo-merge-pro bot commented Dec 26, 2024 •

edited

Loading