Update script commands to fix the failing BiDi tests by pujagani · Pull Request #13737 · SeleniumHQ/selenium

pujagani · 2024-03-26T06:16:56Z

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

W3C BiDi spec was updated to ensure the if browsing context is passed for adding the preload script command, it cannot be empty. This was implemented from Firefox 124 onwards. Updated the command accordingly.

Motivation and Context

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

I have read the contributing document.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

Type

enhancement, bug_fix

Description

Updated Script.java to dynamically include contexts in addPreloadScript commands based on non-empty browsingContextIds, enhancing flexibility and adherence to updated W3C BiDi spec.
Enabled preload script tests for Firefox in ScriptCommandsTest.java by removing @NotYetImplemented(FIREFOX) annotations, indicating these features are now supported.
Modified scriptManager.js in the JavaScript bindings to accept and handle multiple browsing context IDs, allowing for more versatile script management.
Adjusted related tests in script_test.js to reflect these changes, ensuring consistency and correctness in testing.

Changes walkthrough

Relevant files

Enhancement

Script.java `Refactor and Update Script Command for Browsing Contexts` java/src/org/openqa/selenium/bidi/module/Script.java Updated `addPreloadScript` methods to conditionally include `contexts` parameter based on `browsingContextIds` being non-empty. Refactored method parameters into a `Map` for dynamic construction of command parameters.	+37/-24
scriptManager.js `Support Multiple Browsing Contexts in ScriptManager` javascript/node/selenium-webdriver/bidi/scriptManager.js Modified `init` to accept multiple browsing context IDs. Adjusted preload script command to support multiple contexts. Fixed event subscription to use updated context IDs variable.	+12/-4

Tests

ScriptCommandsTest.java `Enable Preload Script Tests for Firefox` java/test/org/openqa/selenium/bidi/script/ScriptCommandsTest.java Removed `@NotYetImplemented(FIREFOX)` annotations, indicating support for Firefox in preload script tests. Minor adjustments to test assertions and parameters.	+0/-5
script_test.js `Update ScriptManager Tests for Browsing Context Changes` javascript/node/selenium-webdriver/test/bidi/script_test.js Updated tests to pass an array of browsing context IDs to `ScriptManager`. Renamed a test for clarity.	+8/-8

✨ PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

qodo-code-review · 2024-03-26T06:17:47Z

PR Description updated to latest commit (e6d52db)

Copy walkthrough table to "Files Changed" Tab

qodo-code-review · 2024-03-26T06:18:03Z

PR Review

⏱️ Estimated effort to review [1-5]	2, because the changes are mostly structural, involving updates to parameter handling in method calls. The logic is straightforward and mainly involves checking for empty browsing context IDs and updating method parameters accordingly. The changes are consistent across multiple methods, which simplifies the review process.
🧪 Relevant tests	Yes
🔍 Possible issues	Possible Bug: The changes assume `browsingContextIds` is always initialized and can be checked for emptiness. If there's any scenario where `browsingContextIds` could be `null`, this could lead to a NullPointerException. It might be safer to also check for nullity.
🔍 Possible issues	Consistency Issue: The JavaScript changes introduce checks for both array and string types of `browsingContextIds`. It's unclear if similar type flexibility is required or handled in the Java changes. Ensuring consistent handling of `browsingContextIds` types across different implementations could prevent potential bugs.
🔒 Security concerns	No

✨ Review tool usage guide:

Overview:
The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:

/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

With a configuration file, use the following template:

[pr_reviewer]
some_config1=...
some_config2=...

See the review usage page for a comprehensive guide on using this tool.

qodo-code-review · 2024-03-26T06:18:05Z

PR Code Suggestions

