8

This question has bothered me for a few days, and it feels like several practices contradict each other.

Example

Iteration 1

public class FooDao : IFooDao
{
    private IFooConnection fooConnection;
    private IBarConnection barConnection;

    public FooDao(IFooConnection fooConnection, IBarConnection barConnection)
    {
        this.fooConnection = fooConnection;
        this.barConnection = barConnection;
    }

    public Foo GetFoo(int id)
    {
        Foo foo = fooConection.get(id);
        Bar bar = barConnection.get(foo);
        foo.bar = bar;
        return foo;
    }
}

Now, when testing this, I would fake IFooConnection and IBarConnection, and use Dependency Injection (DI) when instantiating FooDao.

I can change the implementation, without changing the functionality.

Iteration 2

public class FooDao : IFooDao
{
    private IFooBuilder fooBuilder;

    public FooDao(IFooConnection fooConnection, IBarConnection barConnection)
    {
        this.fooBuilder = new FooBuilder(fooConnection, barConnection);
    }

    public Foo GetFoo(int id)
    {
        return fooBuilder.Build(id);
    }
}

Now, I won't write this builder, but imagine it does the same thing FooDao did before. This is just a refactoring, so obviously, this doesn't change the functionality, and so, my test still passes.

IFooBuilder is Internal, since it only exists to do work for the library, i.e it isn't a part of the API.

The only problem is, I no longer comply to Dependency Inversion. If I rewrote this to fix that problem, it might look like this.

Iteration 3

public class FooDao : IFooDao
{
    private IFooBuilder fooBuilder;

    public FooDao(IFooBuilder fooBuilder)
    {
        this.fooBuilder = fooBuilder;
    }

    public Foo GetFoo(int id)
    {
        return fooBuilder.Build(id);
    }
}

This should do it. I have changed the constructor, so my test needs to change to support this (or the other way around), this isn't my issue though.

For this to work with DI, FooBuilder and IFooBuilder need to be public. This means that I should write a test for FooBuilder, since it suddenly became part of my library's API. My problem is, clients of the library should only be using it through my intended API design, IFooDao, and my tests acts as clients. If I don't follow Dependency Inversion my tests and API are more clean.

In other words, all I care about as the client, or as the test, is to get the correct Foo, not how it is built.

Solutions

  • Should I simply not care, write the tests for FooBuilder even though it is only public to please DI? - Supports iteration 3

  • Should I realise that expanding the API is a downside of Dependency Inversion, and be very clear about why I chose not to comply to it here? - Supports iteration 2

  • Do I put too much emphasis on having a clean API? - Supports iteration 3

EDIT: I want to make it clear, that my problem is not "How to test internals?", rather it is something like "Can I keep it internal, and still comply to DIP, and should I?".

  • I can edit my question to add the tests if you want. – Chris Wohlert May 31 '16 at 9:23
  • 1
    "FooBuilder and IFooBuilder need to be public". Surely only IFooBuilder needs to be public? FooBuilder need only be exposed to the DI system via some sort of "get default implementation" method that returns an IFooBuilder and so can remain internal. – David Arno May 31 '16 at 9:52
  • I guess that could be a solution. – Chris Wohlert May 31 '16 at 10:30
  • 2
    This has been asked here on Programmers at least a dozen times, using different terms: "I want to make a difference between public for library APIs and public for testing". Or in the other form "I want to keep methods private to avoid appearing them in the API, how do I test those private methods?". The answers are always "live with the broader public API", "live with tests through the public API", or "make methods internal and make them visible to the test code". – Doc Brown May 31 '16 at 17:34
  • I would probably not replace the ctor, but introduce a new one and chain them together. – RubberDuck Jun 1 '16 at 0:00
3

I'd go with:

Should I realise that expanding the API is a downside of Dependency Inversion, and be very clear about why I chose not to comply to it here? - Supports iteration 2

However, in The S.O.L.I.D. Principles of OO and Agile Design (a at 1:08:55) Uncle Bob says that his rule about dependency injection is don't inject everything, you inject only at strategic locations. (He also mentions that the topics of dependency inversion and dependency injection are the same).

That is, dependency inversion is not meant to be applied on all class dependencies in a program. You should do it at strategic locations (when it pays of (or might pay of)).

  • 2
    This comment from Bob is perfect, thank you very much for sharing this. – Chris Wohlert Jun 2 '16 at 9:10
5

I'm going to share some experiences/observations of various code bases I've seen. N.B. I'm not advocating any approach in particular, just sharing with you where I see such code is going:

Should I simply not care, write the tests for FooBuilder even though it is only public to please DI

Before DI and automated testing, the accepted wisdom was that something should be restricted to the scope required. Nowadays however, it isn't uncommon to see methods that could be left internal made public for ease of testing (since this usually happens in another assembly). N.B. there is a directive to expose internal methods but this more or less amounts to the same thing.

Should I realise that expanding the API is a downside of Dependency Inversion, and be very clear about why I chose not to comply to it here?

DI does undoubtedly incur an overhead with additional code.

Do I put too much emphasis on having a clean API

Since DI frameworks do introduce additional code, I've noticed a shift towards code that while written in the DI style does not use DI per se i.e. the D from SOLID.

In summary:

  1. Access modifiers are sometimes loosened to simplify testing

  2. DI does introduce additional code

