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 high-level logging API #14135

Merged
merged 6 commits into from
Jun 19, 2024

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Jun 14, 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

Partial implementation of #13992

Motivation and Context

Provide high-level API that is easy for users to use.

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 a new Script class to handle JavaScript error and console message logging.
  • Integrated the Script class into the WebDriver class with a new script method.
  • Enhanced error handling in LogInspector for removing non-existent callbacks.
  • Added comprehensive tests for the Script class methods, including error handling.

Changes walkthrough 📝

Relevant files
Enhancement
logInspector.js
Add error handling for callback removal                                   

javascript/node/selenium-webdriver/bidi/logInspector.js

  • Added error handling for removing non-existent callbacks.
  • Introduced a check to verify if the callback ID exists before
    deletion.
  • +6/-0     
    script.js
    Introduce Script class for logging handlers                           

    javascript/node/selenium-webdriver/lib/script.js

  • Introduced a new Script class to handle JavaScript error and console
    message logging.
  • Added methods to add and remove JavaScript error and console message
    handlers.
  • +63/-0   
    webdriver.js
    Integrate Script class into WebDriver                                       

    javascript/node/selenium-webdriver/lib/webdriver.js

  • Integrated the new Script class into the WebDriver class.
  • Added a script method to initialize and return a Script instance.
  • +12/-0   
    Tests
    webdriver_script_test.js
    Add tests for Script class logging handlers                           

    javascript/node/selenium-webdriver/test/lib/webdriver_script_test.js

  • Added tests for the new Script class methods.
  • Included tests for adding and removing JavaScript error and console
    message handlers.
  • Added a test for error handling when removing a non-existent handler.
  • +91/-0   

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

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5]

    3

    🧪 Relevant tests

    Yes

    🔒 Security concerns

    No

    ⚡ Key issues to review

    Error Handling:
    The removeCallback method in logInspector.js now throws an error if the callback ID is not found. Ensure that all calling code properly handles this exception.

    Initialization Pattern:
    The #init method in Script class uses a pattern that might be confusing as it mixes async initialization with regular constructor usage. Consider documenting this pattern clearly or revising the approach to initialization.

    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 14, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Return early after deleting the callback to simplify the code

    Instead of using a let variable to track if the callback ID was found, you can return
    early once the callback is deleted. This will make the code more concise and potentially
    more efficient.

    javascript/node/selenium-webdriver/bidi/logInspector.js [69-79]

    -let hasId = false
     for (const [, callbacks] of this.listener) {
       if (callbacks.has(id)) {
         callbacks.delete(id)
    -    hasId = true
    +    return
       }
     }
     
    -if (!hasId) {
    -  throw Error(`Callback with id ${id} not found`)
    -}
    +throw Error(`Callback with id ${id} not found`)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly identifies an opportunity to simplify the code by using an early return instead of a flag variable. This change enhances readability and efficiency.

    8
    Performance
    Initialize the #logInspector in the constructor to avoid multiple initializations

    The #init method should be called only once in the constructor to avoid multiple
    initializations. You can use a flag to check if initialization is complete.

    javascript/node/selenium-webdriver/lib/script.js [24-39]

     constructor(driver) {
       this.#driver = driver
    +  this.#initPromise = this.#init()
     }
     
     async #init() {
       if (this.#logInspector !== undefined) {
         return
       }
       this.#logInspector = await logInspector(this.#driver)
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion is valid and improves the initialization pattern of #logInspector, ensuring it's done once, which is a good practice for performance and maintainability.

    7
    Possible issue
    Mark the script method as async to ensure proper initialization

    The script method should be marked as async to ensure that the Script instance is fully
    initialized before being used.

    javascript/node/selenium-webdriver/lib/webdriver.js [1109-1117]

    -script() {
    +async script() {
       if (this.#script === undefined) {
         this.#script = new Script(this)
    +    await this.#script.#init()
       }
     
       return this.#script
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: The suggestion correctly points out the need for the script method to be asynchronous to ensure the Script instance is fully initialized. This is important for correct operation but is a moderate issue.

    6
    Add a timeout to the delay function to prevent infinite waits

    Add a timeout to the delay function to avoid potential infinite waits if the promise is
    not resolved.

    javascript/node/selenium-webdriver/test/lib/webdriver_script_test.js [37-39]

     function delay(ms) {
    -  return new Promise((resolve) => setTimeout(resolve, ms))
    +  return new Promise((resolve, reject) => {
    +    const timeout = setTimeout(resolve, ms)
    +    setTimeout(() => {
    +      clearTimeout(timeout)
    +      reject(new Error('Delay timed out'))
    +    }, ms + 5000)
    +  })
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Adding a timeout to the delay function is a good practice to avoid potential infinite waits. However, the context of usage (in tests) and the specific implementation suggested might not be necessary or optimal, thus the moderate score.

    5

    @titusfortner titusfortner merged commit 8960ff0 into SeleniumHQ:trunk Jun 19, 2024
    12 of 14 checks passed
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Oct 29, 2024
    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.

    2 participants