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?

"My concern is that it's too tightly coupling the test to the code." Then that's an indication that you're using coupling too tightly. There are all kinds of mocks you could use instead, if introduced properly in the interfacing design.
– πάντα ῥεῖApr 7 '18 at 18:07

It might be better to test the overall outcome, for example that the data was in fact persisted to the database. You should be able to use an in-memory EF provider to do this.
– MatthewSep 27 '18 at 17:24

5 Answers
5

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.

I don't write tests for trivial methods, because I can be confident that they work just by looking at them. - if this is an advice, then one of the worst I've ever heard.
– t3chb0tApr 21 '18 at 19:58

1

@t3chb0t Keep in mind that Kent Beck, the guy who wrote the literal book on TDD has said things like "If code is so simple that it can't possibly break, and you measure that the code in question doesn't actually break in practice, then you shouldn't write a test for it..." and "If I don’t typically make a kind of mistake (like setting the wrong variables in a constructor), I don’t test for it." I would argue that someone who won't look at a trivial method like the one in this question and move on without a test around it, won't be able to be productive. Some things just aren't worth the time.
– DogsApr 21 '18 at 20:30

I don't want to argue about OP's example because it's garbage... but as far as the so called trivial methods are concerned, I've seen many times how such overconfidence about not testing them led to hard to track bugs. The catch is, that trivial code is usualy the mosty widely used, like simple comparers where you would think you cannot possibly make a mistake and yet the equality or sorting result isn't as expected because nobody tested it believing it doesn't have to be tested.
– t3chb0tApr 21 '18 at 20:31

1

I don't write things like getters and comparators for no reason. Something somewhere in your program is using the comparator and won't work if the comparator doesn't work. If you don't have tests around the code that's calling the comparator, you have much bigger problems than testing the comparator.
– DogsApr 21 '18 at 20:36

1

Someone who can't look at "return this.name;" and be completely confident in what it does will surely be unable to be confident that the test they wrote is testing what they think it is. Not to mention the fact that a lot of code of that nature is Auto generated. At some point you have to stop dealing in absolutes and accept that some code is less complicated than the code you would need in order to test it, and that writing tests in such situations is not a productive activity.
– DogsApr 21 '18 at 20:47

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.

This is exactly why I was asking the question: "This is just plumbing code; there's no real functionality being tested..." Would that you mean think unit tests here are unnecessary? Not only is there no logic being tested here, the test is brittle in that a change to how the save is done will break the test.
– Bob HornApr 8 '18 at 1:04

@BobHorn: IMHO, tests for this kind of plumbing code serve mostly to increase the coverage metrics. Chances are that if you call the wrong method or forget to call a method, that the same mistake exists in the expectations of the unittest as well.
– Bart van Ingen SchenauApr 8 '18 at 7:03

1

"Are you confident that the method works the way you expect it to?" I don't believe, at all, that the confidence (or lack of) you have in a method in it's current state constitutes the need for unit tests or not. Unit test coverage also helps with preventing/identifying breaking changes in the future.
– JᴀʏMᴇᴇApr 11 '18 at 13:00

@JᴀʏMᴇᴇ: Code coverage is only a useful metric if you write tests that verify code correctness.
– Robert HarveyApr 11 '18 at 14:50

1

@RobertHarvey - yep, but 'correctness' is subject to any change (albeit an incorrect change). It might be correct now, but tests also (not exclusively) help (not guarantee) in ensuring that the unit stays correct.
– JᴀʏMᴇᴇApr 11 '18 at 15:27

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.

So are you saying that, yes, a data method like this should indeed be tested for how it saves the data?
– Bob HornApr 8 '18 at 1:05

1

Your goal is to ensure that it saves the data. It appears that the only way to do it, given the current implementation, is to check whether SaveChanges was called. If the implementation was different, say it was calling this.SaveFoos() where SaveFoos is private, then the test would find a different way to check that the changes were successfully saved (thus testing both Write and the private SaveFoos at once).
– Arseni MourzenkoApr 8 '18 at 10:20

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.

I'd like to help define the context for you. What else are you looking for? We have a Foo entity, and we're saving it to the database. That's it. Any business logic would have been applied at the business logic layer (and tests would exist for that logic), then that layer calls the data method to save it to the database. Does that help?
– Bob HornApr 8 '18 at 1:06

@BobHorn - in that case testing persistence layer by mocking DbContext seems useless. You need to test that given value successfully saved to the database. For that you will write integration test which include actual domain logic with actual persistence layer.
– FabioApr 8 '18 at 1:47

My rules for when to mock are similar, with an important nuance. I NEVER mock 3rd party classes. Following this rule encourages good use of wrapping 3rd party code. As a result, I find I usually only ever mock out my repository classes.
– DogsApr 14 '18 at 14:12

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.