Skip to content

[bidi][js] Add high-level logging API#14135

Merged
titusfortner merged 6 commits intoSeleniumHQ:trunkfrom
pujagani:add-high-level-logging
Jun 19, 2024
Merged

[bidi][js] Add high-level logging API#14135
titusfortner merged 6 commits intoSeleniumHQ:trunkfrom
pujagani:add-high-level-logging

Conversation

@pujagani
Copy link
Contributor

@pujagani pujagani commented Jun 14, 2024 •
edited by qodo-code-review bot
Loading

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

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Jun 14, 2024

    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.

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Jun 14, 2024 •
    edited by pujagani
    Loading

    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

    @pujagani pujagani mentioned this pull request Jun 14, 2024
    Open
    @titusfortner titusfortner merged commit 8960ff0 into SeleniumHQ:trunk Jun 19, 2024
    @seedysoft seedysoft mentioned this pull request Jun 21, 2024
    Merged
    @pujagani pujagani mentioned this pull request Jul 3, 2024
    Merged
    8 tasks
    @MiguelPelegrina MiguelPelegrina mentioned this pull request Jul 12, 2024
    Merged
    @DariuszPiskorowski DariuszPiskorowski mentioned this pull request Jul 13, 2024
    Closed
    @aravindb26 aravindb26 mentioned this pull request Jul 16, 2024
    Open
    @x-way x-way mentioned this pull request Jul 24, 2024
    Closed
    @DariuszPiskorowski DariuszPiskorowski mentioned this pull request Aug 9, 2024
    Merged
    @x-way x-way mentioned this pull request Aug 10, 2024
    Closed
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Oct 29, 2024
    @pujagani @titusfortner @sandeepsuryaprasad
    [bidi][js] Add high-level logging API (SeleniumHQ#14135)
    19f84eb 
    Related to SeleniumHQ#13992

Co-authored-by: Titus Fortner <titusfortner@users.noreply.github.com>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @qodo-code-review qodo-code-review[bot] qodo-code-review[bot] left review comments

    Assignees

    No one assigned

    Labels

    P-enhancement PR with a new feature Review effort [1-5]: 3

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @pujagani @titusfortner