Add some AllianceTracker unit tests #13835

JMoravec · 2025-11-06T00:51:22Z

Just some additional explicit tests done on the AllianceTracker class. Only functional thing that's added is a null check on the stream, as a "null" alliance here will cause a NullPointerException.

Let me know if that's actually expected on that method. I can remove/add a comment there.

frigoref

@JMoravec Thanks for the PR so far. There are some points that need to be addressed.
Please let me know if there is anything that needs clarification.

frigoref · 2025-11-06T07:50:08Z

game-app/game-core/src/main/java/games/strategy/engine/data/AllianceTracker.java

  public Set<GamePlayer> getPlayersInAlliance(final String allianceName) {
    return alliances.entries().stream()
-        .filter(e -> e.getValue().equals(allianceName))
+        .filter(e -> e.getValue() != null && e.getValue().equals(allianceName))


We should ensure before creating an entry to avoid null values not at every place where we would like to access such a value.

It's been awhile since I've done Java, is the standard to have a @nonnull annotation (the lombok one maybe?) and raise an exception on instantiation if the input is null?

Another good point for our documentation. Generally we avoid those annotations unless it is violating the general design assumption from above (null only in local scope).
Want to submit a PR on the documentation update as well? 😇

It's been awhile since I've done Java, is the standard to have a @nonnull annotation (the lombok one maybe?) and raise an exception on instantiation if the input is null?

@nonnull indicates a never null value. IE: something you don't need to check for nullity, because it simply just wont' be null.

and raise an exception on instantiation if the input is null?

Depends where the input is coming from. Ideally you would just guarantee the input to never be null and you can skip the null check.

The modern "rules-of-thumb" for java nullity I would summarize as:

never return null

never pass null

never let null "escape" the local scope of a method.

TripleA was written well before those good rules-of-thumb were a thing.

Concretely:

if a null value injected here will go on to cause a NullPointerException later, then we do want a null check here that throws. That is the "fail early principle", detect error conditions as early as you can and fail when those conditions are detected.

otherwise, the question is, have we dealt with all of the callers? Why can the passed value be null? Those callers are violating "never pass null", so they should each in turn be fixed. Short of that, we have to just deal with null values until we can properly remove the usage of nulls across method boundaries.

frigoref · 2025-11-06T08:06:01Z

game-app/game-core/src/test/java/games/strategy/engine/data/AllianceTrackerTest.java

+    final GamePlayer chretian = gameData.getPlayerList().getPlayerId("chretian");
    final AllianceTracker allianceTracker = gameData.getAllianceTracker();
    final RelationshipTracker relationshipTracker = gameData.getRelationshipTracker();
+    // Validate that the expected setup for the alliance is what we expect from the data


The second half of the comment is redundant, the assert-methods clearly express a validation.
If you see a comment here as valuable, I suggest shortening it to "Validate the expected setup for the alliance"

frigoref · 2025-11-06T08:11:59Z

game-app/game-core/src/test/java/games/strategy/engine/data/AllianceTrackerTest.java

    }
+
+    @Test
+    void testAllianceTrackerState() {


Change to a specific name giving information about the exact test method / test case, e.g., testAddToAllianceWithTwoPlayerToSameAlliance.

I agree with @frigoref 's feedback here. The heuristic is, based on the knowledge that this test has failed, and the test name, would you know why it failed?

frigoref · 2025-11-06T08:12:54Z

game-app/game-core/src/test/java/games/strategy/engine/data/AllianceTrackerTest.java

+
+      // No alliances exist on creation
+      assertTrue(allianceTracker.getAlliances().isEmpty());
+      allianceTracker.addToAlliance(player1, allianceName);


I'd suggest to have a separate block for the method that is actually tested to visually separate it from the data preparation before and the validation/checks after.

frigoref · 2025-11-06T08:35:25Z

game-app/game-core/src/test/java/games/strategy/engine/data/AllianceTrackerTest.java

+      assertTrue(allianceTracker.getAlliances().isEmpty());
+      allianceTracker.addToAlliance(player1, allianceName);
+      allianceTracker.addToAlliance(player2, allianceName);
+      assertEquals(1, allianceTracker.getAlliances().size());


A local variable definition for allianceTracker.getAlliances() (e.g. alliances) could help structure the checks.
Later also allianceTracker.getAllies(player1) -> player1Allies and getPlayersInAlliance(allianceName) playersInAlliance.
This makes it easier to read the actual validation/check done via the assert-method and also structures the code into blocks improving again the readability.

frigoref · 2025-11-06T08:39:50Z

game-app/game-core/src/test/java/games/strategy/engine/data/AllianceTrackerTest.java

+    }
+
+    @Test
+    void testAllianceTrackerNullAlliance() {


Getting back to the above state design goal of not having any null values passed around (only in the scope of one method), I assume this test method is not needed.
Note, that we do not want to make each method bullet-proof as we are the ones using them. Hence, we can rely on general design decisions like this one.

Arguably this method could throw a an explicit NullPointerException due to the null input:

allianceTracker.addToAlliance(player1, null);

frigoref · 2025-11-06T08:42:06Z

game-app/game-core/src/test/java/games/strategy/engine/data/AllianceTrackerTest.java

 import static org.hamcrest.Matchers.is;
-import static org.junit.jupiter.api.Assertions.assertFalse;
-import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.*;


This is causing one of several failed checks for the PR

> Task :game-app:domain-data:check [ant:checkstyle] [WARN] /home/runner/work/triplea/triplea/game-app/game-core/src/test/java/games/strategy/engine/data/AllianceTrackerTest.java:5:47: Using the '.*' form of import should be avoided - org.junit.jupiter.api.Assertions.*. [AvoidStarImport]

Please make sure all checks are passed and if not provide additional commits to correct them.

Yeah, I didn't notice this change locally when pushing up and assumed when I ran spotlessCheck it would fix things like this. I probably need to change an InetlliJ setting so it doesn't auto import *'s

Right. And we should probably add this to the setup guide ad well.

Yeah, looks missing: https://github.com/triplea-game/triplea/blob/master/docs/development/how-to/ide-setup/intellij-setup.md

AFAIK the way to do fix the star import is to update a setting in intellij that is basically "how many imports before using a start", and update that value to '999'

DanVanAtta

Added a few comments, it's good to see the contributions.

@frigoref / @JMoravec , I recommend checking out this video:

Why Good Developers Write Bad Tests
https://www.youtube.com/watch?v=hM_ex4-xu4E

After the video, a number of fixes to the test cases become a lot more obvious IMO, strongly recommend the video to you both!

DanVanAtta · 2025-11-15T20:29:23Z

game-app/game-core/src/main/java/games/strategy/engine/data/AllianceTracker.java

  public Set<GamePlayer> getPlayersInAlliance(final String allianceName) {
    return alliances.entries().stream()
-        .filter(e -> e.getValue().equals(allianceName))
+        .filter(e -> e.getValue() != null && e.getValue().equals(allianceName))


It's been awhile since I've done Java, is the standard to have a @nonnull annotation (the lombok one maybe?) and raise an exception on instantiation if the input is null?

@nonnull indicates a never null value. IE: something you don't need to check for nullity, because it simply just wont' be null.

and raise an exception on instantiation if the input is null?

Depends where the input is coming from. Ideally you would just guarantee the input to never be null and you can skip the null check.

The modern "rules-of-thumb" for java nullity I would summarize as:

never return null

never pass null

never let null "escape" the local scope of a method.

TripleA was written well before those good rules-of-thumb were a thing.

Concretely:

if a null value injected here will go on to cause a NullPointerException later, then we do want a null check here that throws. That is the "fail early principle", detect error conditions as early as you can and fail when those conditions are detected.

otherwise, the question is, have we dealt with all of the callers? Why can the passed value be null? Those callers are violating "never pass null", so they should each in turn be fixed. Short of that, we have to just deal with null values until we can properly remove the usage of nulls across method boundaries.

DanVanAtta · 2025-11-15T20:33:02Z

game-app/game-core/src/test/java/games/strategy/engine/data/AllianceTrackerTest.java

 import static org.hamcrest.Matchers.is;
-import static org.junit.jupiter.api.Assertions.assertFalse;
-import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.*;


Yeah, looks missing: https://github.com/triplea-game/triplea/blob/master/docs/development/how-to/ide-setup/intellij-setup.md

AFAIK the way to do fix the star import is to update a setting in intellij that is basically "how many imports before using a start", and update that value to '999'

DanVanAtta · 2025-11-15T20:36:45Z

game-app/game-core/src/test/java/games/strategy/engine/data/AllianceTrackerTest.java

+    }
+
+    @Test
+    void testAllianceTrackerNullAlliance() {


Arguably this method could throw a an explicit NullPointerException due to the null input:

allianceTracker.addToAlliance(player1, null);

DanVanAtta · 2025-11-15T20:45:08Z

game-app/game-core/src/test/java/games/strategy/engine/data/AllianceTrackerTest.java

    }
+
+    @Test
+    void testAllianceTrackerState() {


I agree with @frigoref 's feedback here. The heuristic is, based on the knowledge that this test has failed, and the test name, would you know why it failed?

JMoravec added 2 commits November 5, 2025 18:09

Create more explicit checks for testAddAlliance

086869d

Add some more AllianceTracker unit tests

f879745

frigoref requested changes Nov 6, 2025

View reviewed changes

frigoref assigned frigoref and JMoravec and unassigned frigoref Nov 9, 2025

DanVanAtta reviewed Nov 15, 2025

View reviewed changes

Add some AllianceTracker unit tests #13835

Are you sure you want to change the base?

Add some AllianceTracker unit tests #13835

Conversation

JMoravec commented Nov 6, 2025

Uh oh!

frigoref left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanVanAtta left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants