Logic in data provider vs test body

Question

Lets say we have a class Cat:

class Cat{
    public eat(String food){
        if (food.contains("cat")){
            burp();
        }
    }

    private burp(){
        System.out.println("burp!!");
    }
}

And we have the following code to test it (using Mockito and TestNG):

@DataProvider
public Object[][] dataProviderForTestFeedCat() {
    return new Object[][] {
      {"catfood", true},
      {"birdfood", false},
      {"dogfood", false},
      {"food for cat", true}
    };
}

@Test(dataProvider = "dataProviderForTestFeedCat")
public void testFeedCat(String food, boolean shouldBurp){
    Cat cat = new Cat();
    cat.eat(food)
    verify(cat, times(shouldBurp ? 1 : 0)).burp();
}

Someone suggested to me that its better to avoid shouldBurp ? 1 : 0 logic inside the test body and rather than have the count provided by the data provider directly:

@DataProvider
public Object[][] dataProviderForTestFeedCat() {
    return new Object[][] {
      {"catfood", 1},
      {"birdfood", 0},
      {"dogfood", 0},
      {"food for cat", 1}
    };
}

@Test(dataProvider = "dataProviderForTestFeedCat")
public void testFeedCat(String food, int burpCount){
    Cat cat = new Cat();
    cat.eat(food)
    verify(cat, times(burpCount)).burp();
}

Their argument is that the test body itself should be as simple as possible.
But a counter argument would be that the data provider should be simple and should only provide the raw data. It should not care that the test is actually generating an integer out the boolean to verify if a method is called or not.
A boolean also feels more intuitive and natural given that its very unlikely for this particular method to be executed twice in a single invocation in our real code.

What is the better approach?

Answer 1

I think your colleague's point is correct, but focusing on a detail that is much too fine-grained for what your test aims to achieve. But, in your colleague's defense, your code implies that it aims for these details, while I suspect it doesn't actually need it.

For example, if the amount of burping was variable, e.g. a cat burps twice when eating "catfish", then I hope you immediately see the need to test the specific amount of burps as well. Your current test doesn't actually allow for that (since you've hardcoded the 1 and 0), and your colleague's proposed test would allow for it.

In other words, in your colleague's proposed test the assertion would be "how often did the cat burp?". The answer to that is a number, so it makes sense for the expected outcome to be expressed numerically, which means your colleague is right.

However, I surmise that the goal of your current test is not to count the burps, but rather to confirm that the cat burped (at least once). The assertion here is "did the cat burp?". The answer to that is either yes or no, so it makes sense to express your expected outcome as a boolean value.

However...

... if this second case is indeed the purpose of your test, then you should not be testing if the cat burped precisely once (shouldBurp ? 1 : 0), but rather whether the amount of burps > 0. You are testing whether the cat burped, not whether it burped precisely once.

I think this is the basis for your colleague's advice. He saw you testing for a specific amount (that is variable), yet you did not pass it as a method parameter.

The answer here is that you should prefer to express your expected result in a way that matches the question that this test represents. First ask yourself what question this test seeks an answer to, and then put that answer in a format (data type) that matches the question.

I would suggest that depending on whether the amount of burps is relevant for the test or not, that you either follow your colleagues advice (if the amount is relevant) or assert for > 0 (if the amount isn't relevant).

---

A boolean also feels more intuitive and natural given that its very unlikely for this particular method to be executed twice in a single invocation in our real code.

Whenever I hear someone describe something as "unlikely" as a reason for why something shouldn't be accounted for, that is an immediate red flag.

Something is either possible or impossible. And you should only ever ignore the things that are impossible. If it's highly unlikely, that means it's not impossible, and you therefore cannot ignore it.

However, it being possible doesn't mean that it is relevant for this specific test. As I established above, if your test simply doesn't care about the amount of burps, and is only interested in their (non-)zero nature, then you can in fact ignore the existence of multiple burps for this test specifically.

Answer 2

    return new Object[][] {
      {"catfood", true},
      {"birdfood", false},
      {"dogfood", false},
      {"food for cat", true}
    };

Have you considered using a richer language to describe your domain?

    return new Object[][] {
      {"catfood", BURPS},
      {"birdfood", DOES_NOT_BURP},
      {"dogfood", DOES_NOT_BURP},
      {"food for cat", BURPS}
    };

BURPS/DOES_NOT_BURP could be a primitive value, or it could be an object that implements your validation criteria:

public void testFeedCat(String food, ???? desiredBehavior){
    Cat cat = new Cat();
    cat.eat(food);

    desiredBehavior.verify(cat);
}

Another possibility is that you are struggling with the fact that your tests definitions are coupled to your inputs, rather than to your specifications

public void catsBurpWhenFedCatFood(String catFood) {
    Cat cat = new Cat();
    cat.eat(catFood);

    BURPS.verify(cat)
}

Kevlin Henney has a number of very good videos talking about test design.

Along the way, you might also consider separating the measurement you make from the evaluation of the measurement.

private Cat catThatHasBeenFed(String food) {
    Cat cat = new Cat();
    cat.eat(food);
    return cat;
}

// "catfood", "food for cat"
public void catsBurpWhenFedCatFood(String catFood) {
    Cat cat = catThatHasBeenFed(catFood);

    BURPS.verify(cat)
}

// "dogfood", "birdfood"
public void catsDoNotBurpWhenEatingNotCatFood(String notCatFood) {
    Cat cat = catThatHasBeenFed(catFood);

    DOES_NOT_BURP.verify(cat)
}

Of course, here we aren't really interested in measuring cat, but instead the noises that the cat makes.

private List catThatHasBeenFed(String food) {
    Microphone microphone = new Microphone();
    microphone.start();
    {
        Cat cat = new Cat();
        cat.eat(food);
    }
    microphone.stop();

    List recordings = microphone.recordings();
    return recordings;
}

public void catsBurpWhenFedCatFood(String catFood) {
    List recordings = catThatHasBeenFed(catFood);

    BURPS.verify(recordings)
}