-1

I have a simple data method that does this:

public void Write(Foo foo)
{
  db.Foos.Add(foo);
  db.SaveChanges();
}

I was asked to write unit tests for this. To do so, I had to create a fake DbContext and DbSet. I'm basically just asserting that Add() was called on the DbSet, and that Save() was called on the DbContext.

My concern is that it's too tightly coupling the test to the code. The test is basically dictating how the code does its job, instead of what it does.

I'm just looking for a sanity check here. Am I wrong? Is it good to have tests like this?

Bob Horn
  • 2,347

5 Answers5

6

The decision on whether or not to unit test a method lies in the answer to the question "Are you confident that the method works the way you expect it to?" If the answer is yes, then no further unit testing is required.

Of course, that might not be your decision to make.

My concern is that it's too tightly coupling the test to the code.

Unit tests don't need to be decoupled, except insofar as you use interfaces to allow the use of mocks and stubs. Decoupling as a technique is only useful for production code, not test harnesses. The test harness is going to fall away anyway when you do your production build.

To do so, I had to create a fake DbContext and DbSet. I'm basically just asserting that Add() was called on the DbSet, and that Save() was called on the DbContext.

Frankly, these kinds of unit tests are not all that interesting. This is just plumbing code; there's no real functionality being tested, other than passing data around. If code like this falls over because you didn't call the right method, it will become clearly obvious when you perform your integration testing anyway.

If you're still going to go down this path, and the number of tests you need to write is very large, consider code-generating not only the tests but also the actual plumbing code, or use a generic repository instead.

Robert Harvey
  • 200,592
6

Ultimately tests are a tool. Like all tools, there's a cost, and a possible benefit. In this case, the test has a high cost (the test is dictating implementation details and will break if the implementation changes, preventing refactoring) and a low benefit (the code you're testing is so simple that its trivially verifiable), so I cannot justify this test.

I don't write tests for trivial methods, because I can be confident that they work just by looking at them. And even if they don't work, they're likely being used in a larger context anyways, and if you're testing behaviors rather than methods, your other tests should catch the problem.

Mantras like "you must achieve 100% code coverage" or "you must write a test for every method" are ill-conceived and stand in opposition of productive engineers who are trying to deliver quality code that furthers their organization's goals.

To do so, I had to create a fake DbContext and DbSet. I'm basically just asserting that Add() was called on the DbSet, and that Save() was called on the DbContext.

My concern is that it's too tightly coupling the test to the code. The test is basically dictating how the code does its job, instead of what it does.

This is exactly correct. Such a test is not asserting behavior, but instead merely restating the code you implemented inside out. A test like this asserts that the code hasn't changed, not that the behavior hasn't changed.

As an example, imagine if db.Foo had another method, called AddAndSave(...) that handled both operations for you. If you replaced your implementation with one that simply calls this AddAndSave method, the test would fail, and yet the behavior is the same. Even worse, imagine if db had a method UndoLastSave() that undid the previous save call. You could add this to the end of your method, and the test would still pass even though your method no longer has the desired behavior.

Usually, when I see people writing these kinds of tests, it is a symptom of testing at the wrong granularity. It is commonly stated that unit tests should test one method only, but that is a warped interpretation of unit testing. Your system need not be isolated to a single method or class, because there is nothing that forces you to treat those as your 'unit'. I would instead try writing a test more along the lines of this (please excuse my pseudo-code):

//given system.Clear() Foo foo = FooFactory.SomeFoo()

//when system.Write(foo)

//then assertTrue(system.Contains(foo))

The idea here is that we want to test the behavior of write. How did the state of the world change after write was called? This test calls more than one method on the system, and that's fine.

Dogs
  • 1,196
2

This “simple data method” is not very different from any other methods you test.

As any other method, this one could have regressions. For instance, what happens if somebody erroneously comments the second line? Right, the code is still executed, no exceptions being thrown, and you have a nasty runtime bug which is quite difficult to track down once it starts occurring in production.

Tests are expected to reduce the risk of introducing those regressions. Since this is business code, no matter how simple, it should be covered. Note, however, that:

  • If this is an isolated method (i.e. you don't have the same one for Bar and Baz and hundreds of other types), writing a test for it should be extremely simple and straightforward.

  • If there are hundreds of methods which do exactly the same, but for different types, then think about using code generation instead. There is no need to write this method by hand in the first place, and you don't test the code you don't write (testing the code generator, however, is essential).

The test is basically dictating how the code does its job, instead of what it does.

Unit testing is white-box testing, so tests are based on the actual code. You should, however, try to think your test in functional terms. Here, the method is too basic to make any difference: the functional need is, when I Write a Foo, to ensure that Foo is successfully added to the sequence, and that the change is saved. There are really no much different ways to code or test it.

Arseni Mourzenko
  • 137,583
2

You give very little context.

You should write tests which cover whole "feature" you are testing and mock only dependencies which makes your tests slow (web services, file systems, database).
When tests cover actual production code as much as possible - you will have freedom to refactor/optimize/re-design your code without touching a tests and will have quick feedback about correctness of your application.

You are right - mocking DbContext will tightly couple your test to implementation details, where changing a code whithout changing behaviour will force you to change tests too. For example if you decide to use DbContext.Foos.AddRange(new[] {foo}).

You didn't gave us full context - you just have a method which saves given object to the database. Because you do not have other logic to test - you need to test that given object actually was saved to the database. You can run test against actual database and call it "integration" test. Based on the framework you are using you can execute tests against "InMemory" database which keep test quick enough.

Fabio
  • 3,166
0

Yes, you are wrong. Your method does 2 diff things. It adds foo to Foos, which changes db's state, and it saves everything. Testing that both of those things happen is worth it. The fact that you don't like all the leg work of making mocks is a separate problem. You might be able to improve the testability by passing in a db object/context/whatever.

Josh
  • 175
  • 9