Open doxakis opened 4 years ago
Thank you for the detailed report!
Does the result.Should().Be(2)
assertion fail (actually returns 0
)? I'm trying to reproduce this on Mac but haven't been able to yet.
Hi,
It returns 0, but it should not be possible to evaluate: actual => actual.IsDeepEqual(expected, null)
, since IsDeepEqual
come from the package DeepEqual and could not be loaded, since there is missing binding redirect.
I am not sure where it should throw. Without knowing the implementation details, I think it would make sense when I am calling the method of the mock since it has to evaluate it to determine the results somewhere.
When a error occur in the expression, does it catch all exceptions ?
Thanks
@doxakis Yes when an arg matcher (like Arg.Is
) fails with an exception it catches it and regards that as a non-match.
Hi,
how about catching the specific exception FileLoadException ?
Here is a suggestion :
public bool IsSatisfiedBy(object argument)
{
if (!IsCompatibleWith(argument)) return false;
try
{
return _matcher.IsSatisfiedBy(argument);
}
catch (System.IO.FileLoadException)
{
// The exception that is thrown when a managed assembly is found, but cannot be loaded.
// We cannot determine if the matcher is satisfied by the argument since we cannot evaluate it.
throw;
}
catch
{
return false;
}
}
Thanks!
@alexandrnikitin @zvirja: Do you have any thoughts on the implications of catching this? Any alternatives you can think of? Regards, David
@dtchepak There is no new message since. Do you prefer to ignore this issue for now or wait or you are comfortable with the fix?
Sorry for the delay @doxakis , thank you for following up.
I still need to get some agreement with the team on how to approach this. My only reservation with the fix is if FileLoadException
is for some reason legitimately thrown from the arg matcher then it would cause Received()
checks to fail when looking for a match among calls. This could be worked around by wrapping try/catch
for that specific matcher logic, so I don't think this is a big problem, but I always like to be cautious with potentially breaking changes.
Is this something you need quickly or can you work around this for now?
Hi,
I understand, no problem. I just wanted to do a follow up.
I don't need it now since I fixed the missing binding redirect in my projects.
It would be a nice to have and make debugging tests easier.
Thanks.
@doxakis Thank you for the great description. I don't think that catching and re-throwing a specific exception is a good idea. Especially not related to NSub. The assembly loading logic shouldn't be concern of NSub. On the other hand, I'm not sure why we swallow all exceptions in ArgumentSpecification.cs
@dtchepak Do you remember why we have this?
It's probably .NET Runtime failure that it couldn't catch it at compile time. I would expect at least a warning (is there one?). As a workaround for your issue, is there a way to force assembly loading via a cli flag? I only found this SO issue that has programmatic way to do it.
@alexandrnikitin Thanks for taking a look at this. 🙇
IIRC the reason is so specifying arg matchers can be kept as simple as possible. For example:
sub.AddPerson(new Person { Name = "Bob" });
sub.AddPerson(new Person { Name = null });
sub.AddPerson(null);
sub.AddPerson(new Person { Name = "Alice" });
sub.Received().AddPerson(Arg<Person>.Is(p => p.Name.StartsWith("A")));
If this has multiple calls, some with a null
Name
field, then this will ignore those calls and the test can still pass if there is a matching call. We can use ?.
now, but back when we started C# didn't have this and we'd have to write Arg<Person>.Is(p => p != null && p.Name != null && p.Name.StartsWith("A"))
.
Maybe a more modern example:
sub.Received().AddDepartment(Arg<Department>.Is(d => d.LookupCode() == 42)
If LookupCode()
can throw an exception, we'd need:
Arg<Department>.Is(d => {
try { return d.LookupCode() == 42; }
catch (SomeException e) { return false; }
})
(Untested, may need to extract that to a separate method if that can't be represented as an Expression
.)
It also means people can re-use assertion/matcher libraries to an extent.
I agree that from a pure design perspective it does not make sense for NSub to handle this. BUT we've never been shy about making odd design decisions to make things easier for people (sub.Call().Returns(42)
reads well and is great to write but makes no sense when you think purely from tech design), which is why I've been considering this. I do worry about unintended consequences of this, but maybe FileLoadException
is a special enough case that it would be ok to make this change. 🤷♂ (Worst case is someone would need to add exception handling like the LookupCode()
example above?)
It's probably .NET Runtime failure that it couldn't catch it at compile time. I would expect at least a warning (is there one?).
This is a very good point. If it gives a warning then I think it is ok for NSub not to handle this.
Hi guys,
I was thinking a bit about this scenario and also found that things are complicated.
Personally, I feel that capturing the FileLoadException
is a bad idea, as it implies very specific knowledge deeply inside NSubstitute library. To me it looks like we are trying to solve some architectural issue with a patch. So I would use this as a given when considering options left.
Other two options I see are following:
I would prefer just remove the catch, but am actually file with any way we solve it.
Stopping capturing the exception would be a fairly big breaking change I think, especially for anyone that is using assertion libraries with arg matchers. I think it is reasonable(ish) that if an argument matcher throws on an input then that counts as a non-match.
We could potentially provide a matcher that does does re-throw, and then reimplement the current matchers in terms of that one. That means people could opt-in to using matchers that have either behaviour. That would also give us the option for switching default behaviour in a later major release if we feel that is worthwhile.
@dtchepak I see. In this case, I think the solution is just to do nothing as of today and accept a fact that sometimes some weird issue could happen 😕 Then after we implement #239, we could revise our approach regarding the catch.
Describe the bug When there is a missing binding redirect, the mock does not throw an error when evaluating with Arg.Is(), which make it hard to debug the issue. It hides the fact that it could not load the assembly and simply return the default value when calling the method of the mock.
To Reproduce
c# code:
csproj file:
Expected behaviour It simply returns the default value when calling the method of the mock (see method Test1). I expected to get a error to help quickly identify the issue. For example, in the method Test2, the unit test fail with:
Environment:
Additional context
csproj file: