[java] Handle system proxy setting for arguments passed to selenium manager

[bhecquet]

This PR handles System proxy setting when using selenium manager

🔗 Related Issues

Fixes #17358

💥 What does this PR do?

The PR adds "system" proxy in the exclusion when building argument list of selenium manager

I've also added some units tests on this part, and also checked in a real test environment

🤖 AI assistance

• No substantial AI assistance used
• AI assisted (complete below)
  • Tool(s):
  • What was generated:
  • I reviewed all AI output and can explain the change

💡 Additional Considerations

N/A

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

Review Summary by Qodo

Handle system proxy setting for Selenium Manager arguments

🐞 Bug fix

Walkthroughs

Description

• Exclude SYSTEM proxy type from Selenium Manager arguments
• Add unit tests for SYSTEM, AUTODETECT, and DIRECT proxy types
• Prevent system proxy settings from being passed to manager

File Changes

1. java/src/org/openqa/selenium/remote/service/DriverFinder.java 🐞 Bug fix +2/-1

Exclude SYSTEM proxy type from manager arguments

• Added check to exclude Proxy.ProxyType.SYSTEM from proxy arguments passed to Selenium Manager
• Prevents system proxy configuration from being incorrectly forwarded to the manager

java/src/org/openqa/selenium/remote/service/DriverFinder.java

---

2. java/test/org/openqa/selenium/remote/service/DriverFinderTest.java 🧪 Tests +56/-0

Add proxy type handling unit tests

• Added import for ProxyType enum
• Created parameterized test helper method createsArgumentsForSeleniumManagerWithProxySettings()
• Added three new test cases for SYSTEM, AUTODETECT, and DIRECT proxy types
• Tests verify that proxy settings are correctly handled and not passed to Selenium Manager

java/test/org/openqa/selenium/remote/service/DriverFinderTest.java

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (1) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 18 rules

1. Mockito mocks in DriverFinderTest 📘 Rule violation ▣ Testability

Description

The new tests rely on Mockito (when, doReturn, verify*) instead of using real or
contract-driven integrations or simple in-memory fakes. This can reduce test reliability and diverge
from real behavior without an enforced contract.

Code

java/test/org/openqa/selenium/remote/service/DriverFinderTest.java[R214-251]

