[java] Fix failing java tests

[asolntsev]

🔗 Related Issues

💥 What does this PR do?

Fixes some failing Java tests

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

Review Summary by Qodo

Fix failing Java tests for Firefox browser compatibility

🐞 Bug fix 🧪 Tests

Walkthroughs

Description

• Enable BrowserCommandsTest for Firefox by removing @NotYetImplemented annotations
• Add Firefox-specific @Ignore annotation for alert-related test
• Skip SetNetworkConditionsTest for Firefox beta in test configuration
• Remove unused NotYetImplemented import from BrowserCommandsTest

File Changes

1. java/test/org/openqa/selenium/FormHandlingTest.java 🧪 Tests +1/-0

Add Firefox-specific alert test ignore annotation

• Added @Ignore annotation for Firefox to handleFormWithJavascriptAction test
• Reason: alerts are no longer triggered in Firefox

java/test/org/openqa/selenium/FormHandlingTest.java

---

2. java/test/org/openqa/selenium/bidi/browser/BrowserCommandsTest.java 🧪 Tests +0/-4

Enable Firefox support for download behavior tests

• Removed @NotYetImplemented(FIREFOX) annotations from three download behavior tests
• Removed unused NotYetImplemented import statement
• Tests now enabled for Firefox as setDownloadBehavior is implemented

java/test/org/openqa/selenium/bidi/browser/BrowserCommandsTest.java

---

3. .skipped-tests ⚙️ Configuration changes +1/-0

Skip SetNetworkConditionsTest for Firefox beta

• Added SetNetworkConditionsTest-firefox-beta to skipped tests list
• Reason: setNetworkConditions enabled in Firefox beta but not stable Firefox

.skipped-tests

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📎 Requirement gaps (0)

1. Firefox download behavior leaks ✗ 🐞 Bug ☼ Reliability

Description

BrowserCommandsTest now runs the download-behavior tests on Firefox (removed
@NotYetImplemented(FIREFOX)), but resetDownloadBehavior still skips Firefox, so a final
denied/allowed download state can persist into later tests that reuse the same WebDriver instance.

Code

java/test/org/openqa/selenium/bidi/browser/BrowserCommandsTest.java[R147-149]

  @Test
  @NeedsFreshDriver
