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

[js][bidi]: implement bidi setCacheBehavior command #15136

Open
wants to merge 6 commits into
base: trunk
Choose a base branch
from

Conversation

navin772
Copy link
Contributor

@navin772 navin772 commented Jan 23, 2025

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

This PR adds the bidi method setCacheBehavior defined in the bidi specs for the JS bindings - https://w3c.github.io/webdriver-bidi/#command-network-setCacheBehavior.

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 setCacheBehavior method to manage network cache behavior.

  • Implemented validation for cache behavior and context IDs.

  • Added tests for setCacheBehavior covering valid and invalid scenarios.

  • Enhanced BiDi network functionality in JavaScript bindings.


Changes walkthrough 📝

Relevant files
Enhancement
network.js
Implement `setCacheBehavior` method in network module       

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

  • Added setCacheBehavior method to set network cache behavior.
  • Included validation for cache behavior values and context IDs.
  • Integrated BiDi command for network.setCacheBehavior.
  • +27/-0   
    Tests
    network_test.js
    Add tests for `setCacheBehavior` method                                   

    javascript/node/selenium-webdriver/test/bidi/network_test.js

  • Added test cases for setCacheBehavior method.
  • Covered valid behaviors (default, bypass) with and without contexts.
  • Tested error handling for invalid cache behavior values.
  • +33/-0   

    Need help?
  • Type /help how to ... in the comments thread for any question about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Input Validation

    The contexts parameter validation is missing. The code should validate that contexts is either null or a non-empty array of valid context IDs before sending the command.

    if (contexts) {
      command.params.contexts = contexts
    }

    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 23, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    ✅ Validate array input parameters
    Suggestion Impact:The commit implemented array validation for contexts parameter with similar but more comprehensive checks

    code diff:

    +    if (contexts !== null) {
    +      if (
    +        !Array.isArray(contexts) ||
    +        contexts.length === 0 ||
    +        contexts.some((c) => typeof c !== 'string' || c.trim() === '')
    +      ) {
    +        throw new Error('Contexts must be an array of non-empty strings')
    +      }

    Add input validation for contexts array to ensure it contains only valid, non-empty
    strings when provided

    javascript/node/selenium-webdriver/bidi/network.js [378-380]

     if (contexts) {
    +  if (!Array.isArray(contexts) || contexts.some(c => !c || typeof c !== 'string')) {
    +    throw new Error('Contexts must be an array of non-empty strings')
    +  }
       command.params.contexts = contexts
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding validation for the contexts parameter is important for preventing runtime errors and ensuring data integrity. The suggestion adds proper type checking and validation that was missing from the original implementation.

    8
    General
    ✅ Add missing error test case
    Suggestion Impact:The commit added a test case for invalid context ID types, which aligns with the suggestion's goal of testing invalid contexts parameter handling

    code diff:

    +      it('throws error for invalid context id types', async function () {
    +        await driver.get(Pages.emptyPage)
    +        await assert.rejects(
    +          async () => await network.setCacheBehavior('default', ''),
    +          /Contexts must be an array of non-empty strings/,
    +        )
    +        await assert.rejects(
    +          async () => await network.setCacheBehavior('default', ['', ' ']),
    +          /Contexts must be an array of non-empty strings/,
    +        )
    +      })

    Add test case to verify error handling when contexts parameter contains invalid
    values

    javascript/node/selenium-webdriver/test/bidi/network_test.js [240-246]

     it('throws error for invalid cache behavior', async function () {
       await driver.get(Pages.emptyPage)
       await assert.rejects(
         async () => await network.setCacheBehavior('invalid'),
         /Cache behavior must be either "default" or "bypass"/,
       )
    +  await assert.rejects(
    +    async () => await network.setCacheBehavior('default', [null, 123, '']),
    +    /Contexts must be an array of non-empty strings/,
    +  )
     })
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves test coverage by adding validation for invalid contexts parameter, which complements the first suggestion and helps ensure the error handling works correctly.

    7

    * @throws {Error} If behavior is invalid or context IDs are invalid
    */
    async setCacheBehavior(behavior, contexts = null) {
    if (!['default', 'bypass'].includes(behavior)) {
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Please help add enum-like structure for 'default' and 'bypass' values.

    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.

    3 participants