+  void createsArgumentsForSeleniumManagerWithProxySettings(ProxyType proxyType) throws IOException {
+    when(service.getExecutable()).thenReturn(null);
+    when(service.getDriverProperty()).thenReturn("property.selenium.manager.empty");
+    when(service.getDriverEnvironmentVariable())
+        .thenReturn("ENVIRONMENT_VARIABLE_IGNORES_SELENIUM_MANAGER");
+
+    Proxy proxy = new Proxy().setProxyType(proxyType);
+    Capabilities capabilities =
+        new ImmutableCapabilities(
+            "browserName",
+            "chrome",
+            "browserVersion",
+            "beta",
+            "proxy",
+            proxy,
+            "goog:chromeOptions",
+            Map.of("binary", browserFile.toString()));
+    DriverFinder finder = new DriverFinder(service, capabilities, seleniumManager);
+
+    List<String> arguments = new ArrayList<>();
+    arguments.add("--browser");
+    arguments.add("chrome");
+    arguments.add("--browser-version");
+    arguments.add("beta");
+    arguments.add("--browser-path");
+    arguments.add(browserFile.toString());
+    Result result = new Result(0, "", driverFile.toString(), browserFile.toString());
+    doReturn(result).when(seleniumManager).getBinaryPaths(arguments);
+
+    assertThat(finder.getDriverPath()).isEqualTo(driverFile.toString());
+    assertThat(finder.getBrowserPath()).isEqualTo(browserFile.toString());
+    verify(service, times(1)).getExecutable();
+    verify(service, times(1)).getDriverName();
+    verify(service, times(1)).getDriverProperty();
+    verify(service, times(1)).getDriverEnvironmentVariable();
+    verifyNoMoreInteractions(service);
+    verify(seleniumManager, times(1)).getBinaryPaths(arguments);
+    verifyNoMoreInteractions(seleniumManager);

Evidence

PR Compliance ID 389270 disallows mocks in tests unless they are backed by a machine-checked
contract. The added test helper method uses Mockito stubbing and verification (when, doReturn,
verifyNoMoreInteractions), indicating a mocking-based test approach rather than a
real/contract-driven integration or simple fake implementation.

Rule 389270: Avoid mocks in tests; use real or contract-driven integrations
java/test/org/openqa/selenium/remote/service/DriverFinderTest.java[214-251]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New/modified tests use Mockito mocks, but the compliance rule requires avoiding mocks in favor of real integrations, contract-driven fakes, or simple in-memory fake implementations.

## Issue Context
`createsArgumentsForSeleniumManagerWithProxySettings(...)` stubs and verifies behavior via Mockito calls (e.g., `when(...)`, `doReturn(...)`, `verify(...)`).

## Fix Focus Areas
- java/test/org/openqa/selenium/remote/service/DriverFinderTest.java[214-251]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Duplicate proxy-setting test methods ✓ Resolved 📘 Rule violation ≡ Correctness

Description

DriverFinderTest declares the test methods
createsArgumentsForSeleniumManagerWithSystemProxySettings, ...WithAutodetectProxySettings, and
...WithDirectProxySettings twice, which will fail Java compilation and break CI. This prevents the
test suite from running and undermines the goal of adding test coverage for the Selenium Manager
proxy-argument behavior change.

Code

java/test/org/openqa/selenium/remote/service/DriverFinderTest.java[R214-227]

+  @Test
+  void createsArgumentsForSeleniumManagerWithSystemProxySettings() throws IOException {
+    createsArgumentsForSeleniumManagerWithProxySettings(ProxyType.SYSTEM);
+  }
+
+  @Test
+  void createsArgumentsForSeleniumManagerWithAutodetectProxySettings() throws IOException {
+    createsArgumentsForSeleniumManagerWithProxySettings(ProxyType.AUTODETECT);
+  }
+
+  @Test
+  void createsArgumentsForSeleniumManagerWithDirectProxySettings() throws IOException {
+    createsArgumentsForSeleniumManagerWithProxySettings(ProxyType.DIRECT);
+  }

Evidence

PR Compliance ID 4 requires that changes be covered by tests without reducing CI reliability, but
Java forbids duplicate method signatures within the same class, so repeating the same three @Test
methods causes a compile-time error. In DriverFinderTest.java, the three methods appear once at
lines 199–212 and are then repeated again in a newly added block at lines 214–227, creating the
duplicate declarations that block compilation and test execution.

AGENTS.md: Include/Update Tests for Code Changes and Prefer Small Unit Tests Over Browser Tests
java/test/org/openqa/selenium/remote/service/DriverFinderTest.java[199-227]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`DriverFinderTest` contains duplicate declarations (same names/signatures) for three `@Test` methods: `createsArgumentsForSeleniumManagerWithSystemProxySettings`, `...WithAutodetectProxySettings`, and `...WithDirectProxySettings`. This will cause a Java compile-time error (e.g., `method ... is already defined`) and block the test suite from compiling/running, harming CI reliability.

## Issue Context
The file already defines these three tests once earlier in the class, and then defines them again in a newly added block in the new-change focus area. If the intent was to add new coverage, it should be done without duplicating method signatures (e.g., remove the duplicate block, or rename/refactor if a distinct test is actually intended).

## Fix Focus Areas
- java/test/org/openqa/selenium/remote/service/DriverFinderTest.java[214-227]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Dangling --proxy argument 🐞 Bug ≡ Correctness

Description

DriverFinder.toArguments() can add --proxy without a following proxy value when ProxyType is
neither DIRECT/AUTODETECT/SYSTEM but no ssl/http/pac value is set (e.g., default UNSPECIFIED or
MANUAL with only socksProxy/noProxy). This can cause Selenium Manager to mis-parse subsequent flags
(since it appends more args after the provided list) and fail driver/browser resolution.

Code

java/src/org/openqa/selenium/remote/service/DriverFinder.java[R153-160]

    Proxy proxy = Proxy.extractFrom(options);
    if (proxy != null
        && proxy.getProxyType() != Proxy.ProxyType.DIRECT
-        && proxy.getProxyType() != Proxy.ProxyType.AUTODETECT) {
+        && proxy.getProxyType() != Proxy.ProxyType.AUTODETECT
+        && proxy.getProxyType() != Proxy.ProxyType.SYSTEM) {
      arguments.add("--proxy");
      if (proxy.getSslProxy() != null) {
        arguments.add(proxy.getSslProxy());

Evidence

The code adds --proxy based only on proxy type, but only conditionally appends a value
(ssl/http/pac). Proxy defaults to UNSPECIFIED, and MANUAL can be set via noProxy/socksProxy without
setting http/ssl/pac—so the value can be missing. SeleniumManager then appends additional flags
after the provided arguments, meaning a trailing --proxy can consume the next flag as its value.
The existing DriverFinderTest shows the intended contract is --proxy followed by a value.

java/src/org/openqa/selenium/remote/service/DriverFinder.java[133-167]
java/src/org/openqa/selenium/Proxy.java[39-89]
java/src/org/openqa/selenium/Proxy.java[284-333]
java/src/org/openqa/selenium/manager/SeleniumManager.java[266-279]
java/test/org/openqa/selenium/remote/service/DriverFinderTest.java[155-185]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`DriverFinder.toArguments()` may append `--proxy` without any proxy value for some valid `Proxy` configurations (e.g., `ProxyType.UNSPECIFIED` default, or `ProxyType.MANUAL` where only `socksProxy`/`noProxy` is set). This can break Selenium Manager CLI parsing because additional flags are appended later.

### Issue Context
Selenium Manager is invoked with the list from `DriverFinder.toArguments()`, and then `SeleniumManager#getBinaryPaths` appends additional flags (e.g., `--language-binding`, `--output`). A dangling `--proxy` means the next appended flag can be consumed as the proxy value.

### Fix Focus Areas
- java/src/org/openqa/selenium/remote/service/DriverFinder.java[153-166]
- java/src/org/openqa/selenium/manager/SeleniumManager.java[266-279]
- java/test/org/openqa/selenium/remote/service/DriverFinderTest.java[155-250]

### Suggested fix
- Compute a `proxyArg` first (first non-null of `sslProxy`, `httpProxy`, `proxyAutoconfigUrl`, and *optionally* `socksProxy` if Selenium Manager supports it).
- Only add `--proxy` when `proxyArg != null && !proxyArg.isEmpty()`.
- Consider explicitly excluding `ProxyType.UNSPECIFIED` (or treating it as "no proxy").

### Tests to add/adjust
- Add a unit test where capabilities include `new Proxy()` (UNSPECIFIED) and assert Selenium Manager is called **without** `--proxy`.
- Add a unit test for `new Proxy().setNoProxy("...")` (MANUAL but no http/ssl/pac) and assert Selenium Manager is called **without** `--proxy` (or with an explicit supported value, depending on intended behavior).
- (Optional) Add a unit test for `setSocksProxy` verifying behavior matches the intended Selenium Manager support.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Whitespace-only lines added ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

Two whitespace-only blank lines (with trailing spaces) were added, creating formatting-only churn
unrelated to the functional change. This violates the repository’s trim_trailing_whitespace = true
setting, increasing diff noise and review burden.

Code

java/test/org/openqa/selenium/remote/service/DriverFinderTest.java[R268-269]

+    
+    

Evidence

The cited lines in java/test/org/openqa/selenium/remote/service/DriverFinderTest.java at 268-269
are blank lines that contain whitespace, demonstrating trailing spaces on otherwise empty lines.
This is both unnecessary formatting-only churn (called out by PR Compliance ID 2) and inconsistent
with the repository’s EditorConfig rule that trailing whitespace should be trimmed
(trim_trailing_whitespace = true).

AGENTS.md: Avoid Repository-Wide Refactors or Formatting-Only Churn
java/test/org/openqa/selenium/remote/service/DriverFinderTest.java[268-269]
.editorconfig[5-12]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Whitespace-only blank lines (blank lines containing trailing spaces) were introduced, creating formatting-only churn and violating the repository’s `trim_trailing_whitespace = true` setting.

## Issue Context
These lines do not affect behavior and only add diff noise, but they are inconsistent with expected formatting rules (EditorConfig) and increase review burden.

## Fix Focus Areas
- java/test/org/openqa/selenium/remote/service/DriverFinderTest.java[268-269]
- .editorconfig[5-12]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Non-hermetic system-property tests 🐞 Bug ☼ Reliability

Description

The new proxy-type tests assume the JVM system property named by service.getDriverProperty() is
unset; if property.selenium.manager.empty is set externally, DriverFinder#getBinaryPaths() will
skip calling Selenium Manager and these tests will fail or stop validating argument-building
behavior.

Code

java/test/org/openqa/selenium/remote/service/DriverFinderTest.java[R213-218]

+    when(service.getExecutable()).thenReturn(null);
+    when(service.getDriverProperty()).thenReturn("property.selenium.manager.empty");
+    when(service.getDriverEnvironmentVariable())
+        .thenReturn("ENVIRONMENT_VARIABLE_IGNORES_SELENIUM_MANAGER");
+
+    Proxy proxy = new Proxy().setProxyType(proxyType);

Evidence

The tests hard-code the driver property key to property.selenium.manager.empty, and DriverFinder
consults that key via System.getProperty(...) before deciding whether to invoke Selenium Manager;
therefore an externally-set system property can change the code path and invalidate the test’s
assumptions.

java/test/org/openqa/selenium/remote/service/DriverFinderTest.java[212-217]
java/src/org/openqa/selenium/remote/service/DriverFinder.java[93-105]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`DriverFinderTest` stubs `getDriverProperty()` to `property.selenium.manager.empty` but does not ensure that `System.getProperty("property.selenium.manager.empty")` is cleared/unset during the test. If that property is set in the JVM (e.g., via build/test runner options), `DriverFinder` will skip Selenium Manager, and the tests will not assert the intended argument list.

### Issue Context
`DriverFinder#getBinaryPaths()` checks `System.getProperty(service.getDriverProperty())` before calling `seleniumManager.getBinaryPaths(arguments)`.

### Fix Focus Areas
- java/test/org/openqa/selenium/remote/service/DriverFinderTest.java[212-240]
- java/src/org/openqa/selenium/remote/service/DriverFinder.java[93-105]

### Proposed fix
In the relevant tests (including the new helper), explicitly clear the property before exercising `DriverFinder`, and optionally restore it after:
- `System.clearProperty("property.selenium.manager.empty");`

Alternatively, use a unique per-test property key (e.g., include the test method name) and clear it defensively.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-code-review]

Persistent review updated to latest commit b05d10a

[qodo-code-review]

Code review by qodo was updated up to the latest commit 837e941

[qodo-code-review]

Code review by qodo was updated up to the latest commit 30f8e00

[qodo-code-review]

Code review by qodo was updated up to the latest commit cd9c553

[qodo-code-review]

Code review by qodo was updated up to the latest commit 5abb041

[qodo-code-review]

Code review by qodo was updated up to the latest commit cf8b797

[diemol]

@bhecquet can you please format the code with ./go format first?

[qodo-code-review]

Code review by qodo was updated up to the latest commit e38b35b

[bhecquet]

Hello @diemol

formatting done

[qodo-code-review]

Code review by qodo was updated up to the latest commit d2fd544