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? #1367

Closed
martintmk opened this issue Jun 29, 2023 · 13 comments
Closed

Introduce Polly.Testing package? #1367

martintmk opened this issue Jun 29, 2023 · 13 comments
Labels
v8 Issues related to the new version 8 of the Polly library.
Milestone

Comments

@martintmk
Copy link
Contributor

When adopting Polly V8 I thought having package as this might be useful for authors or adopters of Polly.

It would simplify some of the common scenarios:

  • Assertion of strategies in the pipeline (is my pipeline constructed correctly?)
  • Assertion of telemetry enrichers.

Basically, we could just rename and polish https://github.com/App-vNext/Polly/tree/main/test/Polly.TestUtils and publish it as a package.

Wdyt?

@martintmk martintmk added the v8 Issues related to the new version 8 of the Polly library. label Jun 29, 2023
@martintmk martintmk added this to the v8.0.0 milestone Jun 29, 2023
@martincostello
Copy link
Member

I'll ponder this further as I try v8 out more in some internal projects.

Typically I discourage people from testing the Polly implementation in of itself (like mocking things), and more encourage people to observe the effects of its usage with the real thing, as that often finds misconfigurations that mocking doesn't. Plus a no-op pipeline is easy to configure.

I'm not against the idea as such, but it's more to ship and maintain if the amount of things actually useful to others (compared to our own test suite) is negligible.

@PeterCsalaHbo
Copy link

PeterCsalaHbo commented Jun 29, 2023

Most of the cases (what I have seen) the policy parameters (like maxRetryCount, sleepDuration, etc.) are coming from configuration files.

It is pretty easy to use the wrong configuration value (unintentionally) in a given policy/strategy setup. (related issue)

So, providing built-in assertions against policy/strategy parameters after they have been built IMHO is really valuable.

@martintmk
Copy link
Contributor Author

Typically I discourage people from testing the Polly implementation in of itself (like mocking things), and more encourage people to observe the effects of its usage with the real thing, as that often finds misconfigurations that mocking doesn't. Plus a no-op pipeline is easy to configure.

I agree here. The behavior of strategies should not be tested. One thing I wanted to test was the composition of strategies, basically that the final strategy I am composing contains all necessary inner-strategies.

For example:
https://github.com/dotnet/extensions/blob/50330cb5a4dd8fdd4baff5fe678d86372895971d/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/StandardHedgingTests.cs#L191

The test relies on internal naming in Polly and that's why I thought we could provide something to simplify the scenario. Basically, allow some assertions to check that the retry/timeout, etc. is present in the pipeline.

I'm not against the idea as such, but it's more to ship and maintain if the amount of things actually useful to others (compared to our own test suite) is negligible.

Agree. We might see if there are more requests from adopters and then decide to publish new package.

It is pretty easy to use the wrong configuration value (unintentionally) in a given policy/strategy setup.

Not anymore, all options are verified when the strategy is being build :). One thing that can come handy is allowing to also assert that values for the options have expected values.

@martincostello
Copy link
Member

The test relies on internal naming in Polly and that's why I thought we could provide something to simplify the scenario. Basically, allow some assertions to check that the retry/timeout, etc. is present in the pipeline.

That might be helpful though. In the code I'm migrating for #1365 the tests previously asserted on the property key to imply the types configured via the convention the code used.

Maybe there's a way we could provide a way to make it easier to introspect a configured pipeline in some way as a first-class API feature (not something test-specific)? It might make debugging easier too (e.g. custom debugger proxies or similar).

@martintmk
Copy link
Contributor Author

martintmk commented Jun 30, 2023

Maybe there's a way we could provide a way to make it easier to introspect a configured pipeline in some way as a first-class API feature (not something test-specific)? It might make debugging easier too (e.g. custom debugger proxies or similar).

I am really hesitant to do this, because the consuming code should not care what and how many inner strategies are inside a pipeline. The only use-case for this are the tests. Ideally we would have the option to do this:

// build pipeline
var strategy = new ResilienceStrategyBuilder()
    .AddRetry(new())
    .AddAdvancedCircuitBreaker(new())
    .AddTimeout(new TimeoutStrategyOptions())
    .AddStrategy(new MyStrategy())
    .Build();

// assert pipeline
var strategies = strategy.GetInnerStrategies();
strategies[0].GetStrategyType().Should().Be(ResilienceStrategyType.Retry);
strategies[1].GetStrategyType().Should().Be(ResilienceStrategyType.CircuitBreaker);
strategies[2].GetStrategyType().Should().Be(ResilienceStrategyType.RateLimiter);
strategies[3].GetStrategyType().Should().Be(ResilienceStrategyType.Unknown); // custom one
strategies[3].GetType().Should().BeOfType<MyStrategy>(); // yay, we can see MyStrategy

And the only clean place where to put this (at least to me) is the Polly.Testing package.

@martintmk
Copy link
Contributor Author

It might make debugging easier too (e.g. custom debugger proxies or similar).

Good idea, we can have some debug proxies to easily show what strategies are inside a pipeline.

@PeterCsalaHbo
Copy link

Hey @martintmk , is there anyway to perform something like this?

var strategy = new ResilienceStrategyBuilder()
    .AddRetry(new())
    .AddAdvancedCircuitBreaker(new())
    .AddTimeout(new() { Timeout = TimeSpan.FromSeconds(3) })
    .Build();

// assert
var strategies = strategy.GetInnerStrategies();
strategies[2].GetOptions<TimeoutStrategyOptions>().Timeout.Should().Be(TimeSpan.FromSeconds(3));

@martintmk
Copy link
Contributor Author

Hey @martintmk , is there anyway to perform something like this?

var strategy = new ResilienceStrategyBuilder()
    .AddRetry(new())
    .AddAdvancedCircuitBreaker(new())
    .AddTimeout(new() { Timeout = TimeSpan.FromSeconds(3) })
    .Build();

// assert
var strategies = strategy.GetInnerStrategies();
strategies[2].GetOptions<TimeoutStrategyOptions>().Timeout.Should().Be(TimeSpan.FromSeconds(3));

@PeterCsalaHbo , this is something that I hoped the testing package could help with.

Basically, asserting pipeline composition and options.

I think this does not belong in main package because apps consuming the resilience strategies should not care about what's inside.

@PeterCsalaHbo
Copy link

PeterCsalaHbo commented Jul 11, 2023

Hey @martintmk let me try to amend my example to make my point more clear

Let's suppose you have the following config

{
  "ResiliencyOptions": {
     "RetryDelay": "00:00:02",
     "Timeout": "00:00:04",
     "...": "..."
  }
}

And here is your strategy setup

var strategy = new ResilienceStrategyBuilder()
    .AddRetry(new() { BaseDelay = TimeSpan.Parse(config.Timeout) }) //unintended
    .AddAdvancedCircuitBreaker(new())
    .AddTimeout(new() { Timeout = TimeSpan.Parse(config.Timeout) })
    .Build();

In the above example I've used config.Timeout inside the RetryStrategyOptions instead of config.RetryDelay by mistake.

It would be great if this mistake could be revealed by an unit test

var strategies = strategy.GetInnerStrategies();
strategies[2].GetOptions<TimeoutStrategyOptions>().Timeout.Should().Be(TimeSpan.FromSeconds(4)); //passes
strategies[0].GetOptions<RetryStrategyOptions>().BaseDelay.Should().Be(TimeSpan.FromSeconds(2)); //fails

apps consuming the resilience strategies should not care about what's inside.

IMHO, they should. It is a nasty bug which is hard to catch. By providing utilities to verify the strategy has been built up correctly, minimizes the chance of the above issue.

@martintmk
Copy link
Contributor Author

IMHO, they should. It is a nasty bug which is hard to catch. By providing utilities to verify the strategy has been built up correctly, minimizes the chance of the above issue.

I agree 100% with you. We should have utilities to verify the pipeline and options. However, these should not be in the main Polly.Core package. Doing strategy.GetInnerStrategies at runtime in the application will only cause issues as it effectively allows you to skip and execute strategies outside of pipeline.

That's why I think we should expose Polly.Testing package that introduces those utilities. It will be a small package, however, important for testing Polly V8.

@martincostello Can I give this a go, just to see how much we would have to introduce?

@martincostello
Copy link
Member

I'm happy for us to prototype something and see what comes up, but I'm still not fully convinced.

I typically validate the behaviour (I said retry 4 times, does it retry 4 times?) rather than the configuration (I set a property that I believe makes it retry 4 times but I don't check that it has the intended effect).

@martintmk
Copy link
Contributor Author

I typically validate the behaviour (I said retry 4 times, does it retry 4 times?)

I usually do the same, but then I realized we are just validating Polly implementation then. We should encourage to validate only the options configuration that was passed to strategy. (alongside any custom provided callbacks).

Of course sometimes you really want to verify that Polly behaves correctly, but those tests are harder to setup and should be kept at minimum or to be classified as integration tests.

Let me give this a try.

@martincostello
Copy link
Member

If it helps, here's some code where I take this approach to validating the Polly setup: https://github.com/martincostello/polly-sandbox/blob/polly-v8/tests/PollySandbox.Tests/ResilienceTests.cs

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

No branches or pull requests

3 participants