3

I've joined a new team of great colleagues and a difference of opinion has come up concerning including //given //when and //then comment blocks in unit tests.

For example

public void mapToitemList_MapsCarsToitemsCorrectly_ForSingleFurrari() {
    // given
    String commercialProductName = "Furrari";
    String productNumber = "12423";
    BigDecimal numberOfScrews = BigDecimal.TEN;
    ValveType valveType = ValveType.GOOD;

    Car car = new Car() //
                    .withCommercialProductName(commercialProductName) //
                    .withProductCarNumber(productNumber) //
                    .withNumberOfScrewsAmount(new NumberOfScrewsAmount().withValue(numberOfScrews)) //
                    .withBalances(new Engine().withValves(new ValveType().withValue(valveType)));
    List<Car> carList = Collections.singletonList(car);

    // when
    List<Item> mappedListOfCars = CarMapper.mapToItemList(carList);

    // then 
    assertThat(mappedListOfCars).isNotEmpty();
    Item carAsItem = mappedListOfCars.get(0);

    assertThat(carAsItem.getCommercialProductName()).isEqualTo(commercialProductName);
    assertThat(carAsItem.getNumberOfSetsOfScrews().getValue()).isEqualTo(Convertor.numbersOfScrewsToNumberOfSets(numberOfScrews).getValue());

}

Currently my main reason to include them is because I'm used to it and the main counter-argument I'm getting is that comments should be shunned.

Does the added structure of these comments merit the use of comments? Martin Fowler mentions some people use them, but passes no judgment on it.

  • Using clearly structured code plus comments vs. clearly structured code only is a dichotomy similar to using correctly indented code with delimiters (C/Java syntax) vs. correctly indented code only (Python/Haskell Syntax). Leaving out the redundant elements seems like an obvious idea, but getting people to do it often provokes a furious hostility totally inappropriate to the question. I doubt we'll achieve a consensus about this question. – Kilian Foth Mar 9 '18 at 14:58
  • I've changed the code example to be more real-life. I used an actual test we have and changed all object types. Sorry for adding this in late (1h after posting the question) - I feel like it doesn't undermine any answers however. – Spork Mar 9 '18 at 15:16
  • "Comments should be shunned" is a poor argument. Proper use of comments is more nuanced than that. That said, you really shouldn't need these comments. – Robert Harvey Mar 9 '18 at 15:49
  • 1
    I use a similar comment style with 'arrange, act, assert'. – Carra Mar 9 '18 at 15:51
  • 3
    For this particular test, with more than 10 lines of setup, I found the "when" comment incredibly useful, as it indicates here is the one thing we are actually testing!. That said, perhaps all that setup should be in a separate little function. – user949300 Mar 9 '18 at 20:41
6

Code comments are usually taken as a bad practice because:

  • They add new artifacts to maintain in the future.
  • The code should be as simple and well written to be self-explanatory.

That said, as usual, there are no absolute truths. Personally, this kind of comments is positive to me as they add additional semantics to the code and let me know which section of the test I'm working on. Besides, it also enforces following a certain test coding structure.

So, to sum up:

  • Code comments that add no semantic to the code like // Iterate over the list elements before a foreach loop: Bad.
  • Code comments that add good semantic and enforces good practices: Good.

But I want to remark this is rather a personal/organizational preference than an absolute truth.

10

Clearly structuring your tests in a given–when–then or arrange–act–assert schema is a good idea – but this structure should be evident in your code, even without comments. Often, a blank line between each section is entirely sufficient to communicate this division. Extra comments would then just be noise.

Things become more complicated when you also assert preconditions, when the action contains multiple distinct steps, or when describing the expected state requires extra preparation. Many good test cases do not fit into a five line test method. In these more complicated scenarios, it may be sensible to clarify the intent of each section with a comment.

  • What would sway you to 'clarify the intent of each section'? You mention the unit test length in lines, perhaps also seperation of concerns? The unit tests I write and encounter usually are >5 lines - so I'm very interested in your threshold. – Spork Mar 9 '18 at 13:47
  • 1
    Many testing examples are very short and obvious. Unfortunately, a lot of guidelines live in this fantasy. But real tests are often more messy. So when the role of each section is no longer obvious for whatever reason, then clarifying through comments can be sensible. There cannot be an objective guideline under which threshold comments are unnecessary. It boils down to this truism: if the code is already obvious, you don't need comments. – amon Mar 9 '18 at 13:53
  • At the risk of sounding like a stuck record, if the real test is messy, then refactor it into a set of separate functions. In my experience there is no justification for having a test method of more than a dozen lines or so and thus no reason to put those ugly comments in the method. – David Arno Mar 9 '18 at 14:58
  • @David Arno not disagreeing per se, but I've seen a lot of posts basically saying "don't sweat unit test organization, a little sloppiness is o.k." – user949300 Mar 9 '18 at 20:44
0

The main problem I have with this sort of convention is that it's so proscriptive. It effectively mandates that a test must follow the arrange-act-assert pattern, regardless of the test. And, as a result, you can end up with code such as in your example:

// when
List<Item> mappedListOfCars = CarMapper.mapToItemList(carList);

// then 
assertThat(mappedListOfCars).isNotEmpty();
Item carAsItem = mappedListOfCars.get(0);

Why is List<Item> mappedListOfCars = ... in "when" rather than "given"? Why are you doing Item carAsItem = ... in "then"? The simple answer is because you have to shoehorn the test into those three steps.

So get rid of those comments. They serve no useful purpose and the test is easier to read without them.

  • 1
    The mapToItemList is the method under test, which is why it's in when. It seems like this answer is only about Spec by example / given when then itself, not the commenting of the sections. This is not a discussion point in my team - we all feel firmly that the structure adds a lot. – Spork Mar 9 '18 at 19:17
  • @Spork that’s the problem. There are four methods of Car also under test in your example. But because you have tucked them away in the “given” section, you have ignored them. – David Arno Mar 9 '18 at 19:24
  • They are not under test - there is no assertion on their effects. If you are arguing that this could be improved using mocks or builders I would agree, but that, like the discussion about the principle itself, is not answering my question. – Spork Mar 9 '18 at 19:26
  • @Spork, You assign commercialProductName to Car via the withCommercialProductName method. That then flows through the mapToItemList method before you eventually assert assertThat(carAsItem.getCommercialProductName(.... Both methods are therefore under test. And no, builders and mocks would not help as then you'd be needless testing a mock rather than those real methods. Do not fall into the "unit test == test one method" trap. It leads to poor quality tests. – David Arno Mar 9 '18 at 19:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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