Skip to content

Conversation

@asolntsev
Copy link
Contributor

@asolntsev asolntsev commented Dec 9, 2025

User description

In case of test failure, it's important that the error message shows all the needed information: what was the actual/expected value etc. This commit makes many asserts show this info.

For example,

  • assertThat(a == b).isTrue() does NOT show what were a and b values;
  • assertThat(list.contain("foo")).isTrue() does NOT show what was the list value;
  • assertThat(model == 32 || model == 64).isTrue() does NOT show what was model value

etc.

💥 What does this PR do?

Replaces some AssertJ asserts by similar, but better asserts.

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Tests, Enhancement


Description

  • Replace weak AssertJ assertions with stronger alternatives for better error messages

  • Use hasValue(), isEmpty(), isPresent() instead of .isPresent().isTrue()

  • Apply asInstanceOf() with type factories for type-safe assertions

  • Simplify test code with containsExactly(), containsEntry(), anyMatch()

  • Add equals(), hashCode(), toString() methods to value objects


Diagram Walkthrough

flowchart LR
  A["Weak Assertions<br/>assertThat.isTrue()"] -->|Replace| B["Strong Assertions<br/>hasValue/isEmpty"]
  C["Type Casts<br/>Manual casting"] -->|Replace| D["Type-Safe<br/>asInstanceOf()"]
  E["Verbose Checks<br/>Multiple asserts"] -->|Simplify| F["Fluent Chains<br/>Single assert"]
  G["Value Objects<br/>No equality"] -->|Add| H["equals/hashCode<br/>toString"]
Loading

File Walkthrough

Relevant files
Tests
41 files
ScriptCommandsTest.java
Replace Optional assertions with hasValue/isEmpty               
+45/-47 
CallFunctionParameterTest.java
Improve Optional and List assertions with better methods 
+42/-38 
ByAllTest.java
Simplify list assertions and use List.of() factory             
+21/-70 
LocalValueTest.java
Replace Optional checks with hasValue() assertions             
+17/-22 
WebElementToJsonConverterTest.java
Use asInstanceOf() for type-safe collection assertions     
+25/-45 
ExecutingAsyncJavascriptTest.java
Replace type casts with asInstanceOf() and improve assertions
+35/-37 
EvaluateParametersTest.java
Replace Optional assertions with hasValue() method             
+11/-14 
SetGeolocationOverrideTest.java
Improve Map assertions with containsKey/doesNotContainKey
+30/-29 
RouteTest.java
Use static import for assertThat instead of Assertions     
+9/-10   
HttpMessageTest.java
Replace collection contains checks with direct assertions
+13/-13 
StorageCommandsTest.java
Simplify cookie assertions and extract helper method         
+16/-18 
LocateNodesTest.java
Replace Optional checks with isPresent and hasValue           
+5/-7     
RemoteWebDriverBuilderTest.java
Replace AtomicBoolean.get() assertions with direct checks
+5/-5     
PrintOptionsTest.java
Use asInstanceOf() for Map type assertions                             
+11/-13 
LogInspectorTest.java
Replace Optional.isPresent().isTrue() with isPresent()     
+3/-3     
AvailableLogsTest.java
Replace set contains checks with direct assertions             
+11/-17 
SetScriptingEnabledTest.java
Refactor test helper to use Optional.map() and isEmpty() 
+7/-7     
JsonTest.java
Simplify Map assertions with containsEntry()                         
+3/-5     
CombinedInputActionsTest.java
Replace fuzzy position matching with isCloseTo() offset   
+3/-7     
TakesScreenshotTest.java
Use assumeThat and improve numeric assertions                       
+4/-4     
CombinedInputActionsTest.java
Replace fuzzy matching with isCloseTo() assertions             
+3/-1     
FilterTest.java
Replace AtomicBoolean.get() with direct assertions             
+3/-4     
TemporaryFilesystemTest.java
Improve File assertions with isDirectory() and startsWith()
+11/-11 
FormHandlingTest.java
Replace string endsWith check with assertion method           
+2/-2     
FirefoxProfileTest.java
Replace stream anyMatch check with assertion method           
+2/-5     
ArchitectureTest.java
Improve numeric assertions with isIn() method                       
+3/-3     
PrintPageTest.java
Replace contains check with assertion method                         
+3/-3     
CookieImplementationTest.java
Simplify cookie set assertions with contains()                     
+3/-5     
ErrorHandlerTest.java
Replace type check with isInstanceOf() assertion                 
+2/-1     
BrowsingContextTest.java
Rename helper method for clarity and improve assertions   
+3/-3     
JsonOutputTest.java
Replace string startsWith/endsWith checks with assertions
+2/-2     
RemoteFirefoxDriverTest.java
Chain File assertions with exists() and isNotEmpty()         
+1/-2     
TakesFullPageScreenshotTest.java
Replace numeric comparisons with isGreaterThan()                 
+2/-2     
OutputTypeTest.java
Replace File.exists() check with assertion method               
+1/-1     
NetworkInterceptorRestTest.java
Replace AtomicBoolean.get() and improve string assertions
+2/-2     
W3CHttpResponseCodecTest.java
Add suppression annotation for removal warnings                   
+1/-0     
WebNetworkTest.java
Replace contains check with assertion method                         
+1/-1     
ElementDomAttributeTest.java
Use containsIgnoringCase() for style attribute check         
+1/-1     
ElementAttributeTest.java
Use containsIgnoringCase() for style attribute check         
+1/-1     
NetworkInterceptorTest.java
Replace AtomicBoolean.get() with direct assertion               
+1/-1     
ExecutingJavascriptTest.java
Chain Boolean assertions with isInstanceOf() and isEqualTo()
+1/-2     
Enhancement
5 files
RegExpValue.java
Add equals, hashCode, toString methods to RegExpValue       
+24/-3   
PageMargin.java
Replace HashMap with Map.of() factory method                         
+5/-8     
PrimitiveProtocolValue.java
Add toString() method for better debugging                             
+5/-0     
RemoteValue.java
Add toString() method for better debugging                             
+6/-0     
Architecture.java
Improve error message formatting with quoted architecture
+1/-1     
Dependencies
1 files
BUILD.bazel
Add jspecify dependency for null annotations                         
+1/-0     
Additional files
2 files
BrowsingContextInspectorTest.java +0/-1     
ScriptEventsTest.java +0/-2     

@selenium-ci selenium-ci added C-java Java Bindings B-build Includes scripting, bazel and CI integrations B-devtools Includes everything BiDi or Chrome DevTools related B-support Issue or PR related to support classes labels Dec 9, 2025
@selenium-ci
Copy link
Member

Thank you, @asolntsev for this code suggestion.

The support packages contain example code that many users find helpful, but they do not necessarily represent
the best practices for using Selenium, and the Selenium team is not currently merging changes to them.

After reviewing the change, unless it is a critical fix or a feature that is needed for Selenium
to work, we will likely close the PR.

We actively encourage people to add the wrapper and helper code that makes sense for them to their own frameworks.
If you have any questions, please contact us

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Dec 9, 2025

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: 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: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
No auditing: The PR modifies test assertions and helper logic around geolocation but adds no logging of
critical actions (e.g., permission changes or overrides), which may be acceptable for
tests but cannot be verified as compliant from the diff alone.

Referred Code
@NeedsSecureServer
class SetGeolocationOverrideTest extends JupiterTestBase {
  private Map<String, Object> getBrowserGeolocation(
      WebDriver driver, String userContext, String origin) {
    JavascriptExecutor executor = (JavascriptExecutor) driver;
    Permission permission = new Permission(driver);

    permission.setPermission(
        Map.of("name", "geolocation"), PermissionState.GRANTED, origin, userContext);

    return (Map<String, Object>)
        executor.executeAsyncScript(
            "const callback = arguments[arguments.length - 1];\n"
                + "        navigator.geolocation.getCurrentPosition(\n"
                + "            position => {\n"
                + "                const coords = position.coords;\n"
                + "                callback({\n"
                + "                    latitude: coords.latitude,\n"
                + "                    longitude: coords.longitude,\n"
                + "                    accuracy: coords.accuracy,\n"


 ... (clipped 13 lines)

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:
Test-only handling: The changes strengthen assertions and type checks in tests but do not introduce or
validate runtime error handling paths; robustness for failures depends on production code
not visible in the diff.

Referred Code
  driver.get(pages.ajaxyPage);

  Object result = executor.executeAsyncScript("arguments[arguments.length - 1]([]);");
  assertThat(result).asInstanceOf(LIST).isEmpty();
}

@Test
void shouldBeAbleToReturnAnArrayObjectFromAnAsyncScript() {
  driver.get(pages.ajaxyPage);

  Object result = executor.executeAsyncScript("arguments[arguments.length - 1](new Array());");
  assertThat(result).asInstanceOf(LIST).isEmpty();
}

@Test
void shouldBeAbleToReturnArraysOfPrimitivesFromAsyncScripts() {
  driver.get(pages.ajaxyPage);

  Object result =
      executor.executeAsyncScript(
          "arguments[arguments.length - 1]([null, 123456789012345, 'abc', true, false]);");


 ... (clipped 41 lines)

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:
No input validation: The PR updates assertions and casting patterns in tests without introducing validation or
sanitization for external inputs, which is expected for tests but cannot confirm overall
security posture from the diff.

Referred Code
  EvaluateResultSuccess successResult = (EvaluateResultSuccess) result;
  assertThat(successResult.getResult().getType()).isEqualTo("array");
  assertThat(successResult.getResult().getValue())
      .get()
      .asInstanceOf(list(RemoteValue.class))
      .hasSize(2)
      .extracting("value")
      .extracting("value")
      .containsExactly("ARGUMENT_STRING_VALUE", 42L);
}

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
Copy link
Contributor

qodo-code-review bot commented Dec 9, 2025

PR Code Suggestions ✨

No code suggestions found for the PR.

@asolntsev asolntsev self-assigned this Dec 9, 2025
In case of test failure, it's important that the error message shows all the needed information: what was the actual/expected value etc. This commit makes many asserts show this info.

For example,
* `assertThat(a == b).isTrue()` does NOT show what were `a` and `b` values;
* `assertThat(list.contain("foo")).isTrue()` does NOT show what was the `list` value;
* `assertThat(model == 32 || model == 64).isTrue()` does NOT show what was `model` value

etc.
@asolntsev asolntsev force-pushed the fix/java-assertj-asserts branch from b22498f to 9ba2391 Compare December 9, 2025 06:33
it seems this functionality has been implemented in Edge.
@asolntsev asolntsev merged commit 6cb53a2 into SeleniumHQ:trunk Dec 9, 2025
37 of 38 checks passed
@asolntsev asolntsev deleted the fix/java-assertj-asserts branch December 9, 2025 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-build Includes scripting, bazel and CI integrations B-devtools Includes everything BiDi or Chrome DevTools related B-support Issue or PR related to support classes C-java Java Bindings Review effort 2/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants