[Java] Migrate the String.Format to String.Format(Locale.US) to handle different langauges for Edge, Chrome and Firefox

[MustafaAgamy]

User description

Description

• This is a contributions to fix: https://github.com/SeleniumHQ/selenium/issues/13930

*Changes Done:

• Migrate the String.Format to Formatter.Format in :
• ChromeDriverSerivce.java
• EdgeDriverService.java
• RemoteWebDriver.java

Motivation and Context

*Issue explaination:

• Having System Date & Language in Arabic caused with driver setup. Using String.Format for ports and some other lines caused them to be localized in the system language and in arabic case it caused the numbers and some other character to be localized in arabic, e.g. the port. And that caused an issues when appending the port to the URL in order to setup the browser

*Fix explaination:

• Now it's utilizing the Formatter.Format and forcing the localization to be in (English.US) ny passing the corresponding Locale in the Formatter Constructor

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.
• [x] I have added tests to cover my changes.
• [x] All new and existing tests passed.

---

PR Type

Bug fix

---

Description

• Updated ChromeDriverService, EdgeDriverService, and RemoteWebDriver to use Formatter.format with Locale.US to ensure consistent formatting across different system locales.
• This change addresses issues where system locale settings (like Arabic) affected the formatting of numbers and text in logs and port settings, potentially causing errors in browser setup.
• The use of Locale.US ensures that numbers and text are formatted in a standard way, avoiding issues related to localization.

---

Changes walkthrough 📝

Relevant files

Bug fix

ChromeDriverService.java Use Formatter with Locale.US in ChromeDriverService            java/src/org/openqa/selenium/chrome/ChromeDriverService.java Replaced String.format with Formatter.format using Locale.US for consistent number formatting. Ensured log file paths and log levels are formatted without localization issues. +6/-4      EdgeDriverService.java Standardize EdgeDriverService Formatting with Locale.US    java/src/org/openqa/selenium/edge/EdgeDriverService.java Switched from String.format to Formatter.format with Locale.US to avoid localization in log and port settings. Adjusted logging and port settings to use non-localized number formats. +6/-4      RemoteWebDriver.java Implement Formatter in RemoteWebDriver for Uniform Output java/src/org/openqa/selenium/remote/RemoteWebDriver.java Updated string formatting to use Formatter with Locale.US for consistent command and log messages. Modified session handling and logging to prevent localization issues. +8/-6

---

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[CLAassistant]

All committers have signed the CLA.

[qodo-code-review[bot]]

PR Description updated to latest commit (https://github.com/SeleniumHQ/selenium/commit/1c526d0055f79762d81a280e3e18616aaa78e155)

• [ ] Copy walkthrough table to "Files Changed" Tab

[qodo-code-review[bot]]

PR Review 🔍

⏱️ Estimated effort to review [1-5]	2, because the changes are straightforward and localized to string formatting across a few files. The logic is simple, replacing String.format with Formatter to handle locale-specific formatting issues.
🧪 Relevant tests	No
⚡ Possible issues	Resource Leak: Each new Formatter instance should be properly closed to avoid resource leaks. Consider using try-with-resources or ensuring each formatter is closed after use.
	Consistency Issue: In some places, Locale.US is used, and in one place, Locale.ENGLISH is used. It's important to ensure consistency across the application unless intentionally varied.
🔒 Security concerns	No

[qodo-code-review[bot]]

PR Code Suggestions ✨

Category	Suggestion	Score
Resource management	✅ Ensure resource management by properly closing Formatter objects Suggestion Impact: The commit addressed the resource management issue by removing the use of Formatter objects entirely and replacing them with String.format, which does not require explicit resource management. code diff: List<String> args = new ArrayList<>(); - args.add(String.valueOf(new Formatter(Locale.US).format("--port=%d", getPort())); + args.add(String.format("--port=%d", getPort())); // Readable timestamp and append logs only work if log path is specified in args // Cannot use logOutput because goog:loggingPrefs requires --log-path get sent if (getLogFile() != null) { - args.add(String.valueOf(new Formatter(Locale.ENGLISH).format("--log-path=%s", getLogFile().getAbsolutePath()))); + args.add(String.format("--log-path=%s", getLogFile().getAbsolutePath())); if (Boolean.TRUE.equals(readableTimestamp)) { args.add("--readable-timestamp"); } @@ -302,10 +303,10 @@ } if (logLevel != null) { - args.add(String.valueOf(new Formatter(Locale.US).format("--log-level=%s", logLevel.toString().toUpperCase()))); + args.add(String.format("--log-level=%s", logLevel.toString().toUpperCase())); } if (allowedListIps != null) { - args.add(String.valueOf(new Formatter(Locale.US).format("--allowed-ips=%s", allowedListIps))); + args.add(String.format("--allowed-ips=%s", allowedListIps)); Ensure that the Formatter objects are properly closed to prevent resource leaks. Use try-with-resources to manage the Formatter lifecycle. java/src/org/openqa/selenium/chrome/ChromeDriverService.java [287] -args.add(String.valueOf(new Formatter(Locale.US).format("--port=%d", getPort()))); +try (Formatter formatter = new Formatter(Locale.US)) { + args.add(String.valueOf(formatter.format("--port=%d", getPort()))); +} Suggestion importance[1-10]: 9 Why: The suggestion correctly identifies a resource management issue with the Formatter class, which is crucial to prevent potential resource leaks in Java.	9
Performance	✅ Optimize resource usage by using a single Formatter instance Suggestion Impact: The commit replaced multiple Formatter instances with a single String.format call, which aligns with the suggestion's intention to optimize resource usage. code diff: - args.add(String.valueOf(new Formatter(Locale.US).format("--port=%d", getPort()))); + args.add(String.format("--port=%d", getPort())); // Readable timestamp and append logs only work if log path is specified in args // Cannot use logOutput because goog:loggingPrefs requires --log-path get sent if (getLogFile() != null) { - args.add(String.valueOf(new Formatter(Locale.US).format("--log-path=%s", getLogFile().getAbsolutePath()))); + args.add(String.format("--log-path=%s", getLogFile().getAbsolutePath())); if (Boolean.TRUE.equals(readableTimestamp)) { args.add("--readable-timestamp"); } @@ -296,7 +297,7 @@ } if (logLevel != null) { - args.add(String.valueOf(new Formatter(Locale.US).format("--log-level=%s", logLevel.toString().toUpperCase()))); + args.add(String.format("--log-level=%s", logLevel.toString().toUpperCase())); } if (Boolean.TRUE.equals(silent)) { args.add("--silent"); @@ -305,7 +306,7 @@ args.add("--verbose"); } if (allowedListIps != null) { - args.add(String.valueOf(new Formatter(Locale.US).format("--allowed-ips=%s", allowedListIps))); + args.add(String.format("--allowed-ips=%s", allowedListIps)); Use a single Formatter instance within the method to optimize resource usage and improve performance. java/src/org/openqa/selenium/edge/EdgeDriverService.java [281] -args.add(String.valueOf(new Formatter(Locale.US).format("--port=%d", getPort()))); +try (Formatter formatter = new Formatter(Locale.US)) { + args.add(String.valueOf(formatter.format("--port=%d", getPort()))); + if (getLogFile() != null) { + args.add(String.valueOf(formatter.format("--log-path=%s", getLogFile().getAbsolutePath()))); + } + if (logLevel != null) { + args.add(String.valueOf(formatter.format("--log-level=%s", logLevel.toString().toUpperCase()))); + } + if (allowedListIps != null) { + args.add(String.valueOf(formatter.format("--allowed-ips=%s", allowedListIps))); + } +} Suggestion importance[1-10]: 8 Why: This suggestion is valid and would improve performance by reducing the number of Formatter instances created, which is a good practice in resource management.	8
Code simplification	✅ Simplify code and reduce object creation by using direct String conversion Suggestion Impact: The commit replaced instances of String.valueOf with String.format, which aligns with the suggestion to simplify code and reduce unnecessary object creation. code diff: - String.valueOf(new Formatter(Locale.US).format("Forwarding %s on session %s to remote", command.getName(), sessionId))); + String.format("Forwarding %s on session %s to remote", command.getName(), sessionId)); try { log(sessionId, command.getName(), command, When.BEFORE); response = executor.execute(command); @@ -728,7 +735,7 @@ && toLog instanceof Response) { Response responseToLog = (Response) toLog; Response copyToLog = new Response(new SessionId((responseToLog).getSessionId())); - copyToLog.setValue(String.valueOf(new Formatter(Locale.US).format("*%s response suppressed*", commandName))); + copyToLog.setValue(String.format("*%s response suppressed*", commandName)); copyToLog.setState(responseToLog.getState()); text = String.valueOf(copyToLog); } @@ -759,11 +766,11 @@ LOG.log( Level.WARNING, () -> - String.valueOf(new Formatter(Locale.US).format( + String.format( "Support for Legacy Capabilities is deprecated; You are sending the following" + " invalid capabilities: %s; Please update to W3C Syntax:" + " https://www.selenium.dev/blog/2022/legacy-protocol-support/", - invalid))); + invalid)); } } @@ -817,9 +824,9 @@ platformName = "unknown"; } - return String.valueOf(new Formatter(Locale.US).format( + return String.format( "%s: %s on %s (%s)", - getClass().getSimpleName(), caps.getBrowserName(), platformName, getSessionId())); + getClass().getSimpleName(), caps.getBrowserName(), platformName, getSessionId()); Replace String.valueOf with direct String conversion for cleaner code and to avoid unnecessary object creation. java/src/org/openqa/selenium/remote/RemoteWebDriver.java [731] -copyToLog.setValue(String.valueOf(new Formatter(Locale.US).format("*%s response suppressed*", commandName))); +copyToLog.setValue(new Formatter(Locale.US).format("*%s response suppressed*", commandName).toString()); Suggestion importance[1-10]: 7 Why: The suggestion improves code readability and efficiency by eliminating an unnecessary method call, which is a good practice for cleaner and more efficient code.	7
Robustness	Enhance robustness by handling exceptions in toUpperCase() method Consider handling potential exceptions that might be thrown by the toUpperCase() method in a locale-sensitive manner. java/src/org/openqa/selenium/remote/RemoteWebDriver.java [298] -args.add(String.valueOf(new Formatter(Locale.US).format("--log-level=%s", logLevel.toString().toUpperCase()))); +try { + args.add(String.valueOf(new Formatter(Locale.US).format("--log-level=%s", logLevel.toString().toUpperCase(Locale.ROOT)))); +} catch (Exception e) { + // Handle exception +} Suggestion importance[1-10]: 6 Why: This suggestion adds robustness by considering locale sensitivity and exception handling, although toUpperCase() typically does not throw exceptions that need to be caught in this context, making the suggestion slightly less relevant.	6

[pujagani]

I think the changes in the PR are not necessary. I looked into this further to realize the findings below:

  public static String format(String format, Object... args) {
        return new Formatter().format(format, args).toString();
    } 

This creates a Formatter instance:

  public Formatter() {
        this(Locale.getDefault(Locale.Category.FORMAT), new StringBuilder());
    }

So setting the default using "setDefault(Locale)" should work out of the box without any changes.

[pujagani]

Before we close this PR, can you try on your end to double-check if it works as expected using "setDefault"?

[MustafaAgamy]

@pujagani I just did this to minimize the scope of the affects

But setting the setting the Default to Locale.US should work

You can call it In the same create Args methods once

I will change the fixes to that and give it a try?

[MustafaAgamy]

@pujagani

Done can you please check and let me know ? (it worked local) Also maybe another pipeline run to make sure everything is working as expected?

[MustafaAgamy]

@pujagani I revisited the formatting issue

Should be good now

[titusfortner]

@diemol this look ok?

[pujagani]

https://github.com/SeleniumHQ/selenium/pull/13934#issuecomment-2136539473 - I think we don't need to make any changes. Just the tests are sufficient.

[MustafaAgamy]

@pujagani

But without forcing the localization to be In English, users having their OS default language In arabic will still counter the same issue, won't they?

[titusfortner]

Is this correct summary? When a user's default language isn't English, then the code is going to fail unless they add Locale.setDefault(Locale.US).

Then is the question whether we add it to the library itself or whether the user needs to add it to their test? Is there a scenario in which adding that code can cause other problems?

[MustafaAgamy]

@titusfortner I don't see adding it to Selenium itself causing any issues from My POV tbh

And also many of the users In the middle east are facing start up failures cuz of locale issue

[pujagani]

If users are facing a startup problem, it might be good idea to add it on their end. I am not sure if Selenium should assume which default locale to set. Rather the users can themselves add the locale of their choice. What we can do instead is have a test to show how to do that (which a part of this PR does) and have a blog/documentation regarding this. Does that make sense?

[MustafaAgamy]

I Tested it against some langauges like chinese and japanese, only arabic had this issue because of the url setup

• Users are getting start up failure exceptions with no indication of What went wrong at all so having a documentation might not be enough for them or at least provide a meaningful error for them

• Selenium already got Selenium Manager for better drivers handling for the end user by following the Batteries Included approach, isn't that the same? Because having the port set In Arabic will never work, so shouldn't that be handled within Selenium itself?

[pujagani]

I understand your point and agree we should have better error handling for this or an informative message. My concern here is that for example if someone wants to use Chinese but now we are setting the locale to US, that is not the expected behavior. Probably, we need to find a different solution or only set the locale to US if it is set to Arabic and probably reset it after start up, not sure what is the correct way to approach this. @titusfortner @diemol

[krmahadevan]

@MustafaAgamy - Just curious. Even the JVM arguments way of setting the locale doesnt help here is it?

[MustafaAgamy]

@krmahadevan I didn't try to be honest

[MustafaAgamy]

@pujagani

What if we minimize the scope by moving back to String.format with locale set to en for the required parts only, I guess this way the end user's default locale won't get affected where it's not really needed

[krmahadevan]

@krmahadevan I didn't try to be honest

@MustafaAgamy can you give it a try and post back the results?

[pujagani]

@pujagani

What if we minimize the scope by moving back to String.format with locale set to en for the required parts only, I guess this way the end user's default locale won't get affected where it's not really needed

I think this might be the best thing to do. After understanding the issue properly, my thoughts were on the same line. Keep the scope limited to start up. It might also be good to leave a comment in the code as to why it is done.

[pujagani]

@MustafaAgamy - Just curious. Even the JVM arguments way of setting the locale doesnt help here is it?

@MustafaAgamy Can you please try this out? If that works, then no code change require and we can document it.

[MustafaAgamy]

@pujagani sorry I didnt have access to My laptop for the last couple of days

I'll try today and let you know

[pujagani]

@MustafaAgamy No rush. We appreciate your contribution and time to Selenium.

[joerg1985]

Just my 2 cent: I think calling Locale.setDefault(Locale.US) is a bad idea, this will change propably change alot of things.

Why using String.format here at all, if we do not want Java to format the String?

// users might have a default local with not latin characters set, String.format would break the argument
args.add("--port=" + getPort());

[MustafaAgamy]

@pujagani The JVM Arguments seems to be working

I added them to the VM Arguments In intelliji and the issue seems to be fixed

But we will have to document it for the users

[pujagani]

Thank you for trying it out. Can we close this PR then? Can you help document it too?

[MustafaAgamy]

We won't need the tests? If yes, then I can Remove the changed code lines and only Keep the test files. Also where should I document it?

[pujagani]

Yes, we can only keep the tests. You can help add the documentation by creating a PR in our website repo https://github.com/SeleniumHQ/seleniumhq.github.io.