[java] Fix flaky tests

[asolntsev]

User description

💥 What does this PR do?

Fixes few Java tests

🔧 Implementation Notes

See every commit separately

🔄 Types of changes

• Bug fix (backwards compatible)

---

PR Type

Bug fix

---

Description

• Prevent NPE in NettyServer.stop() when called on already stopped server

• Fix appServer reference after test environment restart with secure mode

• Ensure browser cleanup in BiDiSessionCleanUpTest using @AfterEach

• Refactor test setup to use field initialization instead of manual setup

---

Diagram Walkthrough

flowchart LR
  A["NettyServer.stop()"] -->|"add isStarted check"| B["Prevent NPE on double stop"]
  C["JupiterTestBase"] -->|"update appServer reference"| D["Fix secure env restart"]
  E["BiDiSessionCleanUpTest"] -->|"add @AfterEach cleanup"| F["Ensure browser closure"]
  E -->|"field initialization"| G["Simplify test setup"]

Loading

File Walkthrough

Relevant files

Bug fix

NettyServer.java Prevent NPE when stopping already stopped server                  java/src/org/openqa/selenium/netty/server/NettyServer.java Add isStarted() check at beginning of stop() method to prevent NPE Return early if server is not running to avoid null pointer on this.channel +4/-0      BiDiSessionCleanUpTest.java Add guaranteed browser cleanup with @AfterEach                      java/test/org/openqa/selenium/bidi/BiDiSessionCleanUpTest.java Add @AfterEach method to ensure browser cleanup even if test fails Initialize driver as field with BiDi-enabled FirefoxOptions Remove manual driver.quit() calls from test methods Catch NoSuchSessionException in cleanup for already-closed browsers Make class final and reorganize imports +16/-18  JupiterTestBase.java Update appServer reference after environment restart          java/test/org/openqa/selenium/testing/JupiterTestBase.java Update appServer reference after restarting test environment in secure mode Fixes issue where appServer was still pointing to previous non-secure environment Prevents DNS resolution errors when accessing secure URLs +1/-0

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Swallowed exception: The new cleanup method silently swallows NoSuchSessionException during driver.quit(), which can hide unexpected session-loss failures and reduce diagnosability. Referred Code @AfterEach void closeBrowser() { try { driver.quit(); } catch (NoSuchSessionException ignore) { // browser already closed by test } Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Synchronize method to prevent race conditions Add the synchronized keyword to the stop() method signature and declare the channel field as volatile to prevent race conditions and ensure thread safety during server shutdown. java/src/org/openqa/selenium/netty/server/NettyServer.java [129-149] @Override -public void stop() { +public synchronized void stop() { if (!isStarted()) { return; } try { Future<?> bossShutdown = bossGroup.shutdownGracefully(); Future<?> workerShutdown = workerGroup.shutdownGracefully(); bossShutdown.sync(); workerShutdown.sync(); channel.closeFuture().sync(); } catch (InterruptedException e) { Thread.currentThread().interrupt(); throw new UncheckedIOException(new IOException("Shutdown interrupted", e)); } finally { channel = null; } } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential race condition in the stop() method and proposes a valid solution by adding synchronized. This enhances thread safety beyond the PR's initial fix, making the server shutdown process more robust in a concurrent environment.	Medium
	Check for active channel In the stop() method, enhance the initial check to also verify that the channel is active (channel.isActive()) before attempting shutdown, preventing redundant operations on an already closed channel. java/src/org/openqa/selenium/netty/server/NettyServer.java [131-133] -if (!isStarted()) { +if (channel == null || !channel.isActive()) { return; } Apply / Chat Suggestion importance[1-10]: 6 __ Why: This suggestion improves upon the PR's change by adding a check for channel.isActive(). This makes the shutdown logic more robust by preventing attempts to close a channel that is already inactive, which is a good practice for Netty applications.	Low
Learned best practice	Move resource creation into setup Avoid creating the browser in a field initializer (which can fail before JUnit runs @AfterEach); create it in @BeforeEach and keep @AfterEach null-safe so cleanup runs reliably. java/test/org/openqa/selenium/bidi/BiDiSessionCleanUpTest.java [34-43] -private final FirefoxDriver driver = - new FirefoxDriver(((FirefoxOptions) Browser.FIREFOX.getCapabilities()).enableBiDi()); +private FirefoxDriver driver; + +@BeforeEach +void startBrowser() { + driver = new FirefoxDriver(((FirefoxOptions) Browser.FIREFOX.getCapabilities()).enableBiDi()); +} @AfterEach void closeBrowser() { + if (driver == null) { + return; + } try { driver.quit(); } catch (NoSuchSessionException ignore) { // browser already closed by test } } Apply / Chat Suggestion importance[1-10]: 5 __ Why: Relevant best practice - Ensure external resources created by tests are always cleaned up by structuring creation/cleanup so failures cannot bypass teardown.	Low
General	Nullify thread groups after shutdown In the finally block of the stop() method, set bossGroup and workerGroup to null after shutdown to help prevent potential resource leaks if the server is reused. java/src/org/openqa/selenium/netty/server/NettyServer.java [135-148] try { Future<?> bossShutdown = bossGroup.shutdownGracefully(); Future<?> workerShutdown = workerGroup.shutdownGracefully(); bossShutdown.sync(); workerShutdown.sync(); channel.closeFuture().sync(); } catch (InterruptedException e) { Thread.currentThread().interrupt(); throw new UncheckedIOException(new IOException("Shutdown interrupted", e)); } finally { channel = null; + bossGroup = null; + workerGroup = null; } Apply / Chat Suggestion importance[1-10]: 4 __ Why: The suggestion to nullify bossGroup and workerGroup in the finally block is a good practice for cleanup. However, since the NettyServer instance is typically discarded after stop(), the practical impact of this change on preventing resource leaks is minor.	Low
Update