4

I do a lot of coding in python and got a lot of if conditions without an else statement so to say partial branches.

E.g.:

# if a certain kwarg was passed to a function call
if kwargs.get('a_option'):
    # overwrite an entry in an already existing dict:
    config['a_option'] = kwargs['a_option']

This always shows up in my test coverage as a partial hit unless I mark it with a pragma: no branch.

So my question is, is it considered bad practice to use partial branches? And if it is, are there any popular ways how to avoid them? (not something like "just add else without any statements like in python else: pass")

Note that this is refering to branching in general and not only python. I just used that as example but I'm interested in general how such things are handled in no matter what language.

Igl3
  • 149
  • 3
  • 2
    I don't understand why that's showing as a partial hit. What is it that the test provider thinks it's not hitting? Sounds like a flaw in that test provider to me. It's extremely good practice to have "partial branches", rather than pointless, empty `else` blocks. – David Arno Feb 16 '18 at 15:02
  • I actually can't tell what the reasons are that `coverage.py` is considering the `else` part of a branch as a lack of coverage (showing in the coverage percentage unless excluded with the pragma). But I guessed this implicates either bad practice in my coding or this is just related to the interpretation of my tests and the coverage. I mean, why should I test an `else: do nothing`. Maybe it's just me being confused about why the coverage is even warning about this and concluding, there could be a design flaw. – Igl3 Feb 16 '18 at 15:10
  • In this example you could do better. The String 'a_option' is used three times. So there is room for improvement. Maybe you could map all kwargs to the config in one step? Why do You use two different ways do access the value (first time wiht get('key') and second time wiht ['key']) ? – Tobias Otto Feb 16 '18 at 15:19
  • This is just a very simple example to show a partial branch and not to be improved :P – Igl3 Feb 16 '18 at 15:24

3 Answers3

6

You must distinguish between line coverage and branch coverage. Line coverage measures the percentage of lines that were executed, branch coverage the percentage of branches that were taken. An if always has two branches (for True and False), irrespective of the presence or absence of an else block.

Let's look at this simplified code:

def under_test(cond):
  foo()
  if cond:
    bar()
  baz()

If we run under_test(True) we get full line coverage, as all code foo(); bar(); baz() will be executed. But the if still has only partial coverage, because we didn't take both branches – it was never false. We need another test case under_test(False) that executes foo(); baz() to get full branch coverage.

The branch coverage reports can guide you towards test cases that you missed (a white-box testing methodology). If a branch is never executed in your tests, this suggests three possible courses of action:

  • You might determine that this behaviour isn't needed, so you remove the conditional. If you follow TDD, this conditional should never have been written because it doesn't have a test.
  • The behaviour is important, so you add a test case to exercise this branch.
  • The behaviour is not very important, so you annotate the line to exclude it from coverage. This is sensible for debugging code such as an assert False that should never be reached.
amon
  • 132,749
  • 27
  • 279
  • 375
  • 2
    Ah, that makes sense. If @Igle only has tests where `kwargs.get('a_option')` is `true`, then there is indeed incomplete coverage. Proper coverage requires that a `false` test exist too. So the presence or lack of an `else` is actually orthogonal to the partial hit report. – David Arno Feb 16 '18 at 17:55
1

Your code needs to have correct logic. That is, a function should compute a value, display something, etc. in accordance with its contract. If that means using an if without an else, so be it. Some code branches may be really difficult or impossible to cover with automated unit tests, and that is okay.

Having 100% test coverage is a laudable, but impossible, goal. Cover as much code in test as you can, leave the rest for manual testing. Remember, your code needs to be correct and tests that prove it are a means to an end: delivering software with as few bugs as possible. Checking off the "100% code coverage" box on some checklist is not the end goal.

One option you might consider is refactoring, but only if this is a nontrivial amount of code:

someFunction() {
  if (a) {
    foo(bar);
  }
}

foo(bar) {
  // Do a bunch of stuff
}

You can now cover the internals of the if in their own method without needing to set up condition a in the first method, which may be tricky to do correctly. Again, do not take this as blanket advice to take the guts of any conditional and split it into a new function: this makes sense sometimes, other times, not so much. There is no "best practice" (that term makes me cringe more the older I get). You simply need to develop an eye for how to refactor code in a way that your team feels is readable and easily maintainable.

-4

As one can see in this example the rule not to use an if without an else leads often to a better software-design.

Yes, it is a bad practice to use partial branches.

The only way to avoid them is too think about the else-case. Please ask yourself, what should happen, when in the other case. Is there really no need for any special handling of the other case? Is there any way (like in the example) to optimize the sourcecode?

Tobias Otto
  • 131
  • 6