Skip to content

[java][BiDi] implement emulation #16070

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
wants to merge 18 commits into
base: trunk
Choose a base branch
from

Conversation

Delta456
Copy link
Member

@Delta456 Delta456 commented Jul 18, 2025

User description

🔗 Related Issues

💥 What does this PR do?

Implements emulation BiDi spec in Java

🔧 Implementation Notes

Uses same BiDi approach which is used for other modules

💡 Additional Considerations

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Enhancement


Description

  • Implement BiDi emulation module for geolocation override

  • Add geolocation coordinates and error handling classes

  • Create comprehensive test suite for emulation functionality

  • Update build configurations for new emulation package


Diagram Walkthrough

flowchart LR
  A["Emulation Module"] --> B["GeolocationCoordinates"]
  A --> C["GeolocationPositionError"]
  A --> D["SetGeolocationOverrideParameters"]
  B --> E["Validation & Mapping"]
  C --> E
  D --> E
  E --> F["BiDi Command Execution"]
  G["EmulationTest"] --> A
  H["Build Config Updates"] --> A
Loading

File Walkthrough

Relevant files
Enhancement
4 files
Emulation.java
Main emulation module class implementation                             
+46/-0   
GeolocationCoordinates.java
Geolocation coordinates data model with validation             
+130/-0 
GeolocationPositionError.java
Geolocation error handling model                                                 
+28/-0   
SetGeolocationOverrideParameters.java
Parameters class for geolocation override command               
+72/-0   
Tests
1 files
EmulationTest.java
Comprehensive test suite for emulation functionality         
+202/-0 
Configuration changes
5 files
Browser.java
Add SSL certificate handling for HTTPS testing                     
+16/-2   
BUILD.bazel
Build configuration for emulation package                               
+23/-0   
BUILD.bazel
Add emulation dependency to module build                                 
+1/-0     
BUILD.bazel
Export emulation package in remote module                               
+1/-0     
BUILD.bazel
Test suite build configuration for emulation                         
+37/-0   

@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 Jul 18, 2025
@Delta456 Delta456 marked this pull request as draft July 18, 2025 11:33
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ❌

5678 - Not compliant

Non-compliant requirements:

  • Fix ChromeDriver connection failure error after first instance
  • Resolve "ConnectFailure (Connection refused)" error for subsequent ChromeDriver instances
  • Ensure proper ChromeDriver instantiation without connection issues

1234 - Not compliant

Non-compliant requirements:

  • Fix JavaScript execution in link href on click() for Firefox
  • Ensure JavaScript alerts work properly with Selenium 2.48+ versions
  • Restore functionality that worked in version 2.47.1
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Hardcoded Value

The type field is hardcoded to "positionUnavailable" without any way to customize it or handle different error types that might be needed for comprehensive geolocation error handling.

String type = "positionUnavailable";
Package Visibility

The constructor has package-private visibility which may limit extensibility and proper instantiation from outside the package, potentially requiring a builder pattern or factory method.

