mwhelan / Specify

Specify is an opinionated .Net Core testing library that builds on top of BDDfy from TestStack
http://specify-dotnet.readthedocs.org/
MIT License
20 stars 8 forks source link

Ensure that concrete classes can be mocked. Fixes #14 #15

Closed shaynevanasperen closed 7 years ago

shaynevanasperen commented 7 years ago

Allows TinyMockingContainer to mock concrete classes. Changes ContainerFor<TSut> to create SystemUnderTest using reflection and fetching of dependencies from the container.

shaynevanasperen commented 7 years ago

@mwhelan It's good that you found a simpler way of fixing #14 by just doing this. However, I found a scenario where it doesn't work. If one of the constructor parameters is a sealed class, TinyMockingContainer will try to mock it, which obviously fails. You need to also check that it is not sealed, like this. By simply skipping it, the eventual call to base.Get(serviceType, key) will try to resolve it as usual and it works.

Also, I'm curious to know why you think this should be done, especially when you already have an instance of the scenario passed in as a parameter of the containing method. Why would anyone ever be registering the actual test class scenario in the container? If there is no solid reason for this then why pollute the container by having it be automatically registered upon calling container.Get(testObject.GetType())?

I'm also curious as to why you didn't also do this. Perhaps because it's unrelated to #14?

mwhelan commented 7 years ago

Hi @shaynevanasperen . Thanks for all your excellent feedback. I will comment on each point you've made separately.

Firstly, as to the reason I do this in ScenarioRunner:

var scenario = (IScenario<TSut>)container.Get(testObject.GetType());

Well, it's probably a case of YAGNI, because I haven't put any work into this feature, but I wanted to support the ability to inject services you want to test into the test class, from your application's IoC container, the way that Xunit.Ioc does. Here is an example from their site:

public class MyComponentFacts
{
    private readonly IMyComponent _myComponent;

    public MyComponentFacts(IMyComponent myComponent)
    {
        _myComponent = myComponent;
    }

    [Fact]
    public void ItDoesThatThingItsSupposedToDo()
    {
        var result = _myComponent.DoThing();
        Assert.True(result);
    }
}

Specify registers all the Specify test classes in the IoC container at startup, so this probably works already, but I haven't written any tests for it or otherwise tried to verify that as I've not deliberately set about implementing the feature.

mwhelan commented 7 years ago

Hi @shaynevanasperen .

Regarding your final point, about why I don't construct the SUT with reflection. One of the goals of Specify is to provide a consistent syntax and style for both unit tests (that use an automocking container) and larger tests (that use the application's IoC container), and for people to be able to plug in their particular mocking/IoC framework. I think the best way to do that is for Specify to provide a Get/Set abstraction and delegate to each container to resolve the SUT in the way that makes sense for that container. Constructing the SUT is useful for the mocking container but I would prefer that the IoC containers resolve the SUT themselves as a key requirement of those tests is for them to be consistent with how the application works.

So, if I was going to use reflection to construct the SUT I would do it in TinyMockingContainer, but I would wait for a use case that demanded I do that. So far, the use cases I have found haven't warranted it.

shaynevanasperen commented 7 years ago

Hi @mwhelan. Thank you for the detailed answers.

Regarding your first answer, that's pretty cool, and makes sense. It would be good to add some tests for that and sample code showing how it works (when either of us get around to it).

Regarding your second answer, my apologies for not being more clear. Your actual explanation makes perfect sense to me and I agree with that line of thinking. But what I actually meant with my question is that you can simply delegate to returning Container.SystemUnderTest (just like ScenarioFor<TSut, TStory> does) which already has the same logic, rather than duplicating the code. So instead of:

protected TSut SUT
{
    get { return _sut ?? (_sut = Container.Get<TSut>()); }
    set { _sut = value; }
}

You just do this:

protected TSut SUT
{
    get { return Container.SystemUnderTest; }
    set { Container.SystemUnderTest = value; }
}

And then get rid of the _sut field.

mwhelan commented 7 years ago

@shaynevanasperen To your first point, about sealed class parameters, I would be keen to understand this, create some test cases, and implement a fix.

mwhelan commented 7 years ago

@shaynevanasperen , thanks for this PR and for raising a number of excellent issues. I wanted to isolate the bug fix to TinyMockingContainer and not affect other containers and so I did not accept this PR and instead just implemented a simple bug fix. I did use your code in the simple fix though and want to give you credit for the work. I have also accepted your subsequent PR and would also like to resolve the issue you've raised with sealed class parameters, very likely also with code you've included here. Thanks!