From a244e69dd50f7c0f51e7bc134544e65e6da7309c Mon Sep 17 00:00:00 2001 From: asvitkine Date: Fri, 14 Jul 2023 22:07:11 -0400 Subject: [PATCH] Fix another cause of "Could not find step name" (2.6 bug). (#11780) This one is caused by non-determinism when choose an arbitrary unit type in a firing group to name that group. When the ordering of the provided units changed during the rounds of a battle, it resulted in different step names generated. This PR fixes that problem by choosing the first name alphabetically. Adds test coverage. --- .../general/FiringGroupSplitterGeneral.java | 8 +- .../steps/fire/general/TargetGroup.java | 2 +- .../FiringGroupSplitterGeneralTest.java | 74 ++++++++++--------- 3 files changed, 48 insertions(+), 36 deletions(-) diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/steps/fire/general/FiringGroupSplitterGeneral.java b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/steps/fire/general/FiringGroupSplitterGeneral.java index 1b3d4e925f2..f260699843b 100644 --- a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/steps/fire/general/FiringGroupSplitterGeneral.java +++ b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/steps/fire/general/FiringGroupSplitterGeneral.java @@ -15,6 +15,7 @@ import java.io.Serializable; import java.util.ArrayList; import java.util.Collection; +import java.util.Comparator; import java.util.List; import java.util.function.Function; import java.util.function.Predicate; @@ -154,14 +155,17 @@ private void generateNamedGroups( final Collection targetGroups, final Collection canFire, final Collection enemyUnits) { - if (targetGroups.size() == 1) { firingGroups.addAll( buildFiringGroups(name, canFire, enemyUnits, CollectionUtils.getAny(targetGroups))); } else { // use the first unitType name of each TargetGroup as a suffix for the FiringGroup name for (final TargetGroup targetGroup : targetGroups) { - final UnitType type = CollectionUtils.getAny(targetGroup.getFiringUnits(canFire)).getType(); + // Sort units first, before getting the unit type to use for the name, so that we choose + // the same name regardless of the order of units provided. + List firingUnits = targetGroup.getFiringUnits(canFire); + firingUnits.sort(Comparator.comparing(u -> u.getType().getName())); + UnitType type = CollectionUtils.getAny(firingUnits).getType(); firingGroups.addAll( buildFiringGroups(name + " " + type.getName(), canFire, enemyUnits, targetGroup)); } diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/steps/fire/general/TargetGroup.java b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/steps/fire/general/TargetGroup.java index ada01ff32b5..9bca73303ef 100644 --- a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/steps/fire/general/TargetGroup.java +++ b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/steps/fire/general/TargetGroup.java @@ -27,7 +27,7 @@ class TargetGroup { this.targetUnitTypes = targetUnitTypes; } - public Collection getFiringUnits(final Collection units) { + public List getFiringUnits(final Collection units) { return CollectionUtils.getMatches(units, Matches.unitIsOfTypes(firingUnitTypes)); } diff --git a/game-app/game-core/src/test/java/games/strategy/triplea/delegate/battle/steps/fire/general/FiringGroupSplitterGeneralTest.java b/game-app/game-core/src/test/java/games/strategy/triplea/delegate/battle/steps/fire/general/FiringGroupSplitterGeneralTest.java index f7b62d6c419..94340198fee 100644 --- a/game-app/game-core/src/test/java/games/strategy/triplea/delegate/battle/steps/fire/general/FiringGroupSplitterGeneralTest.java +++ b/game-app/game-core/src/test/java/games/strategy/triplea/delegate/battle/steps/fire/general/FiringGroupSplitterGeneralTest.java @@ -13,6 +13,7 @@ import static games.strategy.triplea.delegate.battle.steps.MockGameData.givenGameData; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.is; @@ -338,6 +339,14 @@ void noFiringGroupIfAllTargetsAreExcluded() { assertThat(firingGroups, is(empty())); } + private Unit givenUnitWithCannotTarget(String name, Set cannotTarget) { + final Unit fireUnit = givenAnyUnit(); + lenient().when(fireUnit.getType().getName()).thenReturn(name); + lenient().when(fireUnit.toString()).thenReturn(name); + when(fireUnit.getUnitAttachment().getCanNotTarget()).thenReturn(cannotTarget); + return fireUnit; + } + @Test void twoFiringGroupsWithCanNotTarget() { final Unit targetUnit = givenAnyUnit(); @@ -347,39 +356,38 @@ void twoFiringGroupsWithCanNotTarget() { final Unit targetUnit3 = givenAnyUnit(); final UnitType targetUnit3Type = targetUnit3.getType(); - final Unit fireUnit = givenAnyUnit(); - when(fireUnit.getType().getName()).thenReturn("fireUnit"); - final UnitAttachment unitAttachment = - (UnitAttachment) fireUnit.getType().getAttachment(UNIT_ATTACHMENT_NAME); - when(unitAttachment.getCanNotTarget()).thenReturn(Set.of(targetUnit2Type, targetUnit3Type)); - - final Unit fireUnit2 = givenAnyUnit(); - when(fireUnit2.getType().getName()).thenReturn("fireUnit2"); - final UnitAttachment unitAttachment2 = - (UnitAttachment) fireUnit2.getType().getAttachment(UNIT_ATTACHMENT_NAME); - when(unitAttachment2.getCanNotTarget()).thenReturn(Set.of(targetUnitType)); - - final List firingGroups = - FiringGroupSplitterGeneral.of(OFFENSE, FiringGroupSplitterGeneral.Type.NORMAL, UNITS) - .apply( - givenBattleStateBuilder() - .gameData(givenGameData().withAlliedAirIndependent(true).build()) - .attacker(attacker) - .defender(defender) - .attackingUnits(List.of(fireUnit, fireUnit2)) - .defendingUnits(List.of(targetUnit, targetUnit2, targetUnit3)) - .build()); - - assertThat(firingGroups, hasSize(2)); - assertThat(firingGroups.get(0).getDisplayName(), is(UNITS + " fireUnit")); - assertThat(firingGroups.get(0).getFiringUnits(), contains(fireUnit)); - assertThat(firingGroups.get(0).getTargetUnits(), contains(targetUnit)); - assertThat(firingGroups.get(0).isSuicideOnHit(), is(false)); - - assertThat(firingGroups.get(1).getDisplayName(), is(UNITS + " fireUnit2")); - assertThat(firingGroups.get(1).getFiringUnits(), contains(fireUnit2)); - assertThat(firingGroups.get(1).getTargetUnits(), contains(targetUnit2, targetUnit3)); - assertThat(firingGroups.get(1).isSuicideOnHit(), is(false)); + final Unit fireUnit = + givenUnitWithCannotTarget("fireUnit", Set.of(targetUnit2Type, targetUnit3Type)); + final Unit fireUnit2 = givenUnitWithCannotTarget("fireUnit2", Set.of(targetUnitType)); + final Unit fireUnit3 = givenUnitWithCannotTarget("fireUnit3", Set.of(targetUnitType)); + + // Iterate over several different orderings of attackers to ensure step names are deterministic + // regardless of the order of units passed in. + List attackersOrdering1 = List.of(fireUnit, fireUnit2, fireUnit3); + List attackersOrdering2 = List.of(fireUnit, fireUnit3, fireUnit2); + for (List attackingUnits : List.of(attackersOrdering1, attackersOrdering2)) { + final List firingGroups = + FiringGroupSplitterGeneral.of(OFFENSE, FiringGroupSplitterGeneral.Type.NORMAL, UNITS) + .apply( + givenBattleStateBuilder() + .gameData(givenGameData().withAlliedAirIndependent(true).build()) + .attacker(attacker) + .defender(defender) + .attackingUnits(attackingUnits) + .defendingUnits(List.of(targetUnit, targetUnit2, targetUnit3)) + .build()); + + assertThat(firingGroups, hasSize(2)); + assertThat(firingGroups.get(0).getDisplayName(), is(UNITS + " fireUnit")); + assertThat(firingGroups.get(0).getFiringUnits(), contains(fireUnit)); + assertThat(firingGroups.get(0).getTargetUnits(), contains(targetUnit)); + assertThat(firingGroups.get(0).isSuicideOnHit(), is(false)); + + assertThat(firingGroups.get(1).getDisplayName(), is(UNITS + " fireUnit2")); + assertThat(firingGroups.get(1).getFiringUnits(), containsInAnyOrder(fireUnit2, fireUnit3)); + assertThat(firingGroups.get(1).getTargetUnits(), contains(targetUnit2, targetUnit3)); + assertThat(firingGroups.get(1).isSuicideOnHit(), is(false)); + } } @Test