[bidi][js] Enable tests

pujagani commented 3 weeks ago

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

Enable tests since they should work now in browsers with increased BiDi support.

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.

PR Type

Tests

Description

Enabled multiple previously disabled tests in the BiDi test suite.
Tests include listening to user prompt events, reloading browsing contexts, and locating nodes using various methods.

Changes walkthrough 📝

Relevant files

Tests

browsingcontext_inspector_test.js `Enable user prompt event tests in browsing context inspector` javascript/node/selenium-webdriver/test/bidi/browsingcontext_inspector_test.js Enabled the test for listening to user prompt opened event. Enabled the test for listening to user prompt closed event.	+2/-2
browsingcontext_test.js `Enable browsing context reload test` javascript/node/selenium-webdriver/test/bidi/browsingcontext_test.js - Enabled the test for reloading a browsing context.	+1/-1
locate_nodes_test.js `Enable node location tests in locate nodes` javascript/node/selenium-webdriver/test/bidi/locate_nodes_test.js Enabled the test for locating node with xpath locator. Enabled the test for locating node with max node count. Enabled the test for locating node with given start nodes.	+3/-3

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

codiumai-pr-agent-pro[bot] commented 3 weeks ago

PR Review 🔍

⏱️ Estimated effort to review [1-5]	2, because the changes are straightforward, involving enabling previously disabled tests by changing 'xit' to 'it'. The PR is focused and limited to test files, which simplifies the review process.
🧪 Relevant tests	Yes
⚡ Possible issues	No
🔒 Security concerns	No

codiumai-pr-agent-pro[bot] commented 3 weeks ago

PR Code Suggestions ✨

Category	Suggestion	Score
Possible issue	Add an event listener for the user prompt event before initializing `userpromptOpened` ___ To ensure that the `userpromptOpened` event is properly captured, you should add an event listener for the prompt event before initializing `userpromptOpened`. [javascript/node/selenium-webdriver/test/bidi/browsingcontext_inspector_test.js [162-163]](https://github.com/SeleniumHQ/selenium/pull/14090/files#diff-5a795b4366203e58bdf8bb6749ce666b85a150f2ca2b8d3e9970039347f5881aR162-R163) ```diff let userpromptOpened = null +driver.on('userprompt', (event) => { userpromptOpened = event; }); const browsingConextInspector = await BrowsingContextInspector(driver) ``` Suggestion importance[1-10]: 8 Why: The suggestion correctly identifies a potential issue where the `userpromptOpened` event might not be captured due to the lack of an event listener. This is crucial for the functionality being tested.	8
Possible issue	Add an event listener for the user prompt event before initializing `userpromptClosed` ___ To ensure that the `userpromptClosed` event is properly captured, you should add an event listener for the prompt event before initializing `userpromptClosed`. [javascript/node/selenium-webdriver/test/bidi/browsingcontext_inspector_test.js [184-185]](https://github.com/SeleniumHQ/selenium/pull/14090/files#diff-5a795b4366203e58bdf8bb6749ce666b85a150f2ca2b8d3e9970039347f5881aR184-R185) ```diff let userpromptClosed = null +driver.on('userprompt', (event) => { userpromptClosed = event; }); const browsingConextInspector = await BrowsingContextInspector(driver) ``` Suggestion importance[1-10]: 8 Why: Similar to the first suggestion, this one correctly addresses the need for an event listener to capture the `userpromptClosed` event, which is essential for the test's accuracy and completeness.	8
Possible bug	Add a check to verify that the `browsingContextId` is not null or undefined before initializing `browsingContext` ___ To ensure that the `browsingContext` is properly initialized, you should add a check to verify that the `browsingContextId` is not null or undefined. [javascript/node/selenium-webdriver/test/bidi/locate_nodes_test.js [87-88]](https://github.com/SeleniumHQ/selenium/pull/14090/files#diff-d69c46b8235285967e4b9734bfd4b1b4d7a401f4c80086cb2663cef1f43c8298R87-R88) ```diff +if (!id) { + throw new Error('Failed to get browsing context ID'); +} const browsingContext = await BrowsingContext(driver, { browsingContextId: id, }) ``` Suggestion importance[1-10]: 7 Why: This suggestion improves the robustness of the code by ensuring that a valid `browsingContextId` is available before proceeding, thus preventing potential runtime errors.	7

SeleniumHQ / selenium