SetGeolocationOverrideParameters(
    GeolocationCoordinates coordinates,
Test Assertions

Using basic assert statements instead of proper JUnit assertions reduces test readability and debugging capabilities when tests fail.

assert !r.containsKey("error");

double latitude = ((Number) r.get("latitude")).doubleValue();
double longitude = ((Number) r.get("longitude")).doubleValue();
double accuracy = ((Number) r.get("accuracy")).doubleValue();

assert abs(latitude - coords.getLatitude()) < 0.0001;
assert abs(longitude - coords.getLongitude()) < 0.0001;
assert abs(accuracy - coords.getAccuracy()) < 0.0001;

Copy link
Contributor

qodo-merge-pro bot commented Jul 18, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Use proper JUnit assertions

Replace primitive assert statements with proper JUnit assertions for better test
failure reporting and debugging. JUnit assertions provide clearer error messages
when tests fail.

java/test/org/openqa/selenium/bidi/emulation/EmulationTest.java [74-82]

-assert !r.containsKey("error");
+assertThat(r).doesNotContainKey("error");
 
 double latitude = ((Number) r.get("latitude")).doubleValue();
 double longitude = ((Number) r.get("longitude")).doubleValue();
 double accuracy = ((Number) r.get("accuracy")).doubleValue();
 
-assert abs(latitude - coords.getLatitude()) < 0.0001;
-assert abs(longitude - coords.getLongitude()) < 0.0001;
-assert abs(accuracy - coords.getAccuracy()) < 0.0001;
+assertThat(abs(latitude - coords.getLatitude())).isLessThan(0.0001);
+assertThat(abs(longitude - coords.getLongitude())).isLessThan(0.0001);
+assertThat(abs(accuracy - coords.getAccuracy())).isLessThan(0.0001);
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly recommends replacing Java assert statements with JUnit assertions, which provide more informative failure messages and are standard practice in tests.

Low
Make field private and final

The type field should be private and final to ensure proper encapsulation and
immutability. Add a constructor to initialize the field properly.

java/src/org/openqa/selenium/bidi/emulation/GeolocationPositionError.java [5-11]

 public class GeolocationPositionError {
-  String type = "positionUnavailable";
+  private final String type;
+
+  public GeolocationPositionError() {
+    this.type = "positionUnavailable";
+  }
 
   public Map<String, Object> toMap() {
     return Map.of("type", type);
   }
 }
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly improves encapsulation and immutability by making the type field private and final, which is a good coding practice.

Low
Learned
best practice
Add null check for BiDi instance

Add null check for the BiDi instance returned from getBiDi() method before
assigning it to prevent potential NullPointerException. The getBiDi() method
could return null in some implementations.

java/src/org/openqa/selenium/bidi/emulation/Emulation.java [20]

-this.bidi = ((HasBiDi) driver).getBiDi();
+BiDi biDiInstance = ((HasBiDi) driver).getBiDi();
+if (biDiInstance == null) {
+  throw new IllegalStateException("BiDi instance cannot be null");
+}
+this.bidi = biDiInstance;
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Add null checks and validation for parameters, properties, and return values before using them to prevent NullReferenceException and other runtime errors.

Low
  • Update

@Delta456 Delta456 requested a review from Copilot July 18, 2025 11:39
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements the BiDi emulation module for Java, specifically focusing on geolocation override functionality. The implementation follows the W3C WebDriver BiDi emulation specification and provides a comprehensive API for controlling geolocation behavior in browser contexts.

  • Adds core emulation classes including Emulation, GeolocationCoordinates, and parameter classes
  • Implements geolocation override functionality with proper validation and error handling
  • Creates extensive test coverage for both browsing contexts and user contexts

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
java/src/org/openqa/selenium/bidi/emulation/Emulation.java Main emulation module class providing BiDi command execution
java/src/org/openqa/selenium/bidi/emulation/GeolocationCoordinates.java Geolocation data class with coordinate validation
java/src/org/openqa/selenium/bidi/emulation/SetGeolocationOverrideParameters.java Parameter class for geolocation override commands
java/src/org/openqa/selenium/bidi/emulation/GeolocationPositionError.java Error representation for geolocation failures
java/test/org/openqa/selenium/bidi/emulation/EmulationTest.java Comprehensive test suite covering various emulation scenarios
java/src/org/openqa/selenium/bidi/emulation/BUILD.bazel Build configuration for emulation package
java/test/org/openqa/selenium/bidi/emulation/BUILD.bazel Test build configuration
java/src/org/openqa/selenium/remote/BUILD.bazel Updated remote package dependencies
java/src/org/openqa/selenium/bidi/module/BUILD.bazel Updated module package dependencies

@Delta456 Delta456 force-pushed the java_bidi_emulation branch from e5300bf to 7be9945 Compare July 18, 2025 11:46
@Delta456 Delta456 marked this pull request as ready for review July 30, 2025 19:55
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ❌

1234 - Not compliant

Non-compliant requirements:

  • Fix issue where JavaScript in link's href is not triggered on click() in Selenium 2.48
  • Ensure compatibility with Firefox 42.0
  • Restore functionality that worked in version 2.47.1

5678 - Not compliant

Non-compliant requirements:

  • Fix ChromeDriver ConnectFailure error on Ubuntu 16.04.4
  • Resolve connection refused errors for subsequent ChromeDriver instances
  • Ensure first ChromeDriver instance works without console errors
⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Limited Implementation

The GeolocationPositionError class only supports 'positionUnavailable' error type and lacks support for other standard geolocation error types like 'permissionDenied' and 'timeout'. This may not cover all error scenarios required by the BiDi specification.

public Map<String, Object> toMap() {
  String type = "positionUnavailable";
  return Map.of("type", type);
}
Test Reliability

The test uses custom assert statements instead of JUnit assertions, and the JavaScript execution for geolocation testing may be flaky due to timing issues and browser-specific behavior differences.

assert !r.containsKey("error") : "Geolocation failed with error: " + r.get("error");

double latitude = ((Number) r.get("latitude")).doubleValue();
double longitude = ((Number) r.get("longitude")).doubleValue();
double accuracy = ((Number) r.get("accuracy")).doubleValue();

assert abs(latitude - coords.getLatitude()) < 0.0001
    : "Latitude mismatch: expected " + coords.getLatitude() + ", got " + latitude;
assert abs(longitude - coords.getLongitude()) < 0.0001
    : "Longitude mismatch: expected " + coords.getLongitude() + ", got " + longitude;
assert abs(accuracy - coords.getAccuracy()) < 0.0001
    : "Accuracy mismatch: expected " + coords.getAccuracy() + ", got " + accuracy;

Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add missing coordinates/error validation

Add validation to ensure at least one of coordinates or error is specified.
Currently, both can be null which would result in an invalid geolocation
override request.

java/src/org/openqa/selenium/bidi/emulation/SetGeolocationOverrideParameters.java [41-50]

 if (this.coordinates != null && this.error != null) {
   throw new IllegalArgumentException("Cannot specify both coordinates and error");
+}
+if (this.coordinates == null && this.error == null) {
+  throw new IllegalArgumentException("Must specify either coordinates or error");
 }
 if (this.contexts != null && this.userContexts != null) {
   throw new IllegalArgumentException("Cannot specify both contexts and userContexts");
 }
 
 if (this.contexts == null && this.userContexts == null) {
   throw new IllegalArgumentException("Must specify either contexts or userContexts");
 }
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: This suggestion correctly identifies a missing validation check that would prevent creating an invalid state where both coordinates and error are null, improving the robustness of the class.

Medium
General
Replace assert with JUnit assertions

Replace assert statements with proper JUnit assertions for better test failure
reporting and IDE integration. Assert statements can be disabled with JVM flags
making tests unreliable.

java/test/org/openqa/selenium/bidi/emulation/EmulationTest.java [101-106]

-assert abs(latitude - coords.getLatitude()) < 0.0001
-    : "Latitude mismatch: expected " + coords.getLatitude() + ", got " + latitude;
-assert abs(longitude - coords.getLongitude()) < 0.0001
-    : "Longitude mismatch: expected " + coords.getLongitude() + ", got " + longitude;
-assert abs(accuracy - coords.getAccuracy()) < 0.0001
-    : "Accuracy mismatch: expected " + coords.getAccuracy() + ", got " + accuracy;
+assertEquals(coords.getLatitude(), latitude, 0.0001, 
+    "Latitude mismatch: expected " + coords.getLatitude() + ", got " + latitude);
+assertEquals(coords.getLongitude(), longitude, 0.0001,
+    "Longitude mismatch: expected " + coords.getLongitude() + ", got " + longitude);
+assertEquals(coords.getAccuracy(), accuracy, 0.0001,
+    "Accuracy mismatch: expected " + coords.getAccuracy() + ", got " + accuracy);
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly recommends using JUnit assertions over Java's assert keyword for more reliable and informative test results, which is a standard best practice in testing.

Medium
  • More

private final Double speed;

// Constructor with default accuracy = 1.0
public GeolocationCoordinates(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here also, 4 parameters are optional. So one way to do this would be to have a constructor with fixed parameters and having chained setters for the rest. When values are not set, use the default values mentioned in the spec.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am planning to go with telescoping constructors

@Delta456 Delta456 force-pushed the java_bidi_emulation branch from db8804d to 2892f69 Compare August 1, 2025 10:09
@Delta456 Delta456 requested review from pujagani and navin772 August 1, 2025 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-build Includes scripting, bazel and CI integrations B-devtools Includes everything BiDi or Chrome DevTools related C-java Java Bindings Review effort 3/5 Review effort 4/5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants