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

Introduce Polly.Testing package #1394

Merged
merged 10 commits into from
Jul 11, 2023
Merged

Introduce Polly.Testing package #1394

merged 10 commits into from
Jul 11, 2023

Conversation

martintmk
Copy link
Contributor

@martintmk martintmk commented Jul 11, 2023

Details on the issue fix or feature implementation

Contributes to #1367

With this relatively small package the consumer of Polly can assert the composition of resilience strategies with ease:

// arrange
var strategy = new ResilienceStrategyBuilder<string>()
    .AddFallback(new()
    {
        FallbackAction = _ => Outcome.FromResultAsTask("dummy"),
    })
    .AddRetry(new())
    .AddAdvancedCircuitBreaker(new())
    .AddTimeout(TimeSpan.FromSeconds(1))
    .AddHedging(new())
    .AddConcurrencyLimiter(10)
    .AddStrategy(new CustomStrategy())
    .ConfigureTelemetry(NullLoggerFactory.Instance)
    .Build();

// act
var descriptor = strategy.GetInnerStrategies();

// assert
descriptor.HasTelemetry.Should().BeTrue();
descriptor.Strategies.Should().HaveCount(7);
descriptor.Strategies[0].Options.Should().BeOfType<FallbackStrategyOptions<string>>();
descriptor.Strategies[1].Options.Should().BeOfType<RetryStrategyOptions<string>>();
descriptor.Strategies[2].Options.Should().BeOfType<AdvancedCircuitBreakerStrategyOptions<string>>();
descriptor.Strategies[3].Options.Should().BeOfType<TimeoutStrategyOptions>();
descriptor.Strategies[3].Options
    .Should()
    .BeOfType<TimeoutStrategyOptions>().Subject.Timeout
    .Should().Be(TimeSpan.FromSeconds(1));

descriptor.Strategies[4].Options.Should().BeOfType<HedgingStrategyOptions<string>>();
descriptor.Strategies[5].Options.Should().BeOfType<RateLimiterStrategyOptions>();
descriptor.Strategies[6].StrategyType.Should().Be(typeof(CustomStrategy));

cc @PeterCsalaHbo

Confirm the following

  • I started this PR by branching from the head of the default branch
  • I have targeted the PR to merge into the default branch
  • I have included unit tests for the issue/feature
  • I have successfully run a local build

@martintmk martintmk added the v8 Issues related to the new version 8 of the Polly library. label Jul 11, 2023
@martintmk martintmk added this to the v8.0.0 milestone Jul 11, 2023
Polly.sln Show resolved Hide resolved
src/Polly.Testing/Polly.Testing.csproj Outdated Show resolved Hide resolved
namespace Polly.Testing;

/// <summary>
/// This class provides additional information about <see cref="ResilienceStrategy"/>.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// This class provides additional information about <see cref="ResilienceStrategy"/>.
/// This class provides additional information about a <see cref="ResilienceStrategy"/>.

src/Polly.Testing/ResilienceStrategyDescriptor.cs Outdated Show resolved Hide resolved
src/Polly.Testing/ResilienceStrategyExtensions.cs Outdated Show resolved Hide resolved
src/Polly.Testing/ResilienceStrategyType.cs Outdated Show resolved Hide resolved
@PeterCsalaHbo
Copy link

@martintmk Could you please modify your assertions to include a sample how to check for the AddConcurrencyLimiter's parameter as well?

@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Merging #1394 (5b8fa65) into main (472e961) will increase coverage by 0.06%.
The diff coverage is 100.00%.

❗ Current head 5b8fa65 differs from pull request most recent head adb42c2. Consider uploading reports for the commit adb42c2 to get more accurate results

@@            Coverage Diff             @@
##             main    #1394      +/-   ##
==========================================
+ Coverage   83.73%   83.79%   +0.06%     
==========================================
  Files         270      273       +3     
  Lines        6449     6475      +26     
  Branches     1006     1010       +4     
==========================================
+ Hits         5400     5426      +26     
  Misses        840      840              
  Partials      209      209              
Flag Coverage Δ
linux ?
macos 83.79% <100.00%> (+0.06%) ⬆️
windows ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Polly.Core/ResilienceStrategy.cs 100.00% <100.00%> (ø)
src/Polly.Core/ResilienceStrategyBuilderBase.cs 100.00% <100.00%> (ø)
src/Polly.Testing/InnerStrategiesDescriptor.cs 100.00% <100.00%> (ø)
src/Polly.Testing/ResilienceStrategyDescriptor.cs 100.00% <100.00%> (ø)
src/Polly.Testing/ResilienceStrategyExtensions.cs 100.00% <100.00%> (ø)

@martincostello
Copy link
Member

I think this is lightweight enough to be useful to people - I don't think I'd use it myself personally, but this doesn't look like it would add much maintenance burden.

One thought that occurs to me: should be call this Testing or maybe it's more like Diagnostics or Introspection? Was just thinking to myself that someone could possible have a diagnostics endpoint in their application where they might want to check the configured pipeline at runtime - in that case having to have a dependency on a package called .Testing would look a bit suspect.

@martintmk
Copy link
Contributor Author

I think this is lightweight enough to be useful to people - I don't think I'd use it myself personally, but this doesn't look like it would add much maintenance burden.

One thought that occurs to me: should be call this Testing or maybe it's more like Diagnostics or Introspection? Was just thinking to myself that someone could possible have a diagnostics endpoint in their application where they might want to check the configured pipeline at runtime - in that case having to have a dependency on a package called .Testing would look a bit suspect.

It also shows people that we care about the testing and have native support for it.

One thought that occurs to me: should be call this Testing or maybe it's more like Diagnostics or Introspection?

I am just following the other naming convention such as Microsoft.AspNetCore.Mvc.Testing or Microsoft.Extensions.TimeProvider.Testing. It's not really something we should encourage people to use at runtime.

Was just thinking to myself that someone could possible have a diagnostics endpoint in their application where they might want to check the configured pipeline at runtime - in that case having to have a dependency on a package called .Testing would look a bit suspect.

If there is use-case for this in the future we can emit built-in telemetry without exposing any new API.

@martintmk
Copy link
Contributor Author

@martintmk Could you please modify your assertions to include a sample how to check for the AddConcurrencyLimiter's parameter as well?

Unfortunate thing is that RateLimiting is external API and we do not have it under our control. Assertion such as that is not possible without reflection.

@martintmk martintmk marked this pull request as ready for review July 11, 2023 12:04
@martintmk
Copy link
Contributor Author

I'll use this package to simplify tests such as these:

https://github.com/dotnet/extensions/blob/142fe1886a94e3f251e149f58e5e918161dcd7a5/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/StandardHedgingTests.cs#L166

@martintmk martintmk enabled auto-merge (squash) July 11, 2023 13:52
@martintmk martintmk merged commit 84a6d53 into main Jul 11, 2023
14 checks passed
@martintmk martintmk deleted the mtomka/testing-api branch July 11, 2023 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 Issues related to the new version 8 of the Polly library.
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants