Unit testing of extremely trivial methods (yes or no)

Question

Suppose you have a method:

public void Save(Entity data)
{
    this.repositoryIocInstance.EntitySave(data);
}

Would you write a unit test at all?

public void TestSave()
{
    // arrange
    Mock<EntityRepository> repo = new Mock<EntityRepository>();
    repo.Setup(m => m.EntitySave(It.IsAny<Entity>());

    // act
    MyClass c = new MyClass(repo.Object);
    c.Save(new Entity());

    // assert
    repo.Verify(m => EntitySave(It.IsAny<Entity>()), Times.Once());
}

Because later on if you do change method's implementation to do more "complex" stuff like:

public void Save(Entity data)
{
    if (this.repositoryIocInstance.Exists(data))
    {
        this.repositoryIocInstance.Update(data);
    }
    else
    {
        this.repositoryIocInstance.Create(data);
    }
}

...your unit test would fail but it probably wouldn't break your application...

Question

Should I even bother creating unit tests on methods that don't have any return types* or **don't change anything outside of internal mock?

Answer 1

It's true your test is depending on your implementation, which is something you should avoid (though it is not really that simple sometimes...) and is not necessarily bad. But these kind of tests are expected to break even if your change doesn't break the code.

You could have many approaches to this:

• Create a test that really goes to the database and check if the state was changed as expected (it won't be a unit test anymore)
• Create a test object that fakes a database and do operations in-memory (another implementation for your repositoryIocInstance), and verify the state was changed as expected. Changes to the repository interface would incurr in changes to this object as well. But your interfaces shouldn't be changing much, right?
• See all of this as too expensive, and use your approach, which may incur on unnecessarily breaking tests later (but once the chance is low, it is ok to take the risk)

Answer 2

Don't forget that unit tests isn't just about testing code. It's about allowing you to determine when behaviour changes.

So you may have something that's trivial. However, your implementation changes and you may have a side effect. You want your regression test suite to tell you.

e.g. Often people say you shouldn't test setters/getters since they're trivial. I disagree, not because they're complicated methods, but someone may inadvertently change them through ignorance, fat-finger scenarios etc.

Given all that I've just said, I would definitely implement tests for the above (via mocking, and/or perhaps it's worth designing your classes with testability in mind and having them report status etc.)

Answer 3

Ask yourself two questions. "What is the manual equivalent of this unit test?" and "is it worth automating?". In your case it would be something like:

What is manual equivalent? - start debugger - step into "Save" method - step into next, make sure you're inside IRepository.EntitySave implementation

Is it worth automating? My answer is "no". It is 100% obvious from the code. From hundreds of similar waste tests I didn't see a single which would turn out to be useful.

Answer 4

The general rule of thumb is, that you test all things, that could probably break. If you are sure, that the method is simple enough (and stays simple enough) to not be a problem, that let it out with testing.

The second thing is, you should test the contract of the method, not the implementation. If the test fails after a change, but not the application, then your test tests not the right thing. The test should cover cases that are important for your application. This should ensure, that every change to the method that doesn't break the application also don't fail the test.

Answer 5

The short answer to your question is: Yes, you should definitely test methods like that.

I assume that it is important that the Save method actually saves the data. If you don't write a unit test for this, then how do you know?

Someone else may come along and remove that line of code that invokes the EntitySave method, and none of the unit tests will fail. Later on, you are wondering why items are never persisted...

In your method, you could say that anyone deleting that line would only be doing so if they have malign intentions, but the thing is: Simple things don't necessarily stay simple, and you better write the unit tests before things get complicated.

It is not an implementation detail that the Save method invokes EntitySave on the Repository - it is part of the expected behavior, and a pretty crucial part, if I may say so. You want to make sure that data is actually being saved.

Just because a method does not return a value doesn't mean that it isn't worth testing. In general, if you observe good Command/Query Separation (CQS), any void method should be expected to change the state of something.

Sometimes that something is the class itself, but other times, it may be the state of something else. In this case, it changes the state of the Repository, and that is what you should be testing.

This is called testing Inderect Outputs, instead of the more normal Direct Outputs (return values).

The trick is to write unit tests so that they don't break too often. When using Mocks, it is easy to accidentally write Overspecified Tests, which is why most Dynamic Mocks (like Moq) defaults to Stub mode, where it doesn't really matter how many times you invoke a given method.

All this, and much more, is explained in the excellent xUnit Test Patterns.

Answer 6

A method that does not return any result still changes the state of your application. Your unit test, in this case, should be testing whether the new state is as intended.

Answer 7

"your unit test would fail but it probably wouldn't break your application"

This is -- actually -- really important to know. It may seem annoying and trivial, but when someone else starts maintaining your code, they may have made a really bad change to Save and (improbably) broken the application.

The trick is to prioritize.

Test the important stuff first. When things are slow, add tests for trivial stuff.

Answer 8

When there isn't an assertion in a method, you are essentially asserting that exceptions aren't thrown.

I'm also struggling with the question of how to test public void myMethod(). I guess if you do decide to add a return value for testability, the return value should represent all salient facts necessary to see what changed about the state of the application.

public void myMethod()

becomes

 public ComplexObject myMethod() { 
 DoLotsOfSideEffects()
 return new ComplexObject { rows changed, primary key, value of each column, etc };
 }

and not

public bool myMethod()  
  DoLotsOfSideEffects()
  return true;