Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

[java][bidi]: add test for onNavigationFailed #16241

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
navin772 wants to merge 3 commits into SeleniumHQ:trunk
base: trunk
Choose a base branch
Loading
from navin772:java-onNavigationFailed-test

Conversation

Copy link
Member

@navin772 navin772 commented Aug 22, 2025 •
edited by qodo-merge-pro bot
Loading

User description

🔗 Related Issues

💥 What does this PR do?

Adds test for onNavigationFailed event.

🔧 Implementation Notes

💡 Additional Considerations

Initially, I added test for onDownloadWillBegin also, but it requires some implementation modification and a new DownloadInfo class, so that will come in a separate PR.

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Bug fix (backwards compatible)
  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Tests

Description

  • Add test for onNavigationFailed BiDi event

  • Test validates navigation failure handling with invalid domain

  • Include proper exception handling and assertions

Diagram Walkthrough

flowchart LR
 A["BrowsingContextInspector"] --> B["onNavigationFailed listener"]
 B --> C["Navigate to invalid domain"]
 C --> D["Catch navigation exception"]
 D --> E["Validate NavigationInfo"]
Loading

File Walkthrough
Relevant files
Tests
BrowsingContextInspectorTest.java
Add navigation failed event test

java/test/org/openqa/selenium/bidi/browsingcontext/BrowsingContextInspectorTest.java

  • Add import for NotYetImplemented annotation
  • Create new test method canListenToNavigationFailedEvent
  • Test navigation failure with invalid domain URL
  • Include proper exception handling and assertions
+26/-0

@selenium-ci selenium-ci added C-java Java Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Aug 22, 2025
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

5678 - Partially compliant

Compliant requirements:

Non-compliant requirements:

  • Investigate/resolve ChromeDriver connection failures after first instantiation on Ubuntu.
  • Diagnose "Error: ConnectFailure (Connection refused)" occurring for subsequent ChromeDriver instances.
  • Provide steps or fixes related to Chrome/ChromeDriver versions mentioned.

Requires further human verification:

  • Reproduce connection failure across multiple ChromeDriver instantiations on Ubuntu with specified versions.
  • Validate environment-specific behavior and logs.

1234 - Partially compliant

Compliant requirements:

Non-compliant requirements:

  • Address regression where click() no longer triggers JavaScript in link href in 2.48.x on Firefox 42.
  • Provide fix or test coverage for JS href click handling.

Requires further human verification:

  • Verify behavior on legacy Firefox/Selenium versions outside the scope of current PR.
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Flaky Test Risk

Using a public invalid domain may not consistently fail quickly across environments or networks. Consider using a guaranteed blackhole domain (e.g., example.invalid) or a local unreachable endpoint to reduce flakiness.

 
 context.navigate(
 "http://invalid-domain-that-does-not-exist.test/", ReadinessState.COMPLETE);
 } catch (Exception e) {
 // Expect an exception due to navigation failure
 }
 NavigationInfo navigationInfo = future.get(5, TimeUnit.SECONDS);
 assertThat(navigationInfo.getBrowsingContextId()).isEqualTo(context.getId());
 assertThat(navigationInfo.getUrl())
 .isEqualTo("http://invalid-domain-that-does-not-exist.test/");
}
Exception Handling

Catching a broad Exception and ignoring it can mask unexpected issues. Narrow the catch to the expected exception type(s) or assert that an exception occurred to ensure deterministic behavior.

 
} catch (Exception e) {
 // Expect an exception due to navigation failure
}
Browser Skip Annotation

The @NotYetImplemented(FIREFOX) annotation is added; verify if other browsers need gating (e.g., EDGE/SAFARI) or if the event is consistently implemented across them to avoid intermittent CI failures.

 
@NotYetImplemented(FIREFOX)
void canListenToNavigationFailedEvent()

Copy link
Contributor

qodo-merge-pro bot commented Aug 22, 2025 •
edited
Loading

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion Impact
High-level
Clarify failure source and determinism

The test relies on an external invalid domain to trigger onNavigationFailed,
which can be nondeterministic across environments, DNS setups, and offline
modes, causing flaky behavior. Prefer a deterministic local fixture (e.g., a
controlled server route that closes connection or returns a known failure) and
assert on the BiDi error data (status/error code) rather than only URL equality
to ensure cross-browser reliability.

Examples:

java/test/org/openqa/selenium/bidi/browsingcontext/BrowsingContextInspectorTest.java [239-258] 
 void canListenToNavigationFailedEvent()
 throws ExecutionException, InterruptedException, TimeoutException {
 try (BrowsingContextInspector inspector = new BrowsingContextInspector(driver)) {
 CompletableFuture<NavigationInfo> future = new CompletableFuture<>();
 inspector.onNavigationFailed(future::complete);
 BrowsingContext context = new BrowsingContext(driver, driver.getWindowHandle());
 try {
 context.navigate(
 ... (clipped 10 lines)

Solution Walkthrough:

Before:

@Test
void canListenToNavigationFailedEvent() {
 // ... setup
 inspector.onNavigationFailed(future::complete);
 BrowsingContext context = new BrowsingContext(driver, driver.getWindowHandle());
 try {
 // Navigate to a public, non-existent domain
 context.navigate(
 "http://invalid-domain-that-does-not-exist.test/", ReadinessState.COMPLETE);
 } catch (Exception e) {
 // Hope navigation fails as expected
 }
 NavigationInfo navigationInfo = future.get(5, TimeUnit.SECONDS);
 assertThat(navigationInfo.getUrl())
 .isEqualTo("http://invalid-domain-that-does-not-exist.test/");
}

After:

@Test
void canListenToNavigationFailedEvent() {
 // Use a local server to provide a deterministic failure
 String urlThatFails = appServer.whereIs("/close-connection");
 // ... setup
 inspector.onNavigationFailed(future::complete);
 BrowsingContext context = new BrowsingContext(driver, driver.getWindowHandle());
 try {
 // Navigate to a local, controlled failure endpoint
 context.navigate(urlThatFails, ReadinessState.COMPLETE);
 } catch (Exception e) {
 // Expected failure
 }
 NavigationInfo navigationInfo = future.get(5, TimeUnit.SECONDS);
 assertThat(navigationInfo.getUrl()).isEqualTo(urlThatFails);
 // Ideally, also assert on the specific error type from the event
}
Suggestion importance[1-10]: 9

__

Why: This suggestion correctly identifies a critical source of potential test flakiness by using an external, non-deterministic URL and proposes a robust, standard alternative using a local test server.

High
General
Assert navigation actually failed

Narrow the try block to only catch the navigation call and add an assertion that
an exception occurred to ensure the test truly exercises a navigation failure
path. Otherwise, a silent success would mask a broken test.

java/test/org/openqa/selenium/bidi/browsingcontext/BrowsingContextInspectorTest.java [247-252] 

+boolean navigationFailed = false;
 try {
 context.navigate(
 "http://invalid-domain-that-does-not-exist.test/", ReadinessState.COMPLETE);
 } catch (Exception e) {
- // Expect an exception due to navigation failure
+ navigationFailed = true;
 }
+assertThat(navigationFailed).isTrue();
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: This is a good suggestion that improves test correctness by explicitly asserting that the navigation call threw an exception, which is a key assumption of this test case.

Medium
Relax brittle URL equality check

Use a more lenient URL assertion to avoid flakiness from automatic URL
normalization (e.g., trailing slashes, scheme changes, punycode). Assert the URL
starts with or contains the expected host instead of strict equality. This keeps
the test resilient across browsers and network stacks.

java/test/org/openqa/selenium/bidi/browsingcontext/BrowsingContextInspectorTest.java [254-257] 

 NavigationInfo navigationInfo = future.get(5, TimeUnit.SECONDS);
 assertThat(navigationInfo.getBrowsingContextId()).isEqualTo(context.getId());
 assertThat(navigationInfo.getUrl())
- .isEqualTo("http://invalid-domain-that-does-not-exist.test/");
+ .contains("invalid-domain-that-does-not-exist.test");
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly points out that using isEqualTo for URL checks can be brittle; switching to contains makes the test more robust against minor URL normalization differences.

Low
Learned
best practice
Add null check before use

Guard against the case where the event may not fire and future completes
exceptionally or with null. Validate the future completion and the
navigationInfo instance before dereferencing its fields to avoid
NullPointerExceptions in flaky environments.

java/test/org/openqa/selenium/bidi/browsingcontext/BrowsingContextInspectorTest.java [254-257] 

 NavigationInfo navigationInfo = future.get(5, TimeUnit.SECONDS);
+assertThat(navigationInfo).as("navigation failed event should be received").isNotNull();
 assertThat(navigationInfo.getBrowsingContextId()).isEqualTo(context.getId());
 assertThat(navigationInfo.getUrl())
 .isEqualTo("http://invalid-domain-that-does-not-exist.test/");
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Add null checks and validation before using values that may be absent to prevent runtime errors.

Low
  • Update

@navin772 navin772 added this to the Selenium 4.36 milestone Aug 22, 2025
Copy link
Member Author

Hi @pujagani, could you review this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pujagani pujagani Awaiting requested review from pujagani

@diemol diemol Awaiting requested review from diemol

Assignees
No one assigned
Labels
B-devtools Includes everything BiDi or Chrome DevTools related C-java Java Bindings Review effort 2/5
Projects
None yet
Milestone
Selenium 4.36
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /