[java][bidi]: add test for onNavigationFailed

[navin772]

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

---

[qodo-merge-pro]

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()

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	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]

Hi @pujagani, could you review this PR?

[pujagani]

Thank you! LGTM @navin772