-
Notifications
You must be signed in to change notification settings - Fork 7
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
Detect re-entrant substitute calls #12
Comments
[GH-16] - non virtual received calls
I will take a look at this one. However, this might have a negative impact on the performance as I will have to recursively analyze method bodies used as an argument of Returns method |
I'm just noting down potential cases for detection without thinking about feasibility. 😄 Feel free to ignore/close any suggestions! |
I did an initial look into and I have some working PoC. Can you guys advise about what kind of message do you expect to see? How should I show to the user that given Returns setup, calls (possible multiple) other Returns methods inside? Should we show warning on the root method or maybe in every re-entrant call, or maybe do you have some other ideas |
I think something like:
I think the root method should be sufficient. What do you think? |
It might just be sufficient to warn on |
@dtchepak The warning/diagnostic on the root call seems to be ok. I need one clarification though, putting aside potential performance problems, are all reentrant calls problematic from the perspective of NSubstitute or only the ones which are evaluated eagerly? Please take a look at the examples below and comment on them substitute.Bar().Returns(ReturnThis); // no warning as invoking with delegate ?
substitute.Bar().Returns(MyMethod()); // no warning as MyMethod() returns a delegate?
substitute.Bar().Returns(ReturnThis()); // warning as invoking with result of function which calls Returns ?
substitute.Bar().Returns(ReturnForAnyArgsThis()); // warning as invoking with result of function which calls ReturnWithAnyArg ?
substitute.Bar().Returns(WhenDoThis()); // warning as invoking with result of function which calls When..Do?
private int ReturnThis(CallInfo arg)
{
Substitute.For<IFoo>().Bar().Returns(1);
return 1;
}
private int ReturnThis()
{
Substitute.For<IFoo>().Bar().Returns(1);
return 1;
}
private int ReturnForAnyArgsThis()
{
Substitute.For<IFoo>().Bar().ReturnForAnyArg(1);
return 1;
}
private int WhenDoThis()
{
var @for = Substitute.For<IFoo>();
@for.When(x => x.Bar()).Do(yy => { });
return 1;
}
Func<CallInfo, int> MyMethod()
{
return ReturnThis;
} |
Hi @tpodolak, substitute.Bar().Returns(ReturnThis); // no warning as invoking with delegate ?
substitute.Bar().Returns(MyMethod()); // no warning as MyMethod() returns a delegate?
substitute.Bar().Returns(ReturnThis()); // warning as invoking with result of function which calls Returns ?
substitute.Bar().Returns(ReturnForAnyArgsThis()); // warning as invoking with result of function which calls ReturnWithAnyArg ?
substitute.Bar().Returns(WhenDoThis()); // warning as invoking with result of function which calls When..Do? This seems correct. I did some testing with these examples and all the "no warning" examples passed, all the "warning" examples failed. For completeness, here is the test setup I used: public interface IFoo {
int Bar();
}
[Test]
public void Samples() {
var substitute = Substitute.For<IFoo>();
//substitute.Bar().Returns(ReturnThis); // no warning as invoking with delegate ?
//substitute.Bar().Returns(MyMethod()); // no warning as MyMethod() returns a delegate?
substitute.Bar().Returns(ReturnThis()); // warning as invoking with result of function which calls Returns ?
//substitute.Bar().Returns(ReturnForAnyArgsThis()); // warning as invoking with result of function which calls ReturnWithAnyArg ?
//substitute.Bar().Returns(WhenDoThis()); // warning as invoking with result of function which calls When..Do?
Assert.AreEqual(1, substitute.Bar());
}
// ... copied and pasted the delegates from the earlier question ... |
Not sure how feasible this is but...
Call a substitute from within the
.Returns(...)
block of another substitute can cause issues:nsubstitute/NSubstitute#26
nsubstitute/NSubstitute#357
Note that
sub.Blah().Returns(x => CreateFoo())
can be ok, whereassub.Blah().Returns(CreateFoo())
can cause the problem (whenCreateFoo
also setsReturns()
values).The text was updated successfully, but these errors were encountered: