JUnit extending abstract test class

Question

I have been thinking quite some time and asked an StackOverflow question about extending abstract test classes, but I haven't been able to do it until now.

I'll list my approach and then discuss some pros and cons and am asking you for a review of the code.

Consider the PlayerAction interface:

public interface PlayerAction {
 boolean isActionAllowed(final Player player);
 void performAction(final Player player) throws PlayerActionNotAllowedException;
}

In the invisible documentation I have written that in both methods that player != null must hold.

The abstract test class that should be extended, is the following:

@Ignore
public abstract class PlayerActionAbstractTest {
 static {
 assertTrue(true);
 }
 private final Supplier<PlayerAction> playerActionSupplier;
 private PlayerAction playerAction;
 public PlayerActionAbstractTest() {
 this.playerActionSupplier = null;
 }
 protected PlayerActionAbstractTest(final Supplier<PlayerAction> playerActionSupplier) {
 this.playerActionSupplier = Objects.requireNonNull(playerActionSupplier, "playerActionSupplier");
 }
 @Before
 final public void beforePlayerActionAbstractTest() {
 playerAction = playerActionSupplier.get();
 }
 @Test(expected = NullPointerException.class)
 public void testIsActionAllowedNullPlayer() {
 playerAction.isActionAllowed(null);
 }
 @Test(expected = NullPointerException.class)
 public void testPerformActionNullPlayer() {
 playerAction.performAction(null);
 }
}

An concrete implementation, I'll spare you (削除) all (削除ここまで) most irrelevant code:

public class AttackMonsterActionTest extends PlayerActionAbstractTest {
 private Player self;
 private Player opponent;
 private final static PlayerConfiguration PLAYER_CONFIGURATION = new PlayerConfigurationBuilder()
 .hitpoints(100)
 .turnAction(p -> { })
 .handCapacity(5)
 .fieldMonsterCapacity(5)
 .deckCards(Arrays.asList(new MonsterCard("Random", 5, 5, MonsterModus.HEALING)))
 .build();
 public AttackMonsterActionTest() {
 super(() -> new AttackMonsterAction(0, 0, Player.createFromConfiguration(PLAYER_CONFIGURATION, "Opponent")));
 }
 @Before
 public void before() {
 self = Player.createFromConfiguration(PLAYER_CONFIGURATION, "Self");
 opponent = Player.createFromConfiguration(PLAYER_CONFIGURATION, "Opponent");
 }
 @Test
 public void testConstructor() {
 new AttackMonsterAction(0, 0, opponent);
 }
 @Test(expected = IllegalArgumentException.class)
 public void testConstructorNegativeMonsterIndex() {
 new AttackMonsterAction(-1, 0, opponent);
 }
 @Test(expected = IllegalArgumentException.class)
 public void testConstructorNegativeTargetMonsterIndex() {
 new AttackMonsterAction(0, -1, opponent);
 }
 @Test(expected = IllegalArgumentException.class)
 public void testConstructorTooHighTargetMonsterIndex() {
 new AttackMonsterAction(0, 5, opponent);
 }
 @Test(expected = NullPointerException.class)
 public void testConstructorNullPlayer() {
 new AttackMonsterAction(0, 0, null);
 }
 //more custom tests
}

And it now executes the tests from PlayerActionAbstractTest on my AttackMonsterActionTest class!

The pros are that the instance only needs to be passed in once and most importantly that you must pass it in or you will get a compiler error. Another pro is that the list of classes to be tested is not defined in the abstract testclass itself.

The cons possibly are that issues can arise with dependencies and mock objects, but I'm not too sure about that and would love to have that included in a review.

Does this seem to be a reasonable way to do it?

Small update, changed signature in PlayerActionAbstractTest from public void superBefore() to final public void superBefore() to ensure that a compiler errors, rather than warning, will be thrown on an accidental override.

Second small update, changed signature in PlayerActionAbstractTest from final public void superBefore() to private void before(), now it cannot be overridden anymore.

Last update, appereantly the latest change was showing weird behaviours when executing unit tests on my whole project, rather than a single file. I have changed the following:

1. Changed signature in PlayerActionAbstractTest from private void before() to final public void beforePlayerActionAbstractTest(), as the @Before method needs to be public and in case inheriting multiple times is possible, beforeSuper() would clash.
2. Changed the public PlayerActionAbstractTest(final Supplier<PlayerAction> playerActionSupplier) constructor to protected PlayerActionAbstractTest(final Supplier<PlayerAction> playerActionSupplier), to fix that a test class must only have one public zero-args constructor.
3. Added public PlayerActionAbstractTest() to make it have one public zero-args constructor.
4. Added @Ignore to ensure that this abstract class itself isn't being tested.

Answer 1

You are not doing your tests 'appropriately', and this is part of your issue.

There is no need for the 'supplier'. The supplier is an instance field, and it's an extra level of abstraction that is unnecessary. JUnit has concepts at play that allows it to run tests in parallel. Having instance fields that are not part of the before/after system can lead to problems.

Instead of the supplier, you should have an abstract method, and your @Before method should be changed:

protected abstract PlayerAction supplyPlayerAction();
@Before
final public void beforePlayerActionAbstractTest() {
 playerAction = supplyPlayerAction();
}

Then, in your concrete class, instead of trying to beat the circular construction logic, all you need to do is to implement the concrete method supplyPlayerAction() like:

protected PlayerAction supplyPlayerAction() {
 return new AttackMonsterAction(0, 0, Player.createFromConfiguration(PLAYER_CONFIGURATION, "Opponent"))
}

This way the @Before things happen in the predictable order, etc.

In your pro/con terminology, you cannot implement a concrete class of the abstract class, without implementing the abstract method. The method is called in the @Begin in the Super Class, and the Super Class @Begins are called before the concrete class.

This is the way it should be done.