In the light of 1 & 2, some developers are of the view that this is too much of a sacrifice and so simply elect to depend on abstractions initially with the option to retro-fit DI later.

  • 4
    It does indeed which is why some developers opt for the InternalsVisibleTo attribute for internal methods rather than expose them as public. – Robbie Dee May 31 '16 at 9:25
  • 1
    "Nowadays however, it isn't uncommon to see methods that could be left internal made public for ease of testing (since this usually happens in another assembly). " - really ? – Brian Agnew May 31 '16 at 14:01
  • 3
    @BrianAgnew alas yes. Its an anti-pattern along with "we must have 100% unit test coverage", including getters and setters :-( Why are coders so unthinking nowadays?! – gbjbaanb Jun 1 '16 at 9:03
  • 2
    @ChrisWohlert simple solution. Don't replace the ctor. Add a new one. Chain them together. Leave your existing test in place. It implicitly tests your builder and your code is still covered. Only write tests if its valuable to do so. There is a point where you get diminishing returns on your investment. – RubberDuck Jun 1 '16 at 9:18
  • 1
    @ChrisWohlert there is absolutely nothing wrong with providing a convenience constructor. You're still injecting the class' true dependencies. How do you think DI was done before all those "fancy" IoC container frameworks came around? – RubberDuck Jun 1 '16 at 9:25
0

I don't particularly buy this notion that you have to expose private methods (by whatever means) in order to perform suitable testing. I would also suggest that your client facing API is not the same as your object's public method e.g. here's your public interface:

interface IFooDao {
   public Foo GetFoo(int id);
}

and there's no mention of your FooBuilder

In your original question, I would take the third route, using your FooBuilder. I would perhaps test your FooDao using a mock, thus concentrating on your FooDao functionality (you can always inject a real builder if it's easier) and I would have separate tests around your FooBuilder.

(I'm surprised mocking hasn't been referred to in this question/answer - it goes hand-in-hand with testing DI/IoC-patterned solutions)

Re. your comment:

As stated, the builder provides no new functionality, so I should not need to test it, however, DIP makes it part of the API, which forces me to test it.

I don't think this widens the API you present to your clients. You can restrict the API your clients use via interfaces (if you so wish). I would also suggest that your tests shouldn't just surround your public-facing components. They should embrace all the classes (implementations) that support that API underneath. e.g. here's the REST API for the system I'm currently working on:

(in pseudo-code)

void doSomeCalculation(json : CalculationRequestObject);

I have one API method, and 1,000's of tests - the vast majority of which are around the objects supporting this.

  • I don't particularly buy this notion that you have to expose private methods (by whatever means) in order to perform suitable testing - I don't believe anyone is advocating this... – Robbie Dee Jun 1 '16 at 10:04
  • But you said in the above - 'Access modifiers are sometimes loosened to simplify testing' ? – Brian Agnew Jun 1 '16 at 10:05
  • Internals can be exposed to the test assembly and the implicit interface implementation creates public methods. I was never advocating exposing private methods... – Robbie Dee Jun 1 '16 at 10:08
  • From my PoV, 'Internals can be exposed to the test assembly' amounts to the same thing. I'm not convinced that testing internals (whether marked as private or otherwise) is a worthwhile exercise, unless as a regression test whilst refactoring poorly structured code – Brian Agnew Jun 1 '16 at 10:10
  • Hi Brian, I would, like you, fake the IFooBuilder when parsed to FooDao, but doesn't this mean I would need another test for the implementation of IFooBuilder? We are finally approaching the real issue, and not whether or not I should expose internals. – Chris Wohlert Jun 1 '16 at 10:13
0

Why do you want to follow the DI in this case if not for testing purposes? If not only your public API, but also your tests are really cleaner without a IFooBuilder parameter (as you wrote), it does not make sense to introduce such a class. DI is not an end in itself, it should help you to make your code more testable. If you do not want to write automated tests for that particular level of abstraction, don't apply DI at that level.

However, lets assume for a moment you want to apply DI for allowing to create unit tests for the FooDao constructor in isolation without the building process, but still do not want a FooDao(IFooBuilder fooBuilder) constructor in your public API, only a constructor FooDao(IFooConnection fooConnection, IBarConnection barConnection). Then provide both, the former as "internal", the latter as "public":

public class FooDao : IFooDao
{
    private IFooBuilder fooBuilder;

    // only for testing purposes!    
    internal FooDao(IFooBuilder fooBuilder)
    {
        this.fooBuilder = fooBuilder;
    }

    // the public API
    public FooDao(IFooConnection fooConnection, IBarConnection barConnection)
    {
        this.fooBuilder = new FooBuilder(fooConnection, barConnection);
    }    

    public Foo GetFoo(int id)
    {
        return fooBuilder.Build(id);
    }
}

Now you can keep FooBuilder and IFooBuilder internal, and apply the InternalsVisibleTo to make them accessible by unit tests.

  • The first part of your question is exactly what I am looking for. An answer for the question: "Should I use DIP here". I don't want that second constructor though. If I don't comply to DIP, I don't need to make IFooBuilder public, and I can stop faking / testing it. In short, you're telling me to stick to iteration 2. – Chris Wohlert Jun 1 '16 at 10:49
  • Honestly, if you remove everything from "However", I will accept the answer. I think the first part explains very well when and why I should / shouldn't use DI. – Chris Wohlert Jun 1 '16 at 11:55
  • I'd just add the caveat that DI isn't just about testing - it merely simplifies swapping one concretion with another. As to whether you need this here is something only you can answer. As 17 of 26 wonderfully puts it - somewhere between YAGNI & PYIAC. – Robbie Dee Jun 1 '16 at 12:32
  • @ChrisWohlert: I am not going to remove the 2nd part just because you don't like to apply it for your situation. You do not need tests on that level - fine, apply part 1, don't use DI here. You want tests and avoid to have certain parts in the public API - fine, you need two entry points in your code with different access modifiers, so apply part 2. Everone can pick the solution which suits him best. – Doc Brown Jun 2 '16 at 5:04
  • Sure everyone can pick the solution they want, but you misunderstood the question if you think I wanted to test the builder. That's what I tried to get rid off. – Chris Wohlert Jun 2 '16 at 5:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.