You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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"]
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 (Exceptione) {
// Expect an exception due to navigation failure
}
NavigationInfonavigationInfo = future.get(5, TimeUnit.SECONDS);
assertThat(navigationInfo.getBrowsingContextId()).isEqualTo(context.getId());
assertThat(navigationInfo.getUrl())
.isEqualTo("http://invalid-domain-that-does-not-exist.test/");
}
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 (Exceptione) {
// Expect an exception due to navigation failure
}
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.
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.
@TestvoidcanListenToNavigationFailedEvent() {
// Use a local server to provide a deterministic failureStringurlThatFails = appServer.whereIs("/close-connection");
// ... setupinspector.onNavigationFailed(future::complete);
BrowsingContextcontext = newBrowsingContext(driver, driver.getWindowHandle());
try {
// Navigate to a local, controlled failure endpointcontext.navigate(urlThatFails, ReadinessState.COMPLETE);
} catch (Exceptione) {
// Expected failure
}
NavigationInfonavigationInfo = 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.
+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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
selenium-ci
added
C-java
User description
🔗 Related Issues
💥 What does this PR do?
Adds test for
onNavigationFailedevent.🔧 Implementation Notes
💡 Additional Considerations
Initially, I added test for
onDownloadWillBeginalso, but it requires some implementation modification and a newDownloadInfoclass, so that will come in a separate PR.🔄 Types of changes
PR Type
Tests
Description
Add test for
onNavigationFailedBiDi eventTest validates navigation failure handling with invalid domain
Include proper exception handling and assertions
Diagram Walkthrough
File Walkthrough
BrowsingContextInspectorTest.java
Add navigation failed event testjava/test/org/openqa/selenium/bidi/browsingcontext/BrowsingContextInspectorTest.java
NotYetImplementedannotationcanListenToNavigationFailedEvent