Mage: Unit Testing Framework enhancements

[drmDev]

There's a few known issues in XMage's UT framework that prevent some cards from being testable. I'll continue to add them here as tasks to complete.

• [x] Handling for creature able to block multiple creatures (ex WatcherInTheWeb)

• [ ] A wide variety of targetting issues (see RealitySmasherTest) - seems to be related to the UT framework having separate handling for targetting that does not always match in game.

• [ ] Ability to force how the mana is tapped for casting spells / activating abilities (ex. casting multiple spells in the same turn requiring specific mana costs can sometimes fail due to not having the right mana available to use it)

• [x] Handling to differentiate between cards with the same name, i.e. a creature named Nacatl War-Pride vs the token that is a copy of itself named Nacatl War-Pride (ex. be able to block the actual creature instead of the token)

• [ ] Some abilities such as Prowl do not seem to be supported. See ProwlTest

• [x] Illegal moves that would not be permitted in a game fail silently such as declaring illegal blockers or attacking with summoning sick creatures

• [ ] Casting a spell without mana available will fail silently and not cause the unit test to fail

• [ ] A permanent can only be targeted once by multi-target spells and abilities regardless if they can be targeted multiple times

• [ ] Number of targets a spell has isn't enforced. The wrong number of targets will either cause the spell not to be cast or ignore extra targets.

• [ ] We can't differentiate between attacking (illegally) multiple times each combat and attacking over multiple combat stages on the same turn.

(feel free to add more issues you've noticed from using the UT framework)

[Simown]

You can differentiate between 1 token and 1 copy with "Card Name[copy only]" or "Card Name[no copy]" (although not tested that but there seems to be support for it), when there is more than one creature (or copy) with the same name on the battlefield then it's anyone's guess. It would be nice to have getPermenant("Card Name", X) and getPermenant("Card Name", Y) where X entered the battlefield before Y.

[drmDev]

Thanks @Simown ! commit 1a464356647a9b9dfa2d98ad1a6fab29d7063ae8 allows blocking multiple creatures!

[drmDev]

Also reminded me... updated those unit tests with comments on currently failing ones, and renamed the Lilianna one from the stupidly named "testMe" in commit:
43af2e503ceb995bee85b410f8de9d4880911765

[Simown]

Blocking still needs a bit of work - non-flyers blocking flyers for example fails silently - even though it's an illegal block and the creature remains unblocked. The blocking abilities aren't being checked in the TestPlayer just the number of blockers - need to work out if they can actually be checked.

The test shouldn't really continue as if the block never happened, this could have knock on effects for example if there is a precondition "blocked this combat" and the block wasn't valid but went ahead anyway.

Edit: Added a checkbox for this issue

[Simown]

Updated the list.

Nearly finished the work on handling multiple targets - creatures are indexed as to when they entered the battlefield and can be accessed "Creature:0", "Creature:1" etc. Creature 0 can always be selected using "Creature" (without an index).

Need to do a bit of tidying up and fix up some unit tests that try to run tests with illegal moves in them.

[drmDev]

Looking forward to it! Appreciate all the time and effort you're putting into this!

[Simown]

Apologies for the delay getting this support in - I'd nearly finished then life got in the way. Hopefully I'll be free and get it in tonight.

There is an overriding problem with the tests in that it exposes the internals to the test writer. Meaning you have to pass "Creature1^Creature2^Creature3...." as a parameter, for example. Ideally you'd want to pass an array or list of creatures in and the underlying methods construct this test-specific string. Passing strings around to drive the tests which work with string matching isn't ideal or resilient to errors... but that's an issue for the future I hope to look at.

[drmDev]

Very good point. It did always seem bad to have to add string delimiters for the list of creatures to use etc.

[Simown]

Can differentiate between permanents you control now in the tests: da3a3ec8758c5e62107bc17cb624669416ef9e80

Blockers are validated and summoning sick creatures can no longer attack. Attackers still need work which is covered by the most recent checkbox.

[drmDev]

summoning sick creatures can no longer attack whoops... I'm sure this breaks like 100 tests I've made haha. It's fine, they shouldn't have been able to in the first place.

Great work! I'm fine with going back and fixing up any tests that need be after this.

[Simown]

It's fine. I fixed all known passing tests before I committed so you should be safe :) Just have to remember that in future. The actual test would fail if you did anyway, we just assert it properly.

[Simown]

TestPlayerExpectedErrorsTest.java contain some tests for the assertions in the tests. Going to start writing some documentation for this stuff now it's a bit tidier

[drmDev]

good idea! I should try to make sure of these improvements with adding a test for the multiple Combat Celebrants exerting for multiple combat phases issues #3345 most recently