JasonBock / Rocks

A mocking library based on the Compiler APIs (Roslyn + Mocks)
MIT License
263 stars 20 forks source link

Virtual Properties cause NullReferenceException #313

Closed rainman-63 closed 6 months ago

rainman-63 commented 7 months ago

We have a use case where we need to Mock a class (TestClass2 in the sample), that is inheriting from another class which has a virtual property (TestClass's ILogger in the sample). When using RockCreate to generate the mock with expectations, creating an instance of the mock will cause a null reference exception.

The root cause is due to the fact that the virtual property is being set in the base's constructor, but the Mock's Expectations property hasn't been set yet so when the handler attempts to set the virtual property Expectations is still null. The simple solution is to remove the virtual keyword and everything works fine, but in our case we don't own the code we need to mock.

To Reproduce Sample project will produce the error RocksTesting.zip

Instead of mocking the "problem class" we can Mock all it's dependencies and then create an instance, in our case that will be a fair bit of effort and requires in-depth knowledge of how the "problem class" is using the dependencies (which we'd like to avoid). Any suggestions for a work around would greatly appreciated?

As a side note, must say I'm really enjoying using the Rocks mocking library, awesome job!

Thanks!

JasonBock commented 7 months ago

Calling virtual members in constructors can be dangerous because of the order of execution, as you're seeing right now. Also, since a derived type can override the member, the base type can't assume that everything is initialized correctly, which is why it's bad practice to call virtual members in constructors. Since you said you that you don't own the code, I'm assuming you mean the "TestClass2" in your example. Unfortunately, there's nothing that I can think of to get around this. Rocks has to pass that expectations object to the mock's constructor, and that has to call the base class constructor first. What really should happen is that the author of TestClass2 should not set Logger in the constructor. Rather, it should provide a factory pattern, like a static Create() method that would create an instance of the type, set the virtual property, and then return the object. That would get rid of the issue you're seeing.

JasonBock commented 7 months ago

Side note: I'm not sure the factory patten would let you create a "mock" here either. I'll keep thinking about this, but....I can't think of anything that Rocks can do to get rid of the exception based on the design of the constructor calling the virtual member.

rainman-63 commented 7 months ago

Yah, unfortunately it's MS's code in the Identity Framework (UserManager and SignInManager) and I don't want to reimplement them at this point, although it would have probably been quicker since I've had to add a caching decorator and dealing with all the testing issues their code causes. I took the approach of just creating mock instances of all the dependencies, it works fine just adds a ton of code.

Once I figured out what was happening, I didn't figure there would be an "easy" fix.

Thanks for the reply, and the awesome mocking framework.

JasonBock commented 7 months ago

Hey @rainman-63, I just tried something, and it might fix your issue. Try doing this in your "TestClass2":

public override sealed ILogger Logger 
{ 
    get => base.Logger; 
    set => base.Logger = value; 
}

The test passes now. The reason is that Logger is no longer virtual, so Rocks ignores it. If all you want to do is defer to what the base class is doing, then this seems like it might be an option without having to write too much extra code on your end.

If this seems like a viable solution, let me know as I'd like to update the docs to reflect this scenario.

rainman-63 commented 6 months ago

Thanks @JasonBock, I'll give it a try.

JasonBock commented 6 months ago

@rainman-63 did you get a chance to try this? No rush, just wanted to know if this was a viable strategy or not.

rainman-63 commented 6 months ago

Hey @JasonBock, I was able to use your recommendation by making a MockUserManager class that inherited from UserManager and then overrode the ILogger property as you suggested. It worked quite well, I was able to use Rocks to create a mock of my MockUserManager class without any issues.

JasonBock commented 6 months ago

Excellent! I'll add this to my docs.