Category	Suggestions
Enhancement	Use `Collections.singletonMap` for single-entry maps. Consider using `Collections.singletonMap` for single-entry maps to improve readability and slightly enhance performance. java/src/org/openqa/selenium/bidi/module/Script.java [265-266] -parameters.put("functionDeclaration", functionDeclaration); +Map<String, Object> parameters = Collections.singletonMap("functionDeclaration", functionDeclaration);
Enhancement	Use template literals for constructing error messages. Use template literals for better readability when constructing error messages. javascript/node/selenium-webdriver/bidi/scriptManager.js [37] -throw Error('WebDriver instance must support BiDi protocol') +throw new Error(`WebDriver instance must support BiDi protocol but does not.`);
Maintainability	Refactor repeated code into a private method to reduce duplication. Refactor the repeated code for creating and populating the `parameters` map into a private method to reduce code duplication. java/src/org/openqa/selenium/bidi/module/Script.java [283-285] -Map<String, Object> parameters = new HashMap<>(); -parameters.put("functionDeclaration", functionDeclaration); -parameters.put("arguments", arguments); +private Map<String, Object> createParameters(String functionDeclaration, List<ChannelValue> arguments) { + Map<String, Object> parameters = new HashMap<>(); + parameters.put("functionDeclaration", functionDeclaration); + parameters.put("arguments", arguments); + return parameters; +}
Maintainability	Simplify logic by combining conditions for `this._browsingContextIds`. Combine the conditions checking `this._browsingContextIds` to simplify the logic and improve code readability. javascript/node/selenium-webdriver/bidi/scriptManager.js [176-182] -if (Array.isArray(this._browsingContextIds) && this._browsingContextIds.length > 0) { - params.contexts = this._browsingContextIds -} -if (typeof this._browsingContextIds === 'string') { - params.contexts = new Array(this._browsingContextIds) +if (this._browsingContextIds) { + params.contexts = Array.isArray(this._browsingContextIds) ? this._browsingContextIds : [this._browsingContextIds]; }
Possible issue	Use consistent property names within the class. Ensure consistent use of `this.bidi` and `this._bidi` within the class to avoid potential bugs related to accessing undefined properties. javascript/node/selenium-webdriver/bidi/scriptManager.js [40] -this.bidi = await this._driver.getBidi() +this._bidi = await this._driver.getBidi()

✨ Improve tool usage guide:

Overview:
The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:

/improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

With a configuration file, use the following template:

[pr_code_suggestions]
some_config1=...
some_config2=...

See the improve usage page for a comprehensive guide on using this tool.

diemol

Thank you, @pujagani!

…array

qodo-code-review Bot added P-enhancement PR with a new feature bug_fix labels Mar 26, 2024

qodo-code-review Bot added the Review effort [1-5]: 2 label Mar 26, 2024

diemol approved these changes Mar 26, 2024

View reviewed changes

pujagani added 3 commits March 26, 2024 08:23

[bidi][java] Update Script command to add check for browsing context …

8fdd727

…array

[java] Update tests

fab0e4a

[js] Ensure script command uses the browsing contexts correctly

64a3472

diemol force-pushed the update-script-command branch from e6d52db to 64a3472 Compare March 26, 2024 07:23

diemol merged commit 9931d0a into SeleniumHQ:trunk Mar 26, 2024

Navigation Menu

Uh oh!

Uh oh!

Update script commands to fix the failing BiDi tests - #13737

Update script commands to fix the failing BiDi tests#13737
diemol merged 3 commits into
SeleniumHQ:trunkfrom
pujagani:update-script-command

pujagani commented Mar 26, 2024 •

edited by qodo-code-review Bot

Loading

Uh oh!

qodo-code-review Bot commented Mar 26, 2024

Uh oh!

qodo-code-review Bot commented Mar 26, 2024

Uh oh!

qodo-code-review Bot commented Mar 26, 2024 •

edited

Loading

Uh oh!

diemol left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

pujagani commented Mar 26, 2024 • edited by qodo-code-review Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

Description

Motivation and Context

Types of changes

Checklist

Type

Description

Changes walkthrough

Uh oh!

qodo-code-review Bot commented Mar 26, 2024

Uh oh!

qodo-code-review Bot commented Mar 26, 2024

PR Review

Uh oh!

qodo-code-review Bot commented Mar 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions

Uh oh!

diemol left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pujagani commented Mar 26, 2024 •

edited by qodo-code-review Bot

Loading

qodo-code-review Bot commented Mar 26, 2024 •

edited

Loading