Skip to content
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

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

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Jun 7, 2024

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

  • 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

Enhancement, Tests


Description

  • Added handling for UserPromptOpened and UserPromptClosed events in browsingContextInspector.js.
  • Defined new classes UserPromptOpened and UserPromptClosed in browsingContextTypes.js.
  • Added tests for UserPromptOpened and UserPromptClosed events in browsingcontext_inspector_test.js.

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

    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 7, 2024

    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

    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 7, 2024

    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]

     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]

    -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]

    -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
    Copy link
    Contributor Author

    pujagani commented Jun 7, 2024

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

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant