Bad exception thrown when fake's base's constructor fails

[matkoch]

Related to #365, FIE throws a FakeCreationException when calling a fakable method (with struct return type) in the constructor. However, the current exception message isn't really helpful, because it doesn't outline that possibility.

FakeItEasy.Core.FakeCreationException : 
  Failed to create fake of type "ClassLibrary7.DomainType".

  Below is a list of reasons for failure per attempted constructor:
    No constructor arguments failed:
      No usable default constructor was found on the type ClassLibrary7.DomainType.
      An exception was caught during this call. Its message was:
      Exception has been thrown by the target of an invocation.

As long as #365 is not solved somehow, FIE should either state this situation in the exception, or, what i prefer, introduce a new exception type for virtual/abstract calls in the constructor. Also, the thrown FakeCreationException doesn't expose the inner exception - it is null. I think it could be wrapped.

[blairconrad]

Agreed, that error message is weak. I've recently been plagued by these types of TargetInvocationException errors thrown by failing attempts to make a fake.

@FakeItEasy/owners, I'd had in mind an idea to catch TargetInvocationException in fake construction and unwrap, like we do in FakeItEasy.Core.FakeManager.EventRule.RaiseEvent. Any objections? I don't think it's too heavy-handed in this case.

[blairconrad]

Changing title - was FakeCreationException thrown for abstract member call in constructor.

Issues already exist for FIE's deficiencies around overrideable methods being called from constructors (#368, #365), so we'll focus a little more here on the exception message (which may arise from other causes than those in the mentioned issues).

[blairconrad]

@matkoch, I think that not exposing the inner exception may have been intentional to create "prettier" error messages, although perhaps only @patrik-hagne can comment on this.

When all relevant information is included in the "pretty" message, this is not a bad thing. When the info is missing, it's a detriment. I'm not specifically against including the inner exception, but suspect that unwrapping the TargetInvocationException will provide quite a bit of value right off the bat, and may be sufficient.

[adamralph]

So what happens currently? We swallow the actual exception and throw a new FakeCreationException with no InnerException set? That certainly sounds unhelpful.

[blairconrad]

Yes, that. (Almost?) Always. In most cases, we go to (sometimes herculean) efforts to provide a helpful error message. When it works, it's great, since we have very tidy, helpful messages that aren't obscured by giant stacktraces.

When it doesn't work, we get errors like above.

As I say, I can probably be convinced to include the InnerException, but I think unwrapping the TargetInvocationException will have no downside.

[blairconrad]

Including InnerException is more work than I thought, but unwrapping the TargetInvocationException and beefing up the failure message is not too difficult:

FakeItEasy.Core.FakeCreationException
  Failed to create fake of type "FakeItEasy.IntegrationTests.ClassWhoseConstructorCallsAVirtualValueTypeMethod".

  Below is a list of reasons for failure per attempted constructor:
    No constructor arguments failed:
      No usable default constructor was found on the type FakeItEasy.IntegrationTests.ClassWhoseConstructorCallsAVirtualValueTypeMethod.
      An exception of type System.NullReferenceException was caught during this call. Its message was:
      Object reference not set to an instance of an object.
         at Castle.Proxies.ClassWhoseConstructorCallsAVirtualValueTypeMethodProxy.VirtualMethod()
         at Castle.Proxies.ClassWhoseConstructorCallsAVirtualValueTypeMethodProxy..ctor(IInterceptor[] )

(VirtualMethod is my virtual method that returns a value type.)

Do we think this is sufficient? If not, I can continue looking at attaching the inner exception.

[blairconrad]

One of the reason that setting the InnerException is tricky is that there may be multiple constructors that have been tried, with multiple of them having thrown exceptions. I guess in that case we could use an InnerException list, but I wonder if it isn't getting too fancy.

[matkoch]

Actually, I think this trying is a code smell. Instead FIE should know in advance, what constructor has to be invoked (e.g., given through passed arguments), but that's a different story.

If you have multiple exceptions being thrown during construction, I would definitely expose the related ConstructorInfo with them as a property. You could also put the signature into the message.