-  @NotYetImplemented(FIREFOX)
  void canSetDownloadBehaviorDenied() throws InterruptedException {

Evidence

The tests set global download behavior (allowed/denied). Cleanup intentionally does not reset
download behavior for Firefox, and the test infrastructure reuses a thread-local driver across
tests/classes unless removed, so the last download behavior set during this class can affect
subsequent tests in the same JVM run on Firefox.

java/test/org/openqa/selenium/bidi/browser/BrowserCommandsTest.java[62-67]
java/test/org/openqa/selenium/bidi/browser/BrowserCommandsTest.java[133-164]
java/test/org/openqa/selenium/testing/JupiterTestBase.java[66-88]

Agent prompt

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

### Issue description
`BrowserCommandsTest` now enables the download-behavior tests on Firefox (by removing `@NotYetImplemented(FIREFOX)`), but the `@AfterEach resetDownloadBehavior` hook still skips Firefox. This can leak a global download behavior (e.g., `denied()`) into later tests because the Selenium JUnit extension reuses the same thread-local WebDriver across test classes unless explicitly removed.

### Issue Context
The test methods call `browser.setDownloadBehavior(...)` and may leave the final state as `denied()` on Firefox; currently the cleanup is guarded by `if (detect() != FIREFOX)`.

### Fix Focus Areas
- java/test/org/openqa/selenium/bidi/browser/BrowserCommandsTest.java[62-67]
- java/test/org/openqa/selenium/bidi/browser/BrowserCommandsTest.java[133-164]

### Suggested fix approach
- Remove the Firefox guard and reset download behavior for Firefox as well (since these tests now rely on `setDownloadBehavior` working on Firefox), **or**
- If Firefox still cannot accept the reset call reliably, explicitly `removeDriver()` for Firefox in an `@AfterEach`/`@AfterAll` cleanup to prevent state reuse.

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

---

2. canSetDownloadBehaviorWithUserContext ungated 📘 Rule violation ☼ Reliability

Description

The PR removes the Firefox-specific gating (@NotYetImplemented(FIREFOX)) so this test now runs
unconditionally, risking CI failures when the feature is not supported by the Firefox driver in some
environments. This violates the requirement to gate integration tests by feature/driver support.

Code

java/test/org/openqa/selenium/bidi/browser/BrowserCommandsTest.java[R166-170]

  @Test
  @NeedsFreshDriver
-  @NotYetImplemented(FIREFOX)
  void canSetDownloadBehaviorWithUserContext() throws InterruptedException {
    String userContext = browser.createUserContext();

Evidence

PR Compliance ID 12 requires gating integration tests based on feature/driver support; the updated
code shows this Firefox gating has been removed while the class still handles Firefox as a special
case elsewhere.

java/test/org/openqa/selenium/bidi/browser/BrowserCommandsTest.java[62-67]
java/test/org/openqa/selenium/bidi/browser/BrowserCommandsTest.java[166-208]
Best Practice: Learned patterns

Agent prompt

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

## Issue description
`canSetDownloadBehaviorWithUserContext()` now runs on Firefox after removing `@NotYetImplemented(FIREFOX)`; if Firefox support is incomplete in some CI environments, this can cause unstable failures.

## Issue Context
Compliance requires integration tests be gated/skipped when the driver/environment lacks feature support.

## Fix Focus Areas
- java/test/org/openqa/selenium/bidi/browser/BrowserCommandsTest.java[62-67]
- java/test/org/openqa/selenium/bidi/browser/BrowserCommandsTest.java[166-208]

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

---

3. Remote variant not skipped ☑ 🐞 Bug ≡ Correctness

Description

.skipped-tests excludes SetNetworkConditionsTest-firefox-beta, but the Bazel macros generate an
additional SetNetworkConditionsTest-firefox-beta-remote target for selenium-remote suites, which
will still be included under //... in CI.

Code

.skipped-tests[27]

+-//java/test/org/openqa/selenium/bidi/emulation:SetNetworkConditionsTest-firefox-beta

Evidence

GitHub Actions CI runs bazel test ... //... -- <negative patterns from .skipped-tests>, and
selenium_test.bzl generates a *-remote test target when the BUILD tags include
selenium-remote. The emulation suite is tagged selenium-remote, so skipping only the non-remote
target leaves the generated *-remote target unexcluded.

.skipped-tests[18-28]
scripts/github-actions/ci-build.sh[18-25]
java/test/org/openqa/selenium/bidi/emulation/BUILD.bazel[4-12]
java/private/selenium_test.bzl[89-141]

Agent prompt

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

### Issue description
`.skipped-tests` currently excludes only `SetNetworkConditionsTest-firefox-beta`, but Bazel generates a corresponding `SetNetworkConditionsTest-firefox-beta-remote` target for suites tagged `selenium-remote`. Since CI runs `//...` without tag filters, the `-remote` variant can still run.

### Issue Context
The emulation test suite BUILD file includes `tags = ["selenium-remote"]`, and `selenium_test.bzl` appends a `-remote` test per browser when that tag is present.

### Fix Focus Areas
- .skipped-tests[24-28]
- java/test/org/openqa/selenium/bidi/emulation/BUILD.bazel[4-12]
- java/private/selenium_test.bzl[89-141]

### Suggested fix approach
Add the missing negative target pattern:
- `-//java/test/org/openqa/selenium/bidi/emulation:SetNetworkConditionsTest-firefox-beta-remote`
so both the local and remote variants are excluded consistently (similar to how other firefox-beta entries include both forms).

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

---

ⓘ The new review experience is currently in Beta. Learn more

[qodo-code-review]

Review Summary by Qodo

Fix failing Java tests for Firefox and improve test assertions

🐞 Bug fix 🧪 Tests

Walkthroughs

Description

• Enable BrowserCommandsTest for Firefox by removing @NotYetImplemented annotations
• Fix alert handling in Firefox by setting IGNORE mode as default capability
• Improve WebScriptTest assertions using log collection instead of latches
• Add SetNetworkConditionsTest to skipped tests for Firefox beta
• Refactor DOM mutation and script pinning tests for better clarity

File Changes

1. java/test/org/openqa/selenium/UnexpectedAlertBehaviorTest.java 🐞 Bug fix +4/-1

Add Firefox alert handling test skip

• Added DISMISS_AND_NOTIFY import and used it in test
• Added @Ignore annotation for Firefox in canDismissUnhandledAlertsByDefault test
• Firefox now sets IGNORE mode by default, causing test to fail without this annotation

java/test/org/openqa/selenium/UnexpectedAlertBehaviorTest.java

---

2. java/test/org/openqa/selenium/WebScriptTest.java 🧪 Tests +65/-51

Improve test assertions using log collection

• Refactored canAddDomMutationHandler to use CopyOnWriteArrayList instead of AtomicReference and
 CountDownLatch
• Refactored canRemoveDomMutationHandler with improved assertions and test flow
• Improved canUnpinScript with log collection and timestamp formatting for better debugging
• Extracted triggerDomMutation and mutationHandler helper methods for code reuse

java/test/org/openqa/selenium/WebScriptTest.java

---

3. java/test/org/openqa/selenium/bidi/browser/BrowserCommandsTest.java 🐞 Bug fix +1/-9

Enable download behavior tests for Firefox

• Removed @NotYetImplemented(FIREFOX) annotations from canSetDownloadBehaviorAllowed,
 canSetDownloadBehaviorDenied, and canSetDownloadBehaviorWithUserContext tests
• Removed Firefox-specific conditional logic in resetDownloadBehavior method
• Removed unused imports for FIREFOX and detect()
• setDownloadBehavior is now supported in Firefox

java/test/org/openqa/selenium/bidi/browser/BrowserCommandsTest.java

---

View more (2)

4. java/test/org/openqa/selenium/testing/drivers/Browser.java 🐞 Bug fix +2/-0

Set Firefox alert handling default capability

• Added IGNORE import from UnexpectedAlertBehaviour
• Set UNHANDLED_PROMPT_BEHAVIOUR capability to IGNORE for Firefox options
• Ensures Firefox uses IGNORE mode by default for alert handling

java/test/org/openqa/selenium/testing/drivers/Browser.java

---

5. .skipped-tests ⚙️ Configuration changes +2/-0

Skip network conditions tests for Firefox beta

• Added SetNetworkConditionsTest-firefox-beta to skipped tests
• Added SetNetworkConditionsTest-firefox-beta-remote to skipped tests
• setNetworkConditions feature is enabled in Firefox beta but not in stable Firefox

.skipped-tests

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. logs ArrayList used concurrently ☑ 📘 Rule violation ☼ Reliability

Description

A non-thread-safe ArrayList is mutated from an async console log handler while being read/cleared
on the test thread, which can cause data races and flaky or failing tests. This violates the
requirement to ensure concurrent code is race-safe with correct synchronization/visibility.

Code

java/test/org/openqa/selenium/WebScriptTest.java[R260-292]

+    List<String> logs = new ArrayList<>();
+    CountDownLatch latch = new CountDownLatch(1);

-    String pinnedScript =
-        ((RemoteWebDriver) driver).script().pin("() => { console.log('Hello!'); }");
+    Script script = ((RemoteWebDriver) driver).script();
+    String pinnedScript = script.pin("() => { console.log('Hello!'); }");

-    long id =
-        ((RemoteWebDriver) driver)
-            .script()
-            .addConsoleMessageHandler(consoleLogEntry -> latch.countDown());
+    DateTimeFormatter formatter =
+        DateTimeFormatter.ofPattern("HH:mm:ss:SSS").withZone(systemDefault());

-    page = appServer.whereIs("/bidi/logEntryAdded.html");
+    long id =
+        script.addConsoleMessageHandler(
+            log -> {
+              String time = formatter.format(ofEpochMilli(log.getTimestamp()));
+              String message = String.format("%s %s", log.getText(), time);
+              logs.add(message);
+              latch.countDown();
+            });

-    driver.get(page);
+    try {
+      page = appServer.whereIs("/bidi/logEntryAdded.html");
+      assertThat(logs).hasSize(0);

-    ((RemoteWebDriver) driver).script().unpin(pinnedScript);
+      driver.get(page);
+      assertThat(latch.await(10, SECONDS)).isTrue();

-    driver.get(page);
+      assertThat(logs).as("Chrome logs once, FireFox logs twice").isNotEmpty();
+      assertThat(logs.get(0)).startsWith("Hello!");

-    assertThat(latch.getCount()).isEqualTo(1L);
+      script.unpin(pinnedScript);

-    ((RemoteWebDriver) driver).script().removeConsoleMessageHandler(id);
+      logs.clear();
+      driver.get(page);
+      assertThat(logs).as("Script has been unpinned, no logs anymore.").isEmpty();

Evidence

PR Compliance ID 10 requires correct synchronization for shared mutable state. In canUnpinScript,
logs is a plain ArrayList that is appended to inside addConsoleMessageHandler(...) (likely on
a different thread) while the main test thread asserts on and clears logs, creating an unsafe
concurrent access pattern.

java/test/org/openqa/selenium/WebScriptTest.java[260-292]
Best Practice: Learned patterns

Agent prompt

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

## Issue description
`canUnpinScript()` uses a non-thread-safe `ArrayList` (`logs`) that is mutated by an async console log handler while also being read/cleared by the test thread, creating a data race and potential flakiness.

## Issue Context
The console message handler registered via `script.addConsoleMessageHandler(...)` can be invoked asynchronously. The test concurrently performs `assertThat(logs)...` and `logs.clear()` while the handler may still be appending.

## Fix Focus Areas
- java/test/org/openqa/selenium/WebScriptTest.java[260-292]

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

---

2. Mutation removal assertion racy 🐞 Bug ☼ Reliability

Description

WebScriptTest.canRemoveDomMutationHandler asserts mutations is empty immediately after triggering
a DOM mutation, but the mutation event is delivered asynchronously and may arrive after the
assertion, turning the test into a false positive. This can also produce intermittent failures if a
late event sneaks in before the assertion in some runs.

Code

java/test/org/openqa/selenium/WebScriptTest.java[R205-220]

+  void canRemoveDomMutationHandler() {
+    List<String> mutations = new CopyOnWriteArrayList<>();
+    Script script = ((RemoteWebDriver) driver).script();
+    long id = script.addDomMutationHandler(mutationHandler(mutations));

    driver.get(pages.dynamicPage);
+    triggerDomMutation();
+    assertThat(mutations).isNotEmpty();
+
+    script.removeDomMutationHandler(id);

-    ((RemoteWebDriver) driver).script().removeDomMutationHandler(id);
+    mutations.clear();
+    driver.get(pages.dynamicPage);
+    triggerDomMutation();
+    assertThat(mutations).isEmpty();
+  }

Evidence

The test removes the handler, clears the shared list, triggers a mutation, and immediately asserts
the list is empty. However, BiDi event dispatch runs on a background executor and calls consumers
asynchronously, so the list can remain empty at assertion time even when an event will be delivered
shortly thereafter.

java/test/org/openqa/selenium/WebScriptTest.java[203-220]
java/src/org/openqa/selenium/bidi/Connection.java[263-275]
java/src/org/openqa/selenium/bidi/Connection.java[336-384]
java/src/org/openqa/selenium/remote/RemoteScript.java[84-121]

Agent prompt

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

### Issue description
`canRemoveDomMutationHandler` currently checks `mutations.isEmpty()` immediately after triggering a mutation, but mutation events are delivered asynchronously, so the test can pass before the event arrives.

### Issue Context
BiDi events are dispatched on a cached thread pool (`Connection.EXECUTOR`), and listeners are invoked asynchronously.

### Fix Focus Areas
- java/test/org/openqa/selenium/WebScriptTest.java[203-220]
- java/src/org/openqa/selenium/bidi/Connection.java[263-275]

### Concrete fix approach
- After removing the handler, validate absence using a negative wait window:
 - add a `CountDownLatch gotMutationAfterRemove = new CountDownLatch(1)`
 - use a handler that signals the latch only for the "after remove" phase (e.g., an `AtomicBoolean removedPhase`)
 - after `triggerDomMutation()`, assert `assertThat(gotMutationAfterRemove.await(1, SECONDS)).isFalse()`
- Alternatively, keep `mutations` but wait briefly and assert it stays empty (less precise, but avoids false positives).

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

---

3. Teardown can fail on unsupported 🐞 Bug ☼ Reliability

Description

BrowserCommandsTest.resetDownloadBehavior unconditionally sends browser.setDownloadBehavior with
downloadBehavior: null in @AfterEach; if any environment/browser rejects the command or null reset
semantics, teardown will throw and fail/obscure test results. This is especially risky because the
PR also removed Firefox-specific guarding and NotYetImplemented(FIREFOX) annotations.

Code

java/test/org/openqa/selenium/bidi/browser/BrowserCommandsTest.java[R61-63]

  final void resetDownloadBehavior() {
-    if (detect() != FIREFOX) {
-      browser.setDownloadBehavior(new SetDownloadBehaviorParameters(null));
-    }
+    browser.setDownloadBehavior(new SetDownloadBehaviorParameters(null));
  }

Evidence

The teardown always calls browser.setDownloadBehavior(new SetDownloadBehaviorParameters(null)).
The Browser module always sends the BiDi command directly, and the parameter object serializes null
as {downloadBehavior: null}; there is no try/catch, so any BiDiException/WebDriverException in
teardown will fail the test regardless of the test’s own assertions.

java/test/org/openqa/selenium/bidi/browser/BrowserCommandsTest.java[60-63]
java/src/org/openqa/selenium/bidi/module/Browser.java[71-77]
java/src/org/openqa/selenium/bidi/browser/SetDownloadBehaviorParameters.java[48-63]

Agent prompt

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

### Issue description
`BrowserCommandsTest.resetDownloadBehavior` can fail teardown by unconditionally sending `browser.setDownloadBehavior` with a `null` downloadBehavior payload.

### Issue Context
The BiDi Browser module does not guard or swallow errors for `browser.setDownloadBehavior`, so any unsupported implementation will throw and break teardown.

### Fix Focus Areas
- java/test/org/openqa/selenium/bidi/browser/BrowserCommandsTest.java[60-63]
- java/src/org/openqa/selenium/bidi/module/Browser.java[71-77]
- java/src/org/openqa/selenium/bidi/browser/SetDownloadBehaviorParameters.java[48-63]

### Concrete fix approach
- Wrap the teardown call in a narrow try/catch (e.g., `BiDiException` / `WebDriverException`) and log/ignore failures in teardown, **or**
- Reintroduce capability-based/driver-based guarding (e.g., only reset if the test successfully set download behavior, or if the current browser is known to support it).

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

---

4. Firefox prompt default overridden ☑ 🐞 Bug ⚙ Maintainability

Description

Browser.FIREFOX globally forces unhandledPromptBehavior=ignore for every Firefox test session,
which changes baseline behavior and causes default-alert-behavior coverage to be skipped/altered.
This can mask regressions in Firefox’s actual default unhandled-prompt behavior.

Code

java/test/org/openqa/selenium/testing/drivers/Browser.java[R138-140]

      FirefoxOptions options = new FirefoxOptions().configureFromEnv();
+      options.setCapability(UNHANDLED_PROMPT_BEHAVIOUR, IGNORE);

Evidence

Browser.FIREFOX.getCapabilities sets the capability, and WebDriverBuilder merges
Browser.detect().getCapabilities into every created driver’s capabilities (other capabilities only
override if they explicitly set the same key). UnexpectedAlertBehaviorTest now explicitly ignores
FIREFOX for the "by default" scenario because the test harness no longer represents browser defaults
for Firefox.

java/test/org/openqa/selenium/testing/drivers/Browser.java[135-154]
java/test/org/openqa/selenium/testing/drivers/WebDriverBuilder.java[63-67]
java/src/org/openqa/selenium/Capabilities.java[98-104]
java/test/org/openqa/selenium/UnexpectedAlertBehaviorTest.java[74-81]

Agent prompt

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

### Issue description
Firefox sessions created by the test harness always set `unhandledPromptBehavior=ignore`, which changes baseline behavior for unrelated tests and reduces coverage for "default" unexpected-alert behavior.

### Issue Context
`WebDriverBuilder` merges `Browser.detect().getCapabilities()` into all sessions.

### Fix Focus Areas
- java/test/org/openqa/selenium/testing/drivers/Browser.java[135-154]
- java/test/org/openqa/selenium/testing/drivers/WebDriverBuilder.java[63-67]
- java/test/org/openqa/selenium/UnexpectedAlertBehaviorTest.java[74-81]

### Concrete fix approach
- Gate the Firefox `UNHANDLED_PROMPT_BEHAVIOUR` override behind a system property (so CI can enable it without hardcoding it for all runs), or
- Apply it only for the specific tests/suites that need it by setting capabilities explicitly in those tests.

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

---

5. New skipped tests lack context 🐞 Bug ⚙ Maintainability

Description

Two SetNetworkConditionsTest targets were added to .skipped-tests for firefox-beta runs, reducing
coverage without an accompanying reason or tracking reference. This makes it harder to know when/why
to re-enable them.

Code

.skipped-tests[R27-28]

+-//java/test/org/openqa/selenium/bidi/emulation:SetNetworkConditionsTest-firefox-beta
+-//java/test/org/openqa/selenium/bidi/emulation:SetNetworkConditionsTest-firefox-beta-remote

Evidence

.skipped-tests is used to disable entire test targets; the PR adds two new entries for firefox-beta
and firefox-beta-remote.

.skipped-tests[24-29]

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 skipped test targets were added without any reference to an issue/bug explaining the skip.

### Issue Context
Skipped tests tend to become permanent unless they carry a reason/owner.

### Fix Focus Areas
- .skipped-tests[24-29]

### Concrete fix approach
- Add an inline comment or structured reference (issue URL/ID) near the new entries indicating why they are skipped and what condition should re-enable them (if the file format allows comments), or track the rationale in the PR description and link it from the skip list if comments are not supported.

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

---

ⓘ The new review experience is currently in Beta. Learn more

[cgoldberg]

@asolntsev thanks... looks like everything Java related is passing in RBE