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]: fix storage module instance checks and add user context cookie test #15467

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

Conversation

navin772
Copy link
Member

@navin772 navin772 commented Mar 21, 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.

Motivation and Context

This PR fixes the instance checks for storage module and adds the Storage key to the map in case of the StorageKeyPartitionDescriptor class.

The PR adds a test for cookie in a user context and enables some other tests for browsers.

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

Bug fix, Tests


Description

  • Fixed instance checks for Storage module partition descriptors.

  • Added a new test for cookies in user contexts.

  • Enabled previously ignored cookie-related tests.

  • Introduced a utility function to detect hostname in fileserver.js.


Changes walkthrough 📝

Relevant files
Enhancement
partitionDescriptor.js
Add `type` key to `StorageKeyPartitionDescriptor` map       

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

  • Added type key to the internal map in StorageKeyPartitionDescriptor.
  • +1/-0     
    fileserver.js
    Add and export `getHostName` utility function                       

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

  • Added getHostName function to detect hostname.
  • Exported getHostName for external use.
  • +11/-0   
    Bug fix
    storage.js
    Fix partition descriptor instance checks in `Storage`       

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

  • Fixed instance checks for partition descriptors in getCookies,
    setCookie, and deleteCookies methods.
  • +3/-3     
    Tests
    storage_test.js
    Add and enable tests for cookie operations                             

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

  • Added a test for cookies in a user context.
  • Enabled previously ignored tests for cookie operations.
  • Added imports for new dependencies like StorageKeyPartitionDescriptor.
  • +71/-5   

    Need help?
  • Type /help how to ... in the comments thread for any questions 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

    Instance Check Fix

    The PR fixes instance checks for partition descriptors. The original code used an incorrect syntax for checking if an object is an instance of multiple types. JavaScript's instanceof operator doesn't work with parentheses grouping as it was attempted in the original code.

      !(partition instanceof BrowsingContextPartitionDescriptor || partition instanceof StorageKeyPartitionDescriptor)
    ) {
    Test Assertions

    The new test for cookies in user context has duplicate assertions. There are two identical blocks checking if cookies were found and if their values match, which is redundant and could be simplified.

    assert.strictEqual(result.cookies.length > 0, true, 'Expected at least one cookie')
    assert.strictEqual(result.cookies[0].value.value, cookie.value)
    
    // Check if cookies were found
    assert.strictEqual(result.cookies.length > 0, true, 'Expected at least one cookie')
    assert.strictEqual(result.cookies[0].value.value, cookie.value)

    Copy link
    Contributor

    qodo-merge-pro bot commented Mar 21, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Improve error message clarity
    Suggestion Impact:The commit implemented the suggested change by updating all three error messages in the file to be more specific about the expected types. The error messages now mention 'BrowsingContextPartitionDescriptor or StorageKeyPartitionDescriptor' instead of just 'PartitionDescriptor'.

    code diff:

    -      throw new Error(`Params must be an instance of PartitionDescriptor. Received:'${partition}'`)
    +      throw new Error(
    +        `Params must be an instance of BrowsingContextPartitionDescriptor or StorageKeyPartitionDescriptor. Received:'${partition}'`,
    +      )

    The error message should be more specific about the expected types. Currently it
    mentions "PartitionDescriptor" which is a parent class, but the check is
    specifically for BrowsingContextPartitionDescriptor or
    StorageKeyPartitionDescriptor.

    javascript/node/selenium-webdriver/bidi/storage.js [56-61]

     if (
       partition !== undefined &&
       !(partition instanceof BrowsingContextPartitionDescriptor || partition instanceof StorageKeyPartitionDescriptor)
     ) {
    -  throw new Error(`Params must be an instance of PartitionDescriptor. Received:'${partition}'`)
    +  throw new Error(`Params must be an instance of BrowsingContextPartitionDescriptor or StorageKeyPartitionDescriptor. Received:'${partition}'`)
     }

    [Suggestion has been applied]

    Suggestion importance[1-10]: 5

    __

    Why: The suggestion improves the error message by making it more specific about the expected types (BrowsingContextPartitionDescriptor or StorageKeyPartitionDescriptor) instead of the generic parent class (PartitionDescriptor). This enhances code clarity and debugging experience.

    Low
    • Update

    @navin772 navin772 requested review from pujagani and harsha509 March 21, 2025 07:45
    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