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

Classes can't be resolved from the container the same way that interfaces can #23

Open shaynevanasperen opened 7 years ago

shaynevanasperen commented 7 years ago

Given code like this:

public interface IFoo
{
    string Bar { get; }
}

public class Foo
{
    public virtual string Bar { get; }
}

public class Subject
{
    public string DoSomethingWithInterface(IFoo foo)
    {
        return foo.Bar;
    }

    public string DoSomethingWithClass(Foo foo)
    {
        return foo.Bar;
    }
}

public class TestInterface : ScenarioFor<Subject>
{
    string _result;

    void GivenAFooWithBar() => The<IFoo>().Bar.Returns("Bar");
    void WhenDoingSomething() => _result = SUT.DoSomethingWithInterface(The<IFoo>());
    void ThenTheResultIsBar() => _result.Should().Be(The<IFoo>().Bar);
}

public class TestClass : ScenarioFor<Subject>
{
    string _result;

    void GivenAFooWithBar() => The<Foo>().Bar.Returns("Bar");
    void WhenDoingSomething() => _result = SUT.DoSomethingWithClass(The<Foo>());
    void ThenTheResultIsBar() => _result.Should().Be(The<Foo>().Bar);
}

The second test fails on the line GivenAFooWithBar(). It should work the same as with the interface.

mwhelan commented 7 years ago

Hey @shaynevanasperen . Sounds like an issue with registering/resolving in the underlying container. Are you using the out of the box Tiny container or another one, like Autofac?

shaynevanasperen commented 7 years ago

Hello @mwhelan . Yes, I am using the out of the box Tiny container (actually the one that wraps it called TinyMockingContainer). I think the underlying problem is that the auto-mocking container tries too hard to construct real objects all the way down the object graph, instead of just mocking the constructor arguments of the SUT.

There needs to be a distinction between requesting a real object like the SUT versus requesting a mocked object. I think that distinction needs to be expressed explicitly in the ContainerFor<T> so that it is clear when you are expecting a real object versus a mocked object. Currently, the SystemUnderTest property uses the same mechanism to fetch an object from the container as the The<T>() method in the ScenarioFor<T> class.

I think that the The<T>() method should only check the container for an existing registered mock, or create and register a shallow mock if necessary. Whereas the SystemUnderTest property should be the only place where the greediest constructor is found and a real object is created using shallow mocks for all the parameters. Perhaps some different logic applies here though, where the ScenarioRunner fetches an instance of the scenario from the container. I'm not sure about that use case however.

mwhelan commented 7 years ago

I am keen to keep things as simple as possible. The intention is to just wrap the underlying IoC container or mocking framework and delegate to them because I want users to get exactly the same result as if they used it without Specify. I think I have stepped off that path a little bit with some of the behaviour I have begun to implement in the TinyMockingContainer and I think we are right to step back a bit and not continue down a road which leads to reinventing the wheel.

The The() method works for all containers, both IoC and mocking, so it is right to just delegate to the underlying container Get method. So, regarding mocking, the TinyMockingContainer Get method is where we can target the desired behaviour.

mwhelan commented 7 years ago

I don't think a mocking or IoC container can automatically create classes like Card, that have value types in the constructor - would it put true or false into the boolean parameter and how much would it put into the Account Balance parameter? These are crucial to whether the test passes or fails and meaningful to the logic and understanding of the test, so I think you would want to see their values in the test itself as they directly affect whether it succeeds or fails.

On reflection, Specify is already setup to handle this situation. This is actually the sort of scenario that the Set methods on the container are intended for. Rather than letting Specify or the mocking framework attempt to create the dependency without all the info they would need, we can create it by instantiating it directly or passed the additional info to our mocking framework, such as I've done with NSubstitute here:

    public void Given_the_Card_is_disabled()
    {
        //The<Card>().Enabled.Returns(false);
        //The<Card>().AccountBalance.Returns(100);

        // Register the dependency in the container in several ways
        // - with direct instantiation or using your mocking framework
        SetThe<Card>(new Card(false, 100));
        SetThe<Card>(Substitute.For<Card>(false, 100));
        Container.Set(new Card(false, 100));

Understandably you might not like the noise of having to set this info in every test. There are a number of ways to resolve that - anything you can do with a C# class really, such as inheriting from a base class for that group of tests. Specify has another option for this though. It lets you register the Card in the container in the bootstrapper class so that now the container knows how to construct it - just as you would have to in your application code.

    public override void ConfigureContainer(TinyIoCContainer container)
    {
        container.Register<Card>((c, p) => new Card(true, 150));
    }

You would get a new instance of that card in every test that requested it and you could optionally override it in each test. This code describes some of the scenarios this enables:

    public void Given_the_Card_is_disabled()
    {
        // Values set in the bootstrapper
        var defaultBalance = The<Card>().AccountBalance; // 150

        // Override the value for this test. 
        // Next test will still get the default from bootstrapper
        Container.Set(new Card(false, 100));
        var newBalance = The<Card>().AccountBalance;    // 100

        // The dependency is a singleton for the life of the test
        // You can change values just like any other variable
        The<Card>().AccountBalance = 20;
        var updatedbalance = The<Card>().AccountBalance; // 20
    }

Like I say, I am very deliberately trying to keep Specify very lightweight and delegate the heavy lifting to the mocking or IoC frameworks. That is incredibly powerful, as it means that you can bring whatever one of those you like and know the best and use its full power. For example, you might want different Card default values returning for different groups of tests, so you could probably get the Container to achieve that.

Also, I would say that mocking and unit testing came second. The first motivation for Specify was for BDD subcutaneous MVC tests, where I wanted to be able to use the same IoC container in the tests that I use in my app, which brings with it the inference that you've already done the hard work of setting up the container in the app, and you would largely want to use that exactly as it is in the tests, apart from overriding particular dependencies on a test by test basis where it makes sense.

mwhelan commented 7 years ago

By the way, I like the health warning in NSubstitute's docs where they say that you can substitute for classes, but that it has its risks and should only be done in cases of emergency. I think Specify has to offer similar advice as we are just forwarding on to frameworks like NSub.

This is how you’ll normally create substitutes for types. Generally this type will be an interface, but you can also substitute classes in cases of emergency.

Warning: Substituting for classes can have some nasty side-effects. For starters, NSubstitute can only work with virtual members of the class, so any non-virtual code in the class will actually execute! If you try to substitute for your class that formats your hard drive in the constructor or in a non-virtual property setter then you’re asking for trouble. If possible, stick to substituting interfaces.

With the knowledge that we’re not going to be substituting for classes, here is how you create a substitute for a class that has constructor arguments:

var someClass = Substitute.For<SomeClassWithCtorArgs>(5, "hello world");

And the example shows they don't try to populate value type parameters but provide a way for the user to provide them, which I think is consistent with what I'm advocating for how Specify handles it.

I think that resolves this issue and we can close it.

shaynevanasperen commented 7 years ago

@mwhelan Those are all very good points, and I'm fully aware of the side-effects of mocking classes. Sometimes you just don't have a choice though (when it is a class defined in a 3rd party library) and other times I prefer being able to be completely DRY with no duplication of public API between my interface and the only class which will ever implement it.

Moq, one of the most popular mocking libraries, doesn't work like NSubstitute. It requires a class to have a parameterless constructor in order for it to be mocked.

Given the following code:

public class Foo
{
    public string GetValue(Bar bar)
    {
        return bar.GetValue().ToString();
    }
}

public class Bar
{
    readonly ILogger _logger;

    public Bar(ILogger logger)
    {
        _logger = logger;
    }

    public virtual int GetValue()
    {
        _logger.LogInformation("GetValue was called");
        return 1;
    }
}

public class FooTest
{
    [Test]
    public void GettingValueShouldWork()
    {
        var bar = new Mock<Bar>();
        bar.Setup(x => x.GetValue()).Returns(3);
        var sut = new Foo();
        var result = sut.GetValue(bar.Object);
        result.Should().Be("3");
    }
}

It fails on the line var bar = new Mock<Bar>(); with the following error message:

Can not instantiate proxy of class: Bar. Could not find a parameterless constructor.

By modifying Bar to have a parameterless protected constructor, it can then be mocked with Moq:

public class Bar
{
    readonly ILogger _logger;

    public Bar(ILogger logger)
    {
        _logger = logger;
    }

    protected Bar() { }

    public virtual int GetValue()
    {
        _logger.LogInformation("GetValue was called");
        return 1;
    }
}

The main reason I want to be able to use The() in my Specify scenarios outside of referring to constructor parameters of the SystemUnderTest, is so that I can test public methods of SystemUnderTest which take as their arguments either classes or interfaces which need to be mocked, and I don't want to have to define fields in my test to hold those arguments. For example, instead of writing this:

public class FooTest : ScenarioFor<Foo>
{
    Bar _bar;
    string _result;

    void GivenABarWhichReturns3()
    {
        _bar = Substitute.For<Bar>();
        _bar.GetValue().Returns(3);
    }

    void WhenGettingValue() => _result = SUT.GetValue(_bar);

    void ThenTheResultIs3AsString() => _result.Should().Be("3");
}

I'd like to write this:

public class FooTest : ScenarioFor<Foo>
{
    string _result;

    void GivenABarWhichReturns3() => The<Bar>().GetValue().Returns(3);

    void WhenGettingValue() => _result = SUT.GetValue(The<Bar>());

    void ThenTheResultIs3AsString() => _result.Should().Be("3");
}

This is the style I became used to when I used to use MSpec, and I'd love to be able to continue leveraging the container in order to reduce the number of fields in my test classes.