13

Say I have a method like this:

public void OrderNewWidget(Widget widget)
{
   if ((widget.PartNumber > 0) && (widget.PartAvailable))
   {
        WigdetOrderingService.OrderNewWidgetAsync(widget.PartNumber);
   }
}

I have several such methods in my code (the front half to an async Web Service call).

I am debating if it is useful to get them covered with unit tests. Yes there is logic here, but it is only guard logic. (Meaning I make sure I have the stuff I need before I allow the web service call to happen.)

Part of me says "sure you can unit test them, but it is not worth the time" (I am on a project that is already behind schedule).

But the other side of me says, if you don't unit test them, and someone changes the Guards, then there could be problems.

But the first part of me says back, if someone changes the guards, then you are just making more work for them (because now they have to change the guards and the unit tests for the guards).

For example, if my service assumes responsibility to check for Widget availability then I may not want that guard any more. If it is under unit test, I have to change two places now.

I see pros and cons in both ways. So I thought I would ask what others have done.

Improve this question
4
  • 18
    You are not making "more work" for the maintainers. If they change logic, they must change the corresponding unit tests. That's how it works. I don't see your cons: if your unit test didn't require changing, then it wouldn't be testing anything, would it? You might as well question whether unit testing is useful at all.
    – Andres F.
    Nov 28, 2012 at 17:30
  • 9
    This is off topic, but I would change the logic to throw an exception if the part number is 0 or less, or if the part is not available, as in my opinion it would be a bug to allow someone to call that method with a bogus widget, silently masking another problem.
    – Matthew
    Nov 28, 2012 at 19:03
  • 2
    @Matthew very good point. This function lies. The naming tells you it's gonna order something. And then it doesn't but you will never know, unless you apply the same logic as is inside, which iviolates DRY. In other words: if the design is changed to be more correct, this question maybe wouldn't have been asked in the first place.
    – stijn
    Nov 28, 2012 at 21:20
  • 2
    but it is not worth the time" (I am on a project that is already behind schedule). We are software developers. The only time we are on schedule is when we are dead :)
    – maple_shaft ♦
    Nov 30, 2012 at 14:38

5 Answers 5

Reset to default
27

Part of me says "sure you can unit test them, but it is not worth the time" (I am on a project that is already behind schedule).

It's three very short tests. You spent as much time asking yourself the question.

But the other side of me says, if you don't unit test them, and someone changes the Guards, then there could be problems.

Listen to this side.

But the first part of me says back, if someone changes the guards, then you are just making more work for them (because now they have to change the guards and the unit tests for the guards).

If your maintainer is a TDD nut, you're making it more difficult for them. Any change I make without there being a related change or addition of tests leads to my having to think hard. In fact, I would probably add the tests before I go ahead and make the change.

The first part of you is just plain wrong. Give the second part a pat on the back and stop thinking about it.

Improve this answer
4
  • +1 for succinctness even though I wrote an answer too heh
    – Jimmy Hoffa
    Nov 28, 2012 at 17:51
  • 3
    +1. Yes, if the requirements change, the some tests will have to be adapted.
    – Olivier Jacot-Descombes
    Nov 28, 2012 at 18:10
  • While I like what you say, I have many instances of this kind of call in my code (the front half of an async call). So it is not just 3 unit tests. Still, if it is the "right" way then I want to get them done.
    – Vaccano
    Nov 28, 2012 at 21:47
  • @Vaccano: The more of them you have to write, the more logical paths you don't have tests for and the more necessary it is to write them.
    – pdr
    Nov 28, 2012 at 22:14
9

It would simplify unit testing if the guard logic and the actual ordering were separate methods.

In the Widget class

public bool IsReadyForOrdering { get { return PartNumber > 0 && PartAvailable; } }

or an equivalent method somewhere else

public bool IsWidgetReadyForOrdering(Widget widget)
{
    return widget.PartNumber > 0 && widget.PartAvailable;
}

The order method

public void OrderNewWidget(Widget widget)
{
   if (IsWidgetReadyForOrdering(widget)) {
        WigdetOrderingService.OrderNewWidgetAsync(widget.PartNumber);
   }
}

Now testing IsWidgetReadyForOrdering became easy. Don't think a long time about it any more. Test it!

Improve this answer
16
  • 1
    Oof, stateful logic in a property.. -1 that should be a method and it should take the PartNumber, PartAvailable due to it's simplicity. Make it protected if it need not be a part of the public API but not a property.. Also I generally disagree. The guard logic inside the method is totally fine, and in my eyes better because it's such a small method you're just polluting the class to make an already small method smaller. Add it to the class in this case only if it's repetitive logic.
    – Jimmy Hoffa
    Nov 28, 2012 at 17:40
  • 1
    First, this doesn't answer the question. Second, it's untrue. You need to write three tests either way. Third, it exposes implementation details to calling code, unnecessarily.
    – pdr
    Nov 28, 2012 at 17:50
  • 8
    @JimmyHoffa: where do you see any stateful logic in that property? In fact, the example shows how to separate the state-changing logic (OrderNewWidget) from the non-statechanging logic (ReadyForOrdering). And I am a strong believer that extracting even that small functions will improve the code, and not making it worse, as you state. So +1.
    – Doc Brown
    Nov 28, 2012 at 17:54
  • 2
    The method OrderNewWidget is probably in another class than Widget, since it has a Widget argument. Since the method has no return value, testing it is not obvious. You would have to inject a WigdetOrderingService-mock that tracks the OrderNewWidgetAsync call.
    – Olivier Jacot-Descombes
    Nov 28, 2012 at 17:57
  • 2
    @JimmyHoffa: actually, your definition of "stateless" means "static" in C#. Thus, what you say is "properties should not access state of an object". But even dumb getters access state variables, so that would mean "write only static properties" - which does not seem to make much sense.
    – Doc Brown
    Nov 28, 2012 at 20:37
6

If you don't have time in your schedule for unit testing but you have time put aside for solid QA usage, then ask if you can steal some of that QA time to write unit tests, or if you can spend some of the QA period doing unit tests, or perhaps just deal with non-unit tested code.. Unfortunately schedules that are immovable force you to make concessions or work yourself to death, I generally suggest the first option because the second one will result in you incapable of supporting/maintaining the system correctly for the term of it.

That said, to your general question of testing guard statements; Yes! Absolutely test guard statements! Those are important parts of the behaviour of that method, you wouldn't want to find out somebody misunderstood something doing a bug-fix and removed your guards or changed the && to an || would you? Unit tests will ensure that a) you actually got the logic on your guards correct and b) nobody breaks that logic later without getting a complaint when they run the unit tests telling them it should be that way for some reason.

Improve this answer
0

There are some excellent answers above, and the points they make are very important. But one that seems to have been missed is that of you have a comprehensive set of unit tests, they read like an extremely detailed specification of the code. If you omit testing validation just because the code is so easy it's hard to see how it could go wrong, part of your specification is missing. If I came into a team where these tests had been missed out, I would actually assume your service wasn't validating its arguments.

Improve this answer
-5

Code is code. You should try to get 100% coverage when testing. If it weren't important it wouldn't be there.

Improve this answer

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.