Don’t unit test trivial code

Hopefully just a brief post in response to one I consider to be spectacularly wrong.

TL;DR

  • Don’t test other people’s code.
  • Don’t test implementation details.
  • Don’t pad your test suite with pointless tests.
  • Don’t have code which is only ever called by tests.
  • TDD is awesome; don’t make it look bad.

What’s all this about then?

The author of the above-linked post knows a hell of a lot about TDD and the various disciplines that go along with it, and I’ve generally got a lot of respect for him and his work, but when you’re wrong, you’re wrong, and when you’re an authority figure and you’re wrong it needs to be pointed out in case you make a lot of other people wrong too.

The assertion (no pun intended) at issue is that this line of code requires a unit test to cover it:

public int Year { get; set; }

The proposed tests for this single line of code are:

[Fact]
public void GetYearReturnsAssignedValue()
{
    var sut = new DateViewModel();
    sut.Year = 2013;
    Assert.Equal(2013, sut.Year);
}

[Fact]
public void GetYearReturnsAssignedValue2()
{
    var sut = new DateViewModel();
    sut.Year = 2010;
    Assert.Equal(2010, sut.Year);
}

The post goes on to discuss using parameterized tests to “save time”, but never thinks to suggest writing some reflection code that finds all the public types in an assembly, then all the public properties on all of those types, and tests setting them to all the possible values to make sure it always works, if you will excuse the reductio ad absurdum (of course, this wouldn’t show up in your code coverage, so you’d probably end up using T4 templates to generate parameterized tests and then check them into your test suite…).

Anyway, here are a few reasons why it’s wrong.

1. It’s testing the compiler, not your code

The line of code being tested is syntactic sugar for a property with a backing field. The tests are checking only that the compiler has done its job properly, and that’s already been extensively tested by the people who wrote the compiler. You don’t need to test it again. This applies to all language features, standard library functions and classes, and public APIs. You don’t test the .NET StreamWriter class to make sure it writes to a stream; you test the code you’ve written to make sure it writes the right thing to the stream.

2. It’s test-driving an implementation detail, not application logic

This Year property is clearly used by some application logic, otherwise it is redundant and should be excised. Both the getter and the setter for the property will be covered by the tests which test whatever the class with the year property actually does, and hopefully with a representative range of valid values.

There is no user story on the board which says “As a developer using the DateViewModel, I want to set the Year property to a positive integer, so that I can get it back again later”.

There may be a user story that says “As a user, I want to be prevented from accidentally adding events in the past”, in which case there should be some validation code that happens in the view-model when the Year property changes, and that should be tested.

In the paragraph headed “Causality”, the author makes the argument that “the tests drive the implementation”. This is true, but what you are implementing here is not a Year property; it’s some kind of higher-level date-related functionality that is required by your application. The Year property is a part of that implementation; it is not the implementation itself.

3. Pointless tests are worse than no tests at all

Using the suggested approach to testing, it would probably be possible to achieve 100% coverage of all public properties and methods in a project without actually testing 100% of the application logic. Now you have a metric that tells you you’ve done everything right, even though you’ve done everything wrong.

For example, let’s say our hypothetical DateViewModel also has Day and Month properties, and a Date property, all of which have getters and setters. Setting any of Day, Month or Year should change the Date property, and setting the Date property should change all the others. But with tests that test each property in isolation, you have 100% code coverage, and no useful information at all. Conversely, if you have a test for the Year property that asserts against the Date property, and a test for the Date property that asserts against the Year property, then the other tests are redundant, and redundant tests serve only to make the test suite look better than it really is.

4. It hides code redundancy

Code coverage is not just a metric for making sure you’re testing properly, it’s an excellent way of detecting redundant code. If you have worthless tests covering essentially non-functional code, then neither the compiler nor your coverage tool will ever be able to say “this property doesn’t appear to be used by anything”.

5. It presents TDD in a terrible light

The author of the post suggests that Bob Martin’s statement that “you don’t need to test-drive trivial code” is “horrible advice for novices”. I would submit that the counter-advice he goes on to give is far more horrible, since it makes TDD look unnecessarily time-consuming, pedantic and pointless. Telling people that they can’t apply judgement, common sense and pragmatism to TDD is going to result in them writing the whole thing off as a bad job, which is a far worse outcome than them maybe skipping the odd test here and there (which they will learn not to do over time, as those missed tests come back and bite them in the backside).

2013-03-12: The comments have descended into abusive nonsense, and been disabled.

Comments

  1. “It’s test-driving an implementation detail, not application logic”

    I think that is completely wrong. The way code behaves on the outside is part of the interface specification of the class under test, probably even part of the specification of an actual interface. The fact that the implementation is indeed trivial is something that *you* know when you write it, but the tests aren’t supposed to know that – because the triviality is what is *actually* an implementation detail.

    By not testing “trivial” code you rely on that implementation detail and neglect the documentation and specification function that tests also have.

    Also, you potentially have no safeguard against someone making the code non-trivial later on – if no test turns red they’ll assume everything is okay; and unless there are proper integration tests, that might remain hidden until peculiar behavior occurs at runtime.

    • My point was that the Year property in the example given is itself an implementation detail of a larger piece of functionality, and it should be tested as part of the tests for that larger piece, not in isolation.

      Imagine this test expressed using Spec-style syntax:

      Given that I have a DateViewModel,
      When I set the Year to 2013,
      It should set the Year to 2013.

      It’s fatuous. Are you going to do that for every property in your entire system? How about on DTO classes?

      I’m not arguing against TDD, or the need for as-close-to-100%-as-possible test coverage; the point is that dumb property accessors are either implicitly covered by higher-level tests, or they are redundant and should be removed completely.

      I have a lot of respect for the “documentation and specification function” of tests, but the fact that an integer property can be set to an integer is neither valuable documentation or meaningful specification. It’s something that is made blindingly obvious by the existence of an integer property.

      The old argument that someone might make a trivial implementation non-trivial later is equally facile. The only reason for changing the implementation of an automatic property is to introduce new functionality, such as a PropertyChanged event, in which case tests should be introduced to cover the new behaviour.

  2. Are *you* sure?

    I suggest re-reading the post.

    He makes none of the coverage arguments you’re straw-manning.

    The WritablePropertyAssertion is the reflection thing you you speak of and can be used at any desired granularity – your entire hard drive or a single property. Unfortunately Mark’s Idioms stuff is underdocumented at present but definitely has legs.

    I’ve held your view but it’s through painful experience over time I’m coming to realize that wishy washy “I’ll test it pragmatically” versus a clear set of principles as Mark elucidates leave you confused with lots of tests that fall between two stools of being nice minimal stressing of the critical stuff and overkill.

    Mark walks this walk and it shows:- http://blog.ploeh.dk/2013/03/01/AutoFixture3

  3. I don’t agree.

    It’s the opposite — It’s a waste of time if you don’t it.

    Let’s imagine that after 3 years someone removes the automatic property and (mistakenly) does:

    public int Year
    {
    get
    {
    return this.year;
    }
    set
    {
    return this.year * byte.MaxValue;
    }
    }

    What happens then?

    • Let’s set aside anything resembling reality and common sense and imagine that that scenario could ever actually ensue.

      If it did, then tests of the actual application logic and functionality would break.

      If somebody makes that bizarre change (perhaps as a parting shot after being fired?), and none of your tests start failing, then there is a very real problem with your test suite which would potentially have been concealed by the presence of a pointless “WhenISetAPropertyThePropertyIsSet” test.

    • The moment it stops being a trivial property is when you should add the test. I agree with Mark (this one, not the linked one :P), adding tests to test trivial properties is a waste of time. It’s actually going to waste my time during maintenance too, because the moment I see a test on a trivial property I’m going to have to dig in deeper to figure out why the writer thought it was necessary to write a whole test for GIGO.

  4. Assuming that the property name is Total (not Year).

    How would you know that the functionality is broken? Would you use deterministic input throughout your tests?

    • I don’t understand this question, nor the purpose or functionality of this “Total” property you have introduced into the conversation.

  5. I had much the same reaction when I read this post. Writing unit tests for automatic properties seems to me to be a case of TDDOCD. As you point out, if the Year property is actually useful then both getting and setting should participate in another unit test. Supposing DateViewModel is part of a WPF app, then the real risk is not that Microsoft somehow accidentally ship a C# compiler that can’t create automatic properties correctly, but that I’ve got my binding syntax wrong in the view, or that my post-compile PropertyChanged notify weaving has stopped working. IMO unit tests have most value when guarding the most fragile parts of the system.

    Then again, I must admit, as much as I admire the “write failing tests first” philosophy of TDD, its not the way I normally work. I suppose if you are 100% committed to writing no code without a failing test, it more or less forces you in the direction he is going.

  6. “You’re testing the compiler”. The thing here is that .NET has Property as a built-in concept. So indeed: when you declare a Property, it makes not much sense to write other than auto-discoverable tests to it.

    On the other hand, in a language where you want to _implement_ a property by using getters and setters, I believe it _does_ make sense to test that the property acts ‘property-like’. Especially because these languages are often used in the context of system programming, where from time to time people need to optimize for storage and they start packing data types into bit fields and the like, which proves not to be trivial.

    But that’s an edge-case. I agree: don’t test language idioms.

    • in other languages (e.g. Java) getters and setters are never written by hands – they are autogenerated by IDE, which makes such tests test IDE, which is pointless

  7. In many cases I completely agree that data associated with objects will be covered as a side effect by testing the behaviour of an application. If what you are building is an application where the UI or service interface is the only thing you care about, I agree that it would be pointless to explicitly test every data member of every class.

    However, if that’s the case, why expose those data members as properties (setters/getters)? Why not expose them as public fields? That would achieve the exact same effect (and no: I wouldn’t propose unit testing public fields).

    If you make the design choice to add a property to a class, there must be a reason for it. What would that reason be?

    • There are many reasons why you might need an automatic property instead of a field, but I’ll go with the absolute simplest case: implementing an interface. Interfaces cannot declare fields, only methods and properties.

      • Yes, that’s true.

        It’s going to be really difficult not to derail the discussion completely, but properties on interfaces is a design smell, so I wonder what other reasons there are for adding properties?

        • In MVP you often have property setters on the view.

          • Do they have to be properties, or could they be fields?

            • What about WPF or Silverlight? Don’t those ViewModels require properties in order for the UI to bind to them? Surely you don’t think every single property that the user will manipulate via the ui needs a unit test. Then you’re just testing that the WPF bindings work properly, and as the author of this post wrote, smart people have already tested that extensively.

        • Speaking just from the .Net perspective:

          I looked into this a couple years ago, if I recall correctly public fields and properties are compiled differently in .Net. Changing the implementation details of a property doesn’t change the signature of the library, so another library referencing the older version could use the newer as well. Changing from a public field to a property, however, changes the signature and would be a breaking change to the consuming library.

          Another reason would be general consistency with .Net design. MS’s design guidelines specifying not exposing public or protected fields. Disregarding the quality of the arguments, if any large portion of the .Net ecosphere is following a set of guidelines, then following those same guidelines helps make the code more approachable/readable.

  8. I totally agree with your sentiment Mark. I thing I would almost go one further, the whole notion of properties and the ‘I can change the implementation of getter/setters under the covers’ is the single biggest source of code morass in .NET project. You do not need many other property calls inside a setter/getter auto method to end up in ‘diamond of death’ territory of hard to follow logic.

    I can stomach properties for reflection purposes such as data binding and/or notification/reactive purposes, but for all other intents I take a readonly field any day of the week (and thereby not be concerned with testing the ability to give it an initial value…..)

  9. I totally agree with you Mark,

    Something that I find absolutely futile, and worse is the fact that some people do NOT know how to write unit tests and obviously do not understand their value. We have a suite of unit tests at my current place of work, and had at one previous to this, and I would say that 95% of the tests were utterly futile, as the management did not once look at what each of the tests were testing, just that fact that they wanted over 200 tests per project.

    What I also find soul destroying is when I make a change which breaks a test, so I have to fix one of these futile tests. Now this broken test does not point me in the direction that something in the business logic has changed and we have avoided disaster by having the test there, NO, it is a trivial test that tests nothing other than its’ own logic. Nothing in the application has changed, nor has broken, but only the test.

    No need to test the .NET framework, nor test the bleeding obvious, merely to pad out your own tests, write proper unit tests that test the business logic, or DO NOT BOTHER – you are not only fooling yourself, but probably your managers too.

    • We have over 6 million lines of production code and god knows how many poorly written, redundant “ball and chain” type tests which either test at the wrong abstraction, test meaninglessly, etc. It is always a bad thing when improvements to a system are hampered as opposed to enhanced by the test suite.

    • for such a silly system, the obvious silly answer is to Assert.True() at the end of each test?

  10. I agree with you and the argument “what if someone breaks your code in the future” is invalid for several reasons.
    1. The code I have written is off limits for anyone else.
    2. If someone messes with my code anyway then it’s his responsibility to not break it.
    3. If someone messes up my code, he might as well mess up my tests just in order to get them green again. So testing EVERYTHING does not make things more safe.

    • Splendid Light? YAGNI principle applied?? It’s not YAGNI, it’s a “I would like to encourage everyone to create extremely fragile systems for some illogical sense of purity”.

      Never recommend public fields. Ever. They’re binary incompatible and a breaking change for when you want to change it to a property – i.e. exposing internal implementation details of your class – breaking the first rule of encapsulation.

      Unnecessary rudeness removed by blog owner.

  11. That is trivial code, and if the developer used register 18 instead of 17, what is to say that when writing a test for it, he would have tested that register 18 held his value which it did.

    Hey presto, all tests pass – everything must be fine – nope, still wrong

  12. :-)

  13. “TL;DR
    Don’t test other people’s code.
    Don’t test implementation details.
    Don’t pad your test suite with pointless tests. [...]”

    If your project is truly doing TDD for its complete lifetime (I have yet to see a project that meets this description), maintenance is eventually going to be done, which involves adding new functionality. I would claim that it is not just worthwhile but necessary to break the above 3 rules. Whenever adding new code, I add tests to “bracket” existing functionality in several boundaries around the code I am going to change. This provides some assurance that I have not broken something when I add my new code and my new tests (supposedly in the other order but not always).

    … and of course, adding testing to a project that never had it blatantly breaks rule #1.

    • Your interpretation of rules #1 and #2 is either deliberately disingenuous or you’re not very bright.

      As for rule #3, by asserting that it is worthwhile and necessary to “pad your test suite with pointless tests”, I think we can safely say you’ve removed any lingering doubts that you had something to bring to this conversation.

  14. I think the use of word “trivial” is a bit unfortunate in whole discussion. My *IsInvoiceNameHighlighted* property might be as trivial implementation-wise as *CanEditInvoice* property, but messing up one of them might fall into way, way different realms of “trivial” for my application user. Same goes with Dan’s example of registers swap – 1 line assembly program might be indeed trivial, yet it’s “business value” (for lack of better phrase) is far from trivial (as example shown).

    • It’s important to distinguish “trivial code” from “trivial functionality”; ideally, there wouldn’t be any of the latter in any application (except maybe fart apps, but then, ideally those wouldn’t exist).

  15. While I don’t want to weigh in on the value of setter and getter I have on several occasions had to assist in scaling up an application. We set a team requirement that if code is being placed behind either the setter or the getter than the team member making the change is required to create a test case for it. I tend to lean towards being pragmatic there is little value in testing the compiler and if my team members modify a Property to add code behind either the setter or the getter than they have the responsibility to create a test case.

  16. Where do we draw the line of trivial? I’ve worked on one codebase where it was quite a common pattern to conceal a lazy-loader behind a property. That’s just one line of common, boring, predictable code that isn’t a part of any business rules. Should it be tested?

  17. Whatever – I’m used to casting pearls before swine: it goes with the territory. What I was attempting to do was to point out that, since even a one-line program could have a bug so severe that it necessitated an operating system patch, there’s no such thing as trivial code. I’m sorry if that was confusing. I could draw you a diagram if you require one. The register thing was an aside: it’s the sort of thing real programmers (of which you might end up being an example one day, if you try really hard) like to talk about.

    • “since even a one-line program could have a bug so severe that it necessitated an operating system patch, there’s no such thing as trivial code”

      Non sequitur; on a par with saying “since Sphynx cats have no fur, there’s no such thing as fluffy kittens.”

Trackbacks

  1. [...] trivial code – &Don’t unit test trivial code – Mark Seemann and Mark Rendle continue the TDD testing discussion with a look at the testing of [...]

  2. […] Don’t unit test trivial code – by Mark Rendle […]

Follow

Get every new post delivered to your Inbox.

Join 3,742 other followers

%d bloggers like this: