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.

Sign up for GitHub

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

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

Merged
merged 6 commits into from
Jan 21, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fe86936
Add Formatting with Locale.ROOT for the port at :
MustafaAgamy Jan 20, 2025
b7b05d2
Merge branch 'SeleniumHQ:trunk' into trunk
MustafaAgamy Jan 20, 2025
6685d7b
add try and catch with finally to handle switching the locale default
MustafaAgamy Jan 20, 2025
7169a27
reverse
MustafaAgamy Jan 20, 2025
eef4b73
Merge branch 'trunk' into trunk
MustafaAgamy Jan 20, 2025
6072c95
Fix formatting
pujagani Jan 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion java/src/org/openqa/selenium/chrome/ChromeDriverService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ protected void loadSystemProperties() {
@Override
protected List<String> createArgs() {
List<String> args = new ArrayList<>();
args.add(String.format("--port=%d", getPort()));
args.add(String.format(Locale.ROOT, "--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
Expand Down
2 changes: 1 addition & 1 deletion java/src/org/openqa/selenium/edge/EdgeDriverService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ protected void loadSystemProperties() {
@Override
protected List<String> createArgs() {
List<String> args = new ArrayList<>();
args.add(String.format("--port=%d", getPort()));
args.add(String.format(Locale.ROOT, "--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
Expand Down
3 changes: 2 additions & 1 deletion java/src/org/openqa/selenium/firefox/GeckoDriverService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import org.openqa.selenium.Capabilities;
import org.openqa.selenium.WebDriverException;
Expand Down Expand Up @@ -219,7 +220,7 @@ protected void loadSystemProperties() {
@Override
protected List<String> createArgs() {
List<String> args = new ArrayList<>();
args.add(String.format("--port=%d", getPort()));
args.add(String.format(Locale.ROOT, "--port=%d", getPort()));

int wsPort = PortProber.findFreePort();
args.add(String.format("--websocket-port=%d", wsPort));
Expand Down
3 changes: 2 additions & 1 deletion java/src/org/openqa/selenium/ie/InternetExplorerDriverService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import org.openqa.selenium.Capabilities;
import org.openqa.selenium.WebDriverException;
Expand Down Expand Up @@ -217,7 +218,7 @@ protected void loadSystemProperties() {
@Override
protected List<String> createArgs() {
List<String> args = new ArrayList<>();
args.add(String.format("--port=%d", getPort()));
args.add(String.format(Locale.ROOT, "--port=%d", getPort()));

if (logLevel != null) {
args.add(String.format("--log-level=%s", logLevel));
Expand Down
13 changes: 1 addition & 12 deletions java/src/org/openqa/selenium/remote/service/DriverService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ protected Map<String, String> getEnvironment() {
}

protected URL getUrl(int port) throws IOException {
return new URL(String.format("http://localhost:%d", port));
return new URL(String.format(Locale.ROOT, "http://localhost:%d", port));
}

protected Capabilities getDefaultDriverOptions() {
Expand Down Expand Up @@ -505,17 +505,6 @@ public DS build() {
port = PortProber.findFreePort();
}

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: \""
+ String.format("--port=%d", port)
+ "\", 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()));
}

if (timeout == null) {
timeout = getDefaultTimeout();
}
Expand Down
33 changes: 15 additions & 18 deletions java/test/org/openqa/selenium/chrome/ChromeDriverFunctionalTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,23 +204,20 @@ void canExecuteCdpCommands() {

@Test
@NoDriverBeforeTest
void shouldThrowNumberFormatException() {
Locale arabicLocale = new Locale("ar", "EG");
Locale.setDefault(arabicLocale);

int port = PortProber.findFreePort();
ChromeDriverService.Builder builder = new ChromeDriverService.Builder();
builder.usingPort(port);

assertThatExceptionOfType(NumberFormatException.class)
.isThrownBy(builder::build)
.withMessage(
"Couldn't format the port numbers because the System Language is arabic: \""
+ String.format("--port=%d", port)
+ "\", 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/");

Locale.setDefault(Locale.US);
void shouldLaunchSuccessfullyWithArabicDate() {
try {
Locale arabicLocale = new Locale("ar", "EG");
Locale.setDefault(arabicLocale);

int port = PortProber.findFreePort();
ChromeDriverService.Builder builder = new ChromeDriverService.Builder();
builder.usingPort(port);
builder.build();

} catch (Exception e) {
throw e;
} finally {
Locale.setDefault(Locale.US);
}
}
}
33 changes: 15 additions & 18 deletions java/test/org/openqa/selenium/edge/EdgeDriverFunctionalTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,23 +198,20 @@ void canExecuteCdpCommands() {

@Test
@NoDriverBeforeTest
void shouldThrowNumberFormatException() {
Locale arabicLocale = new Locale("ar", "EG");
Locale.setDefault(arabicLocale);

int port = PortProber.findFreePort();
EdgeDriverService.Builder builder = new EdgeDriverService.Builder();
builder.usingPort(port);

assertThatExceptionOfType(NumberFormatException.class)
.isThrownBy(builder::build)
.withMessage(
"Couldn't format the port numbers because the System Language is arabic: \""
+ String.format("--port=%d", port)
+ "\", 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/");

Locale.setDefault(Locale.US);
void shouldLaunchSuccessfullyWithArabicDate() {
try {
Locale arabicLocale = new Locale("ar", "EG");
Locale.setDefault(arabicLocale);

int port = PortProber.findFreePort();
EdgeDriverService.Builder builder = new EdgeDriverService.Builder();
builder.usingPort(port);
builder.build();

} catch (Exception e) {
throw e;
} finally {
Locale.setDefault(Locale.US);
}
}
}
34 changes: 15 additions & 19 deletions java/test/org/openqa/selenium/firefox/FirefoxDriverTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
package org.openqa.selenium.firefox;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.AssertionsForClassTypes.assertThatExceptionOfType;
import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;
import static org.junit.jupiter.api.Assertions.fail;
import static org.junit.jupiter.api.Assumptions.assumeTrue;
Expand Down Expand Up @@ -273,24 +272,21 @@ void canSetContext() {

@Test
@NoDriverBeforeTest
void shouldThrowNumberFormatException() {
Locale arabicLocale = new Locale("ar", "EG");
Locale.setDefault(arabicLocale);

int port = PortProber.findFreePort();
GeckoDriverService.Builder builder = new GeckoDriverService.Builder();
builder.usingPort(port);

assertThatExceptionOfType(NumberFormatException.class)
.isThrownBy(builder::build)
.withMessage(
"Couldn't format the port numbers because the System Language is arabic: \""
+ String.format("--port=%d", port)
+ "\", 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/");

Locale.setDefault(Locale.US);
void shouldLaunchSuccessfullyWithArabicDate() {
try {
Locale arabicLocale = new Locale("ar", "EG");
Locale.setDefault(arabicLocale);

int port = PortProber.findFreePort();
GeckoDriverService.Builder builder = new GeckoDriverService.Builder();
builder.usingPort(port);
builder.build();

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

private static class CustomFirefoxProfile extends FirefoxProfile {}
Expand Down
29 changes: 13 additions & 16 deletions java/test/org/openqa/selenium/ie/InternetExplorerDriverTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,24 +147,21 @@ void testPersistentHoverCanBeTurnedOff() throws Exception {

@Test
@NoDriverBeforeTest
void shouldThrowNumberFormatException() {
Locale arabicLocale = new Locale("ar", "EG");
Locale.setDefault(arabicLocale);
void shouldLaunchSuccessfullyWithArabicDate() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

try {
Locale arabicLocale = new Locale("ar", "EG");
Locale.setDefault(arabicLocale);

int port = PortProber.findFreePort();
InternetExplorerDriverService.Builder builder = new InternetExplorerDriverService.Builder();
builder.usingPort(port);
int port = PortProber.findFreePort();
InternetExplorerDriverService.Builder builder = new InternetExplorerDriverService.Builder();
builder.usingPort(port);
builder.build();

assertThatExceptionOfType(NumberFormatException.class)
.isThrownBy(builder::build)
.withMessage(
"Couldn't format the port numbers because the System Language is arabic: \""
+ String.format("--port=%d", port)
+ "\", 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/");

Locale.setDefault(Locale.US);
} catch (Exception e) {
throw e;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

} finally {
Locale.setDefault(Locale.US);
}
}

private WebDriver newIeDriver() {
Expand Down
33 changes: 15 additions & 18 deletions java/test/org/openqa/selenium/safari/SafariDriverTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,23 +137,20 @@ public void canAttachDebugger() {

@Test
@NoDriverBeforeTest
void shouldThrowNumberFormatException() {
Locale arabicLocale = new Locale("ar", "EG");
Locale.setDefault(arabicLocale);

int port = PortProber.findFreePort();
SafariDriverService.Builder builder = new SafariDriverService.Builder();
builder.usingPort(port);

assertThatExceptionOfType(NumberFormatException.class)
.isThrownBy(builder::build)
.withMessage(
"Couldn't format the port numbers because the System Language is arabic: \""
+ String.format("--port=%d", port)
+ "\", 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/");

Locale.setDefault(Locale.US);
void shouldLaunchSuccessfullyWithArabicDate() {
try {
Locale arabicLocale = new Locale("ar", "EG");
Locale.setDefault(arabicLocale);

int port = PortProber.findFreePort();
SafariDriverService.Builder builder = new SafariDriverService.Builder();
builder.usingPort(port);
builder.build();

} catch (Exception e) {
throw e;
} finally {
Locale.setDefault(Locale.US);
}
}
}
Loading