Skip to content

Conversation

@titusfortner
Copy link
Member

@titusfortner titusfortner commented Dec 22, 2025

User description

💥 What does this PR do?

  • Changing several connection.execute to connection.send when we want it to be blocking.
  • Use env.builder() instead of new Builder() so it uses the bazel configured values for the run
  • I removed firefox options tests that I don't think work any more

💡 Additional Considerations

More tests to follow up, just need to get things fixed first.


PR Type

Bug fix, Tests


Description

  • Replace deprecated connection.execute() calls with connection.send()

  • Simplify CDP connection method signatures by removing null parameters

  • Remove flaky Firefox WebExtension profile tests from test suite

  • Re-enable JavaScript tests on RBE by fixing test compatibility issues


Diagram Walkthrough

flowchart LR
  A["CDP Connection Calls"] -->|Replace execute with send| B["Simplified Method Signatures"]
  C["Firefox Options Tests"] -->|Remove WebExtension tests| D["Stable Test Suite"]
  E["Test Configuration"] -->|Re-enable on RBE| F["Passing JS Tests"]
  B --> F
  D --> F
Loading

File Walkthrough

Relevant files
Bug fix
webdriver.js
Refactor CDP connection method calls                                         

javascript/selenium-webdriver/lib/webdriver.js

  • Replace connection.execute() with connection.send() for CDP commands
  • Remove null parameters from method calls (Fetch.enable,
    Network.setCacheDisabled, Page.enable, Runtime.evaluate,
    Page.removeScriptToEvaluateOnLoad)
  • Simplify method signatures across network interception and pinned
    script handling
+17/-37 
Tests
builder_test.js
Simplify builder test setup                                                           

javascript/selenium-webdriver/test/builder_test.js

  • Replace manual Builder instantiation with env.builder() helper method
  • Simplify test setup to use environment-provided builder configuration
+1/-1     
options_test.js
Remove flaky WebExtension tests                                                   

javascript/selenium-webdriver/test/firefox/options_test.js

  • Remove WebExtension profile creation and related test setup
  • Delete flaky "use profile with extension" test case
  • Refactor user agent verification into reusable verifyUserAgent()
    helper function
  • Simplify profile preference test to use single user preference profile
+8/-24   
Configuration changes
.skipped-tests
Re-enable JavaScript tests on RBE                                               

.skipped-tests

  • Remove three JavaScript test entries from skip list
    (test-builder-test.js-chrome, test-chrome-devtools-test.js-chrome,
    test-firefox-options-test.js-firefox)
  • Re-enable previously skipped JavaScript tests on RBE
+0/-3     

@selenium-ci selenium-ci added the B-build Includes scripting, bazel and CI integrations label Dec 22, 2025
@titusfortner titusfortner changed the title [js] add tests on mac and windows [js] fix JS tests on RBE Dec 30, 2025
@titusfortner titusfortner marked this pull request as ready for review December 30, 2025 06:27
@qodo-code-review
Copy link
Contributor

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Unhandled CDP failures: Newly added connection.send(...) calls are awaited without local error handling or
contextual error enrichment, so CDP enable/evaluate failures may surface without
actionable context depending on upstream handling.

Referred Code
  await connection.send('Fetch.enable', {
    handleAuthRequests: true,
  })
  await connection.send('Network.setCacheDisabled', {
    cacheDisabled: true,
  })
}

/**
 * Handle Network interception requests
 * @param connection WebSocket connection to the browser
 * @param httpResponse Object representing what we are intercepting
 *                     as well as what should be returned.
 * @param callback callback called when we intercept requests.
 */
async onIntercept(connection, httpResponse, callback) {
  this._cdpWsConnection.on('message', (message) => {
    const params = JSON.parse(message)
    if (params.method === 'Fetch.requestPaused') {
      const requestPausedParams = params['params']
      if (requestPausedParams.request.url == httpResponse.urlToIntercept) {


 ... (clipped 193 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Await asynchronous function call correctly

Add the await keyword when calling the asynchronous createCDPConnection function
to ensure the connection variable receives the resolved value, not a Promise.

javascript/selenium-webdriver/lib/webdriver.js [1580-1586]

 if (script.handle in this.pinnedScripts_) {
   let connection
   if (Object.is(this._cdpConnection, undefined)) {
-    connection = this.createCDPConnection('page')
+    connection = await this.createCDPConnection('page')
   } else {
     connection = this._cdpConnection
   }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 9

__

Why: This suggestion correctly identifies a missing await for the asynchronous createCDPConnection call within the unpinScript method, which would cause a runtime error.

High
Use send for CDP event commands

Replace connection.execute with connection.send for CDP commands inside the
register method's event handler to align with the ongoing refactoring.

javascript/selenium-webdriver/lib/webdriver.js [1362-1381]

 this._cdpWsConnection.on('message', (message) => {
   const params = JSON.parse(message)
 
   if (params.method === 'Fetch.authRequired') {
-    const requestParams = params['params']
-    connection.execute('Fetch.continueWithAuth', {
-      requestId: requestParams['requestId'],
+    const requestParams = params.params
+    connection.send('Fetch.continueWithAuth', {
+      requestId: requestParams.requestId,
       authChallengeResponse: {
         response: 'ProvideCredentials',
-        username: username,
-        password: password,
+        username,
+        password,
       },
     })
   } else if (params.method === 'Fetch.requestPaused') {
-    const requestPausedParams = params['params']
-    connection.execute('Fetch.continueRequest', {
-      requestId: requestPausedParams['requestId'],
+    const requestPausedParams = params.params
+    connection.send('Fetch.continueRequest', {
+      requestId: requestPausedParams.requestId,
     })
   }
 })

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 8

__

Why: This suggestion correctly points out that connection.execute calls were missed during the refactoring to connection.send within the register method, which would likely cause a runtime error.

Medium
Learned
best practice
Clean up temporary test resources

Ensure the temp profile directory is removed even if the test fails by adding an
after hook (or try/finally) that recursively deletes profileWithUserPrefs.

javascript/selenium-webdriver/test/firefox/options_test.js [47-53]

 before(async function createProfileWithUserPrefs() {
   profileWithUserPrefs = await io.tmpDir()
   await io.write(
     path.join(profileWithUserPrefs, 'user.js'),
     'user_pref("general.useragent.override", "foo;bar");\n',
   )
 })
 
+after(async function cleanupProfileWithUserPrefs() {
+  if (profileWithUserPrefs) {
+    await fs.promises.rm(profileWithUserPrefs, { recursive: true, force: true })
+    profileWithUserPrefs = null
+  }
+})
+

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 5

__

Why:
Relevant best practice - In tests that create external resources (temporary directories/profiles), always clean them up in a finally/after hook so failures don’t leak resources.

Low
  • More

@titusfortner titusfortner merged commit 4e3e8cb into trunk Dec 30, 2025
21 checks passed
@titusfortner titusfortner deleted the js_tests branch December 30, 2025 14:54
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 Review effort 2/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants