7

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.

9
  • 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. Commented Mar 9, 2018 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
    Commented Mar 9, 2018 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. Commented Mar 9, 2018 at 15:49
  • 3
    I use a similar comment style with 'arrange, act, assert'.
    – Carra
    Commented Mar 9, 2018 at 15:51
  • 6
    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
    Commented Mar 9, 2018 at 20:41

6 Answers 6

8

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.

15

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.

4
  • 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
    Commented Mar 9, 2018 at 13:47
  • 3
    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
    Commented Mar 9, 2018 at 13:53
  • 1
    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
    Commented Mar 9, 2018 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
    Commented Mar 9, 2018 at 20:44
2

I’ve seen tests with 3 lines of code accompanied by 3 lines of given/when/then comments.

I see it as a lame attempt to improve test codebase when it becomes hard to read/maintain. Short/simple tests do not need it. Long/complex tests need refactoring and meaningful comments instead of throwing in 3 extra lines of placeholders and calling it done. Try removing all of those placeholders and see which tests are not so much readable; those are the ones that truly need your attention.

In another perspective, that practice conditions the developers to an artificial aid that is not available in most places. It may look nice and pleasing to the eyes; however, do you really need it everywhere to be able to comprehend a test? If no, it’s merely a decoration that is not essential to thought process. To bring this a bit further, my theory is that it is even a kind of distraction that gives you a false sense of understanding (the 3-part structure) when you read a test, and that actually hampers your code conprehension.

And obviously, it’s such a chore/time waster to force yourself and fellow developers to add those extra 3 lines where code is perfectly readable.

In general, it’s not so much useful or harmful. It can be a personal preference/technique.

1

Code changes, comments ain't.

Instead of commenting the code rather use commit messages that describe the state of the code while it evolves. It's a known detail, messages sent through comments could also be sent by method names, so comments shouldn't be included in the code. There might be some acceptable cases when a tight schedule prevails and commenting the code makes it to keep up with the schedule, with the side note comments should be short-term solution to be improved / replaced / removed.

Sure all these should be included in the coding style guide the team uses that is a team choice.

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.

8
  • 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
    Commented Mar 9, 2018 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
    Commented Mar 9, 2018 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
    Commented Mar 9, 2018 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
    Commented Mar 9, 2018 at 19:37
  • 1
    @DavidArno This is a semantical argument. You're arguing that anything that can cause a test failure is therefore under test. I'm arguing that "under test" focuses on what the test is designed to assert. Litmus test: what's your stance on pure methods? It is well documented that they can be used in a test body as long as they themselves are backed by tests. It would IMHO be counterproductive to therefore assume that any additional test that makes use of them but does not assert their workings has that pure method "under test". It dilutes the importance of knowing a test's focus.
    – Flater
    Commented Aug 24, 2023 at 22:54
0

I want to come in with a different answer here, in favor of adding these comments.

TL;DR Use these comments when they make sense. I'm all for it.

But first, I want to be very clear here that this answer is only scoped to the specific given/when/then (or act/arrange/assert, same difference) comments. I agree with the people who are against them when we're talking about code comments in general (e.g. used to fix badly structured code, code should be evident to read, ...), but I think there is a special case for the GWT comments that is not being accounted for in their answers.

GWT (or AAA) is a known concept because it so accurately describes the vast majority of test implementations. You set up some scenario (given/arrange), you let it play out (when/act), you confirm that it behaved the way it needed to in the given scenario (then/assert).

These three sections of the testing process are discrete and should be done one after the other. It would be illogical to e.g. change something about the scenario after you've already acted on it (because then you've just invalidated the first act) or test something before you've acted on the scenario (because what is there to test if not the act itself?).

These three steps are also discrete parts of the developer's mindset when working on a test. Whenever you want to make any change you a test, or you're writing it, you need to be innately aware of whether you're arranging, acting or asserting, because you need to always respect the previous paragraph (i.e. never doing anything out of order).

Having established these points, there is still one other solution: use discrete submethods. Something along the lines of:

public void MyTest()
{
    Arrange();
    Act();
    Assert();
}

This follows the usual guideline on code comments that it's better to structure your code readably than it is to add comments to aid difficult to read code. I actually agree with this advice for real code, but not for unit tests. Tests can get very hairy very quickly when any of its steps require more than a handful of lines, and the submethods introduce several negatives for a testing context:

  • The test is no longer readable from top to bottom, you need to go and jump to different submethods to understand it. Sure, you could maybe put your submethods in the same order, but then you've still introduced additional fluff (method signature and braces) inbetween the relevant bits - at best this approach would be equally as bad as adding a // when comment.
  • Scopes/closures are making it hard for variables/data to flow from one part of the test to the next. Not that it can't be done, but in order to make it work now, it requires additional complexity. Because a test is a very restricted scope, it's reasonable for it to define all of its content in a "global" scope (i.e. scoped to the whole test), but the submethods make this harder to achieve.

So it detracts from readability (first bullet point), and it adds complexity (second bullet point). That's not great.

This is why I consider given/when/then (or act/arrange/assert) comments valid additions to a test body. To summarize the benefits:

  • It can help a developer quickly navigate to the correct section of a test.
  • It adds an easy visual gauge to spot if you've put some logic in the wrong section
  • It does not meaningfully decrease readability (compared to other possible solutions to the first two bullet points)
  • It does not introduce logical complexity (compared to other possible solutions to the first two bullet point)

So yeah, use these comments when they make sense. I'm all for it.


Just to point out some edge cases that people might point out in the comments:

  • Integration tests are a different breed of test where GWT/AAA is not as easy to maintain, as integration tests often entail a more complex orchestration, e.g. GWGWGWT or GWTGWTGWT
  • I'm not saying that a dev would be unable to navigate the code without these comments. I'm saying that it adds more value than it adds problems, so it's a net positive.
  • If there is a specific case where these comments don't add a net positive, then don't use them there. That's not an argument against not using these comments in general, though.

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