Skip to content

Conversation

@asolntsev
Copy link
Contributor

@asolntsev asolntsev commented Dec 9, 2025

User description

💥 What does this PR do?

This PR removes most of "SLEEP N" from Java tests.

🔧 Implementation Notes

Instead of just silly waiting for N seconds, it's better to wait for the needed event. E.g. to wait the needed button on the screen etc.

Thus we make our tests faster and more stable.

🔄 Types of changes

  • Bug fix (backwards compatible)

PR Type

Enhancement, Tests


Description

  • Replace hardcoded Thread.sleep() calls with proper wait conditions

  • Add new waiting condition methods for text matching and containment

  • Initialize test pages with blank page to isolate test state

  • Reduce polling interval in WebDriverWait for faster test execution

  • Update test HTML pages to delay element initialization


Diagram Walkthrough

flowchart LR
  A["Thread.sleep calls"] -->|Replace with| B["ExpectedConditions waits"]
  C["Test isolation issues"] -->|Solve with| D["Blank page initialization"]
  E["Slow polling"] -->|Improve to| F["20ms polling interval"]
  G["Test HTML pages"] -->|Update with| H["Delayed element initialization"]
Loading

File Walkthrough

Relevant files
Code cleanup
1 files
ExpectedConditions.java
Use diamond operator for generic type inference                   
+48/-50 
Test improvement
9 files
CorrectEventFiringTest.java
Replace sleep loop with elementTextToContain wait condition
+3/-23   
CombinedInputActionsTest.java
Replace sleep with wait conditions in hover and drag tests
+22/-21 
DefaultKeyboardTest.java
Remove sleep call and improve test assertions                       
+11/-17 
DefaultMouseTest.java
Replace sleep and polling loops with visibility waits       
+31/-56 
DragAndDropTest.java
Replace sleep and polling with visibility wait conditions
+18/-42 
CombinedInputActionsTest.java
Replace sleep with wait conditions in hover tests               
+22/-20 
DefaultMouseTest.java
Replace sleep and polling loops with visibility waits       
+30/-49 
DragAndDropTest.java
Replace sleep and polling with visibility wait conditions
+21/-44 
PenPointerTest.java
Replace sleep with wait conditions in hover tests               
+30/-39 
Test cleanup
1 files
ElementAttributeTest.java
Remove unused logger and sleep call from test                       
+0/-9     
Enhancement
1 files
WaitingConditions.java
Add elementTextToContain and elementTextToMatch methods   
+39/-21 
Tests
1 files
WaitingConditionsTest.java
New test class for waiting condition methods                         
+58/-0   
Test setup
1 files
JupiterTestBase.java
Add blank page initialization before each test                     
+4/-0     
Performance improvement
1 files
SeleniumExtension.java
Reduce polling interval to 20ms for faster waits                 
+2/-2     
Test page update
5 files
dragAndDropTest.html
Hide elements initially and show after initialization       
+12/-6   
draggableLists.html
Delay element initialization with setTimeout                         
+6/-3     
droppableItems.html
Delay jQuery initialization and show elements after setup
+24/-19 
macbeth.html
Add h1 header element for test visibility                               
+2/-0     
blank_page.html
Clear localStorage and sessionStorage on blank page           
+4/-1     

@selenium-ci selenium-ci added C-java Java Bindings B-atoms JavaScript chunks generated by Google closure 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

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:
Not Applicable: The PR modifies test code and waiting conditions without introducing or altering any
critical action flows that would require audit logging; applicability of audit trails
cannot be determined from the provided diff.

Referred Code
class DragAndDropTest extends JupiterTestBase {

  @Test
  void testDragAndDropRelative() {
    driver.get(pages.dragAndDropPage);
    WebElement img = wait.until(visibilityOfElementLocated(By.id("test1")));
    Point expectedLocation = img.getLocation();
    expectedLocation = drag(img, expectedLocation, 150, 200);
    wait.until(elementLocationToBe(img, expectedLocation));
    expectedLocation = drag(img, expectedLocation, -50, -25);
    wait.until(elementLocationToBe(img, expectedLocation));
    expectedLocation = drag(img, expectedLocation, 0, 0);
    wait.until(elementLocationToBe(img, expectedLocation));
    expectedLocation = drag(img, expectedLocation, 1, -1);
    wait.until(elementLocationToBe(img, expectedLocation));
  }

  @Test
  void testDragAndDropToElement() {
    driver.get(pages.dragAndDropPage);


 ... (clipped 116 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:
Null Returns: Several new ExpectedConditions return null on mismatch without additional context or
logging, which is typical for waits but limits actionable error context if used beyond
testing.

Referred Code
  };
}

public static ExpectedCondition<String> elementTextToContain(
    final By locator, final String expected) {
  return new ExpectedCondition<>() {
    @Override
    public String apply(WebDriver driver) {
      String text = driver.findElement(locator).getText();
      return text.contains(expected) ? text : null;
    }

    @Override
    public String toString() {
      return String.format("element text did not contain \"%s\"", expected);
    }
  };
}

public static ExpectedCondition<String> elementTextToMatch(final By locator, final String regex) {
  return new ExpectedCondition<>() {


 ... (clipped 56 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:
Delayed Init Risks: The added delayed initialization and dynamic showing of elements could create brief states
where event handlers are not attached; while acceptable for tests, validation of external
input is not in scope here and overall security impact cannot be assessed from the diff.

Referred Code
#droppable { width: 150px; height: 150px; padding: 0.5em; float: left; margin: 10px; }
</style>
<script type="text/javascript">
  $(function () {
    setTimeout(function () {
      $("#draggable").draggable();
      $("#droppable").droppable({
        drop: function () {
          $(this).addClass('ui-state-highlight').find('p').html('Dropped!');
        }
      });

      const report_event = function (report_text) {
        const reportElement = $("#drop_reports");
        const origText = reportElement.text();
        reportElement.text(origText + " " + report_text);
      }

      $('body').mousedown(function () {
        report_event('down');
      });


 ... (clipped 25 lines)

Learn more about managing compliance generic rules or creating your own custom rules

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

@qodo-code-review
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Avoid hardcoded values in assertions

In testDragTooFar, replace the assertion hasMessageContaining with a hardcoded
coordinate with hasMessageMatching and a regular expression to make the test
more robust against environmental variations.

java/test/org/openqa/selenium/bidi/input/DragAndDropTest.java [131-148]

 @Test
 void testDragTooFar() {
   driver.get(pages.dragAndDropPage);
   WebElement img = wait.until(visibilityOfElementLocated(By.id("test1")));
 
   assertThatThrownBy(
           () ->
               inputModule.perform(
                   windowHandle,
                   new Actions(driver)
                       .dragAndDropBy(img, Integer.MAX_VALUE, Integer.MAX_VALUE)
                       .getSequences()))
       .as("These coordinates are outside the page - expected to fail.")
       .isInstanceOf(BiDiException.class)
-      .hasMessageContaining("Move target (2147483664, 2147483664) is out of bounds");
+      .hasMessageMatching("Move target \\(.*\\) is out of bounds");
 
   // Release mouse button - move was interrupted in the middle.
   inputModule.perform(windowHandle, new Actions(driver).release().getSequences());
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly points out that asserting a hardcoded coordinate value makes the test brittle. Using a regex to match the error message structure is a significant improvement for test robustness and maintainability.

Medium
Use a more precise mouse action

In the unfocusMenu method, replace the click() action with a moveToElement
action to avoid unintended side effects and more accurately reflect the intent
of moving the mouse.

java/test/org/openqa/selenium/interactions/CombinedInputActionsTest.java [404-410]

 /**
  * Move to a different element to make sure the mouse is not over the menu items (from a previous
  * test).
  */
 private void unfocusMenu() {
-  driver.findElement(By.id("dynamo")).click();
+  new Actions(driver).moveToElement(driver.findElement(By.id("dynamo"))).build().perform();
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that using click() to move the mouse can have unintended side effects and proposes using a moveToElement action instead, which is a more precise and safer approach for the intended purpose of unfocusing a menu.

Medium
Learned
best practice
Centralize safe poll interval

Avoid hard-coding very tight poll intervals; define a constant with safe bounds
and reuse it to prevent overly aggressive polling and flaky behavior.

java/test/org/openqa/selenium/testing/SeleniumExtension.java [310-311]

-this.regularWait = new WebDriverWait(driver, regularWait, Duration.ofMillis(20));
-this.shortWait = new WebDriverWait(driver, shortWait, Duration.ofMillis(20));
+private static final Duration DEFAULT_POLL_INTERVAL = Duration.ofMillis(100);
+...
+this.regularWait = new WebDriverWait(driver, regularWait, DEFAULT_POLL_INTERVAL);
+this.shortWait = new WebDriverWait(driver, shortWait, DEFAULT_POLL_INTERVAL);

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 5

__

Why:
Relevant best practice - Pattern 5: Guard time calculations and use clear, centralized timeout configuration to avoid flaky tests.

Low
  • More

@asolntsev asolntsev marked this pull request as draft December 9, 2025 22:23
@asolntsev asolntsev self-assigned this Dec 9, 2025
@asolntsev asolntsev force-pushed the fix/avoid-sleep-in-tests branch from 1509b2a to 02562d4 Compare December 9, 2025 23:16
It's a good practice to avoid tests affecting each other:
1. Open "about:blank" page - this stops any current activity / background requests / animations on the previous page
2. Open an empty page which clears sessionStorage, localStorage and cookies.

This technique allows reusing the browser between tests, while keeping the tests independent.
Instead of wasting 2 second (which doesn't guarantee the result), just wait for the needed element that should appear as a result of hovering.
if the first try failed, wasting the whole 0.5 second is a huge waste of time. Let's try sooner!
fix the test page usability: make the elements visible only after they are fully initialized.

To force revealing potential flaky tests in the future, I've added a small pause for the initialization code. So the tests should wait until the element appears on the screen - then it's ready for manipulations.
it breaks Py/Rb tests that I cannot fix now. Ideally, we should add waiting to these tests as well.
Also, reset the link to static field after test run - to avoid holding heavy objects in memory till the end of all tests.
NB! This change decrease execution time of `SessionHandlingTest` from ~28s to 7s.
@asolntsev asolntsev force-pushed the fix/avoid-sleep-in-tests branch from 02562d4 to d36727b Compare December 10, 2025 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-atoms JavaScript chunks generated by Google closure 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