[Java] Add Locale.ROOT to avoid port formatting issues for all drivers #15121

MustafaAgamy · 2025-01-20T20:07:22Z

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Added the Locale.ROOT for port formatting to avoid any issues when starting the driver session

Motivation and Context

To Avoid port formatting issues against different languages

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

Description

Added Locale.ROOT to ensure consistent port formatting across all driver services.
Updated getUrl method in DriverService to use Locale.ROOT.
Refactored tests to validate successful driver launches with Arabic locale.
Removed outdated exception handling for Arabic locale in driver services.

Changes walkthrough 📝

Relevant files

Bug fix

5 files

ChromeDriverService.java Use `Locale.ROOT` for port formatting in ChromeDriverService	+1/-1
EdgeDriverService.java Use `Locale.ROOT` for port formatting in EdgeDriverService	+1/-1
GeckoDriverService.java Use `Locale.ROOT` for port formatting in GeckoDriverService	+2/-1
InternetExplorerDriverService.java `Use Locale.ROOT for port formatting in InternetExplorerDriverService`	+2/-1
DriverService.java `Apply Locale.ROOT to getUrl method and remove Arabic locale exception`	+1/-12

Tests

5 files

ChromeDriverFunctionalTest.java `Refactor test to validate Arabic locale compatibility for ChromeDriver`	+2/-10
EdgeDriverFunctionalTest.java `Refactor test to validate Arabic locale compatibility for EdgeDriver`	+2/-10
FirefoxDriverTest.java `Refactor test to validate Arabic locale compatibility for` `FirefoxDriver`	+2/-10
InternetExplorerDriverTest.java `Refactor test to validate Arabic locale compatibility for IEDriver`	+2/-10
SafariDriverTest.java `Refactor test to validate Arabic locale compatibility for SafariDriver`	+2/-10

Need help?
Type /help how to ... in the comments thread for any question about Qodo Merge usage.
Check out the documentation for more information.

- ChromeDriverService - EdgeDriverService - GeckoDriverService - InternetExplorerDriverService and to the getUrl method at: - DriverService Refactor the Arabic Tests in the following accordingly: - ChromeDriverFunctionalTest - EdgeDriverFunctionalTest - FirefoxDriverTest - InternetExplorerDriverTest - SafariDriverTest

qodo-merge-pro · 2025-01-20T20:07:52Z

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 Inconsistent Locale The websocket port formatting is not using Locale.ROOT like the main port formatting, which could potentially cause similar locale-related issues int wsPort = PortProber.findFreePort(); args.add(String.format("--websocket-port=%d", wsPort));

qodo-merge-pro · 2025-01-20T20:08:12Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Verify locale-independent port formatting The test should verify that the port number is correctly formatted with Arabic locale by checking the actual command line arguments passed to the driver service. java/test/org/openqa/selenium/chrome/ChromeDriverFunctionalTest.java [207-217] void shouldLaunchSuccessfullyWithArabicDate() { Locale arabicLocale = new Locale("ar", "EG"); Locale.setDefault(arabicLocale); int port = PortProber.findFreePort(); ChromeDriverService.Builder builder = new ChromeDriverService.Builder(); builder.usingPort(port); - builder.build(); + ChromeDriverService service = builder.build(); + assertThat(service.getArgs()).contains(String.format(Locale.ROOT, "--port=%d", port)); Locale.setDefault(Locale.US); } Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion improves test coverage by verifying that port numbers are correctly formatted regardless of locale settings, which is a key aspect of the PR's changes to use Locale.ROOT for consistent formatting.	7

MustafaAgamy · 2025-01-20T20:14:16Z

@nvborisenko as discussed, please check.
Thanks.

nvborisenko

And tests?

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

MustafaAgamy · 2025-01-20T21:01:20Z

And tests?

You can see the old tests refactored to comply with the current changes so I didn't have to actually create "New" tests @nvborisenko

pujagani · 2025-01-21T04:47:09Z

LGTM. Thank you @MustafaAgamy! Will merge once the CI runs successfully.

asolntsev · 2025-01-21T14:04:38Z

java/test/org/openqa/selenium/ie/InternetExplorerDriverTest.java

-
-    Locale.setDefault(Locale.US);
+    } catch (Exception e) {
+      throw e;


Why do we need this "catch+throw" block?
It should be removed.

It's needed In case the test failed for any reason then we will throw the exception and at the finally block we will revert the locale default back to English regardless

That's why it's following

Try-catch with finally

@MustafaAgamy In this case, please use try { ... } finally { ... }. The "catch" part is not needed.

It doesn't make sense to catch and throw an exception without handling it.

asolntsev · 2025-01-21T14:05:40Z

java/test/org/openqa/selenium/ie/InternetExplorerDriverTest.java

-  void shouldThrowNumberFormatException() {
-    Locale arabicLocale = new Locale("ar", "EG");
-    Locale.setDefault(arabicLocale);
+  void shouldLaunchSuccessfullyWithArabicDate() {


I this this test is not needed at all.
It should be removed.

As disccussed earlier this test is using arabic as a unit test for port formatting logic

@MustafaAgamy
The line Locale.setDefault(arabicLocale) changes the global JVM state which may affect other tests (especially when they are running in parallel).

This test only checks that ChromeDriverService.Builder works with Arabic locale.
Why only this class?

Actually, we need to check that the whole Selenium Webdriver works with Arabic locale.
Instead of this single test, we should run ALL tests with Arabic locale.

For example, in Selenide we run all tests with Turkish locale for the same reason:

tasks.withType(Test).configureEach { useJUnitPlatform() systemProperties['user.country'] = 'TR' systemProperties['user.language'] = 'tr'

So what's the suggestion here?
Have some kind of Environement Properties set to run all tests with Arabic language?

You see, the thing is you don't actually need to run all tests against arabic. You just need to test if the builder will be able to build the driver instance successfully while the System language is arabic, that's pretty much it (That's the AOI - Area of Impact - for this unit test)

We don't even need any environment variables. We can just add these parameters to Bazel script to always run tests with Arabic locale.

(There is a more complex option - run tests with multiple "special" locales: Arabic, Turkish etc. But it's probably overkill.)

P.S. Yes, I see this is AOI of this test. And this is the wrong AOI.

MustafaAgamy added 2 commits January 20, 2025 20:03

Merge branch 'SeleniumHQ:trunk' into trunk

b7b05d2

qodo-merge-pro bot added the Review effort [1-5]: 2 label Jan 20, 2025

MustafaAgamy changed the title ~~Add Locale.ROOT to fix the avoid port formatting issues for all drivers~~ [Java] Add Locale.ROOT to fix the avoid port formatting issues for all drivers Jan 20, 2025

MustafaAgamy changed the title ~~[Java] Add Locale.ROOT to fix the avoid port formatting issues for all drivers~~ [Java] Add Locale.ROOT to avoid port formatting issues for all drivers Jan 20, 2025

nvborisenko reviewed Jan 20, 2025

View reviewed changes

java/test/org/openqa/selenium/firefox/FirefoxDriverTest.java Outdated Show resolved Hide resolved

MustafaAgamy added 3 commits January 20, 2025 21:58

add try and catch with finally to handle switching the locale default

6685d7b

reverse

7169a27

Merge branch 'trunk' into trunk

eef4b73

Fix formatting

6072c95

pujagani approved these changes Jan 21, 2025

View reviewed changes

pujagani merged commit a41a4c2 into SeleniumHQ:trunk Jan 21, 2025
32 of 34 checks passed

asolntsev suggested changes Jan 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Java] Add Locale.ROOT to avoid port formatting issues for all drivers #15121

[Java] Add Locale.ROOT to avoid port formatting issues for all drivers #15121

MustafaAgamy commented Jan 20, 2025 •

edited

Loading

qodo-merge-pro bot commented Jan 20, 2025

qodo-merge-pro bot commented Jan 20, 2025 •

edited

Loading

MustafaAgamy commented Jan 20, 2025

nvborisenko left a comment

MustafaAgamy commented Jan 20, 2025 •

edited

Loading

pujagani commented Jan 21, 2025

asolntsev Jan 21, 2025

MustafaAgamy Jan 21, 2025

asolntsev Jan 21, 2025

asolntsev Jan 21, 2025

MustafaAgamy Jan 21, 2025

asolntsev Jan 21, 2025

MustafaAgamy Jan 21, 2025

asolntsev Jan 21, 2025

[Java] Add Locale.ROOT to avoid port formatting issues for all drivers #15121

[Java] Add Locale.ROOT to avoid port formatting issues for all drivers #15121

Conversation

MustafaAgamy commented Jan 20, 2025 • edited Loading

User description

Description

Motivation and Context

Types of changes

Checklist

PR Type

Description

Changes walkthrough 📝

qodo-merge-pro bot commented Jan 20, 2025

PR Reviewer Guide 🔍

qodo-merge-pro bot commented Jan 20, 2025 • edited Loading

PR Code Suggestions ✨

MustafaAgamy commented Jan 20, 2025

nvborisenko left a comment

Choose a reason for hiding this comment

MustafaAgamy commented Jan 20, 2025 • edited Loading

pujagani commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MustafaAgamy commented Jan 20, 2025 •

edited

Loading

qodo-merge-pro bot commented Jan 20, 2025 •

edited

Loading

MustafaAgamy commented Jan 20, 2025 •

edited

Loading