[DSK] Implement Unable to Scream #13234
Conversation
| this.addAbility(new EnchantAbility(auraTarget)); | ||
|
|
||
| // Enchanted creature loses all abilities and is a Toy artifact creature with base power and toughness 0/2 | ||
| // in addition to its other types. |
There was a problem hiding this comment.
Ability comments in card constructor must use one line style for butter IDE's search support (e.g. by regexp)
| public boolean applies(GameEvent event, Ability source, Game game) { | ||
| Permanent aura = game.getPermanent(source.getSourceId()); | ||
| if (aura == null) | ||
| return false; |
There was a problem hiding this comment.
Try to use {} all the time, even for single command
| castSpell(3, PhaseStep.PRECOMBAT_MAIN, playerA, UNABLE_TO_SCREAM, EmptyNames.FACE_DOWN_CREATURE.getTestCommand(), true); | ||
| castSpell(3, PhaseStep.PRECOMBAT_MAIN, playerA, breakOpen, EmptyNames.FACE_DOWN_CREATURE.getTestCommand()); | ||
|
|
||
| setStopAt(4, PhaseStep.BEGIN_COMBAT); |
There was a problem hiding this comment.
All new tests must run under strict mode (with full and correct choice commands instead AI decisions).
So insert setStrictChooseMode(true); before each execute/setStopAt call.
| */ | ||
| public class UnableToScreamTest extends CardTestPlayerBase { | ||
|
|
||
| public static final String UNABLE_TO_SCREAM = "Unable to Scream"; |
There was a problem hiding this comment.
It's better to write related card's ability text in the test too. I prefer to write it before addCard for each test, but it can be used before card name constant too. It helps with test readability without IDE's tools or on the github website.
| addCard(Zone.BATTLEFIELD, playerA, "Island", 2); | ||
| addCard(Zone.BATTLEFIELD, playerA, "Mountain", 1); | ||
|
|
||
| castSpell(2, PhaseStep.PRECOMBAT_MAIN, playerB, akroma + " using Morph"); |
There was a problem hiding this comment.
I recommend to add additional comments for complex tests with multiples turns or steps. Example:
- prepare face down and attach to it
- try to face up (must be restricted)
Also there are multiple runtime checks that can be run between commands (see checkXXX), not after whole game like assertXXX. It's use same logic as normal commands -- execute it on player's priority. I recommends to insert such checks for complex use cases -- so you can ensure that all goes fine. Even more -- you can run any code and get full access to game objects in the middle of the test by runCode command (can be useful for complex checks or IDE's debugging).
With enabled strict mode it's very easy to write complex tests:
- enable strict mode;
- insert first command;
- run test and see failed result -- it will help to find miss choice;
- insert choice command and run again to see next miss choice;
- insert check commands same way;
There was a problem hiding this comment.
If I understood well, you're saying that I could test all the effects of Unable to Scream within a single test using intermediate check* and runCode methods.
There was a problem hiding this comment.
Yes, it's possible. See test_Optional_ManaVault_1 for check commands usage example and test_MustHaveLandWalkOfTheChosenType for runCode usage example. Also there are useful command waitStackResolved -- it allow to wait whole stack or single object to resolve, so multiple casts/actions can be done and checked in single main step.
P.S. It's not required for PR, just a best practices for game tests writing. You can keep your current tests. Just make sure that a strict mode was enabled.
There was a problem hiding this comment.
BTW there are showXXX commands too -- it prints some game data like battlefield creatures or playable abilities. Can be useful for debug without IDE's debugger (by logs only).
There was a problem hiding this comment.
Thank you, these insights will be very helpfull.
| creature.removeAllAbilities(source.getSourceId(), game); | ||
| } | ||
| } | ||
| return true; |
There was a problem hiding this comment.
It's better to return false here (nothing happen). And return true after removeAllAbilities call.
| } | ||
|
|
||
| @Override | ||
| public ContinuousEffect copy() { |
There was a problem hiding this comment.
make the return type of this method UnableToScreamPreventingEffect ("covariant return")
There was a problem hiding this comment.
Oh thanks, it was an oversight.
Part of #12534