search

SeleniumHQ / selenium

A browser automation framework and ecosystem.
https://selenium.dev
Apache License 2.0
29.77k stars 8.02k forks source link

[bidi][js] Add types for user prompt related events #14097

Closed pujagani closed 3 weeks ago

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

Add user prompt types for events "UserPromptOpened" and "UserPromptClosed". Else it was defaulting to the wrong type.

Motivation and Context

Types of changes

Checklist

PR Type

Enhancement, Tests

Description

Changes walkthrough 📝

Relevant files
Enhancement
browsingContextInspector.js
Add handling for user prompt events in browsing context inspector
javascript/node/selenium-webdriver/bidi/browsingContextInspector.js
  • Added imports for UserPromptOpened and UserPromptClosed.
  • Implemented handling for UserPromptOpened and UserPromptClosed events.

    		•  +4/-2     
    browsingContextTypes.js
    Define classes for user prompt events                                       
    javascript/node/selenium-webdriver/bidi/browsingContextTypes.js
  • Added UserPromptOpened class.
  • Added UserPromptClosed class.
  • Exported new classes.
    		•  +17/-1   
    Tests
    browsingcontext_inspector_test.js
    Add tests for user prompt events in browsing context inspector
    javascript/node/selenium-webdriver/test/bidi/browsingcontext_inspector_test.js
  • Added tests for UserPromptOpened event.
  • Added tests for UserPromptClosed event.
    		•  +47/-32 

    💡 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 localized to specific functionalities related to user prompt events in a JavaScript codebase. The modifications are straightforward and involve adding new classes and handling logic for these events.
    🧪 Relevant tests Yes
    ⚡ Possible issues Possible Bug: The new event handling for 'UserPromptOpened' and 'UserPromptClosed' might not work as expected in Chrome and Edge due to a noted discrepancy in how these browsers handle browsing context IDs. This could lead to failed assertions as noted in the test comments.
    🔒 Security concerns No
    codiumai-pr-agent-pro[bot] commented 3 weeks ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Use a switch statement instead of multiple if-else statements for better readability and maintainability ___ **To improve readability and maintainability, consider using a switch statement instead of
    multiple if-else statements for determining the type of response to create. This will make
    the code easier to extend in the future.** [javascript/node/selenium-webdriver/bidi/browsingContextInspector.js [127-136]](https://github.com/SeleniumHQ/selenium/pull/14097/files#diff-7fbbddd435d48230ba46bb41098623933971e58ee2dd073d0a0f8995633b495dR127-R136) ```diff let response = null -if ('navigation' in params) { - response = new NavigationInfo(params.context, params.navigation, params.timestamp, params.url) -} else if ('type' in params) { - response = new UserPromptOpened(params.context, params.type, params.message) -} else if ('accepted' in params) { - response = new UserPromptClosed(params.context, params.accepted, params.userText) -} else { - response = new BrowsingContextInfo(params.context, params.url, params.children, params.parent) +switch (true) { + case 'navigation' in params: + response = new NavigationInfo(params.context, params.navigation, params.timestamp, params.url) + break + case 'type' in params: + response = new UserPromptOpened(params.context, params.type, params.message) + break + case 'accepted' in params: + response = new UserPromptClosed(params.context, params.accepted, params.userText) + break + default: + response = new BrowsingContextInfo(params.context, params.url, params.children, params.parent) } ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 7 Why: The suggestion to use a `switch` statement improves readability and maintainability, making it easier to manage and extend the code. This is a valid improvement for the given context.
    		7
    Possible issue
    Add a timeout to the await driver.wait(until.alertIsPresent()) calls to handle potential asynchronous issues ___ **To ensure the test cases are more robust and handle potential asynchronous issues,
    consider adding a timeout to the await driver.wait(until.alertIsPresent()) calls.** [javascript/node/selenium-webdriver/test/bidi/browsingcontext_inspector_test.js [179]](https://github.com/SeleniumHQ/selenium/pull/14097/files#diff-5a795b4366203e58bdf8bb6749ce666b85a150f2ca2b8d3e9970039347f5881aR179-R179) ```diff -await driver.wait(until.alertIsPresent()) +await driver.wait(until.alertIsPresent(), 5000) ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 6 Why: Adding a timeout is a good practice to avoid potential infinite waiting issues in asynchronous operations. This suggestion enhances the robustness of the test cases.
    		6
    Best practice
    Add default values for the type and message parameters in the UserPromptOpened constructor ___ **To ensure consistency and avoid potential issues, consider adding default values for the
    type and message parameters in the UserPromptOpened constructor.** [javascript/node/selenium-webdriver/bidi/browsingContextTypes.js [84-87]](https://github.com/SeleniumHQ/selenium/pull/14097/files#diff-0c65ec6304c400b7d144c63e0bc86f13cae0c98cfe1bbd493e474724de11b908R84-R87) ```diff -constructor(browsingContextId, type, message) { +constructor(browsingContextId, type = '', message = '') { this.browsingContextId = browsingContextId this.type = type this.message = message } ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 5 Why: Adding default values for parameters can prevent runtime errors and make the function calls more predictable. However, this is a minor improvement as it depends on the specific use case and requirements.
    		5
    pujagani commented 3 weeks ago

    The test failures are due to connection errors and not due to the changes. Locally, it is working as expected.