Fix issue a cause of "Could not find step name" (2.6 bug). (#11773)

This was happening because the code to generate step names was not excluding units that would not participate in combat, resulting in infrastructure units getting their own steps (which later did not match what the engine generated once the filtering took place).

Uses the same logic as what's done for the battle to exclude units.

This change required adjusting a bunch of tests that were previously not careful about which had mistakes in setting up mock battles where the units didn't match the territory (in terms of sea vs. land). Also makes the tests to mock game data properties leniently, so that only the ones being set to true need to be specified (removing lots of LOC).

Includes a unit test.

Fixes: #10647
Note: Doesn't fix  #11617, as the root cause of that error is different, despite the actual error being the same. I will fix that in a separate PR.

game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/MustFightBattle.java

@@ -1272,6 +1272,7 @@ private List<Unit> removeNonCombatants(
       final boolean attacking,
       final boolean removeForNextRound) {
     int battleRound = (removeForNextRound ? round + 1 : round);
+    // Note: Also done in FiringGroupSplitterGeneral when determining step names. They must match.
     return CollectionUtils.getMatches(
         units,
         Matches.unitCanParticipateInCombat(

game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/steps/fire/general/FiringGroupSplitterGeneral.java

@@ -56,10 +56,28 @@ public enum Type {
 
   @Override
   public List<FiringGroup> apply(final BattleState battleState) {
+    final Collection<Unit> enemyUnits =
+        CollectionUtils.getMatches(
+            battleState.filterUnits(ALIVE, side.getOpposite()),
+            PredicateBuilder.of(Matches.unitIsNotInfrastructure())
+                .andIf(side == DEFENSE, Matches.unitIsSuicideOnAttack().negate())
+                .andIf(side == OFFENSE, Matches.unitIsSuicideOnDefense().negate())
+                .build());
+
+    // Filter participants (same as is done in MustFightBattle.removeNonCombatants()), so that we
+    // don't end up generating combat step names for units that will be excluded.
+    final Predicate<Unit> canParticipateInCombat =
+        Matches.unitCanParticipateInCombat(
+            side == OFFENSE,
+            battleState.getPlayer(OFFENSE),
+            battleState.getBattleSite(),
+            1,
+            enemyUnits);
     final Collection<Unit> canFire =
         CollectionUtils.getMatches(
             battleState.filterUnits(ACTIVE, side),
             PredicateBuilder.of(getFiringUnitPredicate(battleState))
+                .and(canParticipateInCombat)
                 // Remove offense allied units if allied air can not participate
                 .andIf(
                     side == OFFENSE
@@ -68,14 +86,6 @@ public List<FiringGroup> apply(final BattleState battleState) {
                     Matches.unitIsOwnedBy(battleState.getPlayer(side)))
                 .build());
 
-    final Collection<Unit> enemyUnits =
-        CollectionUtils.getMatches(
-            battleState.filterUnits(ALIVE, side.getOpposite()),
-            PredicateBuilder.of(Matches.unitIsNotInfrastructure())
-                .andIf(side == DEFENSE, Matches.unitIsSuicideOnAttack().negate())
-                .andIf(side == OFFENSE, Matches.unitIsSuicideOnDefense().negate())
-                .build());
-
     final List<FiringGroup> firingGroups = new ArrayList<>();
 
     final List<TargetGroup> targetGroups = TargetGroup.newTargetGroups(canFire, enemyUnits);

game-app/game-core/src/test/java/games/strategy/triplea/delegate/battle/steps/MockGameData.java

@@ -16,6 +16,8 @@
 import static games.strategy.triplea.Constants.SUB_RETREAT_BEFORE_BATTLE;
 import static games.strategy.triplea.Constants.TRANSPORT_CASUALTIES_RESTRICTED;
 import static games.strategy.triplea.Constants.WW2V2;
+import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.Mockito.lenient;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
@@ -30,9 +32,11 @@
 import games.strategy.engine.data.ResourceList;
 import games.strategy.engine.data.TechnologyFrontier;
 import games.strategy.engine.data.Territory;
+import games.strategy.engine.data.UnitType;
 import games.strategy.engine.data.UnitTypeList;
 import games.strategy.engine.data.properties.GameProperties;
 import games.strategy.triplea.delegate.TechTracker;
+import java.util.List;
 import java.util.Set;
 
 public class MockGameData {
@@ -102,6 +106,11 @@ public MockGameData withTechnologyFrontier() {
     return this;
   }
 
+  public MockGameData withLenientProperties() {
+    lenient().when(gameProperties.get(anyString(), anyBoolean())).thenReturn(false);
+    return this;
+  }
+
   public MockGameData withTransportCasualtiesRestricted(final boolean value) {
     when(gameProperties.get(TRANSPORT_CASUALTIES_RESTRICTED, false)).thenReturn(value);
     return this;
@@ -189,4 +198,14 @@ public MockGameData withLowLuck(final boolean value) {
     when(gameProperties.get(LOW_LUCK, false)).thenReturn(value);
     return this;
   }
+
+  public MockGameData withUnitTypeList(final List<UnitType> types) {
+    UnitTypeList unitTypeList = new UnitTypeList(gameData);
+    for (var unitType : types) {
+      lenient().when(unitType.getData()).thenReturn(gameData);
+      unitTypeList.addUnitType(unitType);
+    }
+    when(gameData.getUnitTypeList()).thenReturn(unitTypeList);
+    return this;
+  }
 }

game-app/game-core/src/test/java/games/strategy/triplea/delegate/battle/steps/change/suicide/RemoveFirstStrikeSuicideTest.java

@@ -4,8 +4,8 @@
 import static games.strategy.triplea.delegate.battle.BattleState.Side.OFFENSE;
 import static games.strategy.triplea.delegate.battle.FakeBattleState.givenBattleStateBuilder;
 import static games.strategy.triplea.delegate.battle.steps.BattleStepsTest.givenAnyUnit;
-import static games.strategy.triplea.delegate.battle.steps.BattleStepsTest.givenUnitFirstStrikeSuicideOnAttack;
-import static games.strategy.triplea.delegate.battle.steps.BattleStepsTest.givenUnitFirstStrikeSuicideOnDefense;
+import static games.strategy.triplea.delegate.battle.steps.BattleStepsTest.givenSeaUnitFirstStrikeSuicideOnAttack;
+import static games.strategy.triplea.delegate.battle.steps.BattleStepsTest.givenSeaUnitFirstStrikeSuicideOnDefense;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoMoreInteractions;
@@ -34,8 +34,8 @@ public class RemoveFirstStrikeSuicideTest {
   void suicideUnitsRemoved() {
     when(delegateBridge.getDisplayChannelBroadcaster()).thenReturn(mock(IDisplay.class));
 
-    final List<Unit> attackers = List.of(givenAnyUnit(), givenUnitFirstStrikeSuicideOnAttack());
-    final List<Unit> defenders = List.of(givenAnyUnit(), givenUnitFirstStrikeSuicideOnDefense());
+    final List<Unit> attackers = List.of(givenAnyUnit(), givenSeaUnitFirstStrikeSuicideOnAttack());
+    final List<Unit> defenders = List.of(givenAnyUnit(), givenSeaUnitFirstStrikeSuicideOnDefense());
     final MockGameData gameData = MockGameData.givenGameData();
     final BattleState battleState =
         givenBattleStateBuilder()

game-app/game-core/src/test/java/games/strategy/triplea/delegate/battle/steps/fire/air/AirAttackVsNonSubsStepTest.java

@@ -2,7 +2,7 @@
 
 import static games.strategy.triplea.delegate.battle.FakeBattleState.givenBattleStateBuilder;
 import static games.strategy.triplea.delegate.battle.steps.BattleStepsTest.givenAnyUnit;
-import static games.strategy.triplea.delegate.battle.steps.BattleStepsTest.givenUnitCanNotBeTargetedBy;
+import static games.strategy.triplea.delegate.battle.steps.BattleStepsTest.givenSeaUnitCanNotBeTargetedBy;
 import static games.strategy.triplea.delegate.battle.steps.BattleStepsTest.givenUnitDestroyer;
 import static games.strategy.triplea.delegate.battle.steps.BattleStepsTest.givenUnitIsAir;
 import static org.hamcrest.MatcherAssert.assertThat;
@@ -35,7 +35,7 @@ static List<Arguments> stepName() {
             "Attacker has air units and no destroyers vs Defender subs",
             givenBattleStateBuilder()
                 .attackingUnits(List.of(givenAnyUnit(), givenUnitIsAir()))
-                .defendingUnits(List.of(givenUnitCanNotBeTargetedBy(mock(UnitType.class))))
+                .defendingUnits(List.of(givenSeaUnitCanNotBeTargetedBy(mock(UnitType.class))))
                 .build(),
             true),
         Arguments.of(

game-app/game-core/src/test/java/games/strategy/triplea/delegate/battle/steps/fire/air/AirDefendVsNonSubsStepTest.java

@@ -2,7 +2,7 @@
 
 import static games.strategy.triplea.delegate.battle.FakeBattleState.givenBattleStateBuilder;
 import static games.strategy.triplea.delegate.battle.steps.BattleStepsTest.givenAnyUnit;
-import static games.strategy.triplea.delegate.battle.steps.BattleStepsTest.givenUnitCanNotBeTargetedBy;
+import static games.strategy.triplea.delegate.battle.steps.BattleStepsTest.givenSeaUnitCanNotBeTargetedBy;
 import static games.strategy.triplea.delegate.battle.steps.BattleStepsTest.givenUnitDestroyer;
 import static games.strategy.triplea.delegate.battle.steps.BattleStepsTest.givenUnitIsAir;
 import static org.hamcrest.MatcherAssert.assertThat;
@@ -34,7 +34,7 @@ static List<Arguments> stepName() {
         Arguments.of(
             "Defender has air units and no destroyers vs Attacker subs",
             givenBattleStateBuilder()
-                .attackingUnits(List.of(givenUnitCanNotBeTargetedBy(mock(UnitType.class))))
+                .attackingUnits(List.of(givenSeaUnitCanNotBeTargetedBy(mock(UnitType.class))))
                 .defendingUnits(List.of(givenAnyUnit(), givenUnitIsAir()))
                 .build(),
             true),

game-app/game-core/src/test/java/games/strategy/triplea/delegate/battle/steps/fire/general/FiringGroupSplitterGeneralTest.java

@@ -18,17 +18,17 @@
 import static org.hamcrest.Matchers.is;
 import static org.mockito.Mockito.lenient;
 import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.never;
-import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
+import games.strategy.engine.data.GameData;
 import games.strategy.engine.data.GamePlayer;
 import games.strategy.engine.data.Unit;
 import games.strategy.engine.data.UnitType;
 import games.strategy.triplea.attachments.UnitAttachment;
 import games.strategy.triplea.delegate.battle.steps.fire.FiringGroup;
 import java.util.List;
 import java.util.Set;
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.mockito.Mock;
@@ -40,6 +40,16 @@ class FiringGroupSplitterGeneralTest {
   @Mock GamePlayer attacker;
   @Mock GamePlayer defender;
 
+  @BeforeEach
+  void setUp() {
+    final GameData gameData = new GameData();
+
+    lenient().when(attacker.getName()).thenReturn("attacker");
+    lenient().when(attacker.getData()).thenReturn(gameData);
+    lenient().when(defender.getName()).thenReturn("defender");
+    lenient().when(defender.getData()).thenReturn(gameData);
+  }
+
   @Test
   void oneFiringUnitVsOneTargetableUnitMakesOneFiringGroup() {
     final Unit targetUnit = givenAnyUnit();
@@ -216,14 +226,6 @@ void doNotExcludeUnitsOfAlliesIfAlliedAirIndependentIsFalseButItIsDefense() {
     assertThat(firingGroups.get(0).getFiringUnits(), contains(fireUnit, fireUnit2));
     assertThat(firingGroups.get(0).getTargetUnits(), contains(targetUnit));
     assertThat(firingGroups.get(0).isSuicideOnHit(), is(false));
-
-    verify(
-            fireUnit,
-            never()
-                .description(
-                    "Units on defense with AlliedAirIndependent == false"
-                        + "should never call getOwner"))
-        .getOwner();
   }
 
   @Test

game-app/game-core/src/test/java/games/strategy/triplea/delegate/battle/steps/retreat/sub/SubmergeSubsVsOnlyAirStepTest.java

@@ -5,7 +5,7 @@
 import static games.strategy.triplea.delegate.battle.BattleState.Side.OFFENSE;
 import static games.strategy.triplea.delegate.battle.FakeBattleState.givenBattleStateBuilder;
 import static games.strategy.triplea.delegate.battle.steps.BattleStepsTest.givenAnyUnit;
-import static games.strategy.triplea.delegate.battle.steps.BattleStepsTest.givenUnitCanEvadeAndCanNotBeTargetedByRandomUnit;
+import static games.strategy.triplea.delegate.battle.steps.BattleStepsTest.givenSeaUnitCanEvadeAndCanNotBeTargetedByRandomUnit;
 import static games.strategy.triplea.delegate.battle.steps.BattleStepsTest.givenUnitIsAir;
 import static games.strategy.triplea.delegate.battle.steps.MockGameData.givenGameData;
 import static org.hamcrest.MatcherAssert.assertThat;
@@ -90,15 +90,15 @@ static List<Arguments> stepName() {
         Arguments.of(
             "Attacking evaders vs ALL air",
             givenBattleStateBuilder()
-                .attackingUnits(List.of(givenUnitCanEvadeAndCanNotBeTargetedByRandomUnit()))
+                .attackingUnits(List.of(givenSeaUnitCanEvadeAndCanNotBeTargetedByRandomUnit()))
                 .defendingUnits(List.of(givenUnitIsAir(), givenUnitIsAir()))
                 .build(),
             true),
         Arguments.of(
             "Defending evaders vs ALL air",
             givenBattleStateBuilder()
                 .attackingUnits(List.of(givenUnitIsAir(), givenUnitIsAir()))
-                .defendingUnits(List.of(givenUnitCanEvadeAndCanNotBeTargetedByRandomUnit()))
+                .defendingUnits(List.of(givenSeaUnitCanEvadeAndCanNotBeTargetedByRandomUnit()))
                 .build(),
             true));
   }