[refactor] replace JUnit assertions by AssertJ

[asolntsev]

User description

in many cases, it produces a better error message in case of test failure.

🔄 Types of changes

• Cleanup (formatting, renaming)

---

PR Type

Tests

---

Description

• Replace JUnit assertions with AssertJ for better error messages

• Update 30+ test files to use AssertJ fluent API

• Add hasExtension() method to DownloadedFile class

• Refactor test code for improved readability and maintainability

---

Diagram Walkthrough

flowchart LR
  JUnit["JUnit Assertions<br/>assertEquals, assertTrue, etc."]
  AssertJ["AssertJ Assertions<br/>assertThat, assertThatThrownBy"]
  Tests["30+ Test Files"]
  DownloadedFile["DownloadedFile Class"]
  
  JUnit -- "replaced by" --> AssertJ
  Tests -- "updated to use" --> AssertJ
  DownloadedFile -- "enhanced with" --> HasExtension["hasExtension method"]

Loading

File Walkthrough

	Relevant files
Tests	46 files NewSessionPayloadTest.java Replace JUnit assertions with AssertJ                                        +26/-26  RemoteWebDriverDownloadTest.java Refactor assertions and extract helper methods                      +49/-62  AnnotatedConfigTest.java Replace JUnit assertions with AssertJ                                        +28/-32  DefaultWheelTest.java Replace JUnit assertions with AssertJ                                        +36/-35  RouterTest.java Replace JUnit assertions with AssertJ                                        +8/-18    DefaultWheelTest.java Replace JUnit assertions with AssertJ                                        +31/-28  RetryRequestTest.java Replace JUnit assertions with AssertJ                                        +22/-20  AddingNodesTest.java Replace JUnit assertions with AssertJ                                        +11/-27  NativeHttpClientMethodsTest.java Replace JUnit assertions with AssertJ                                        +17/-17  DistributedTest.java Replace JUnit assertions with AssertJ                                        +16/-24  NettyServerTest.java Replace JUnit assertions with AssertJ                                        +23/-26  FederatedCredentialManagementTest.java Replace JUnit assertions with AssertJ                                        +15/-19  IgnoreComparatorUnitTest.java Replace JUnit assertions with AssertJ                                        +27/-30  AppServerTestBase.java Replace JUnit assertions with AssertJ                                        +18/-16  VirtualAuthenticatorTest.java Replace JUnit assertions with AssertJ                                        +27/-24  ConfigTest.java Replace JUnit assertions with AssertJ                                        +8/-10    JdbcBackedSessionMapTest.java Replace JUnit assertions with AssertJ                                        +17/-23  RedisBackedSessionMapTest.java Replace JUnit assertions with AssertJ                                        +10/-14  CookieImplementationTest.java Replace JUnit assertions with AssertJ                                        +15/-9    EndToEndTest.java Replace JUnit assertions with AssertJ                                        +6/-11    SessionSchedulingTest.java Replace JUnit assertions with AssertJ                                        +4/-8      StressTest.java Replace JUnit assertions with AssertJ                                        +16/-21  NodeOptionsTest.java Replace JUnit assertions with AssertJ                                        +3/-5      TomlConfigTest.java Replace JUnit assertions with AssertJ                                        +4/-4      ActionDurationTest.java Replace JUnit assertions with AssertJ                                        +7/-8      JsonConfigTest.java Replace JUnit assertions with AssertJ                                        +4/-4      MapConfigTest.java Replace JUnit assertions with AssertJ                                        +4/-4      CdpVersionFinderTest.java Replace JUnit assertions with AssertJ                                        +5/-5      OverallGridTest.java Replace JUnit assertions with AssertJ                                        +4/-6      ForwardWebDriverCommandTest.java Replace JUnit assertions with AssertJ                                        +6/-5      WebSocketServingTest.java Replace JUnit assertions with AssertJ                                        +10/-8    RelaySessionFactoryTest.java Replace JUnit assertions with AssertJ                                        +5/-5      DistributorDrainingTest.java Replace JUnit assertions with AssertJ                                        +2/-7      Build.java Replace JUnit fail with AssertionError                                      +2/-3      DownloadedFileTest.java Add new test for DownloadedFile.hasExtension                          +41/-0    ReverseProxyHandlerTest.java Replace JUnit assertions with AssertJ                                        +2/-2      ChromeOptionsTest.java Replace JUnit assertions with AssertJ                                        +1/-2      BaseServerOptionsTest.java Replace JUnit assertions with AssertJ                                        +4/-5      FirefoxDriverBuilderTest.java Replace JUnit assertions with AssertJ                                        +1/-2      TeeReaderTest.java Replace JUnit assertions with AssertJ                                        +3/-3      TemporaryFilesystemTest.java Replace JUnit assertions with AssertJ                                        +1/-2      SessionMapTest.java Replace JUnit assertions with AssertJ                                        +1/-2      SessionCleanUpTest.java Replace JUnit assertions with AssertJ                                        +1/-1      NodeTest.java Replace JUnit assertions with AssertJ                                        +1/-1      LocalDistributorTest.java Replace JUnit assertions with AssertJ                                        +1/-1      LocalNodeTest.java Replace JUnit assertions with AssertJ                                        +1/-1
Enhancement	1 files HasDownloads.java Add hasExtension method to DownloadedFile                                +4/-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: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Await condition risk: The new polling helper waitForDownloadedFiles relies on getDownloadedFiles() and a fixed 5-second timeout, which may introduce flakiness or unhandled edge cases depending on runtime behavior not visible in this diff. Referred Code /** ensure we hit no temporary file created by the browser while downloading */ private void waitForDownloadedFiles(WebDriver driver, int expectedFilesCount) { HasDownloads hasDownloads = (HasDownloads) driver; new WebDriverWait(driver, ofSeconds(5)) .until( __ -> { long actualFilesCount = hasDownloads.getDownloadedFiles().stream() .filter((f) -> FILE_EXTENSIONS.stream().anyMatch(f::hasExtension)) .count(); return actualFilesCount == expectedFilesCount; }); } 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: Unreviewed new API: The PR description indicates a new hasExtension() method was added to DownloadedFile, but the implementation is not present in the provided diff, so secure data handling and input validation aspects cannot be verified here. Referred Code // Licensed to the Software Freedom Conservancy (SFC) under one // or more contributor license agreements. See the NOTICE file 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	Add null and empty checks Add null and empty string checks to the hasExtension method to prevent potential NullPointerException and handle edge cases correctly. java/src/org/openqa/selenium/HasDownloads.java [98-100] public boolean hasExtension(String extension) { + if (extension == null || extension.isEmpty()) { + return false; + } return extension.startsWith(".") ? name.endsWith(extension) : name.endsWith('.' + extension); } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential NullPointerException and other edge cases, proposing a change that makes the new hasExtension method more robust.	Medium
	Avoid using a potentially buggy method In RemoteWebDriverDownloadTest.java, modify the WebDriverWait condition to use the deprecated getDownloadableFiles() and String::endsWith instead of the new getDownloadedFiles() and f::hasExtension to make the test more robust. java/test/org/openqa/selenium/grid/router/RemoteWebDriverDownloadTest.java [186-194] new WebDriverWait(driver, ofSeconds(5)) .until( __ -> { + @SuppressWarnings("deprecation") long actualFilesCount = - hasDownloads.getDownloadedFiles().stream() - .filter((f) -> FILE_EXTENSIONS.stream().anyMatch(f::hasExtension)) + hasDownloads.getDownloadableFiles().stream() + .filter(f -> FILE_EXTENSIONS.stream().anyMatch(f::endsWith)) .count(); return actualFilesCount == expectedFilesCount; }); Apply / Chat Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that the test relies on a new, potentially buggy method (hasExtension) and proposes a more robust alternative using existing functionality, which makes the test less fragile.	Low
General	Use order-independent collection assertion In AnnotatedConfigTest.java, replace containsExactly with containsExactlyInAnyOrder for the assertion on values to make the test independent of element order, which is more appropriate for a Set. java/test/org/openqa/selenium/grid/config/AnnotatedConfigTest.java [77] -assertThat(values).containsExactly("cheddar", "gouda"); +assertThat(values).containsExactlyInAnyOrder("cheddar", "gouda"); Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion correctly points out that using an order-independent assertion is more robust for a test involving a Set, making the test less fragile to implementation details.	Low
Learned best practice	Prevent WebDriver leaks on failures Ensure the RemoteWebDriver is quit if augmentation fails (or any exception occurs during creation) by using a try/catch and cleaning up before rethrowing. java/test/org/openqa/selenium/grid/router/RemoteWebDriverDownloadTest.java [178-180] private WebDriver createWebdriver(Capabilities capabilities) { - return new Augmenter().augment(new RemoteWebDriver(server.getUrl(), capabilities)); + RemoteWebDriver raw = new RemoteWebDriver(server.getUrl(), capabilities); + try { + return new Augmenter().augment(raw); + } catch (RuntimeException e) { + raw.quit(); + throw e; + } } Apply / Chat Suggestion importance[1-10]: 5 __ Why: Relevant best practice - Always wrap creation of external contexts or resources in try/finally and perform explicit cleanup in finally to prevent leaks even when exceptions occur.	Low
Update

