TDD Midlife Crisis: White-Box Testing & Refactoring Tests

December 16, 2015 David Julia

sfeatured-pivotallabs-slate

I’ve been undergoing a “Test-Driven Development (TDD) Midlife Crisis”, in which I’ve been critically examining how I test drive code. During this process, I’ve been thinking about the kinds of tests I write, how much I mock, when to mock, and other fundamental questions of test-driven development. Over my Thanksgiving vacation, I re-read the seminal book on TDD, Test-Driven Development By Example by Kent Beck. It’s a quick read, which brought to mind something that I’d been saying for a while without fully understanding the implications: TDD is more about confidence in your code and designs than it is about proving correctness. Proving correctness for everything but mathematical models of algorithms is generally a fool’s errand. I have said this for so long that I must have forgotten that I’d stolen this nugget of wisdom from Kent’s writings. Time to go back to the basics and have a taste of my own, or rather, Kent’s medicine.

In one of the last sections of the book, Mastering TDD, Kent poses a problem:

Given 3 integers representing the lengths of the sides of a triangle, return 1 if the triangle is equilateral, 2 if isosceles, and 3 if scalene. Throw an exception if the triangle is not well-formed. He challenges the reader to try the problem, and see how many tests they end up writing. Kent offers his own solution in Smalltalk using 6 tests. What follows is a narrative of how I implemented my solution, without consulting his first. I did mine in Java, since I’ve been doing a lot of Java programming recently. My full solution is posted on GitHub, where you can follow my somewhat winding path to a correct and, in my opinion, elegant solution.

First I started with a few simple tests, one for each kind of triangle, and of course a few tests for invalid triangles: if a side is negative, that’s invalid; if a side is zero, that’s also invalid. Easy as pie:

@Test
public void triangleType_returns1_whentriangleIsEquilateral() {
    TriangleClassifier classifier = new TriangleClassifier();
    int id = classifier.identifyTriangle(1, 1, 1);
    assertThat("equalateral triangle should return id of 1", id, equalTo(1));
}

@Test
public void triangleType_returns2_whentriangleIsIsosceles() {
    TriangleClassifier classifier = new TriangleClassifier();
    int id = classifier.identifyTriangle(1, 1, 2);
    assertThat("isosceles triangle should return id of 2", id, equalTo(2));
    id = classifier.identifyTriangle(1, 2, 1);
    assertThat("isosceles triangle should return id of 2", id, equalTo(2));
    id = classifier.identifyTriangle(2, 1, 1);
    assertThat("isosceles triangle should return id of 2", id, equalTo(2));
}

@Test
public void triangleType_returns3_whentriangleIsScalene() {
    TriangleClassifier classifier = new TriangleClassifier();
    int id = classifier.identifyTriangle(1, 2, 3);
    assertThat("scalene triangle should return id of 3", id, equalTo(3));
}

@Test
public void triangleType_throwsException_whenTriangleSideIsZeroHeight() {
    TriangleClassifier classifier = new TriangleClassifier();
    try {
        classifier.identifyTriangle(0, 2, 3);
        fail();
    } catch (TriangleClassifier.TriangleMalformedException e) {
    }

    try {
        classifier.identifyTriangle(1, 0, 3);
        fail();
    } catch (TriangleClassifier.TriangleMalformedException e) {
    }

    try {
        classifier.identifyTriangle(1, 1, 0);
        fail();
    } catch (TriangleClassifier.TriangleMalformedException e) {
    }
}

@Test
public void triangleType_throwsException_whenTriangleSideIsNegative(){
    TriangleClassifier classifier = new TriangleClassifier();
    try {
        classifier.identifyTriangle(-1, 2, 3);
        fail();
    } catch (TriangleClassifier.TriangleMalformedException e) {
    }

    try {
        classifier.identifyTriangle(1, -1, 3);
        fail();
    } catch (TriangleClassifier.TriangleMalformedException e) {
    }

    try {
        classifier.identifyTriangle(1, 1, -1);
        fail();
    } catch (TriangleClassifier.TriangleMalformedException e) {
    }
} 

And the corresponding implementation:

public class TriangleClassifier {

    public static final int EQUILATERAL_TRIANGLE_ID = 1;
    public static final int ISOSCELES_TRIANGLE_ID = 2;
    public static final int SCALENE_TRIANGLE_ID = 3;

    public int identifyTriangle(int a, int b, int c) {
        if (a <= 0 || b <= 0 || c <= 0) {
            throw new TriangleMalformedException();
        }

        if (a == b && b == c) {
            return EQUILATERAL_TRIANGLE_ID;
        } else if (a == b || a == c || b == c) {
            return ISOSCELES_TRIANGLE_ID;
        }
        return SCALENE_TRIANGLE_ID;
    }

    public class TriangleMalformedException extends RuntimeException {
    }
}

Okay, this seems to work, but there’s got to be a way to refactor this. Plus the tests for malformed triangles are a little cluttered, with all those assertions. Should I pull out a test helper method? Should I break them out into separate tests? Nahh I get the feeling that in this case, that’d probably just be skirting the real issue, I bet this is a sign that I’m missing out on a refactoring. Should I pull out a private helper class or something? How can I clean this up? Hmm…

*Scratches Head*

AHA! If I just add all 3 sides of the triangle to a Set, then the size of the set will be the number I want. That way I can get rid of the explicit checking of each side to determine the type of triangle. Kent, you clever devil, you wanted me to come up with that, didn’t you!? Well, I’ll do you one better, I’ll use a SortedSet so that I can also just make sure that the smallest side isn’t less than or equal to zero, which will simplify the error checking:

public int identifyTriangle(int a, int b, int c) {
    SortedSet<Integer> sides = sortedSides(a, b, c);

    if (sides.first() <= 0 ) {
        throw new TriangleMalformedException();
    }

    return sides.size();
}

private SortedSet<Integer> sortedSides(int a, int b, int c) {
    SortedSet<Integer> sides = new TreeSet<>();
    sides.add(a);
    sides.add(b);
    sides.add(c);
    return sides;
}

That’s pretty good. Now let’s see if we can clean up the tests. Because we are using a sorted set, I don’t feel the need to check that each individual side is greater than zero in length, so the nonzero and non-negative tests get to have one assertion each:

@Test
public void triangleType_throwsException_whenTriangleSideIsZeroHeight() {
    TriangleClassifier classifier = new TriangleClassifier();
    try {
        classifier.identifyTriangle(0, 2, 3);
        fail();
    } catch (TriangleClassifier.TriangleMalformedException e) {
    }
}

@Test
public void triangleType_throwsException_whenTriangleSideIsNegative(){
    TriangleClassifier classifier = new TriangleClassifier();
    try {
        classifier.identifyTriangle(-1, 2, 3);
        fail();
    } catch (TriangleClassifier.TriangleMalformedException e) {
    }
}

Wait a second, by that same reasoning, I only really need to test one example of a triangle for each type. Since my implementation always sorts sides, I don’t need three examples for an isosceles triangle. This makes me feel a little uncomfortable, because that’s a little more “white box” of an attitude than I tend to take with my testing day-to-day. But let’s go for it, and I’ll see how it affects my confidence in the code. If I feel confidence decreasing, I’ll add back tests, but if I can get by with fewer assertions, then as long as my tests still communicate well, then it’s a net win. Let’s see how those tests look.

We’re going to change them from this:

@Test
public void triangleType_returns2_whentriangleIsIsosceles() {
    TriangleClassifier classifier = new TriangleClassifier();
    int id = classifier.identifyTriangle(1, 1, 2);
    assertThat("isosceles triangle should return id of 2", id, equalTo(2));
    id = classifier.identifyTriangle(1, 2, 1);
    assertThat("isosceles triangle should return id of 2", id, equalTo(2));
    id = classifier.identifyTriangle(2, 1, 1);
    assertThat("isosceles triangle should return id of 2", id, equalTo(2));
}

To this:

@Test
public void triangleType_returns2_whentriangleIsIsosceles() {
    TriangleClassifier classifier = new TriangleClassifier();
    int id = classifier.identifyTriangle(1, 1, 2);
    assertThat("isosceles triangle should return id of 2", id, equalTo(2));
}

I actually feel pretty good about that. I’d have to use an inferior implementation in order to break this, and the problem domain is pretty well-defined, so I’m happy to get rid of the additional assertions. What do you think? Would you keep the three examples of isosceles triangles, or would you axe the two “extra” examples? Two assertions aren’t expensive in terms of test run time, so that’s a bunk argument for axing them. However, if I were pairing and my partner wanted to keep the tests, I probably wouldn’t protest too much if they thought the additional examples clarified things. I personally find the pared down version easier to look at. If we adopt the attitude that Kent preaches, then I can say my confidence is pretty high even without the additional assertions, so we can proceed without those tests.

Okay let’s see how close I am to Kent’s version. I only have five tests, so I suspect I’m missing something.

Kent has a test where he passes in strings as input, but in Java, that’d be a compilation error, so we’re good there. That’s one of the reasons I like statically typed languages: I can have higher confidence with fewer tests. But wait a second: Kent has a test that I’m missing, a case he calls testIrrational. He’s testing a triangle with lengths (1, 2, 3). OH NO! My High School geometry teacher would be so disappointed! I’m sorry Mrs. Marberry! I forgot about the triangle inequality theorem. That Kent guy doesn’t miss a thing. Alright, let’s add in that test case:

@Test
public void triangleType_throwsException_whenTriangleViolatesTriangleInequalityTheorem(){
    TriangleClassifier classifier = new TriangleClassifier();
    try {
        classifier.identifyTriangle(1, 2, 3);
        fail();
        } catch (TriangleClassifier.TriangleMalformedException e) {
    }
}

And the corresponding change in implementation:

public int identifyTriangle(int a, int b, int c) {
    List<Integer> sides = sortedSides(a, b, c);

    if (sides.get(0) + sides.get(1) <= sides.get(2)) {
        throw new TriangleMalformedException();
    }

    return new HashSet(sides).size();
}

All green. Phew. Now we can delete that test for when a side is zero. That’d be an impossible triangle, so that’s covered more generally by the test for violations of the Triangle Inequality Theorem. Actually, by that same logic, we can also delete the negative side test—that’s totally covered as well! While we’re at it, let’s rename those test methods that somehow got out of sync with the real method name. Now we’re down to four well-named tests:

@Test
public void identifyTriangle_returns1_whentriangleIsEquilateral() {
    TriangleClassifier classifier = new TriangleClassifier();
    int id = classifier.identifyTriangle(1, 1, 1);
    assertThat("equilateral triangle should return id of 1", id, equalTo(1));
}

@Test
public void identifyTriangle_returns2_whentriangleIsIsosceles() {
    TriangleClassifier classifier = new TriangleClassifier();
    int id = classifier.identifyTriangle(1, 2, 2);
    assertThat("isosceles triangle should return id of 2", id, equalTo(2));
}

@Test
public void identifyTriangle_returns3_whentriangleIsScalene() {
    TriangleClassifier classifier = new TriangleClassifier();
    int id = classifier.identifyTriangle(5, 4, 3);
    assertThat("scalene triangle should return id of 3", id, equalTo(3));
}

@Test
public void identifyTriangle_throwsException_whenTriangleViolatesTriangleInequalityTheorem(){
    TriangleClassifier classifier = new TriangleClassifier();
    try {
        classifier.identifyTriangle(3, 2, 1);
        fail();
    } catch (TriangleClassifier.TriangleMalformedException e) {}
}

I reordered the numbers around in the inequality theorem test for impossible triangles to cover the fact that my list of sides must be sorted. That logic was previously covered by the test for negative numbers and zero. Surprisingly, when I glance over at Kent’s implementation, he leaves the negative number test. And here I thought he was more extreme than me about deleting redundant tests. I hope I didn’t miss an edge case, but after running through the logic in my head a few times, I’m thoroughly convinced that I didn’t. Now my confidence is just as high as it was with the two tests there, but having just the one test seems more to the point: a triangle is malformed if it violates the Triangle Inequality Theorem. The other tests just danced around that fact.

Let’s do just a little more refactoring to clean things up, since the code is a little confusing as it stands :

public int identifyTriangle(int a, int b, int c) {
    List<Integer> sides = sortedSides(a, b, c);

    if (sides.get(0) + sides.get(1) <= sides.get(2)) {
        throw new TriangleMalformedException();
    }

    return new HashSet(sides).size();
}

I guess that conditional tests whether a triangle is malformed, but what does the hashset(sides).size() do? I can’t read that method at a glance and know what each part of the method is supposed to do. Let’s take a crack at it:

public int identifyTriangle(int a, int b, int c) {
    List<Integer> sides = sidesList(a, b, c);
    if (triangleIsImpossible(sides)) {
        throw new TriangleMalformedException();
    }

    return triangleClassificationId(sides);
}

private int triangleClassificationId(List sides) {
    return new HashSet(sides).size();
}

private boolean triangleIsImpossible(List sides) {
    Collections.sort(sides);
    return sides.get(0) + sides.get(1) <= sides.get(2);
}

That reads a lot better to me! If the triangle is impossible, throw an exception. I went back and forth on whether or not to call this “violatesTriangleInequalityTheorem” vs triangleIsImpossible, but I like the way triangleIsImpossible reads better. We’ve also pushed the sorting into the triangleIsImpossible check, the only place where it’s needed. Then we return the triangleClassificationId, but if I had to do it over again I’d probably just call it numberOfUniqueSides. That’s really what Kent was asking us for.

If I buy into the idea that TDD is not about proving correctness, but is about improving confidence, then adopting more of a white box style of testing makes sense. If you’re adopting a white box style of testing, then you can delete tests that are rendered unnecessary by your implementation, provided that deleting them doesn’t affect your confidence in the code, and either clarifies your intent or has no impact on the ability to convey intent. Overall, this style of testing is a little different than what I tend to do day-to-day. Kent doesn’t do strict isolation testing, mocking out all collaborators, so perhaps it makes even more sense to aggressively refactor tests.

Where a hardcore mockist might not see quite as much value in deleting unneeded tests, when writing “sociable unit tests”, getting rid of unnecessary tests helps you hone in on errors and is even more of a boon. I enjoy this style of aggressively refactoring tests to remove redundancy and improve clarity, and I think I’ll do a bit more of it as I play around more with sociable unit tests, as opposed to the more heavily isolated, mockist tests that I have been writing for a long time. In this particular example, there’s really nothing I’d ever want to mock, but it’s an interesting thought to keep in mind. That said, I’ll leave my ramblings and accounts of my exploration of when to mock and when not to mock, for another post detailing my “TDD midlife crisis”.

Let me know what you think in the comments. What tests or assertions would you have kept? What do you think about this approach of aggressively removing redundancy in tests?

Learn more:

Edited on 12/29/2015 to switch the names for mislabelled Constants ISOSCELES_TRIANGLE_ID and EQUILATERAL_TRIANGLE_ID. They were flip-flopped. The implementation was correct but the names were incorrect.

About the Author

Biography

More Content by David Julia
Previous
The Cloud Foundry Foundation Certification Program
The Cloud Foundry Foundation Certification Program

Yesterday, one of Pivotal’s long term visions became a reality. The Cloud Foundry Foundation released the C...

Next
Build Newsletter: The Present & Future In Our World—Dec 2015
Build Newsletter: The Present & Future In Our World—Dec 2015

As we come to the end of the 2015, we reach the one year milestone for Build Newsletter. In this edition, w...

×

Subscribe to our Newsletter

Thank you!
Error - something went wrong!