[blairconrad]

I don't think the trying is a code smell. It's a feature. If you ask FakeItEasy to make a fake something (or a Dummy, but let's leave that aside for now, although it may come back), it will try to make it however it can. Of course if arguments are passed, it will use the constructor whose arguments match. But if no arguments are specified, it will try a parameterless constructor first. If that doesn't work, it will try to use any available constructor, in descending order of parameter list length.

I don't follow your second paragraph. We do already indicate the matching signature of failed constructors when there are problems. Consider a failed attempt to fake a class that has only one constructor, with signature ClassWithConstructorArguments(string s, AnotherClass c). If that constructor throws, we see:

FakeItEasy.Core.FakeCreationException : 
  Failed to create fake of type "FakeItEasyQuestions.ClassWithConstructorArguments".

  Below is a list of reasons for failure per attempted constructor:
    No constructor arguments failed:
      No usable default constructor was found on the type FakeItEasyQuestions.ClassWithConstructorArguments.
      An exception was caught during this call. Its message was:
      Can not instantiate proxy of class: FakeItEasyQuestions.ClassWithConstructorArguments.
      Could not find a parameterless constructor.
    Constructor with signature (System.String, FakeItEasyQuestions.AnotherClass) failed:
      No constructor matches the passed arguments for constructor.
      An exception was caught during this call. Its message was:
      Exception has been thrown by the target of an invocation.

(Ignore the "Exception has been thrown by the target…" stuff. I don't have my "improved" error reporting with me.)

You can see that the report indicates that there's no parameterless constructor, and that the "Constructor with signature (System.String, FakeItEasyQuestions.AnotherClass) failed", and why. (Although the "why" could be improved.)

[blairconrad]

I'm planning on sending a PR that will have FakeCreationExceptions generated from caught exceptions include in their message the original exception type and stack trace. In the case of the original exception being a TargetInvocationException, its InnerException will be used instead of the original one (unless InnerException is null). To repeat an earlier example, the new messages would look like

  Failed to create fake of type "FakeItEasy.IntegrationTests.ClassWhoseConstructorCallsAVirtualValueTypeMethod".

  Below is a list of reasons for failure per attempted constructor:
    No constructor arguments failed:
      No usable default constructor was found on the type FakeItEasy.IntegrationTests.ClassWhoseConstructorCallsAVirtualValueTypeMethod.
      An exception of type System.NullReferenceException was caught during this call. Its message was:
      Object reference not set to an instance of an object.
         at Castle.Proxies.ClassWhoseConstructorCallsAVirtualValueTypeMethodProxy.VirtualMethod()
         at Castle.Proxies.ClassWhoseConstructorCallsAVirtualValueTypeMethodProxy..ctor(IInterceptor[] )

Although this does not explicitly include an InnerException in the FakeCreationException, I think it satisfies the spirit of the issue and will help users determine what went wrong with their fake creation. I think it also maintains what appears to be FIE's unwritten policy of helpful, comprehensive exception messages when something goes wrong.

Any objections?
@adamralph, you seemed dismayed by the lack of InnerException. Was that a hard requirement for you, or would the presence of the inner exception's type, message, and stack trace suffice?

[adamralph]

@blairconrad your proposal sounds fine. I don't think there's any point trying to engineer a list of inner exceptions. This exception is not designed to be caught, but rather to bubble out to the console so enriching the message appropriately is the right thing to do. If we later discover that people are wanting to use FIE in a more SDK-like manner and actually catch the exception and inspect the inner exceptions then we can revisit but I doubt this will crop up.

[blairconrad]

Great and thanks. I have the work done, but am resisting submitting a PR until #372 and #30 are resolved.

[blairconrad]

#372 is resolved, and #30 has its own PR. I figured I might as well create PR #381. That way someone can decide whether to review #376 or #381, and I'll rebase the other.

[blairconrad]

@matkoch, thanks very much for your work on this issue. Look for your name in the release notes. 🏆

This issue has been fixed in release 2.0.0.