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 dom mutation handlers #14238

Merged
merged 11 commits into from
Jul 11, 2024

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Jul 9, 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

Related to #13992.

Motivation and Context

Support dom-mutation using BiDi.

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 DOM mutation listener script to handle attribute changes and send details through a channel.
  • Integrated the DOM mutation listener script into the script management system.
  • Added methods to add and remove DOM mutation handlers.
  • Implemented tests to verify the functionality of DOM mutation handlers.
  • Updated build configurations to include the new DOM mutation listener script.

Changes walkthrough 📝

Relevant files
Enhancement
bidi-mutation-listener.js
Add DOM mutation listener script                                                 

javascript/bidi-support/bidi-mutation-listener.js

  • Added a new file to handle DOM mutations.
  • Implemented a MutationObserver to listen for attribute changes.
  • Sends mutation details through a channel.
  • +55/-0   
    script.js
    Integrate DOM mutation listener in script management         

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

  • Integrated the DOM mutation listener script.
  • Added methods to add and remove DOM mutation handlers.
  • Implemented callback mechanism for DOM mutations.
  • +56/-0   
    Tests
    webdriver_script_test.js
    Add tests for DOM mutation handlers                                           

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

  • Added tests for DOM mutation handlers.
  • Verified the addition and removal of DOM mutation handlers.
  • +38/-0   
    Configuration changes
    BUILD.bazel
    Add build configuration for DOM mutation listener               

    javascript/bidi-support/BUILD.bazel

    • Added build configuration for the DOM mutation listener script.
    +10/-0   
    BUILD.bazel
    Update build dependencies for DOM mutation listener           

    javascript/node/selenium-webdriver/BUILD.bazel

  • Updated build dependencies to include the DOM mutation listener
    script.
  • +1/-0     
    BUILD.bazel
    Add copy file rule for DOM mutation listener                         

    javascript/node/selenium-webdriver/lib/atoms/BUILD.bazel

    • Added copy file rule for the DOM mutation listener script.
    +7/-0     

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

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Possible Bug:
    The mutation observer in bidi-mutation-listener.js only handles attribute changes and ignores other types of mutations such as childList or characterData. This might not meet the requirements if the intention was to handle all types of DOM mutations.

    Performance Concern:
    The mutation observer setup in bidi-mutation-listener.js may lead to performance issues for pages with high mutation rates since it observes all attributes changes, all character data changes, and all subtree modifications without any throttling or debouncing.

    Code Quality:
    In script.js, the method addDomMutationHandler uses a hardcoded file path which could lead to issues if the file structure changes or if executed in a different environment.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Use crypto.randomUUID() for generating unique IDs instead of Math.random()

    To avoid potential issues with Math.random() generating non-unique IDs, consider using a
    more robust method for generating unique IDs, such as crypto.randomUUID().

    javascript/bidi-support/bidi-mutation-listener.js [30]

    -id = Math.random().toString(36).substring(2) + Date.now().toString(36);
    +id = crypto.randomUUID();
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Using crypto.randomUUID() enhances the uniqueness and security of the ID generation, addressing a potential issue in the PR code.

    8
    Add error handling for the fs.readFileSync call to manage file access issues

    Add error handling for the fs.readFileSync call to manage cases where the file might not
    be found or accessible, preventing potential runtime errors.

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

    -let mutationListener = fs.readFileSync(path.resolve(filePath), 'utf-8').toString()
    +let mutationListener;
    +try {
    +  mutationListener = fs.readFileSync(path.resolve(filePath), 'utf-8').toString();
    +} catch (error) {
    +  console.error('Error reading mutation listener file:', error);
    +  throw error;
    +}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding error handling for file reading operations is crucial to prevent runtime errors and improve the robustness of the code.

    7
    Maintainability
    Use a configuration or environment variable for the file path to improve flexibility

    Instead of hardcoding the file path for bidi-mutation-listener.js, consider using a
    configuration or environment variable to make the path more flexible and maintainable.

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

    -const filePath = '../../../bazel-bin/javascript/node/selenium-webdriver/lib/atoms/bidi-mutation-listener.js'
    +const filePath = process.env.BIDI_MUTATION_LISTENER_PATH || '../../../bazel-bin/javascript/node/selenium-webdriver/lib/atoms/bidi-mutation-listener.js'
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using environment variables for file paths increases flexibility and maintainability, though it's not a critical issue.

    6
    Best practice
    Add a timeout to the DOM mutation handler test to ensure it fails gracefully if the event does not occur

    Add a timeout to the addDomMutationHandler test to ensure it fails gracefully if the
    mutation event does not occur within a reasonable time frame.

    javascript/node/selenium-webdriver/test/lib/webdriver_script_test.js [92-94]

     await driver.script().addDomMutationHandler((m) => {
       message = m
    -})
    +});
    +await new Promise((resolve) => setTimeout(resolve, 5000)); // 5 seconds timeout
     
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Adding a timeout is a good practice to ensure tests fail gracefully, but it's a minor improvement in the context of overall test robustness.

    5

    @pujagani pujagani merged commit 0f68841 into SeleniumHQ:trunk Jul 11, 2024
    10 of 11 checks passed
    @StanislavKharchenko
    Copy link

    Is some documentation how to use this functionality?
    Perhaps it can be useful to web driver waits until DOM changing in the tree (like waitForAngular in the deprecated "Protractor" framework)

    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