From 367defd9958ac88b28206c6841771a71ee9b3ef5 Mon Sep 17 00:00:00 2001 From: Oleg Agafonov Date: Thu, 11 Apr 2024 22:06:58 +0400 Subject: [PATCH] tests: added tests for non-stack delayed trigger and details docs about problem --- .../abilities/enters/BanisherPriestTest.java | 16 +- .../cards/continuous/CommandersCastTest.java | 8 +- .../StateBaseTriggeredAbilityTest.java | 160 ++++++++++++++++++ .../java/org/mage/test/player/TestPlayer.java | 8 +- .../serverside/base/MageTestPlayerBase.java | 2 +- Mage/src/main/java/mage/game/GameImpl.java | 48 +++++- Mage/src/main/java/mage/game/GameState.java | 9 +- 7 files changed, 234 insertions(+), 17 deletions(-) create mode 100644 Mage.Tests/src/test/java/org/mage/test/cards/triggers/StateBaseTriggeredAbilityTest.java diff --git a/Mage.Tests/src/test/java/org/mage/test/cards/abilities/enters/BanisherPriestTest.java b/Mage.Tests/src/test/java/org/mage/test/cards/abilities/enters/BanisherPriestTest.java index 93662ddb4f3..bc5e8e05fec 100644 --- a/Mage.Tests/src/test/java/org/mage/test/cards/abilities/enters/BanisherPriestTest.java +++ b/Mage.Tests/src/test/java/org/mage/test/cards/abilities/enters/BanisherPriestTest.java @@ -15,7 +15,7 @@ import org.mage.test.serverside.base.CardTestPlayerBase; */ public class BanisherPriestTest extends CardTestPlayerBase { - /** + /** * If Banisher Priest leaves the battlefield before its enters-the-battlefield * ability resolves, the target creature won't be exiled. */ @@ -36,10 +36,13 @@ public class BanisherPriestTest extends CardTestPlayerBase { addCard(Zone.BATTLEFIELD, playerB, "Rockslide Elemental"); castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Banisher Priest"); + addTarget(playerA, "Rockslide Elemental"); waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN, 1); // Let Banisher Priest enter the battlefield, but don't let its ETB ability resolve castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerB, "Incinerate", "Banisher Priest"); + setChoice(playerB, true); // put +1/+1 + setStrictChooseMode(true); setStopAt(1, PhaseStep.END_TURN); execute(); @@ -59,6 +62,7 @@ public class BanisherPriestTest extends CardTestPlayerBase { /** * Check that the returning target did not trigger the dies Event of * the dying Banisher Priest + * TODO: will fail until non-stack delayed triggers reworked, search: state.addTriggeredAbility */ @Test public void testReturningTargetDoesNotTriggerDieEventOfBanisherPriest() { @@ -76,10 +80,17 @@ public class BanisherPriestTest extends CardTestPlayerBase { */ addCard(Zone.BATTLEFIELD, playerB, "Rockslide Elemental"); + // cast priest and exile elemental castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Banisher Priest"); + addTarget(playerA, "Rockslide Elemental"); // exile + // destroy priest and return elemental without dies triggers (priest dies before returning) castSpell(1, PhaseStep.POSTCOMBAT_MAIN, playerB, "Incinerate", "Banisher Priest"); + checkStackSize("before die", 1, PhaseStep.POSTCOMBAT_MAIN, playerB, 1); // Cast Incinerate + waitStackResolved(1, PhaseStep.POSTCOMBAT_MAIN, playerB, true); + checkStackSize("before die", 1, PhaseStep.POSTCOMBAT_MAIN, playerB, 0); // no triggers + setStrictChooseMode(true); setStopAt(1, PhaseStep.END_TURN); execute(); @@ -116,12 +127,11 @@ public class BanisherPriestTest extends CardTestPlayerBase { */ addCard(Zone.BATTLEFIELD, playerB, "Seance"); - setStrictChooseMode(true); - setChoice(playerB, "Yes"); addTarget(playerB, "Banisher Priest"); // Return the Banisher Priest from graveyard with Seance addTarget(playerB, "Silvercoat Lion"); + setStrictChooseMode(true); setStopAt(2, PhaseStep.PRECOMBAT_MAIN); execute(); diff --git a/Mage.Tests/src/test/java/org/mage/test/cards/continuous/CommandersCastTest.java b/Mage.Tests/src/test/java/org/mage/test/cards/continuous/CommandersCastTest.java index 2e7ba0b21b2..e7a88471d9a 100644 --- a/Mage.Tests/src/test/java/org/mage/test/cards/continuous/CommandersCastTest.java +++ b/Mage.Tests/src/test/java/org/mage/test/cards/continuous/CommandersCastTest.java @@ -267,10 +267,10 @@ public class CommandersCastTest extends CardTestCommander4PlayersWithAIHelps { /** * Reported bug: https://github.com/magefree/mage/issues/5121 - * Exiling your commander from your graveyard should give you the option to put it in command zone - * We were playing in a restarted-by-Karn game (if that mattered), and a player who exiled their - * commander from graveyard via Delve was not given the opportunity to place it in the command zone. - * Instead, it went directly to the exiled zone. + * Exiling your commander from your graveyard should give you the option to put it in command zone + * We were playing in a restarted-by-Karn game (if that mattered), and a player who exiled their + * commander from graveyard via Delve was not given the opportunity to place it in the command zone. + * Instead, it went directly to the exiled zone. */ @Test public void test_ExileWithDelvePayAndReturn() { diff --git a/Mage.Tests/src/test/java/org/mage/test/cards/triggers/StateBaseTriggeredAbilityTest.java b/Mage.Tests/src/test/java/org/mage/test/cards/triggers/StateBaseTriggeredAbilityTest.java new file mode 100644 index 00000000000..43302d16386 --- /dev/null +++ b/Mage.Tests/src/test/java/org/mage/test/cards/triggers/StateBaseTriggeredAbilityTest.java @@ -0,0 +1,160 @@ +package org.mage.test.cards.triggers; + +import mage.abilities.StateTriggeredAbility; +import mage.abilities.effects.common.CreateTokenEffect; +import mage.constants.PhaseStep; +import mage.constants.Zone; +import mage.game.Game; +import mage.game.events.GameEvent; +import mage.game.permanent.token.SoldierToken; +import org.junit.Ignore; +import org.junit.Test; +import org.mage.test.serverside.base.CardTestPlayerBase; + +/** + * @author JayDi85 + */ +public class StateBaseTriggeredAbilityTest extends CardTestPlayerBase { + + static class CustomStateTriggeredAbility extends StateTriggeredAbility { + + public CustomStateTriggeredAbility(boolean usesStack) { + super(Zone.ALL, new CreateTokenEffect(new SoldierToken())); + this.usesStack = usesStack; + } + + private CustomStateTriggeredAbility(final CustomStateTriggeredAbility ability) { + super(ability); + } + + @Override + public CustomStateTriggeredAbility copy() { + return new CustomStateTriggeredAbility(this); + } + + @Override + public boolean checkTrigger(GameEvent event, Game game) { + return game.getBattlefield().getAllActivePermanents().stream().noneMatch(p -> p.isCreature(game)) + && game.getBattlefield().getAllActivePermanents().stream().noneMatch(p -> p.isEnchantment(game)); + } + + @Override + public String getRule() { + return "If no creates or enchantments on battlefield then create Soldier token"; + } + } + + @Test + public void test_WithoutStack() { + // If no creates or enchantments on battlefield then create Soldier token + addCustomCardWithAbility("test", playerA, new CustomStateTriggeredAbility(false)); + + addCard(Zone.BATTLEFIELD, playerA, "Grizzly Bears", 1); + addCard(Zone.HAND, playerA, "Lightning Bolt", 1); + addCard(Zone.BATTLEFIELD, playerA, "Mountain", 1); + + // no SBA-triggers + checkPermanentCount("before", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Soldier Token", 0); + + // destroy creature and activate SBA-trigger + castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Lightning Bolt", "Grizzly Bears"); + checkStackSize("on cast bolt", 1, PhaseStep.PRECOMBAT_MAIN, playerA, 1); + checkStackObject("on cast bolt", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Cast Lightning Bolt", 1); + // + waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN, true); + checkStackSize("after SBA resolve", 1, PhaseStep.PRECOMBAT_MAIN, playerA, 0); + + setStrictChooseMode(true); + setStopAt(1, PhaseStep.END_TURN); + execute(); + + assertPermanentCount(playerA, "Soldier Token", 1); + } + + @Test + public void test_WithStack() { + // If no creates or enchantments on battlefield then create Soldier token + addCustomCardWithAbility("test", playerA, new CustomStateTriggeredAbility(true)); + + addCard(Zone.BATTLEFIELD, playerA, "Grizzly Bears", 1); + addCard(Zone.HAND, playerA, "Lightning Bolt", 1); + addCard(Zone.BATTLEFIELD, playerA, "Mountain", 1); + + // no SBA-triggers + checkPermanentCount("before", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Soldier Token", 0); + + // destroy creature and activate SBA-trigger + castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Lightning Bolt", "Grizzly Bears"); + checkStackSize("on cast bolt", 1, PhaseStep.PRECOMBAT_MAIN, playerA, 1); + checkStackObject("on cast bolt", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Cast Lightning Bolt", 1); + // + waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN, true); + checkStackSize("after bolt resolve - sba", 1, PhaseStep.PRECOMBAT_MAIN, playerA, 1); + checkStackObject("after bolt resolve - sba", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "If no creates or enchantments", 1); + // + waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN, true); + checkStackSize("after SBA resolve", 1, PhaseStep.PRECOMBAT_MAIN, playerA, 0); + + setStrictChooseMode(true); + setStopAt(1, PhaseStep.END_TURN); + execute(); + + assertPermanentCount(playerA, "Soldier Token", 1); + } + + @Test + @Ignore // TODO: enable after non-stack delayed triggers reworked, search: state.addTriggeredAbility + public void test_GraspOfFate_DelayedTriggerMustResolveImmediately() { + // checking rule: + // The exiled cards return to the battlefield immediately after Grasp of Fate leaves the battlefield. + // Nothing happens between the two events, including state-based actions. + // (2015-11-04) + + // If no creates or enchantments on battlefield then create Soldier token + addCustomCardWithAbility("test", playerA, new CustomStateTriggeredAbility(false)); + + // When Grasp of Fate enters the battlefield, for each opponent, exile up to one target nonland permanent + // that player controls until Grasp of Fate leaves the battlefield. (Those permanents return under their + // owners’ control.) + addCard(Zone.HAND, playerA, "Grasp of Fate", 1); // {1}{W}{W} + addCard(Zone.BATTLEFIELD, playerA, "Plains", 3); + // + addCard(Zone.BATTLEFIELD, playerB, "Grizzly Bears", 1); + // + // Destroy target enchantment + addCard(Zone.HAND, playerA, "Collective Effort", 1); // {1}{W}{W} + addCard(Zone.BATTLEFIELD, playerA, "Plains", 3); + // + addCard(Zone.HAND, playerA, "Lightning Bolt", 1); + addCard(Zone.BATTLEFIELD, playerA, "Mountain", 1); + + // no SBA-triggers + checkPermanentCount("before", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Soldier Token", 0); + + // cast and exile permanent (no SBA-trigger) + activateManaAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "{T}: Add {W}", 3); + castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Grasp of Fate"); + addTarget(playerA, "Grizzly Bears"); + waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN); + checkPermanentCount("after exile", 1, PhaseStep.PRECOMBAT_MAIN, playerA, playerA, "Soldier Token", 0); + checkPermanentCount("after exile", 1, PhaseStep.PRECOMBAT_MAIN, playerA, playerB, "Grizzly Bears", 0); + + // destroy grasp and return bears (no SBA-trigger, see related rules) + activateManaAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "{T}: Add {W}", 3); + castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Collective Effort"); + setModeChoice(playerA, "2"); // Destroy target enchantment + addTarget(playerA, "Grasp of Fate"); + waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN); + checkPermanentCount("after return", 1, PhaseStep.PRECOMBAT_MAIN, playerA, playerA, "Soldier Token", 0); + checkPermanentCount("after return", 1, PhaseStep.PRECOMBAT_MAIN, playerA, playerB, "Grizzly Bears", 1); + + // make sure sba works + castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Lightning Bolt", "Grizzly Bears"); + waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN, playerA); + checkPermanentCount("after sba", 1, PhaseStep.PRECOMBAT_MAIN, playerA, playerA, "Soldier Token", 1); + + setStrictChooseMode(true); + setStopAt(1, PhaseStep.END_TURN); + execute(); + } +} diff --git a/Mage.Tests/src/test/java/org/mage/test/player/TestPlayer.java b/Mage.Tests/src/test/java/org/mage/test/player/TestPlayer.java index 59083137054..eb2b7ebeaf7 100644 --- a/Mage.Tests/src/test/java/org/mage/test/player/TestPlayer.java +++ b/Mage.Tests/src/test/java/org/mage/test/player/TestPlayer.java @@ -190,7 +190,7 @@ public class TestPlayer implements Player { aliases.put(aliasId, objectId); } - public ManaOptions getAvailableManaTest(Game game) { + public ManaOptions getAvailableManaTest(Game game) { // TODO: remove return computerPlayer.getManaAvailable(game); } @@ -2695,7 +2695,7 @@ public class TestPlayer implements Player { } // wrong target settings by addTarget - // how to fix: implement target class processing above + // how to fix: implement target class processing above (if it a permanent target then check "filter instanceof" code too) if (!targets.isEmpty()) { String message; @@ -2704,13 +2704,13 @@ public class TestPlayer implements Player { + "\nCard: " + source.getSourceObject(game) + "\nAbility: " + source.getClass().getSimpleName() + " (" + source.getRule() + ")" + "\nTarget: " + target.getClass().getSimpleName() + " (" + target.getMessage() + ")" - + "\nYou must implement target class support in TestPlayer or setup good targets"; + + "\nYou must implement target class support in TestPlayer, \"filter instanceof\", or setup good targets"; } else { message = this.getName() + " - Targets list was setup by addTarget with " + targets + ", but not used" + "\nCard: unknown source" + "\nAbility: unknown source" + "\nTarget: " + target.getClass().getSimpleName() + " (" + target.getMessage() + ")" - + "\nYou must implement target class support in TestPlayer or setup good targets"; + + "\nYou must implement target class support in TestPlayer, \"filter instanceof\", or setup good targets"; } Assert.fail(message); } diff --git a/Mage.Tests/src/test/java/org/mage/test/serverside/base/MageTestPlayerBase.java b/Mage.Tests/src/test/java/org/mage/test/serverside/base/MageTestPlayerBase.java index c11a7c900fe..d55aea7824b 100644 --- a/Mage.Tests/src/test/java/org/mage/test/serverside/base/MageTestPlayerBase.java +++ b/Mage.Tests/src/test/java/org/mage/test/serverside/base/MageTestPlayerBase.java @@ -256,7 +256,7 @@ public abstract class MageTestPlayerBase { } protected void addCustomCardWithAbility(String customName, TestPlayer controllerPlayer, Ability ability) { - addCustomCardWithAbility(customName, controllerPlayer, ability, null, CardType.ENCHANTMENT, "", Zone.BATTLEFIELD); + addCustomCardWithAbility(customName, controllerPlayer, ability, null, null, "", Zone.BATTLEFIELD); } protected void addCustomCardWithAbility(String customName, TestPlayer controllerPlayer, Ability ability, SpellAbility spellAbility, diff --git a/Mage/src/main/java/mage/game/GameImpl.java b/Mage/src/main/java/mage/game/GameImpl.java index 0a5829f6773..7056db81e92 100644 --- a/Mage/src/main/java/mage/game/GameImpl.java +++ b/Mage/src/main/java/mage/game/GameImpl.java @@ -2068,12 +2068,16 @@ public abstract class GameImpl implements Game { } if (ability instanceof TriggeredManaAbility || ability instanceof DelayedTriggeredManaAbility) { // 20110715 - 605.4 + // 605.4a A triggered mana ability doesn’t go on the stack, so it can’t be targeted, + // countered, or otherwise responded to. Rather, it resolves immediately after the mana + // ability that triggered it, without waiting for priority. Ability manaAbility = ability.copy(); if (manaAbility.getSourceObjectZoneChangeCounter() == 0) { manaAbility.setSourceObjectZoneChangeCounter(getState().getZoneChangeCounter(ability.getSourceId())); } - manaAbility.activate(this, false); - manaAbility.resolve(this); + if (manaAbility.activate(this, false)) { + manaAbility.resolve(this); + } } else { TriggeredAbility newAbility = ability.copy(); newAbility.newId(); @@ -2084,6 +2088,46 @@ public abstract class GameImpl implements Game { newAbility.setSourcePermanentTransformCount(this); } newAbility.setTriggerEvent(triggeringEvent); + + // TODO: non-stack delayed triggers are xmage's workaround to support specific cards + // instead replacement effects usage. That triggers must be executed immediately like mana abilities + // or be reworked, cause current code do not support rule "nothing happens between the two events, + // including state-based actions" + // + // Search related cards by "usesStack = false". + // See conflicting tests in StateBaseTriggeredAbilityTest and BanisherPriestTest + // + // example 1: + // Grasp of Fate: exile ... until Grasp of Fate leaves the battlefield + // The exiled cards return to the battlefield immediately after Grasp of Fate leaves the battlefield. Nothing + // happens between the two events, including state-based actions. + // (2015-11-04) + // + // example 2: + // Banisher Priest: exile ... until Banisher Priest leaves the battlefield + // Banisher Priest's ability causes a zone change with a duration, a new style of ability that's + // somewhat reminiscent of older cards like Oblivion Ring. However, unlike Oblivion Ring, cards + // like Banisher Priest have a single ability that creates two one-shot effects: one that exiles + // the creature when the ability resolves, and another that returns the exiled card to the battlefield + // immediately after Banisher Priest leaves the battlefield. + // (2013-07-01) + // The exiled card returns to the battlefield immediately after Banisher Priest leaves the battlefield. + // Nothing happens between the two events, including state-based actions. The two creatures aren't on + // the battlefield at the same time. For example, if the returning creature is a Clone, it can't enter + // the battlefield as a copy of Banisher Priest. + // (2013-07-01) + // + // + /* possible code: + if (newAbility.isUsesStack()) { + state.addTriggeredAbility(newAbility); + } else { + if (newAbility.activate(this, false)) { + newAbility.resolve(this); + } + }//*/ + + // original code state.addTriggeredAbility(newAbility); } } diff --git a/Mage/src/main/java/mage/game/GameState.java b/Mage/src/main/java/mage/game/GameState.java index ee6dd0abef7..328f5599b67 100644 --- a/Mage/src/main/java/mage/game/GameState.java +++ b/Mage/src/main/java/mage/game/GameState.java @@ -71,7 +71,6 @@ public class GameState implements Serializable, Copyable { private final Map lookedAt = new HashMap<>(); private final Revealed companion; - private DelayedTriggeredAbilities delayed; private SpecialActions specialActions; private Watchers watchers; private Turn turn; @@ -94,8 +93,9 @@ public class GameState implements Serializable, Copyable { private boolean gameOver; private boolean paused; private ContinuousEffects effects; - private TriggeredAbilities triggers; - private List triggered = new ArrayList<>(); + private TriggeredAbilities triggers; // all normal triggers + private DelayedTriggeredAbilities delayed; // all delayed triggers + private List triggered = new ArrayList<>(); // raised triggers, waiting to resolve (can contains both normal and delayed) private Combat combat; private Map values = new HashMap<>(); private Map zones = new HashMap<>(); @@ -233,6 +233,7 @@ public class GameState implements Serializable, Copyable { } public void restore(GameState state) { + // no needs in copy here cause GameState already copied on save and it will be used only one time here this.activePlayerId = state.activePlayerId; this.playerList.setCurrent(state.activePlayerId); this.playerByOrderId = state.playerByOrderId; @@ -1175,6 +1176,8 @@ public class GameState implements Serializable, Copyable { public void addTriggeredAbility(TriggeredAbility ability) { this.triggered.add(ability); + System.out.println("added: " + triggered.size()); + System.out.println(triggered.stream().map(Ability::toString).collect(Collectors.joining("\r\n"))); } public void removeTriggeredAbility(TriggeredAbility ability) {