Skip to content

Conversation

navin772
Copy link
Member

@navin772 navin772 commented Sep 5, 2025

User description

🔗 Related Issues

💥 What does this PR do?

Adds test for onHistoryUpdated event.

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Tests

PR Type

Tests


Description

  • Add test for onHistoryUpdated BiDi event

  • Test uses history.pushState to trigger event

  • Validates browsing context ID and URL changes


Diagram Walkthrough

flowchart LR
  A["Test Setup"] --> B["Navigate to Page"]
  B --> C["Register Event Listener"]
  C --> D["Execute history.pushState"]
  D --> E["Verify Event Data"]
Loading

File Walkthrough

Relevant files
Tests
BrowsingContextInspectorTest.java
Add history updated event test                                                     

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

  • Add import for Optional and Script module
  • Add canListenToHistoryUpdatedEvent test method
  • Test registers listener and triggers event with history.pushState
  • Validates event contains correct browsing context ID and URL
+25/-0   

@navin772 navin772 requested a review from pujagani September 5, 2025 08:14
@selenium-ci selenium-ci added C-java Java Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Sep 5, 2025
Copy link
Contributor

qodo-merge-pro bot commented Sep 5, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

5678 - Partially compliant

Compliant requirements:

Non-compliant requirements:

  • Diagnose and address "ConnectFailure" errors for multiple ChromeDriver instances.
  • Ensure subsequent ChromeDriver instantiations do not produce connection refused errors.
  • Provide environment-specific fix/workaround for Ubuntu 16.04.4 with given versions.
  • Validate behavior across multiple driver instances.

Requires further human verification:

1234 - Partially compliant

Compliant requirements:

Non-compliant requirements:

  • Investigate and fix JS-in-href click behavior regression in 2.48.* with Firefox 42.
  • Reproduce and validate behavior.

Requires further human verification:

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

Flaky Test Risk

The test relies on receiving a HistoryUpdated event within 5 seconds after history.pushState; event timing could be flaky on slower environments. Consider increasing timeout or adding retry/wait for readiness to reduce flakiness.

@Test
@NeedsFreshDriver
void canListenToHistoryUpdatedEvent()
    throws ExecutionException, InterruptedException, TimeoutException {
  try (BrowsingContextInspector inspector = new BrowsingContextInspector(driver);
      Script script = new Script(driver)) {
    CompletableFuture<HistoryUpdated> future = new CompletableFuture<>();

    BrowsingContext context = new BrowsingContext(driver, driver.getWindowHandle());
    context.navigate(appServer.whereIs("/simpleTest.html"), ReadinessState.COMPLETE);

    inspector.onHistoryUpdated(future::complete);

    // Use history.pushState to trigger history updated event
    script.evaluateFunctionInBrowsingContext(
        context.getId(), "history.pushState({}, '', '/new-path')", false, Optional.empty());

    HistoryUpdated historyUpdated = future.get(5, TimeUnit.SECONDS);
    assertThat(historyUpdated.getBrowsingContextId()).isEqualTo(context.getId());
    assertThat(historyUpdated.getUrl()).contains("/new-path");
  }
API Usage Assumption

The evaluateFunctionInBrowsingContext call uses "history.pushState({}, '', '/new-path')" without awaiting completion or verifying execution result; if execution is async or sandboxed, event may not fire. Consider checking evaluation result or ensuring user activation/context permissions if required.

// Use history.pushState to trigger history updated event
script.evaluateFunctionInBrowsingContext(
    context.getId(), "history.pushState({}, '', '/new-path')", false, Optional.empty());

HistoryUpdated historyUpdated = future.get(5, TimeUnit.SECONDS);
assertThat(historyUpdated.getBrowsingContextId()).isEqualTo(context.getId());
assertThat(historyUpdated.getUrl()).contains("/new-path");

Copy link
Contributor

qodo-merge-pro bot commented Sep 5, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Remove non-closeable from resources

Avoid using try-with-resources for a type that does not implement AutoCloseable.
Instantiate Script as a regular variable inside the block so the code compiles
across Selenium versions.

java/test/org/openqa/selenium/bidi/browsingcontext/BrowsingContextInspectorTest.java [289-290]

-try (BrowsingContextInspector inspector = new BrowsingContextInspector(driver);
-    Script script = new Script(driver)) {
+try (BrowsingContextInspector inspector = new BrowsingContextInspector(driver)) {
+  Script script = new Script(driver);
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies that Script does not implement AutoCloseable and therefore cannot be used in a try-with-resources statement, which would cause a compilation error.

High
Wrap expression in arrow function

evaluateFunctionInBrowsingContext expects a function declaration, not a raw
expression. Wrap the call in an arrow function to prevent an "invalid
functionDeclaration" error at runtime.

java/test/org/openqa/selenium/bidi/browsingcontext/BrowsingContextInspectorTest.java [299-300]

 script.evaluateFunctionInBrowsingContext(
-        context.getId(), "history.pushState({}, '', '/new-path')", false, Optional.empty());
+        context.getId(), "() => history.pushState({}, '', '/new-path')", false, Optional.empty());
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly points out that evaluateFunctionInBrowsingContext expects a function declaration, and the provided code passes a raw expression, which would cause a runtime error. The proposed fix is correct.

High
Learned
best practice
Use precise URL path assertion

Parse the URL and assert on its path to validate precisely that the navigation
updated the expected path segment.

java/test/org/openqa/selenium/bidi/browsingcontext/BrowsingContextInspectorTest.java [304]

-assertThat(historyUpdated.getUrl()).contains("/new-path");
+assertThat(java.net.URI.create(historyUpdated.getUrl()).getPath()).isEqualTo("/new-path");
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Add precise validation checks instead of substring assertions to reduce false positives.

Low
  • Update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-devtools Includes everything BiDi or Chrome DevTools related C-java Java Bindings Review effort 2/5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants