1

I'm trying to add tests to a legacy code, and as I start to adding code, I get the feeling that something is wrong.

In the following code, the public method RegisterChange is calling two private methods to:

  1. Get the object to store
  2. Store the object
public class ChangeService {

    IRepository repository;

    public ChangeService(IRepository repository){
        this.repository = repository;
    }

    public bool RegisterChange( int entityId ){ 
        var entity = GetParsedEntity( entityId );       
        SaveEntity( entity );
        return true;
    }

    private Entity GetParsedEntity( int id ) {
        var entity = repository.GetEntityById( id );
        return new Entity{ Name = entity.Name };
    }

    private void SaveEntity( Entity entity ) {
        repository.Save( Entity );
    }
}

public class ChangeServiceFact(){

    [Fact]
    public void When_valid_entity__Should_save_entity(){

        var mock = new Mock<IRepository>();
        var service = new ChangeService(mock.object);

        var result = service.RegisterChange( 0 );

        Assert.True(result);
    }   
}

So, when Im mocking the repository, I had to go and check the private method's code to know which operations to mock.

The problem that I'm seeing with this approach is that, because the code is testing not only the test subject (the public method) but also the private methods, is not clear which should be the test result by looking at the test subject (public method).

In the case that, later on, someone decide to modify one private method (like throwing an exception from GetParsedEntity), the test will continue to pass correctly, but the client code could fail because of this change.

In this particular case, Im using C#, XUnit and Moq, but I think is more a general testing question.

andymaster01
  • 107
  • 1
  • 6
  • Surely the test would *not* pass, because the exception would be thrown when you call `RegisterChange`. If it would *sometimes* throw an exception, then it's up to the person making the change to add a test for that. It should still be in line with the *documented* (ahem) behaviour of the public method. – Jon Skeet Oct 16 '13 at 19:16
  • @JonSkeet, let's says the change to GetParsedInt is not a breaking one, but a new functionality, in that case the old test is not aware of that. I understand a new test would pick up the new functionality test. So, my question is "Is this the normal methodology?" (just to give it a name). Thanks. – andymaster01 Oct 16 '13 at 19:26
  • 1
    Put it this way: whether the code for `GetParsedEntity` is inlined into the public method or not shouldn't affect the test. That's an implementation detail. So if you're not breaking anything, just adding a new piece of functionality which is tested separately, what's your concern? Seems reasonable to me. – Jon Skeet Oct 16 '13 at 19:54
  • @JonSkeet, thanks for your interest. My concern is related to the private methods dependencies. Setting some mocks in a test to be used not for the test subject, but for the private methods being called by the test subject feels brittle, but the private methods are only relevant for the public method, no one else is interested in what they do, so I don't see the point to make them public. I feel there is a gap in this implementation. Thanks again. – andymaster01 Oct 16 '13 at 20:02
  • Why do you think it's brittle? If you move the private method implementation into the public method, why would that change anything? – Jon Skeet Oct 16 '13 at 20:15
  • @JonSkeet I agree, moving the implementation detail to the public method doesn't change anything, but if I keep the private method, at first glance (looking only at the public method implementation) there is no real 'clue' about the dependency with the repository. To be aware that dependency, I will have through the private method's detail. Probably brittle is not the right word, a better one maybe would be "not so clear". – andymaster01 Oct 16 '13 at 20:28
  • 1
    The key is not to be interested in the *implementation* when you design your tests. You should be interested in the *effect* that the documentation/contract describes. Separate "what it's meant to achieve" from "how it's meant to achieve it". (That's where using fakes instead of mocks comes in handy, too...) – Jon Skeet Oct 16 '13 at 20:34
  • @JonSkeet I think I understand what you're saying, I'll recheck the mocks and fakes diferences. Many Thanks. – andymaster01 Oct 16 '13 at 20:41
  • 1
    @andymaster01: while we're on mocks/fakes topic, you might want to give [this question](http://stackoverflow.com/questions/13698969/what-is-the-difference-between-mocks-stubs-and-fake-objects) a read. Long story short, the difference between *mock* and *fake* is rather blurry nowadays. – k.m Oct 16 '13 at 20:46

1 Answers1

3

The problem that I'm seeing with this approach is that, because the code is testing not only the test subject (the public method) but also the private methods, is not clear which should be the test result by looking at the test subject (public method).

The test subject you mention has no visible effect without knowing its full contract. What the full contract here is? The mentioned public method and constructor, which takes dependency. It's the dependency that's important here and interaction with this dependency is what should be tested. Private methods are (as always) implementation detail - irrelevant to unit testing.

Having said that, let's get back to the contract. What is the actual contract of test subject (ChangeService method)? To retrieve object from repository basing on some id, create different object and save the later in the same repository. And this is your test.

[Fact]
public void ChangeService_StoresNewEntityInRepository_BasedOnProvidedId()
{
    const string ExpectedName = "some name";
    var otherEntity = new OtherEntity { Name = ExpectedName };
    var mock = new Mock<IRepository>();
    var service = new ChangeService(mock.object);
    mock.Setup(m => m.GetEntityById(0)).Return(otherEntity);

    service.RegisterChange(0);

    mock.Verify(m => m.SaveEntity(It.Is<Entity>(e => e.Name == ExpectedName));
} 
k.m
  • 30,794
  • 10
  • 62
  • 86
  • So, it's OK that, even looking at the `RegisterChange` method you don't see a direct usage of the IRepository, but looking at the test there is a mock for that class? I think that's my main concern. Thanks. – andymaster01 Oct 16 '13 at 20:36
  • 1
    @andymaster01: It is OK. This is the way that this specific object works - it interacts with dependency, with no visible result (*return value / state change*) to outside world. To check whether its contract has been fulfilled, you *ask* the dependency, not the test object itself. – k.m Oct 16 '13 at 20:45