Skip to content

[java][bidi] Add network module chrome tests#15654

Merged
pujagani merged 3 commits intoSeleniumHQ:trunkfrom
pujagani:add-network-bidi-chrome-test
Apr 22, 2025
Merged

[java][bidi] Add network module chrome tests#15654
pujagani merged 3 commits intoSeleniumHQ:trunkfrom
pujagani:add-network-bidi-chrome-test

Conversation

@pujagani
Copy link
Contributor

@pujagani pujagani commented Apr 22, 2025 •
edited
Loading

🔗 Related Issues

💥 What does this PR do?

Added tests for BiDi network module what work with Chrome.

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Enabling tests

@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 labels Apr 22, 2025
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Apr 22, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

5678 - PR Code Verified

Compliant requirements:

• The PR enables BiDi network command tests for Chrome browser, which may help address ChromeDriver connection issues

Requires further human verification:

• Need to verify if the PR actually fixes the specific "ConnectFailure" error when instantiating multiple ChromeDrivers
• Need to test with the specific Chrome/ChromeDriver versions mentioned in the ticket

1234 - Not compliant

Non-compliant requirements:

• The PR doesn't address the JavaScript click() issue in Firefox

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Error Handling

The error handling in canContinueWithoutAuthCredentials and canCancelAuth tests has been changed significantly. Verify that the new implementation correctly tests the expected behavior when authentication is rejected or canceled.

 
void canContinueWithoutAuthCredentials() {
  try (Network network = new Network(driver)) {
    network.addIntercept(new AddInterceptParameters(InterceptPhase.AUTH_REQUIRED));
    network.onAuthRequired(
        responseDetails -> {
          if (responseDetails.getRequest().getUrl().contains("basicAuth")) {
            network.continueWithAuthNoCredentials(responseDetails.getRequest().getRequestId());
          }
        });
    page = appServer.whereIs("basicAuth");
    BrowsingContext browsingContext = new BrowsingContext(driver, driver.getWindowHandle());

    try {
      browsingContext.navigate(page, ReadinessState.COMPLETE);
      fail("Exception should be thrown");
    } catch (Exception e) {
      assertThat(e).isInstanceOf(WebDriverException.class);
    }
  }
}

@Test
@NeedsFreshDriver
void canCancelAuth() throws InterruptedException {
  try (Network network = new Network(driver)) {
    network.addIntercept(new AddInterceptParameters(InterceptPhase.AUTH_REQUIRED));
    network.onAuthRequired(
        responseDetails -> {
          if (responseDetails.getRequest().getUrl().contains("basicAuth")) {
            // Does not handle the alert
            network.cancelAuth(responseDetails.getRequest().getRequestId());
          }
        });

    AtomicInteger status = new AtomicInteger();
    CountDownLatch latch = new CountDownLatch(1);

    network.onResponseCompleted(
        responseDetails -> {
          if (responseDetails.getRequest().getUrl().contains("basicAuth")) {
            status.set(responseDetails.getResponseData().getStatus());
            latch.countDown();
          }
        });

    page = appServer.whereIs("basicAuth");
    BrowsingContext browsingContext = new BrowsingContext(driver, driver.getWindowHandle());

    try {
      browsingContext.navigate(page, ReadinessState.COMPLETE);
    } catch (Exception BiDiException) {
      // Ignore
      // Only Chromium browsers throw an error because the navigation did not complete as
      // expected.
    }

    latch.await(10, TimeUnit.SECONDS);
    assertThat(status.get()).isEqualTo(401);
  }
Navigation Changes

All navigation calls have been changed from driver.get() to BrowsingContext.navigate() with ReadinessState.COMPLETE. Ensure this doesn't change the test behavior or timing in unexpected ways.

 
BrowsingContext browsingContext = new BrowsingContext(driver, driver.getWindowHandle());

browsingContext.navigate(
    appServer.whereIs("/bidi/logEntryAdded.html"), ReadinessState.COMPLETE);

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Apr 22, 2025 •
edited
Loading

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix URL inconsistency

The URL used in the navigation doesn't match the URL set in the page variable.
The test sets page to the basicAuth URL but then attempts to navigate to the
logEntryAdded.html page, which is inconsistent and could lead to unreliable test
results.

java/test/org/openqa/selenium/bidi/network/NetworkCommandsTest.java [288-304]

 void canFailRequest() {
   try (Network network = new Network(driver)) {
     network.addIntercept(new AddInterceptParameters(InterceptPhase.BEFORE_REQUEST_SENT));
     network.onBeforeRequestSent(
         responseDetails -> network.failRequest(responseDetails.getRequest().getRequestId()));
     page = appServer.whereIs("basicAuth");
     driver.manage().timeouts().pageLoadTimeout(Duration.of(5, ChronoUnit.SECONDS));
 
     assertThatThrownBy(
             () -> {
               BrowsingContext browsingContext =
                   new BrowsingContext(driver, driver.getWindowHandle());
-              browsingContext.navigate(
-                  appServer.whereIs("/bidi/logEntryAdded.html"), ReadinessState.COMPLETE);
+              browsingContext.navigate(page, ReadinessState.COMPLETE);
             })
         .isInstanceOf(WebDriverException.class);
   }
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical inconsistency where the test sets page to basicAuth URL but navigates to a different URL. This mismatch could cause test reliability issues and doesn't properly test the intended functionality of failing requests to the basicAuth endpoint.

High
Learned
best practice
Add null checks for parameters and properties before accessing their members to prevent NullPointerExceptions

Add null checks for the responseDetails and its nested properties before
accessing them to prevent potential NullPointerExceptions.

java/test/org/openqa/selenium/bidi/network/NetworkCommandsTest.java [228-233]

 network.onAuthRequired(
     responseDetails -> {
-      if (responseDetails.getRequest().getUrl().contains("basicAuth")) {
+      if (responseDetails != null && responseDetails.getRequest() != null && 
+          responseDetails.getRequest().getUrl() != null && 
+          responseDetails.getRequest().getUrl().contains("basicAuth")) {
         network.continueWithAuthNoCredentials(responseDetails.getRequest().getRequestId());
       }
     });
  • Apply this suggestion
Suggestion importance[1-10]: 6
Low
  • Update

@pujagani pujagani merged commit ac98d8f into SeleniumHQ:trunk Apr 22, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@pujagani @selenium-ci