Fix another case of cannot find step name error. (#12023)

* Fix another case of cannot find step name error.

* Remove debugging code.

* Fix some intellij comment warnings.

* Fix existing tests.

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

@@ -245,7 +245,7 @@ public Change addAttackChange(
     // mark units with no movement for all but air
     Collection<Unit> nonAir = CollectionUtils.getMatches(attackingUnits, Matches.unitIsNotAir());
     // we don't want to change the movement of transported land units if this is a sea battle
-    // so restrict non air to remove land units
+    // so restrict non-air to remove land units
     if (battleSite.isWater()) {
       nonAir = CollectionUtils.getMatches(nonAir, Matches.unitIsNotLand());
     }
@@ -705,7 +705,7 @@ public void fight(final IDelegateBridge bridge) {
         List.of());
     display.listBattleSteps(battleId, stepStrings);
     if (!headless) {
-      // take the casualties with least movement first
+      // take the casualties with the least movement first
       CasualtySortingUtil.sortPreBattle(attackingUnits);
       CasualtySortingUtil.sortPreBattle(defendingUnits);
       SoundUtils.playBattleType(attacker, attackingUnits, defendingUnits, bridge);
@@ -844,9 +844,9 @@ public Collection<Territory> getAttackerRetreatTerritories() {
         || Properties.getRetreatingUnitsRemainInPlace(gameData.getProperties())) {
       return Set.of(battleSite);
     }
-    // its possible that a sub retreated to a territory we came from, if so we can no longer retreat
-    // there
-    // or if we are moving out of a territory containing enemy units, we cannot retreat back there
+    // it's possible that a sub retreated to a territory we came from, if so we can no longer
+    // retreat there or if we are moving out of a territory containing enemy units, we cannot
+    // retreat back there
     final Predicate<Unit> enemyUnitsThatPreventRetreat =
         PredicateBuilder.of(Matches.enemyUnit(attacker))
             .and(Matches.unitIsNotInfrastructure())
@@ -1260,7 +1260,7 @@ public void execute(final ExecutionStack stack, final IDelegateBridge bridge) {
   }
 
   /**
-   * Removes non combatants from the requested battle side and returns them
+   * Removes non-combatants from the requested battle side and returns them.
    *
    * @return the removed units
    */
@@ -1284,7 +1284,7 @@ public Collection<Unit> removeNonCombatants(final Side side) {
   }
 
   /**
-   * Returns only the relevant non-combatant units present in the specified collection.
+   * Returns only the relevant combatant units present in the specified collection.
    *
    * @return a collection containing all the combatants in units non-combatants include such things
    *     as factories, aa guns, land units in a water battle.

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

@@ -57,28 +57,19 @@ public enum Type {
 
   @Override
   public List<FiringGroup> apply(final BattleState battleState) {
-    final Collection<Unit> enemyUnits =
+    final Collection<Unit> ourUnits = battleState.filterUnits(ACTIVE, side);
+    final Collection<Unit> enemyUnits = battleState.filterUnits(ALIVE, side.getOpposite());
+    final Collection<Unit> enemyCombatants =
         CollectionUtils.getMatches(
-            battleState.filterUnits(ALIVE, side.getOpposite()),
+            getCombatParticipants(battleState, side.getOpposite(), enemyUnits, ourUnits),
             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),
+            getCombatParticipants(battleState, side, ourUnits, enemyUnits),
             PredicateBuilder.of(getFiringUnitPredicate(battleState))
-                .and(canParticipateInCombat)
                 // Remove offense allied units if allied air can not participate
                 .andIf(
                     side == OFFENSE
@@ -88,26 +79,40 @@ public List<FiringGroup> apply(final BattleState battleState) {
                 .build());
 
     final List<FiringGroup> firingGroups = new ArrayList<>();
-
-    final List<TargetGroup> targetGroups = TargetGroup.newTargetGroups(canFire, enemyUnits);
-
+    final List<TargetGroup> targetGroups = TargetGroup.newTargetGroups(canFire, enemyCombatants);
     if (targetGroups.size() == 1) {
-      firingGroups.addAll(buildFiringGroups(groupName, canFire, enemyUnits, targetGroups.get(0)));
+      firingGroups.addAll(
+          buildFiringGroups(groupName, canFire, enemyCombatants, targetGroups.get(0)));
     } else {
       // General firing groups don't have individual names so find commonly used groups and
       // give them unique names
       final List<TargetGroup> airVsSubGroups =
-          targetGroups.stream()
-              .filter(this.filterAirVsSubTargetGroups(enemyUnits))
-              .collect(Collectors.toList());
-      generateNamedGroups(AIR_FIRE_NON_SUBS, firingGroups, airVsSubGroups, canFire, enemyUnits);
+          CollectionUtils.getMatches(targetGroups, filterAirVsSubTargetGroups(enemyUnits));
+      generateNamedGroups(
+          AIR_FIRE_NON_SUBS, firingGroups, airVsSubGroups, canFire, enemyCombatants);
       targetGroups.removeAll(airVsSubGroups);
-
-      generateNamedGroups(groupName, firingGroups, targetGroups, canFire, enemyUnits);
+      generateNamedGroups(groupName, firingGroups, targetGroups, canFire, enemyCombatants);
     }
     return firingGroups;
   }
 
+  private Collection<Unit> getCombatParticipants(
+      BattleState battleState,
+      BattleState.Side side,
+      Collection<Unit> units,
+      Collection<Unit> enemyUnits) {
+    // 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.
+    return CollectionUtils.getMatches(
+        units,
+        Matches.unitCanParticipateInCombat(
+            side == OFFENSE,
+            battleState.getPlayer(side),
+            battleState.getBattleSite(),
+            1,
+            enemyUnits));
+  }
+
   private Predicate<Unit> getFiringUnitPredicate(final BattleState battleState) {
     final Predicate<Unit> predicate =
         (side == OFFENSE)

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

@@ -185,7 +185,7 @@ public static Unit givenUnitDestroyer() {
 
   public static Unit givenUnitSeaTransport() {
     final UnitAndAttachment unitAndAttachment = newSeaUnitAndAttachment();
-    when(unitAndAttachment.unitAttachment.getTransportCapacity()).thenReturn(2);
+    lenient().when(unitAndAttachment.unitAttachment.getTransportCapacity()).thenReturn(2);
     return unitAndAttachment.unit;
   }
 
@@ -2041,4 +2041,50 @@ void nonParticipantsDontCreateExtraStepsWithCannotTarget() {
 
     assertThat(steps, is(basicFightStepStrings()));
   }
+
+  @Test
+  @DisplayName("Verify that extra steps won't be created due to canNotTarget and non-participants")
+  void nonParticipantsOnDefenseDontCreateExtraStepsWithCannotTarget() {
+    // Two attacking air units of different types.
+    final Unit unit1Plane = givenUnitIsAir();
+    lenient().when(unit1Plane.getOwner()).thenReturn(attacker);
+    final Unit unit2Plane = givenUnitIsAir();
+    lenient().when(unit2Plane.getOwner()).thenReturn(attacker);
+
+    // Defending transport with infantry on board.
+    final Unit unit3Transport = givenUnitSeaTransport();
+    lenient().when(unit3Transport.getOwner()).thenReturn(defender);
+    final Unit unit4Infantry = givenAnyUnit();
+    lenient().when(unit4Infantry.getOwner()).thenReturn(defender);
+    lenient().when(unit4Infantry.getTransportedBy()).thenReturn(unit3Transport);
+
+    // One of the planes can't target the infantry. This shouldn't matter since the infantry is a
+    // non-combatant.
+    final UnitType unit4InfantryType = unit4Infantry.getType();
+    when(unit1Plane.getUnitAttachment().getCanNotTarget()).thenReturn(Set.of(unit4InfantryType));
+
+    final var unitTypeList =
+        List.of(
+            unit1Plane.getType(),
+            unit2Plane.getType(),
+            unit3Transport.getType(),
+            unit4Infantry.getType());
+
+    final List<String> steps =
+        givenBattleSteps(
+            givenBattleStateBuilder()
+                .gameData(
+                    givenGameDataWithLenientProperties().withUnitTypeList(unitTypeList).build())
+                .attacker(attacker)
+                .defender(defender)
+                .attackingUnits(List.of(unit1Plane, unit2Plane))
+                .defendingUnits(List.of(unit3Transport, unit4Infantry))
+                .battleSite(givenSeaBattleSite())
+                .amphibious(false)
+                .build());
+
+    List<String> expectedSteps = basicFightStepStrings();
+    expectedSteps.add(attacker.getName() + ATTACKER_WITHDRAW);
+    assertThat(steps, is(expectedSteps));
+  }
 }

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

@@ -205,7 +205,7 @@ public MockGameData withUnitTypeList(final List<UnitType> types) {
       lenient().when(unitType.getData()).thenReturn(gameData);
       unitTypeList.addUnitType(unitType);
     }
-    when(gameData.getUnitTypeList()).thenReturn(unitTypeList);
+    lenient().when(gameData.getUnitTypeList()).thenReturn(unitTypeList);
     return this;
   }
 }

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

@@ -1,12 +1,12 @@
 package games.strategy.triplea.delegate.battle.steps.fire.general;
 
-import static games.strategy.triplea.Constants.UNIT_ATTACHMENT_NAME;
 import static games.strategy.triplea.delegate.battle.BattleState.Side.DEFENSE;
 import static games.strategy.triplea.delegate.battle.BattleState.Side.OFFENSE;
 import static games.strategy.triplea.delegate.battle.BattleStepStrings.AIR_FIRE_NON_SUBS;
 import static games.strategy.triplea.delegate.battle.BattleStepStrings.UNITS;
 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.givenSeaBattleSite;
 import static games.strategy.triplea.delegate.battle.steps.BattleStepsTest.givenUnitFirstStrike;
 import static games.strategy.triplea.delegate.battle.steps.BattleStepsTest.givenUnitIsAir;
 import static games.strategy.triplea.delegate.battle.steps.BattleStepsTest.givenUnitIsSea;
@@ -232,14 +232,12 @@ void doNotExcludeUnitsOfAlliesIfAlliedAirIndependentIsFalseButItIsDefense() {
   @Test
   void excludeSuicideOnDefenseTargetsIfOffense() {
     final Unit targetUnit = givenAnyUnit();
-    final UnitAttachment targetUnitAttachment =
-        (UnitAttachment) targetUnit.getType().getAttachment(UNIT_ATTACHMENT_NAME);
+    final UnitAttachment targetUnitAttachment = targetUnit.getUnitAttachment();
     // this isn't actually called, so mark it as lenient in case the code later changes to call it
     // inadvertently
     lenient().when(targetUnitAttachment.getIsSuicideOnAttack()).thenReturn(true);
     final Unit suicideUnit = givenAnyUnit();
-    final UnitAttachment suicideUnitAttachment =
-        (UnitAttachment) suicideUnit.getType().getAttachment(UNIT_ATTACHMENT_NAME);
+    final UnitAttachment suicideUnitAttachment = suicideUnit.getUnitAttachment();
     when(suicideUnitAttachment.getIsSuicideOnDefense()).thenReturn(true);
     final Unit fireUnit = givenAnyUnit();
 
@@ -263,14 +261,12 @@ void excludeSuicideOnDefenseTargetsIfOffense() {
   @Test
   void excludeSuicideOnAttackTargetsIfDefense() {
     final Unit targetUnit = givenAnyUnit();
-    final UnitAttachment targetUnitAttachment =
-        (UnitAttachment) targetUnit.getType().getAttachment(UNIT_ATTACHMENT_NAME);
+    final UnitAttachment targetUnitAttachment = targetUnit.getUnitAttachment();
     // this isn't actually called, so mark it as lenient in case the code later changes to call it
     // inadvertently
     lenient().when(targetUnitAttachment.getIsSuicideOnDefense()).thenReturn(true);
     final Unit suicideUnit = givenAnyUnit();
-    final UnitAttachment suicideUnitAttachment =
-        (UnitAttachment) suicideUnit.getType().getAttachment(UNIT_ATTACHMENT_NAME);
+    final UnitAttachment suicideUnitAttachment = suicideUnit.getUnitAttachment();
     when(suicideUnitAttachment.getIsSuicideOnAttack()).thenReturn(true);
     final Unit fireUnit = givenAnyUnit();
 
@@ -295,16 +291,23 @@ void excludeSuicideOnAttackTargetsIfDefense() {
   void excludeInfrastructureTargets() {
     final Unit targetUnit = givenAnyUnit();
     final Unit infrastructureUnit = givenAnyUnit();
-    final UnitAttachment infrastructureUnitAttachment =
-        (UnitAttachment) infrastructureUnit.getType().getAttachment(UNIT_ATTACHMENT_NAME);
+    final UnitAttachment infrastructureUnitAttachment = infrastructureUnit.getUnitAttachment();
     when(infrastructureUnitAttachment.getIsInfrastructure()).thenReturn(true);
     final Unit fireUnit = givenAnyUnit();
 
     final List<FiringGroup> firingGroups =
         FiringGroupSplitterGeneral.of(OFFENSE, FiringGroupSplitterGeneral.Type.NORMAL, "")
             .apply(
                 givenBattleStateBuilder()
-                    .gameData(givenGameData().withAlliedAirIndependent(true).build())
+                    .gameData(
+                        givenGameData()
+                            .withUnitTypeList(
+                                List.of(
+                                    targetUnit.getType(),
+                                    infrastructureUnit.getType(),
+                                    fireUnit.getType()))
+                            .withAlliedAirIndependent(true)
+                            .build())
                     .attacker(attacker)
                     .defender(defender)
                     .attackingUnits(List.of(fireUnit))
@@ -320,16 +323,19 @@ void excludeInfrastructureTargets() {
   @Test
   void noFiringGroupIfAllTargetsAreExcluded() {
     final Unit targetUnit = givenAnyUnit();
-    final UnitAttachment infrastructureUnitAttachment =
-        (UnitAttachment) targetUnit.getType().getAttachment(UNIT_ATTACHMENT_NAME);
+    final UnitAttachment infrastructureUnitAttachment = targetUnit.getUnitAttachment();
     when(infrastructureUnitAttachment.getIsInfrastructure()).thenReturn(true);
     final Unit fireUnit = givenAnyUnit();
 
     final List<FiringGroup> firingGroups =
         FiringGroupSplitterGeneral.of(OFFENSE, FiringGroupSplitterGeneral.Type.NORMAL, "")
             .apply(
                 givenBattleStateBuilder()
-                    .gameData(givenGameData().withAlliedAirIndependent(true).build())
+                    .gameData(
+                        givenGameData()
+                            .withUnitTypeList(List.of(targetUnit.getType(), fireUnit.getType()))
+                            .withAlliedAirIndependent(true)
+                            .build())
                     .attacker(attacker)
                     .defender(defender)
                     .attackingUnits(List.of(fireUnit))
@@ -392,17 +398,15 @@ void twoFiringGroupsWithCanNotTarget() {
 
   @Test
   void twoGroupsWhenAirAndSeaVsSub() {
-
     final Unit airUnit = givenUnitIsAir();
     final UnitType airUnitType = airUnit.getType();
 
-    final Unit attackingSeaUnit = givenAnyUnit();
+    final Unit attackingSeaUnit = givenUnitIsSea();
 
     final Unit subUnit = givenUnitIsSea();
-    final UnitAttachment subUnitAttachment =
-        (UnitAttachment) subUnit.getType().getAttachment(UNIT_ATTACHMENT_NAME);
+    final UnitAttachment subUnitAttachment = subUnit.getUnitAttachment();
     when(subUnitAttachment.getCanNotBeTargetedBy()).thenReturn(Set.of(airUnitType));
-    final Unit defendingSeaUnit = givenAnyUnit();
+    final Unit defendingSeaUnit = givenUnitIsSea();
 
     final List<FiringGroup> firingGroups =
         FiringGroupSplitterGeneral.of(OFFENSE, FiringGroupSplitterGeneral.Type.NORMAL, UNITS)
@@ -413,6 +417,7 @@ void twoGroupsWhenAirAndSeaVsSub() {
                     .defender(defender)
                     .attackingUnits(List.of(airUnit, attackingSeaUnit))
                     .defendingUnits(List.of(subUnit, defendingSeaUnit))
+                    .battleSite(givenSeaBattleSite())
                     .build());
 
     assertThat(firingGroups, hasSize(2));