Is it bad practice to repeat logic being tested in unit tests?

Question

Often I find myself writing a unit test for code and repeating some or a lot of the logic in the unit test to make the unit test DRY.

For example, consider the following piece of code:

function speak(conditionOne, conditionTwo) {
  if (conditionOne) {
    return "Hey"
  } else if (conditionTwo) {
    return "Sup"
  } else {
    return "Waddup"
  }
}

Now I could write a unit test using jest, for example, by hardcoding all the cases.

describe('speak', () => {
  test('when condition one but not condition two', () => {
    expect(speak(true, false)).toEqual('Hey')
  })

  test('when condition one and condition two', () => {
    expect(speak(true, true)).toEqual('

  test('when not condition one but condition two', () => {
    expect(speak(false, true)).toEqual('Sup')
  })

  test('when not condition one and not condition two', () => {
    expect(speak(false, false)).toEqual('Waddup')
  })
})

Or I could try to make it a bit more terse using syntactic sugar but still hardcoding the cases and expected outcomes:

describe('speak', () => {
  test.each`
    conditionOne | conditionTwo | value
    ${true}      | ${false}     | ${'Hey'}
    ${true}      | ${true}      | ${'Hey'}
    ${false}     | ${true}      | ${'Sup'}
    ${false}     | ${false}     | ${'Waddup'}
  `('when conditionOne is $conditionOne and conditionTwo is $conditionTwo then $value', ({ conditionOne, conditionTwo, value }) => {
    expect(speak(conditionOne, conditionTwo)).toEqual(value)
  })
})

But you could imagine if there is a decent amount of logic then the number of cases to hardcode in the table become exponential and the outcome isn't always so easy to hardcode as a value.

This is where I think it might be a good idea to make the test more intelligent, but then it just starts to become a repeat of the unit logic (and what if I got the unit logic wrong?)

For example:

describe('speak', () => {
  describe.each`
    conditionOne
    ${true}
    ${false}
  `('when conditionOne is $conditionOne', ({ conditionOne }) => {
     test.each`
       conditionTwo
       ${true}
       ${false}
     `('when conditionTwo is $conditionTwo', ({ conditionTwo }) => {
       let value
       if (conditionOne) {
         value = 'Hey'
       } else if (conditionTwo) {
         value = 'Sup'
       } else {
         value = 'Waddup'
       }

       expect(speak(conditionOne, conditionTwo)).toEqual(value)
     })
})

This now makes it much easier to test when manually writing out the table would be cumbersome. However, it seems like it is prone to the same bugs as the unit itself? But on the other hand it lets me refactor with confidence and I can see the test cases get printed out and make sure they are OK when the tests get run.

Answer 1

In real life, your code is going to contain real methods with meaningful names, real parameters with meaningful names, and real-life implications when they execute. It's those real-life implications that you will be testing in your unit tests, not the results of some abstract foobar if-conditions.

Each unit test should only be testing one thing, not six. Every time you create a branch in your code with an if statement, you should write one or more new tests to exercise that new branch. Doing so increases your code coverage and improves the quality of your tests.

Finally, each test should be given a meaningful name that embodies the real-life implication that it is testing. Something like: InvoiceGrandTotalShouldNotBeNegative, or whatever. See here for a good way to name your tests.

Answer 2

Is it bad practice to repeat logic being tested in unit tests?

No, but sort of.

No, in the sense that the tests are automated checks which verify that the test subject behaves like some simpler system. So if the problem you are trying to solve requires an elaborate dance of logic gates, then you are going to see shadows of that same logic in your tests.

This is especially true when you are doing something that can be modeled as a logic table; in order to measure that behavior, the test is going to have to have a copy of the logic table to work with.

(In some cases, this pushes you towards designs where you pass the logic table as an argument -- the table itself just becomes a big piece of data).

But sort of: often, the logic is actually a means to some higher semantic end; so the tests should be written at the higher level (describing the thing you actually want), rather than down in the implementation details. This may mean that the tests stay the same, but the names of things in the tests get changed.

Kevlin Henney has some wonderful talks about this

• What We Talk About When We Talk About Unit Testing
• Structure and Interpretation of Test Cases

Answer 3

Think of unit tests in this way:

Given a certain state, When an action is performed, Then there is an expected result.

(reference: Given-When-Then)

Your unit tests will be separated into three main sections:

• Setup: your code needs to be in the correct state, or you need to prepare the objects you pass in to your method properly. I.e. the code needs to be in the given state.
• Execution: This is where you are actually calling the method to test. I.e. this is the action to perform.
• Validation: This is where you are checking the results of the execution. You can validate all the expected state, but there should be one and only one correct answer for the given state. I.e. this is the resultant state

Each condition you test should be a separate unit test. If your unit tests look like they are re-implementing your code, then it is probably one of a couple things:

• Your code is very simplistic (i.e. your contrived example)
• Too much implementation detail is leaking out of your code (i.e. it is not well contained)
• You are missing edge cases (i.e. your test values are only within normal operating ranges, and there is no error handling being done)

It may be a bad thing, but it's worth checking. As long as your unit tests are testing in the way outlined above, the tests and the code will be sufficiently different. For example, speak(true,true) is not well defined, and is likely not an expected value. However, your test is assuming the implementation is correct. Instead, validate the actual requirements and put the correct expected answer for that state.

Using my contrived example, the given is implicit since the code is purely functional. But let's speak(true,true) is supposed to be "willkommen". Your test would be written as:

test('when condition one and condition two', () => {
    expect(speak(true, true)).toEqual('willkommen')
}

And you would have to fix your code to make that true.

Answer 4

You ask: within one unit test

• Can one have P(a, b, c); P(d, e, f); P(g, h, i); that is conditionally garded tests, or
• Would it be better to have if (a) P(b, c) else if ....

A simple if-hierarchy can be more readable and more clear. However when the conditions become interdependent and more complex, the first style appeals more to me.

Also as possibly more than one assert / call may happen, which otherwise would be uncaptured logic.

In fact, in contrast to the general "one test - one tested call" paradigm, I find using a helper function (test data construction) and more cases handled fruitfull, calling the helper function more than once.

1. Test failures are still precise, indicating the error.
2. The test code is better readable, and organized. (Not 30 tests split into 7-8-5-10 groups of similar ones.)
3. The maintenance seems better.

So an almost declarative listing of cases has my preference, despite the general rule. But I do not expect to get any points here, nor that it entirely covers your case.