-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,327
  • 3
  • 20
  • 26
  • _"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 07 '18 at 18:07
  • 1
    Possible duplicate of [Where is the line between unit testing application logic and distrusting language constructs?](https://softwareengineering.stackexchange.com/questions/322909/where-is-the-line-between-unit-testing-application-logic-and-distrusting-languag) – gnat Apr 07 '18 at 21:34
  • 4
    This is a worthless test. Trust your intuition. – Frank Hileman Apr 09 '18 at 22:42
  • 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. – Matthew Sep 27 '18 at 17:24

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
  • 198,589
  • 55
  • 464
  • 673
  • 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 Horn Apr 08 '18 at 01: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 Schenau Apr 08 '18 at 07: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 Harvey Apr 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
  • @JᴀʏMᴇᴇ: My point is that **you** are the arbiter of correctness, not code coverage. For that, you must practice discernment, and for that, you need confidence in your methods. – Robert Harvey Apr 11 '18 at 15:29
  • @RobertHarvey - I think I may be missing your point here, or we're talking about different things, it's been a long day. Are you saying having a unit covered doesn't at least help towards preventing a breaking change? – JᴀʏMᴇᴇ Apr 11 '18 at 15:33
  • @JᴀʏMᴇᴇ: Your original assertion is that confidence in whether your method works or not doesn't determine whether you need more (or any) tests. I'm saying that confidence in your method is the only criteria that matters. I'm also saying "don't test trivial methods." If you're unit testing a trivial method that is adequately covered by integration testing anyway, on the off-chance that someone might come along and put in a breaking change, I think you have larger problems. – Robert Harvey Apr 11 '18 at 16:07
  • @RobertHarvey - absolutely, confidence is the only criteria that matters - in the sense that it'd be nice to move forward with confidence that any unwanted change to a method's purpose would be highlighted asap. I interpreted your original comment as "only test the method if you're not confident it works at the moment". Since you said in the case of 'yes' there's no need to test it. i.e. TDD. I may have completely misunderstood you here, I accept that. – JᴀʏMᴇᴇ Apr 12 '18 at 08:15
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,166
  • 7
  • 11
  • _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. – t3chb0t Apr 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. – Dogs Apr 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. – t3chb0t Apr 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. – Dogs Apr 21 '18 at 20:36
  • On the contrary... the most trival code should be tested in every possible way because it's probably being used everywhere and very often. And... it's much easier to test and assure that this simple part is working fine before building more complex components relying on those _simple_ ones. If I know that the dependency works as expected then I can take care of the new component. If I start testing the bigger one, I'm looking for the bug everyhwere... such a waste of time ;-] – t3chb0t Apr 21 '18 at 20:39
  • 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. – Dogs Apr 21 '18 at 20:47
  • @t3chb0t: **Never lose track of what you're testing**. In OP's example, you should not be testing whether `Add` and `SaveChanges` are working as expected (if you want to test for that, test it in a specific unit test for the method itself). The only thing you should be unit testing in `Write(Foo)` is if the method's logic (add, then save) is being executed. But this is in fact **trivially readable** from this method body. Note that if e.g. there is more complex logic that decides whether to add the item or not, which is prone to error or not as trivially readable, then that does merit testing – Flater Sep 28 '18 at 09:20
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
  • 134,780
  • 31
  • 343
  • 513
  • So are you saying that, yes, a data method like this should indeed be tested for *how* it saves the data? – Bob Horn Apr 08 '18 at 01: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 Mourzenko Apr 08 '18 at 10:20
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,086
  • 1
  • 17
  • 25
  • 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 Horn Apr 08 '18 at 01: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. – Fabio Apr 08 '18 at 01: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. – Dogs Apr 14 '18 at 14:12
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
  • 7