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

[dotnet] [bidi] Add network SetCacheBehavior command #15133

Merged
merged 3 commits into from
Jan 22, 2025

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Jan 22, 2025

User description

Description

Expanding BiDi coverage in .NET

Motivation and Context

Fixes #14532

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 command to the Network module in .NET BiDi.

  • Implemented SetCacheBehaviorCommand and related parameters and options.

  • Updated BrowsingContextNetworkModule and NetworkModule to support cache behavior configuration.

  • Added unit tests to validate SetCacheBehavior functionality.


Changes walkthrough 📝

Relevant files
Enhancement
Command.cs
Register `SetCacheBehaviorCommand` in command types           

dotnet/src/webdriver/BiDi/Communication/Command.cs

  • Registered SetCacheBehaviorCommand as a derived type for network
    commands.
  • +1/-0     
    BrowsingContextNetworkModule.cs
    Add `SetCacheBehaviorAsync` to BrowsingContextNetworkModule

    dotnet/src/webdriver/BiDi/Modules/BrowsingContext/BrowsingContextNetworkModule.cs

  • Added SetCacheBehaviorAsync method to configure cache behavior.
  • Integrated SetCacheBehaviorOptions for browsing context.
  • +10/-0   
    NetworkModule.cs
    Implement `SetCacheBehaviorAsync` in NetworkModule             

    dotnet/src/webdriver/BiDi/Modules/Network/NetworkModule.cs

  • Implemented SetCacheBehaviorAsync method in NetworkModule.
  • Added support for SetCacheBehaviorCommandParameters and options.
  • +12/-0   
    SetCacheBehaviorCommand.cs
    Add `SetCacheBehaviorCommand` and related classes               

    dotnet/src/webdriver/BiDi/Modules/Network/SetCacheBehaviorCommand.cs

  • Created SetCacheBehaviorCommand class and parameters.
  • Defined SetCacheBehaviorOptions and CacheBehavior enum.
  • +48/-0   
    Tests
    NetworkTest.cs
    Add unit tests for `SetCacheBehavior`                                       

    dotnet/test/common/BiDi/Network/NetworkTest.cs

  • Added unit tests for SetCacheBehavior functionality.
  • Verified no exceptions are thrown during cache behavior configuration.
  • +7/-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:

    🎫 Ticket compliance analysis ✅

    14532 - Fully compliant

    Compliant requirements:

    • Implement SetCacheBehavior command in .NET BiDi Network module according to W3C WebDriver BiDi spec
    • Support cache behavior configuration through the Network module
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Test Coverage

    The test only verifies that no exceptions are thrown but doesn't validate the actual behavior change when different cache behaviors are set

    public void CanSetCacheBehavior()
    {
        Assert.That(async () => await bidi.Network.SetCacheBehaviorAsync(CacheBehavior.Default), Throws.Nothing);
        Assert.That(async () => await context.Network.SetCacheBehaviorAsync(CacheBehavior.Default), Throws.Nothing);
    }
    Documentation

    The CacheBehavior enum values lack XML documentation comments explaining their purpose and impact

    public enum CacheBehavior
    {
        Default,
        Bypass
    }

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Strengthen cache behavior testing

    The test should verify actual cache behavior changes by making repeated requests and
    checking cache headers or response times, not just that no exceptions are thrown.

    dotnet/test/common/BiDi/Network/NetworkTest.cs [216-220]

     [Test]
    -public void CanSetCacheBehavior()
    +public async Task CanSetCacheBehavior()
     {
    -    Assert.That(async () => await bidi.Network.SetCacheBehaviorAsync(CacheBehavior.Default), Throws.Nothing);
    -    Assert.That(async () => await context.Network.SetCacheBehaviorAsync(CacheBehavior.Default), Throws.Nothing);
    +    await bidi.Network.SetCacheBehaviorAsync(CacheBehavior.Default);
    +    var response1 = await context.NavigateAsync(UrlBuilder.WhereIs("cacheable"));
    +    var response2 = await context.NavigateAsync(UrlBuilder.WhereIs("cacheable"));
    +    Assert.That(response2.FromCache, Is.True, "Response should be cached");
    +    
    +    await bidi.Network.SetCacheBehaviorAsync(CacheBehavior.Bypass);
    +    var response3 = await context.NavigateAsync(UrlBuilder.WhereIs("cacheable"));
    +    Assert.That(response3.FromCache, Is.False, "Response should bypass cache");
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion significantly improves test quality by adding actual verification of cache behavior functionality instead of just checking for exceptions. The proposed test properly validates both default and bypass cache behaviors with real HTTP requests.

    8

    Copy link
    Contributor

    @RenderMichael RenderMichael left a comment

    Choose a reason for hiding this comment

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

    Looks nice!

    @nvborisenko nvborisenko merged commit 43c6e35 into SeleniumHQ:trunk Jan 22, 2025
    10 checks passed
    @nvborisenko nvborisenko deleted the dotnet-bidi-setcache branch January 22, 2025 21:20
    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.

    [🚀 Feature]: [dotnet] [bidi] Support SetCacheBehavior in Network module
    2 participants