Convention? Use 'assertThat' string parameter to describe why we assert a condition

[DanVanAtta]

Instead of:

assertThat(collection, hasSize(2));

Prefer:

assertThat("3 units added, one unit filtered, we expect to have 2", collection, hasSize(2));

The net result between good test case method names, using @DisplayName and an assertion 'comment' string, we'll have very good self-documenting tests and we won't have any 'mystery assertions.' If then any tests fail, we'll be able to easily identify why and what the test was testing to begin with, a first step in fixing any test failure.

Compare that to this test from BigWorldTest:

  @Test
  void testCanalMovementNotStartingInCanalZone() {
    final Territory sz28 = territory("SZ 28 Eastern Mediterranean", gameData);
    final Territory sz27 = territory("SZ 27 Aegean Sea", gameData);
    final Territory sz29 = territory("SZ 29 Black Sea", gameData);
    final IDelegateBridge bridge = MockDelegateBridge.newInstance(gameData, british(gameData));
    advanceToStep(bridge, "CombatMove");
    final MoveDelegate moveDelegate = moveDelegate(gameData);
    moveDelegate.setDelegateBridgeAndPlayer(bridge);
    moveDelegate.start();
    final String error = moveDelegate.move(sz28.getUnits(), new Route(sz28, sz27, sz29));
    assertError(error);
  }

In the above, it's unclear:

• why do we expect an error?
• what exactly is being tested? Which game rule are we testing? What does it mean to test the canal rule when not starting in a canal?
• how does the setup goes on to produce that error?
• which error are we expecting?

If the above were to fail, we'd have to deep-dive into the code to guess what the original was testing, and more likely run the original test through a debugger on master to understand what the original was checking. The above is also brittle as we can have a case where a different error is emitted, the test would still pass. IIRC we've seen examples of tests passing where an error condition changed but still the test passed, and then for the wrong reasons.

[ron-murhammer]

I'm fine with this.

[asvitkine]

Is it always useful?

In my new test (after refactoring), I have the following:

    final List<MoveDescription> moves = batcher.batchMoves();

    // After batching, there should be 4 moves:
    //   Move transports 1 and 2 into position.
    //   Load tank and two infantry onto the transports.
    //   Move transporting all the units together.
    //   Move to unload the loaded units from the transports.
    assertEquals(4, moves.size());

    // Check move of transports 1 and 2 into position.
    assertEquals(new MoveDescription(List.of(transport1, transport2), sz19ToSz18), moves.get(0));

    // Check load of tank and two infantry onto the transports.
    assertEquals(
        new MoveDescription(
            List.of(tank1, inf1, inf2),
            brazilToSz18,
            Map.of(tank1, transport1, inf1, transport2, inf2, transport2)),
        moves.get(1));

    // Check move transporting all the units together.
    assertEquals(
        new MoveDescription(List.of(transport1, tank1, transport2, inf1, inf2), sz18ToSz12),
        moves.get(2));

    // Check move to unload the loaded units from the transports.
    assertEquals(new MoveDescription(List.of(tank1, inf1, inf2), sz12ToAlgeria), moves.get(3));

I'm not sure individual string messages would be helpful here - they'd just make the expectations less readable - and the overall reasoning seems better captured by a surrounding comment.

[DanVanAtta]

Part of the idea is that the comments would be used as the commentary string. eg:

   // Check load of tank and two infantry onto the transports.
   assertEquals(
        new MoveDescription(
            List.of(tank1, inf1, inf2),
            brazilToSz18,
            Map.of(tank1, transport1, inf1, transport2, inf2, transport2)),
        moves.get(1));

to:

assertThat(
  "check load of tank and two infantry onto the transports.",
   moves.get(1),
   is( new MoveDescription(
            List.of(tank1, inf1, inf2),
            brazilToSz18,
            Map.of(tank1, transport1, inf1, transport2, inf2, transport2)));

[DanVanAtta]

A benefit of the above, if the assertion fails, you'll see the comment in the failure output; sometimes that can help you from having to back-reference the code.

[DanVanAtta]

Non-hamcrest version:

   assertEquals(
        new MoveDescription(
            List.of(tank1, inf1, inf2),
            brazilToSz18,
            Map.of(tank1, transport1, inf1, transport2, inf2, transport2)),
        moves.get(1),
 "Check load of tank and two infantry onto the transports.");

[RoiEXLab]

I'd argue that there are some cases where assertions are obvious in some simpler test cases, but as a general convention this seems reasonable to me

[DanVanAtta]

Agree, convention should be "favor ... for non-trivial assertions"; and when in doubt, add a comment. Emphasis on "favor".

[asvitkine]

I took a stab at converting MoveBatcherTest in https://github.com/triplea-game/triplea/pull/5503#pullrequestreview-321572098

I found that refactoring the expected MoveDescription() to a variable was the cleanest when using hamcrest since as you said it was already a lot of parens, and is() was adding yet another set.

I also found that is() and equalTo() and is(equalTo()) all mean the same thing when testing for equality. We should codify the convention there (i.e. to is()).

The names seemed unintuitive to me, I would have expected is(foo) to be "== foo" and not ".equals(foo)", but alas.

[DanVanAtta]

is vs equalTo I'm slightly hesitant to create a convention though, they are both valid, some people feel a bit strongly for one vs another. IMO as long as we are using hamcrest and writing tests, it's plenty good.

FWIW, == is achieved through assertThat(foo, IsSameInstance.sameInstance(bar))

[DanVanAtta]

Revisiting this, we may need to clarify when the assert comment string is useful. I think it's probably useful when:

• If there are many assertions
• assertions are not completely trivially obvious (we need to keep in mind context, when writing tests it can be very obvious, when reviewing tests with no context, it can be extremely non-obvious)
• updating legacy tests

I think it's not useful when:

• test is well focused, a @DisplayName spells out why we expect a certain condition, and the condition reads well enough to seem straight forward. IE: the test is already extremely well documented and focused.

[stale[bot]]

The TripleA team regrets this issue was not solved sooner. To keep focus on latest issues and the most pressing tasks, this issue has been automatically marked as stale because it has not had recent activity. If the issue may be closed, please do so. If there are remaining items, we encourage for those items to be resubmitted as new, independent, concise tasks. The strategy is divide and conquer. Thank you for your contributions.

[DanVanAtta]

We need to document this in conventions as team has agreed to it..

[DanVanAtta]

Now documented: https://github.com/triplea-game/triplea/wiki/Java-Test-Code-Conventions#use-assertthat-string-parameter-to-describe-why-we-assert-a-condition-5527

Note, I clarified a bit that we expect the assertion comment to present most of the time, even when it seems pretty obvious (without context such obvious items become far less obvious and require reverse engineering rather than being directly told to you via this form of documentation).

"Unless completely and absolutely trivial, favor the majority of the time to use the string parameter of 'assertThat' methods to describe why we expect an asserted condition to be true."