Implement BeforeInject callback#66
Conversation
vany0114
left a comment
vany0114
left a comment
There was a problem hiding this comment.
@iCodeSometime thanks for taking the time to open this PR, it looks great! I just left a few minor comments regarding honoring the cancellation token, said that, would be nice to add some tests to cover that scenario as well, you can see some examples in the cancellable scenarios. And BTW nice job with the unit tests you added!
I wonder if we should add the AfterInject API as well 🤔 I think would be nice to give the users both options, I can imagine they might want to take actions before and/or after the chaos is injected.
| { | ||
| if (beforeInjectCallback != null) | ||
| { | ||
| await beforeInjectCallback(context, cancellationToken); |
There was a problem hiding this comment.
now that we're allowing injecting a delegate right before injecting the chaos, I think we should honor the token, just in case the user signaled the token from there:
cancellationToken.ThrowIfCancellationRequested();
| { | ||
| if (beforeInjectCallback != null) | ||
| { | ||
| await beforeInjectCallback(context, cancellationToken); |
| { | ||
| if (beforeInjectCallback != null) | ||
| { | ||
| await beforeInjectCallback(context, cancellationToken); |
| { | ||
| if (beforeInjectCallback != null) | ||
| { | ||
| beforeInjectCallback(context, cancellationToken); |
| { | ||
| if (beforeInjectCallback != null) | ||
| { | ||
| beforeInjectCallback(context, cancellationToken); |
| { | ||
| if (beforeInjectCallback != null) | ||
| { | ||
| beforeInjectCallback(context, cancellationToken); |
| with.Behaviour(async () => | ||
| { | ||
| beforeInjectExecuted.Should().BeTrue(); | ||
| injectedBehaviourExecuted = true; |
There was a problem hiding this comment.
make sure to either remove the async keyword from these delegates or await an EmptyTask so that the compiler doesn't complain about the lack of the await keyword:
var policy = MonkeyPolicy.InjectBehaviourAsync(with =>
with.Behaviour(async () =>
{
beforeInjectExecuted.Should().BeTrue();
injectedBehaviourExecuted = true;
await TaskHelper.EmptyTask;
})
.BeforeInject(async (context, cancellation) =>
{
injectedBehaviourExecuted.Should().BeFalse();
beforeInjectExecuted = true;
await TaskHelper.EmptyTask;
})
.InjectionRate(0.6)
.Enabled()
);
Note: this applies to all the tests you added, you can see the warnings in the build: https://ci.appveyor.com/project/Polly-Contrib/simmy/builds/42270091
There was a problem hiding this comment.
Thank you. I did see the warnings, but haven't had time to address them yet.
|
Thank you for the feedback. Expecting to have time to clean this up in about a week |
2 participants
Closes #65