[cgoldberg]

@asolntsev We are getting some build failures in RBE CI that I think were caused by this PR. Can you take a look?

https://github.com/SeleniumHQ/selenium/actions/workflows/ci-rbe.yml?query=branch%3Atrunk

//java/test/org/openqa/selenium/environment:test-base-spotbugs  FAILED TO BUILD
//java/test/org/openqa/selenium/environment:webserver/NettyAppServerTest FAILED TO BUILD
//java/test/org/openqa/selenium/testing:IgnoreComparatorUnitTest FAILED TO BUILD
//java/test/org/openqa/selenium/testing:IgnoreComparatorUnitTest-spotbugs FAILED TO BUILD

I this in the RBE logs:

Execution result: https://gypsum.cluster.engflow.com/actions/executions/ChCiKHYqIFFcZpSpvzgVZmPKEgdkZWZhdWx0GiUKIF7M3sV-o628sOBBm_mbXyLUpozUkJ-xFsbCdMDu_gOUELcDjava/test/org/openqa/selenium/environment/webserver/AppServerTestBase.java:21: error: [strict] Using type org.assertj.core.api.Assertions from an indirect dependency (TOOL_INFO: "@maven//:org_assertj_assertj_core"). See command below **import static org.assertj.core.api.Assertions.assertThat;                                  ^ ** Please add the following dependencies:   @maven//:org_assertj_assertj_core to //java/test/org/openqa/selenium/environment:test-base  ** You can use the following buildozer command: buildozer 'add deps @maven//:org_assertj_assertj_core' //java/test/org/openqa/selenium/environment:test-base 

(16:53:25) ERROR: /home/runner/work/selenium/selenium/java/test/org/openqa/selenium/environment/BUILD.bazel:88:13: Building java/test/org/openqa/selenium/environment/libtest-base.jar (1 source file) failed: (Exit 1): java failed: error executing Javac command (from target //java/test/org/openqa/selenium/environment:test-base) 

[diemol]

The RBE tests on this PR failed. This shouldn't have been merged.

[asolntsev]

Sure, I will check the failure.
I saw a few failures today, but the error message was something like "Timeout". I couldn't find any reasonable error message, that's why I decided it was some flaky failure.

[titusfortner]

You can rerun the specific tests if you think there was a one off issue with a run, but even "just flaky" tests can't be merged until understood. We can help sort out weird bazel errors when they happen if it isn't obvious.

[asolntsev]

@asolntsev We are getting some build failures in RBE CI that I think were caused by this PR. Can you take a look?

Fixed in #16773

Thank you @titusfortner for fixing it.