diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/move/validation/UnitStackingLimitFilter.java b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/move/validation/UnitStackingLimitFilter.java index b0dc56324a1..986a6ab73ca 100644 --- a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/move/validation/UnitStackingLimitFilter.java +++ b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/move/validation/UnitStackingLimitFilter.java @@ -24,12 +24,13 @@ public class UnitStackingLimitFilter { public static final String PLACEMENT_LIMIT = "placementLimit"; /** - * Returns the subset of units that are valid with respect to any stacking limits in effect. + * Returns the subset of units that are valid with respect to any stacking limits in effect. The + * returned list is mutable and serializable. * *

Note: The passed list of units should have already been filtered for placement restrictions * as otherwise this could return a subset of units that cannot be placed for other reasons. */ - public static Collection filterUnits( + public static List filterUnits( final Collection units, final String limitType, final GamePlayer owner, @@ -40,7 +41,7 @@ public static Collection filterUnits( /** * Same as above, but allows passing `existingUnitsToBePlaced` that have already been selected. */ - public static Collection filterUnits( + public static List filterUnits( final Collection units, final String limitType, final GamePlayer owner, @@ -79,7 +80,10 @@ public static Collection filterUnits( unitsAllowedSoFar.add(unit); } } - return unitsAllowedSoFar.subList(existingUnitsToBePlaced.size(), unitsAllowedSoFar.size()); + // Remove the existing units from the list before returning it. Don't return a sublist as it's + // not serializable. + unitsAllowedSoFar.subList(0, existingUnitsToBePlaced.size()).clear(); + return unitsAllowedSoFar; } /** diff --git a/game-app/game-core/src/test/java/games/strategy/triplea/delegate/move/validation/UnitStackingLimitTest.java b/game-app/game-core/src/test/java/games/strategy/triplea/delegate/move/validation/UnitStackingLimitFilterTest.java similarity index 59% rename from game-app/game-core/src/test/java/games/strategy/triplea/delegate/move/validation/UnitStackingLimitTest.java rename to game-app/game-core/src/test/java/games/strategy/triplea/delegate/move/validation/UnitStackingLimitFilterTest.java index 4498f4d4dae..0469090d7b4 100644 --- a/game-app/game-core/src/test/java/games/strategy/triplea/delegate/move/validation/UnitStackingLimitTest.java +++ b/game-app/game-core/src/test/java/games/strategy/triplea/delegate/move/validation/UnitStackingLimitFilterTest.java @@ -1,51 +1,65 @@ package games.strategy.triplea.delegate.move.validation; -import static games.strategy.triplea.delegate.move.validation.UnitStackingLimitFilter.PLACEMENT_LIMIT; import static games.strategy.triplea.delegate.move.validation.UnitStackingLimitFilter.filterUnits; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.hasSize; +import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.collection.IsEmptyCollection.empty; import static org.triplea.java.collections.CollectionUtils.countMatches; +import games.strategy.engine.data.GamePlayer; +import games.strategy.engine.data.Territory; import games.strategy.engine.data.Unit; import games.strategy.triplea.delegate.AbstractDelegateTestCase; import games.strategy.triplea.delegate.Matches; +import java.io.Serializable; +import java.util.Collection; import java.util.Collections; import java.util.List; import org.junit.jupiter.api.Test; -class UnitStackingLimitTest extends AbstractDelegateTestCase { +class UnitStackingLimitFilterTest extends AbstractDelegateTestCase { + + private static List callFilterUnits( + Collection units, GamePlayer owner, Territory t, Collection existingUnits) { + var result = + filterUnits(units, UnitStackingLimitFilter.PLACEMENT_LIMIT, owner, t, existingUnits); + assertThat(result, instanceOf(Serializable.class)); + return result; + } + + private static List callFilterUnits(Collection units, GamePlayer owner, Territory t) { + var result = filterUnits(units, UnitStackingLimitFilter.PLACEMENT_LIMIT, owner, t); + assertThat(result, instanceOf(Serializable.class)); + return result; + } @Test void testUnitAttachmentStackingLimit() { // we can place 4 List fourTanks = armour.create(4, british); - assertThat(filterUnits(fourTanks, PLACEMENT_LIMIT, british, uk), is(fourTanks)); + assertThat(callFilterUnits(fourTanks, british, uk), is(fourTanks)); // the same four tanks are returned, even if we pass some existing units. - assertThat( - filterUnits(fourTanks, PLACEMENT_LIMIT, british, uk, infantry.create(2, british)), - is(fourTanks)); + assertThat(callFilterUnits(fourTanks, british, uk, infantry.create(2, british)), is(fourTanks)); // and only 2 are returned if we pass 2 existing tanks - assertThat( - filterUnits(fourTanks, PLACEMENT_LIMIT, british, uk, armour.create(2, british)), - hasSize(2)); + assertThat(callFilterUnits(fourTanks, british, uk, armour.create(2, british)), hasSize(2)); // we can't place 5 per the unit attachment's placementLimit List fiveTanks = armour.create(5, british); - assertThat(filterUnits(fiveTanks, PLACEMENT_LIMIT, british, uk), hasSize(4)); + assertThat(callFilterUnits(fiveTanks, british, uk), hasSize(4)); // only 2 can be placed if 2 are already there uk.getUnitCollection().addAll(armour.create(2, british)); - assertThat(filterUnits(fourTanks, PLACEMENT_LIMIT, british, uk), hasSize(2)); + assertThat(callFilterUnits(fourTanks, british, uk), hasSize(2)); // but we can include other units that don't have stacking limits in the list // note: still with the two tanks already in the uk List twoInfantryAndFourTanks = infantry.create(2, british); twoInfantryAndFourTanks.addAll(fourTanks); assertThat(twoInfantryAndFourTanks, hasSize(6)); - var result = filterUnits(twoInfantryAndFourTanks, PLACEMENT_LIMIT, british, uk); + var result = callFilterUnits(twoInfantryAndFourTanks, british, uk); assertThat(result, hasSize(4)); assertThat(countMatches(result, Matches.unitIsOfType(infantry)), is(2)); assertThat(countMatches(result, Matches.unitIsOfType(armour)), is(2)); @@ -55,38 +69,38 @@ void testUnitAttachmentStackingLimit() { void testClassicAaStackingLimit() { // No more aa guns to be placed in UK. List units = aaGun.create(2, british); - assertThat(filterUnits(units, PLACEMENT_LIMIT, british, uk), empty()); + assertThat(callFilterUnits(units, british, uk), empty()); // Remove the aa gun in UK, now one can be placed. uk.getUnitCollection().removeIf(Matches.unitIsOfType(aaGun)); - assertThat(filterUnits(units, PLACEMENT_LIMIT, british, uk), hasSize(1)); + assertThat(callFilterUnits(units, british, uk), hasSize(1)); } @Test void testPlayerAttachmentStackingLimit() { // we can place 3 battleships List units = battleship.create(3, british); - assertThat(filterUnits(units, PLACEMENT_LIMIT, british, uk), is(units)); + assertThat(callFilterUnits(units, british, uk), is(units)); // but not 4 units = battleship.create(4, british); - assertThat(filterUnits(units, PLACEMENT_LIMIT, british, uk), hasSize(3)); + assertThat(callFilterUnits(units, british, uk), hasSize(3)); // we can also place 2 battleships and a carrier units = battleship.create(2, british); units.addAll(carrier.create(1, british)); - assertThat(filterUnits(units, PLACEMENT_LIMIT, british, uk), is(units)); + assertThat(callFilterUnits(units, british, uk), is(units)); // but not 2 battleships and 2 carriers units.addAll(carrier.create(1, british)); var expected = units.subList(0, 3); assertThat(expected, hasSize(3)); - assertThat(filterUnits(units, PLACEMENT_LIMIT, british, uk), is(expected)); + assertThat(callFilterUnits(units, british, uk), is(expected)); // and that the filtered units returned are in order Collections.shuffle(units); expected = units.subList(0, 3); assertThat(expected, hasSize(3)); - assertThat(filterUnits(units, PLACEMENT_LIMIT, british, uk), is(expected)); + assertThat(callFilterUnits(units, british, uk), is(expected)); } }